wscutil: Handle a deprecated network key format

PATCH Applied Andrew Zaborowski 2 participants 11 messages Aug 13, 2020

Share & tools

Andrew Zaborowski

author

2020-08-13 00:50 UTC #0 lore raw

PATCH 1/16 wscutil: Handle a deprecated network key format

Implement a note from the spec saying that implementations should handle NUL-terminated Network Keys inside credentials structures.

---
 src/wscutil.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

src/wscutil.c +13 -1

index 6b445ce4..a41702e8 100644
@@ -362,13 +362,25 @@ static bool extract_network_key(struct wsc_attr_iter *iter, void *data)
 {
 	struct iovec *network_key = data;
 	unsigned int len;
+	const uint8_t *key;
 
 	len = wsc_attr_iter_get_length(iter);
 	if (len > 64)
 		return false;
 
+	/*
+	 * WSC 2.0.5, Section 12, Network Key:
+	 * "Some existing implementations based on v1.0h null-terminate the
+	 * passphrase value, i.e., add an extra 0x00 octet into the end of
+	 * the value. For backwards compatibility, implementations shall be
+	 * able to parse such a value"
+	 */
+	key = wsc_attr_iter_get_data(iter);
+	if (len && key[len - 1] == 0x00)
+		len--;
+
 	network_key->iov_len = len;
-	network_key->iov_base = (void *) wsc_attr_iter_get_data(iter);
+	network_key->iov_base = (void *) key;
 
 	return true;
 }

Signature · 1 line

2.25.1

Andrew Zaborowski

author

2020-08-13 00:50 UTC #1 lore raw

PATCH 2/16 eapol: Use the require_handshake flag for FILS

In both FT or FILS EAPoL isn't used for the initial handshake and only for the later re-keys. For FT we added the eapol_sm_set_require_handshake mechanism to tell EAPoL to not require the initial handshake and we can re-use it for FILS.

---
 src/eapol.c  | 10 ----------
 src/netdev.c | 16 +++++++++++-----
 2 files changed, 11 insertions(+), 15 deletions(-)

src/eapol.c +0 ~~-10~~

index b0036c10..b982e6b6 100644
@@ -2390,16 +2390,6 @@ bool eapol_start(struct eapol_sm *sm)
 
 	sm->started = true;
 
-	/*
-	 * FILS only uses the 4-way for rekeys, so only started needs to be set,
-	 * then we wait for a rekey.
-	 */
-	if (sm->handshake->akm_suite & (IE_RSN_AKM_SUITE_FILS_SHA256 |
-			IE_RSN_AKM_SUITE_FILS_SHA384 |
-			IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA384 |
-			IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA256))
-		return true;
-
 	if (sm->require_handshake)
 		sm->timeout = l_timeout_create(eapol_4way_handshake_time,
 				eapol_timeout, sm, NULL);

src/netdev.c +11 -5

index 9e2690d3..5d3aa862 100644
@@ -2071,6 +2071,12 @@ static void netdev_associate_event(struct l_genl_msg *msg,
 	if (netdev->ap) {
 		ret = auth_proto_rx_associate(netdev->ap, frame, frame_len);
 		if (ret == 0) {
+			bool fils = !!(netdev->handshake->akm_suite &
+					(IE_RSN_AKM_SUITE_FILS_SHA256 |
+					 IE_RSN_AKM_SUITE_FILS_SHA384 |
+					 IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA384 |
+					 IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA256));
+
 			auth_proto_free(netdev->ap);
 			netdev->ap = NULL;

@@ -2081,15 +2087,15 @@ static void netdev_associate_event(struct l_genl_msg *msg,
 			netdev->ignore_connect_event = false;
 
 			/*
-			 * If in FT we need to prevent the 4-way handshake from
-			 * happening, and instead just wait for rekeys
+			 * If in FT and/or FILS we don't force an initial 4-way
+			 * handshake and instead just keep the EAPoL state
+			 * machine for the rekeys.
 			 */
-			if (netdev->in_ft) {
+			if (netdev->in_ft || fils)
 				eapol_sm_set_require_handshake(netdev->sm,
 								false);
-				netdev->in_ft = false;
-			}
 
+			netdev->in_ft = false;
 			return;
 		} else if (ret == -EAGAIN) {
 			/*

Signature · 1 line

2.25.1

Andrew Zaborowski

author

2020-08-13 00:50 UTC #2 lore raw

PATCH 3/16 eapol: Don't re-build the AP RSNE in authenticator mode

In reply to Andrew Zaborowski

sm->handshake already contains our RSN/WPA IE so there's no need to rebuild it for msg 3/4, especially since we hardcode the fact that we only support one pairwise cipher. If we start declaring more supported ciphers and need to include a second RSNE we can first parse sm->hs->authenticator_ie into a struct ir_rsn_info, overwrite the cipher and rebuild it from that struct.

This way we duplicate less code and we hardcode fewer facts about the AP in eapol.c which also helps in adding EAP-WSC.

---
 src/eapol.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

src/eapol.c +4 ~~-15~~

index b982e6b6..ae37f6c2 100644
@@ -1267,17 +1267,16 @@ static void eapol_send_ptk_3_of_4(struct eapol_sm *sm)
 {
 	uint32_t ifindex = sm->handshake->ifindex;
 	uint8_t frame_buf[512];
-	uint8_t key_data_buf[128];
+	unsigned int rsne_len = sm->handshake->authenticator_ie[1] + 2;
+	uint8_t key_data_buf[128 + rsne_len];
+	int key_data_len = rsne_len;
 	struct eapol_key *ek = (struct eapol_key *) frame_buf;
-	int key_data_len;
 	enum crypto_cipher cipher = ie_rsn_cipher_suite_to_cipher(
 				sm->handshake->pairwise_cipher);
 	enum crypto_cipher group_cipher = ie_rsn_cipher_suite_to_cipher(
 				sm->handshake->group_cipher);
 	const uint8_t *kck;
 	const uint8_t *kek;
-	struct ie_rsn_info rsn;
-	uint8_t *rsne;
 
 	sm->replay_counter++;

@@ -1304,17 +1303,7 @@ static void eapol_send_ptk_3_of_4(struct eapol_sm *sm)
 	 * Just one RSNE in Key Data as we only set one cipher in ap->ciphers
 	 * currently.
 	 */
-
-	memset(&rsn, 0, sizeof(rsn));
-	rsn.akm_suites = IE_RSN_AKM_SUITE_PSK;
-	rsn.pairwise_ciphers = sm->handshake->pairwise_cipher;
-	rsn.group_cipher = sm->handshake->group_cipher;
-
-	rsne = key_data_buf;
-	if (!ie_build_rsne(&rsn, rsne))
-		return;
-
-	key_data_len = rsne[1] + 2;
+	memcpy(key_data_buf, sm->handshake->authenticator_ie, rsne_len);
 
 	if (group_cipher) {
 		uint8_t *gtk_kde = key_data_buf + key_data_len;

Signature · 1 line

2.25.1

Andrew Zaborowski

author

2020-08-13 00:50 UTC #3 lore raw

PATCH 4/16 eapol: Don't try setting protocol_version in eapol_rx_auth_packet

In reply to Andrew Zaborowski

In autenticator mode we'll always have protocol_version set from the start so the condition is always going to be false.

---
 src/eapol.c | 3 ---
 1 file changed, 3 deletions(-)

src/eapol.c +0 -3

index ae37f6c2..38c7956e 100644
@@ -2201,9 +2201,6 @@ static void eapol_rx_auth_packet(uint16_t proto, const uint8_t *from,
 {
 	struct eapol_sm *sm = user_data;
 
-	if (!sm->protocol_version)
-		sm->protocol_version = frame->header.protocol_version;
-
 	switch (frame->header.packet_type) {
 	case 3: /* EAPOL-Key */
 		eapol_auth_key_handle(sm, frame);

Signature · 1 line

2.25.1

Andrew Zaborowski

author

2020-08-13 00:50 UTC #4 lore raw

PATCH 5/16 eapol: Use eapol_sm_write in authenticator mode

In reply to Andrew Zaborowski

Throughout the supplicant mode we'd use the eapol_sm_write wrapper but in the authenticator mode we'd call __eapol_tx_packet directly. Adapt eapol_sm_write to use the right destination address and use it consistently.

---
 src/eapol.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

src/eapol.c +7 -8

index 38c7956e..299de13f 100644
@@ -904,8 +904,11 @@ void eapol_sm_set_user_data(struct eapol_sm *sm, void *user_data)
 static void eapol_sm_write(struct eapol_sm *sm, const struct eapol_frame *ef,
 				bool noencrypt)
 {
-	__eapol_tx_packet(sm->handshake->ifindex, sm->handshake->aa, ETH_P_PAE,
-				ef, noencrypt);
+	const uint8_t *dst = sm->handshake->authenticator ?
+		sm->handshake->spa : sm->handshake->aa;
+
+	__eapol_tx_packet(sm->handshake->ifindex, dst, ETH_P_PAE, ef,
+				noencrypt);
 }
 
 static inline void handshake_failed(struct eapol_sm *sm, uint16_t reason_code)

@@ -1024,7 +1027,6 @@ static void eapol_set_key_timeout(struct eapol_sm *sm,
 /* 802.11-2016 Section 12.7.6.2 */
 static void eapol_send_ptk_1_of_4(struct eapol_sm *sm)
 {
-	uint32_t ifindex = sm->handshake->ifindex;
 	const uint8_t *aa = sm->handshake->aa;
 	uint8_t frame_buf[512];
 	struct eapol_key *ek = (struct eapol_key *) frame_buf;

@@ -1062,8 +1064,7 @@ static void eapol_send_ptk_1_of_4(struct eapol_sm *sm)
 	l_debug("STA: "MAC" retries=%u", MAC_STR(sm->handshake->spa),
 			sm->frame_retry);
 
-	__eapol_tx_packet(ifindex, sm->handshake->spa, ETH_P_PAE,
-				(struct eapol_frame *) ek, false);
+	eapol_sm_write(sm, (struct eapol_frame *) ek, false);
 }
 
 static void eapol_ptk_1_of_4_retry(struct l_timeout *timeout, void *user_data)

@@ -1265,7 +1266,6 @@ error_unspecified:
 /* 802.11-2016 Section 12.7.6.4 */
 static void eapol_send_ptk_3_of_4(struct eapol_sm *sm)
 {
-	uint32_t ifindex = sm->handshake->ifindex;
 	uint8_t frame_buf[512];
 	unsigned int rsne_len = sm->handshake->authenticator_ie[1] + 2;
 	uint8_t key_data_buf[128 + rsne_len];

@@ -1335,8 +1335,7 @@ static void eapol_send_ptk_3_of_4(struct eapol_sm *sm)
 	l_debug("STA: "MAC" retries=%u", MAC_STR(sm->handshake->spa),
 			sm->frame_retry);
 
-	__eapol_tx_packet(ifindex, sm->handshake->spa, ETH_P_PAE,
-				(struct eapol_frame *) ek, false);
+	eapol_sm_write(sm, (struct eapol_frame *) ek, false);
 }
 
 static void eapol_ptk_3_of_4_retry(struct l_timeout *timeout,

Signature · 1 line

2.25.1

Andrew Zaborowski

author

2020-08-13 00:50 UTC #5 lore raw

PATCH 6/16 eapol: Basic EAP support in authenticator mode

In reply to Andrew Zaborowski

Handle EAPoL-EAP frames using our eap.c methods in authenticator mode same as we do on the supplicant side. The user (ap.c) will only need to set a valid 8021x_settings in the handshake object, same as on the supplicant side.

---
 src/eapol.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 4 deletions(-)

src/eapol.c +72 -4

index 299de13f..06b8a2f6 100644
@@ -2067,6 +2067,17 @@ static void eapol_eap_complete_cb(enum eap_result result, void *user_data)
 	}
 
 	eap_reset(sm->eap);
+
+	if (sm->handshake->authenticator) {
+		if (L_WARN_ON(!sm->handshake->have_pmk))
+			return;
+
+		sm->mic_len = eapol_get_mic_length(sm->handshake->akm_suite,
+						sm->handshake->pmk_len);
+
+		/* Kick off 4-Way Handshake */
+		eapol_ptk_1_of_4_retry(NULL, sm);
+	}
 }
 
 /* This respresentes the eapResults message */

@@ -2200,7 +2211,34 @@ static void eapol_rx_auth_packet(uint16_t proto, const uint8_t *from,
 {
 	struct eapol_sm *sm = user_data;
 
+	if (proto != ETH_P_PAE || memcmp(from, sm->handshake->spa, 6))
+		return;
+
 	switch (frame->header.packet_type) {
+	case 0:	/* EAPOL-EAP */
+		if (!sm->eap) {
+			l_error("Authenticator received an unexpected "
+				"EAPOL-EAP frame from %s",
+				util_address_to_string(from));
+			return;
+		}
+
+		eap_rx_packet(sm->eap, frame->data,
+				L_BE16_TO_CPU(frame->header.packet_len));
+		break;
+
+	case 1:	/* EAPOL-Start */
+		/*
+		 * The supplicant might have sent an EAPoL-Start even before
+		 * we queued our EAP Identity Request, so this should happen
+		 * mostly while we wait for the EAP Identity Response or before.
+		 * It's safe to ignore this frame in either case.
+		 *
+		 * TODO: if we're already past the full handshake, send a
+		 * new msg 1/4.
+		 */
+		break;
+
 	case 3: /* EAPOL-Key */
 		eapol_auth_key_handle(sm, frame);
 		break;

@@ -2341,16 +2379,46 @@ void eapol_register(struct eapol_sm *sm)
 			sm->protocol_version = sm->handshake->proto_version;
 
 		sm->started = true;
-		/* Since AP/AdHoc only support AKM PSK we can hard code this */
-		sm->mic_len = 16;
 
-		/* kick off handshake */
-		eapol_ptk_1_of_4_retry(NULL, sm);
+		if (sm->handshake->settings_8021x) {
+			sm->eap = eap_new(eapol_eap_msg_cb,
+						eapol_eap_complete_cb, sm);
+			if (!sm->eap)
+				goto eap_error;
+
+			eap_set_key_material_func(sm->eap,
+							eapol_eap_results_cb);
+			eap_set_event_func(sm->eap, eapol_eap_event_cb);
+
+			if (!eap_load_settings(sm->eap,
+						sm->handshake->settings_8021x,
+						"EAP-")) {
+				eap_free(sm->eap);
+				sm->eap = NULL;
+				goto eap_error;
+			}
+		} else {
+			if (L_WARN_ON(!sm->handshake->have_pmk))
+				return;
+
+			sm->mic_len = eapol_get_mic_length(
+						sm->handshake->akm_suite,
+						sm->handshake->pmk_len);
+
+			/* kick off handshake */
+			eapol_ptk_1_of_4_retry(NULL, sm);
+		}
 	} else {
 		sm->watch_id = eapol_frame_watch_add(sm->handshake->ifindex,
 						eapol_rx_packet, sm);
 		sm->protocol_version = sm->handshake->proto_version;
 	}
+
+	return;
+
+eap_error:
+	l_error("Error initializing EAP for ifindex %i",
+			(int) sm->handshake->ifindex);
 }
 
 bool eapol_start(struct eapol_sm *sm)

Signature · 1 line

2.25.1

Denis Kenzior

2020-08-13 16:23 UTC #6 lore raw

Re: [PATCH 06/16] eapol: Basic EAP support in authenticator mode

Hi Andrew,

Quoted 13 lines · Andrew Zaborowski

Handle EAPoL-EAP frames using our eap.c methods in authenticator mode same as we do on the supplicant side. The user (ap.c) will only need to set a valid 8021x_settings in the handshake object, same as on the supplicant side.
---
  src/eapol.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++---
  1 file changed, 72 insertions(+), 4 deletions(-)
src/eapol.c +0 -0
index 299de13f..06b8a2f6 100644

src/eapol.c static void eapol_eap_complete_cb(enum eap_result result, void *user_data) · line 2073

Quoted 7 lines

src/eapol.c +4 -0

@@ -2067,6 +2067,17 @@ static void eapol_eap_complete_cb(enum eap_result result, void *user_data)
  	}
 
  	eap_reset(sm->eap);
+
+	if (sm->handshake->authenticator) {
+		if (L_WARN_ON(!sm->handshake->have_pmk))
+			return;

Shouldn't this result in handshake_failed or something?

Quoted 10 lines

src/eapol.c +7 -0

+
+		sm->mic_len = eapol_get_mic_length(sm->handshake->akm_suite,
+						sm->handshake->pmk_len);
+
+		/* Kick off 4-Way Handshake */
+		eapol_ptk_1_of_4_retry(NULL, sm);
+	}
  }
 
  /* This respresentes the eapResults message */

<snip>

Quoted 16 lines

src/eapol.c +7 -4

@@ -2341,16 +2379,46 @@ void eapol_register(struct eapol_sm *sm)
  			sm->protocol_version = sm->handshake->proto_version;
 
  		sm->started = true;
-		/* Since AP/AdHoc only support AKM PSK we can hard code this */
-		sm->mic_len = 16;
 
-		/* kick off handshake */
-		eapol_ptk_1_of_4_retry(NULL, sm);
+		if (sm->handshake->settings_8021x) {
+			sm->eap = eap_new(eapol_eap_msg_cb,
+						eapol_eap_complete_cb, sm);
+			if (!sm->eap)
+				goto eap_error;
+
+			eap_set_key_material_func(sm->eap,

src/eapol.c void eapol_register(struct eapol_sm *sm) · line 2399

Quoted 10 lines

src/eapol.c +10 -0

+							eapol_eap_results_cb);
+			eap_set_event_func(sm->eap, eapol_eap_event_cb);
+
+			if (!eap_load_settings(sm->eap,
+						sm->handshake->settings_8021x,
+						"EAP-")) {
+				eap_free(sm->eap);
+				sm->eap = NULL;
+				goto eap_error;
+			}

So this seems to be a copy-paste from eapol_start. And the fact that there's no error returned from this function begs the question...

Quoted 12 lines

src/eapol.c +11 -0

+		} else {
+			if (L_WARN_ON(!sm->handshake->have_pmk))
+				return;
+
+			sm->mic_len = eapol_get_mic_length(
+						sm->handshake->akm_suite,
+						sm->handshake->pmk_len);
+
+			/* kick off handshake */
+			eapol_ptk_1_of_4_retry(NULL, sm);
+		}
  	} else {

src/eapol.c void eapol_register(struct eapol_sm *sm) · line 2421

Quoted 10 lines

src/eapol.c +6 -0

  		sm->watch_id = eapol_frame_watch_add(sm->handshake->ifindex,
  						eapol_rx_packet, sm);
  		sm->protocol_version = sm->handshake->proto_version;
  	}
+
+	return;
+
+eap_error:
+	l_error("Error initializing EAP for ifindex %i",
+			(int) sm->handshake->ifindex);

Is ap.c using the eapol API properly? From what I recall the intent was that eapol_sm_register cannot fail and the only thing it does is register the eapol state machine to receive packets. This is needed for early packet detection (early_frame handling added due to race conditions in cfg80211)...

It seems that ap.c never needed to use eapol_start. Perhaps it should? Either way, using eapol_register to do authenticator-specific things (that can fail) seems wrong...

src/eapol.c void eapol_register(struct eapol_sm *sm) · line 2424

Quoted 4 lines

src/eapol.c +0 -0

  }
 
  bool eapol_start(struct eapol_sm *sm)

Regards,
-Denis

Andrew Zaborowski

author

2020-08-13 23:47 UTC #7 lore raw

Re: [PATCH 06/16] eapol: Basic EAP support in authenticator mode

Hi Denis,

Quoted 16 lines · Denis Kenzior

Quoted 15 lines · Andrew Zaborowski
Handle EAPoL-EAP frames using our eap.c methods in authenticator mode same as we do on the supplicant side. The user (ap.c) will only need to set a valid 8021x_settings in the handshake object, same as on the supplicant side.
---
  src/eapol.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++---
  1 file changed, 72 insertions(+), 4 deletions(-)
src/eapol.c +0 -0
index 299de13f..06b8a2f6 100644
@@ -2067,6 +2067,17 @@ static void eapol_eap_complete_cb(enum eap_result result, void *user_data)
      }
 

Quoted 7 lines

src/eapol.c static void eapol_eap_complete_cb(enum eap_result result, void *user_data) · line 2073
Quoted 5 lines
src/eapol.c +4 -0
      eap_reset(sm->eap);
+
+     if (sm->handshake->authenticator) {
+             if (L_WARN_ON(!sm->handshake->have_pmk))
+                     return;
Shouldn't this result in handshake_failed or something?

Probably, although this was intended basically as an assert()

Quoted 72 lines

Quoted 11 lines

src/eapol.c +7 -0

+
+             sm->mic_len = eapol_get_mic_length(sm->handshake->akm_suite,
+                                             sm->handshake->pmk_len);
+
+             /* Kick off 4-Way Handshake */
+             eapol_ptk_1_of_4_retry(NULL, sm);
+     }
  }
 
  /* This respresentes the eapResults message */

<snip>

Quoted 16 lines

src/eapol.c +7 -4

@@ -2341,16 +2379,46 @@ void eapol_register(struct eapol_sm *sm)
                      sm->protocol_version = sm->handshake->proto_version;
 
              sm->started = true;
-             /* Since AP/AdHoc only support AKM PSK we can hard code this */
-             sm->mic_len = 16;
 
-             /* kick off handshake */
-             eapol_ptk_1_of_4_retry(NULL, sm);
+             if (sm->handshake->settings_8021x) {
+                     sm->eap = eap_new(eapol_eap_msg_cb,
+                                             eapol_eap_complete_cb, sm);
+                     if (!sm->eap)
+                             goto eap_error;
+
+                     eap_set_key_material_func(sm->eap,

src/eapol.c void eapol_register(struct eapol_sm *sm) · line 2399

Quoted 10 lines

src/eapol.c +10 -0

+                                                     eapol_eap_results_cb);
+                     eap_set_event_func(sm->eap, eapol_eap_event_cb);
+
+                     if (!eap_load_settings(sm->eap,
+                                             sm->handshake->settings_8021x,
+                                             "EAP-")) {
+                             eap_free(sm->eap);
+                             sm->eap = NULL;
+                             goto eap_error;
+                     }

So this seems to be a copy-paste from eapol_start. And the fact that there's no error returned from this function begs the question...

Quoted 12 lines

src/eapol.c +11 -0

+             } else {
+                     if (L_WARN_ON(!sm->handshake->have_pmk))
+                             return;
+
+                     sm->mic_len = eapol_get_mic_length(
+                                             sm->handshake->akm_suite,
+                                             sm->handshake->pmk_len);
+
+                     /* kick off handshake */
+                     eapol_ptk_1_of_4_retry(NULL, sm);
+             }
      } else {

src/eapol.c void eapol_register(struct eapol_sm *sm) · line 2421

Quoted 10 lines

src/eapol.c +6 -0

              sm->watch_id = eapol_frame_watch_add(sm->handshake->ifindex,
                                              eapol_rx_packet, sm);
              sm->protocol_version = sm->handshake->proto_version;
      }
+
+     return;
+
+eap_error:
+     l_error("Error initializing EAP for ifindex %i",
+                     (int) sm->handshake->ifindex);

Quoted 1 line

It seems that ap.c never needed to use eapol_start. Perhaps it should?

Agreed. I don't know if there was some reason to avoid reusing eapol_start when ap.c was converted to use eapol_sm but I'll try if we can make it work this way and include a patch.

Best regards

Andrew Zaborowski

author

2020-08-13 00:50 UTC #8 lore raw

PATCH 7/16 eap: Simplify sending EAP method responses

In reply to Andrew Zaborowski

Replace the usage of eap_send_response() in the method implementations with a new eap_method_respond that skips the redundant "type" parameter. The new eap_send_packet is used inside eap_method_respond and will be reused for sending request packets in authenticator side EAP methods.

---
 src/eap-aka.c        |  8 +++----
 src/eap-gtc.c        |  2 +-
 src/eap-md5.c        |  3 +--
 src/eap-mschapv2.c   |  4 ++--
 src/eap-private.h    |  5 +---
 src/eap-pwd.c        |  9 ++++----
 src/eap-sim.c        |  6 ++---
 src/eap-tls-common.c |  7 +++---
 src/eap-wsc.c        | 14 +++++------
 src/eap.c            | 55 ++++++++++++++++++++++++++------------------
 src/simutil.c        |  2 +-
 11 files changed, 59 insertions(+), 56 deletions(-)

src/eap-aka.c +4 -4

index 3c815dfd..3ac0b662 100644
@@ -206,7 +206,7 @@ static void check_milenage_cb(const uint8_t *res, const uint8_t *ck,
 		pos += eap_sim_add_attribute(pos, EAP_SIM_AT_AUTS,
 				EAP_SIM_PAD_NONE, auts, EAP_AKA_AUTS_LEN);
 
-		eap_send_response(eap, aka->type, response, 24);
+		eap_method_respond(eap, response, 24);
 
 		return;
 	}

@@ -285,7 +285,7 @@ static void check_milenage_cb(const uint8_t *res, const uint8_t *ck,
 	l_free(aka->chal_pkt);
 	aka->chal_pkt = NULL;
 
-	eap_send_response(eap, aka->type, response, resp_len);
+	eap_method_respond(eap, response, resp_len);
 
 	if (!aka->protected) {
 		eap_aka_finish(eap);

@@ -536,7 +536,7 @@ static void handle_notification(struct eap_state *eap, const uint8_t *pkt,
 			return;
 		}
 
-		eap_send_response(eap, aka->type, response, pos - response);
+		eap_method_respond(eap, response, pos - response);
 
 		aka->state = EAP_AKA_STATE_SUCCESS;

@@ -583,7 +583,7 @@ static void handle_identity(struct eap_state *eap, const uint8_t *pkt,
 			EAP_SIM_PAD_LENGTH, (uint8_t *)aka->identity,
 			strlen(aka->identity));
 
-	eap_send_response(eap, aka->type, response, pos - response);
+	eap_method_respond(eap, response, pos - response);
 }
 
 static void eap_aka_handle_request(struct eap_state *eap,

src/eap-gtc.c +1 -1

index f895ec61..d14fa686 100644
@@ -64,7 +64,7 @@ static void eap_gtc_handle_request(struct eap_state *eap,
 
 	memcpy(response + 5, gtc->password, secret_len);
 
-	eap_send_response(eap, EAP_TYPE_GTC, response, 5 + secret_len);
+	eap_method_respond(eap, response, 5 + secret_len);
 
 	eap_method_success(eap);

src/eap-md5.c +1 -2

index 1dab7df7..df9f5ab2 100644
@@ -79,8 +79,7 @@ static void eap_md5_handle_request(struct eap_state *eap,
 	l_checksum_get_digest(hash, response + 6, 16);
 	l_checksum_free(hash);
 
-	eap_send_response(eap, EAP_TYPE_MD5_CHALLENGE,
-				response, sizeof(response));
+	eap_method_respond(eap, response, sizeof(response));
 
 	/* We have no choice but to call it a success */
 	eap_method_success(eap);

src/eap-mschapv2.c +2 -2

index 3ede3498..49b5335c 100644
@@ -253,7 +253,7 @@ static bool eap_mschapv2_send_response(struct eap_state *eap)
 							MSCHAPV2_CHAL_LEN);
 	memcpy(response->name, state->user, state->user_len);
 
-	eap_send_response(eap, EAP_TYPE_MSCHAPV2, output, sizeof(output));
+	eap_method_respond(eap, output, sizeof(output));
 
 	return true;
 }

@@ -346,7 +346,7 @@ static void eap_mschapv2_handle_success(struct eap_state *eap,
 	eap_method_success(eap);
 
 	buffer[5] = MSCHAPV2_OP_SUCCESS;
-	eap_send_response(eap, EAP_TYPE_MSCHAPV2, buffer, sizeof(buffer));
+	eap_method_respond(eap, buffer, sizeof(buffer));
 
 	/* The eapol set_key_material only needs msk, and that's all we got */
 	eap_set_key_material(eap, session_key, 32, NULL, 0, NULL, 0, NULL, 0);

src/eap-private.h +1 -4

index d2e44397..65ee871d 100644
@@ -114,10 +114,6 @@ void *eap_get_data(struct eap_state *eap);
 enum eap_type eap_get_method_type(struct eap_state *eap);
 const char *eap_get_method_name(struct eap_state *eap);
 
-void eap_send_response(struct eap_state *eap,
-			enum eap_type request_type,
-			uint8_t *buf, size_t len);
-
 void eap_set_key_material(struct eap_state *eap,
 				const uint8_t *msk_data, size_t msk_len,
 				const uint8_t *emsk_data, size_t emsk_len,

@@ -126,6 +122,7 @@ void eap_set_key_material(struct eap_state *eap,
 
 void eap_start_complete_timeout(struct eap_state *eap);
 
+void eap_method_respond(struct eap_state *eap, uint8_t *buf, size_t len);
 bool eap_method_is_success(struct eap_state *eap);
 void eap_method_success(struct eap_state *eap);
 void eap_method_error(struct eap_state *eap);

src/eap-pwd.c +4 -5

index a77f21b9..070d483a 100644
@@ -194,7 +194,7 @@ static void eap_pwd_send_response(struct eap_state *eap,
 
 	/* packet will fit within mtu */
 	if (len <= mtu) {
-		eap_send_response(eap, EAP_TYPE_PWD, pkt, len);
+		eap_method_respond(eap, pkt, len);
 		return;
 	}

@@ -218,7 +218,7 @@ static void eap_pwd_send_response(struct eap_state *eap,
 
 	l_info("sending initial fragment, %zu bytes", mtu);
 
-	eap_send_response(eap, EAP_TYPE_PWD, frag, mtu);
+	eap_method_respond(eap, frag, mtu);
 
 	/* alloc/copy remainder of packet to frag buf */
 	pwd->tx_frag_buf = l_malloc(pwd->tx_frag_remaining);

@@ -593,7 +593,7 @@ static void eap_pwd_send_ack(struct eap_state *eap)
 
 	buf[5] = pwd->state + 1;
 
-	eap_send_response(eap, EAP_TYPE_PWD, buf, 6);
+	eap_method_respond(eap, buf, 6);
 }
 
 #define FRAG_BYTES(mtu, remaining) \

@@ -631,8 +631,7 @@ static void eap_pwd_handle_request(struct eap_state *eap,
 		l_info("sending fragment, %d bytes",
 				frag_bytes + EAP_PWD_HDR_LEN);
 
-		eap_send_response(eap, EAP_TYPE_PWD, frag,
-				frag_bytes + EAP_PWD_HDR_LEN);
+		eap_method_respond(eap, frag, frag_bytes + EAP_PWD_HDR_LEN);
 
 		if (!pwd->tx_frag_remaining) {
 			/* done sending fragments, free */

src/eap-sim.c +3 -3

index b29211f6..93063b7a 100644
@@ -290,7 +290,7 @@ static void handle_start(struct eap_state *eap, const uint8_t *pkt,
 				EAP_SIM_PAD_LENGTH, (uint8_t *)sim->identity,
 				strlen(sim->identity));
 
-	eap_send_response(eap, EAP_TYPE_SIM, response, resp_len);
+	eap_method_respond(eap, response, resp_len);
 
 	return;

@@ -391,7 +391,7 @@ static void gsm_callback(const uint8_t *sres, const uint8_t *kc,
 	l_free(sim->chal_pkt);
 	sim->chal_pkt = NULL;
 
-	eap_send_response(eap, EAP_TYPE_SIM, response, resp_len);
+	eap_method_respond(eap, response, resp_len);
 
 	if (!sim->protected) {
 		/*

@@ -565,7 +565,7 @@ static void handle_notification(struct eap_state *eap, const uint8_t *pkt,
 			return;
 		}
 
-		eap_send_response(eap, EAP_TYPE_SIM, response, pos - response);
+		eap_method_respond(eap, response, pos - response);
 
 		sim->state = EAP_SIM_STATE_SUCCESS;
 		return;

src/eap-tls-common.c +3 -4

index 28febaf0..c3eb5ab3 100644
@@ -332,7 +332,7 @@ static void eap_tls_send_fragment(struct eap_state *eap)
 
 	memcpy(buf + header_len,
 		eap_tls->tx_pdu_buf->data + eap_tls->tx_frag_offset, len);
-	eap_send_response(eap, eap_get_method_type(eap), buf, header_len + len);
+	eap_method_respond(eap, buf, header_len + len);
 
 	eap_tls->tx_frag_last_len = len;
 }

@@ -389,7 +389,7 @@ static void eap_tls_send_response(struct eap_state *eap,
 
 		memcpy(buf + EAP_TLS_HEADER_LEN + extra, pdu, pdu_len);
 
-		eap_send_response(eap, eap_get_method_type(eap), buf, msg_len);
+		eap_method_respond(eap, buf, msg_len);
 		l_free(buf);
 		return;
 	}

@@ -409,8 +409,7 @@ void eap_tls_common_send_empty_response(struct eap_state *eap)
 
 	buf[EAP_TLS_HEADER_OCTET_FLAGS + position] = eap_tls->version_negotiated;
 
-	eap_send_response(eap, eap_get_method_type(eap), buf,
-				EAP_TLS_HEADER_LEN + position);
+	eap_method_respond(eap, buf, EAP_TLS_HEADER_LEN + position);
 }
 
 static int eap_tls_init_request_assembly(struct eap_state *eap,

src/eap-wsc.c +6 -8

index 3e0433e3..0fec29c6 100644
@@ -339,7 +339,7 @@ static void eap_wsc_send_fragment(struct eap_state *eap)
 	}
 
 	memcpy(buf + header_len, wsc->sent_pdu + wsc->tx_frag_offset, len);
-	eap_send_response(eap, EAP_TYPE_EXPANDED, buf, header_len + len);
+	eap_method_respond(eap, buf, header_len + len);
 
 	wsc->tx_last_frag_len = len;
 }

@@ -359,7 +359,7 @@ static void eap_wsc_send_response(struct eap_state *eap,
 		buf[13] = 0;
 		memcpy(buf + EAP_WSC_HEADER_LEN, pdu, pdu_len);
 
-		eap_send_response(eap, EAP_TYPE_EXPANDED, buf, msg_len);
+		eap_method_respond(eap, buf, msg_len);
 		l_free(buf);
 		return;
 	}

@@ -419,8 +419,7 @@ static void eap_wsc_send_nack(struct eap_state *eap,
 	buf[13] = 0;
 	memcpy(buf + EAP_WSC_HEADER_LEN, pdu, pdu_len);
 
-	eap_send_response(eap, EAP_TYPE_EXPANDED, buf,
-						pdu_len + EAP_WSC_HEADER_LEN);
+	eap_method_respond(eap, buf, pdu_len + EAP_WSC_HEADER_LEN);
 	l_free(pdu);
 }

@@ -446,8 +445,7 @@ static void eap_wsc_send_done(struct eap_state *eap)
 	buf[13] = 0;
 	memcpy(buf + EAP_WSC_HEADER_LEN, pdu, pdu_len);
 
-	eap_send_response(eap, EAP_TYPE_EXPANDED, buf,
-						pdu_len + EAP_WSC_HEADER_LEN);
+	eap_method_respond(eap, buf, pdu_len + EAP_WSC_HEADER_LEN);
 	l_free(pdu);
 }

@@ -458,7 +456,7 @@ static void eap_wsc_send_frag_ack(struct eap_state *eap)
 	buf[12] = WSC_OP_FRAG_ACK;
 	buf[13] = 0;
 
-	eap_send_response(eap, EAP_TYPE_EXPANDED, buf, EAP_WSC_HEADER_LEN);
+	eap_method_respond(eap, buf, EAP_WSC_HEADER_LEN);
 }
 
 static void eap_wsc_handle_m8(struct eap_state *eap,

@@ -1123,7 +1121,7 @@ static void eap_wsc_handle_retransmit(struct eap_state *eap,
 		buf[13] = 0;
 		memcpy(buf + EAP_WSC_HEADER_LEN, wsc->sent_pdu, wsc->sent_len);
 
-		eap_send_response(eap, EAP_TYPE_EXPANDED, buf, msg_len);
+		eap_method_respond(eap, buf, msg_len);
 	}
 }

src/eap.c +33 ~~-22~~

69 lines not on this page · raw message

src/simutil.c +1 -1

index eda31a86..8947ad66 100644
@@ -353,7 +353,7 @@ void eap_sim_client_error(struct eap_state *eap, enum eap_type type,
 	buf[9] = 1;
 	l_put_be16(code, buf + 10);
 
-	eap_send_response(eap, type, buf, 12);
+	eap_method_respond(eap, buf, 12);
 }
 
 size_t eap_sim_add_attribute(uint8_t *buf, enum eap_sim_at attr,

Signature · 1 line

2.25.1

Denis Kenzior

Applied

2020-08-13 15:38 UTC #9 lore raw

Re: [PATCH 07/16] eap: Simplify sending EAP method responses

Hi Andrew,

Quoted 18 lines · Andrew Zaborowski

Replace the usage of eap_send_response() in the method implementations with a new eap_method_respond that skips the redundant "type" parameter. The new eap_send_packet is used inside eap_method_respond and will be reused for sending request packets in authenticator side EAP methods.
---
  src/eap-aka.c        |  8 +++----
  src/eap-gtc.c        |  2 +-
  src/eap-md5.c        |  3 +--
  src/eap-mschapv2.c   |  4 ++--
  src/eap-private.h    |  5 +---
  src/eap-pwd.c        |  9 ++++----
  src/eap-sim.c        |  6 ++---
  src/eap-tls-common.c |  7 +++---
  src/eap-wsc.c        | 14 +++++------
  src/eap.c            | 55 ++++++++++++++++++++++++++------------------
  src/simutil.c        |  2 +-
  11 files changed, 59 insertions(+), 56 deletions(-)

<snip>

src/eap.c void eap_send_response(struct eap_state *eap, enum eap_type type, · line 176

Quoted 10 lines

src/eap.c +5 -1

@@ -179,7 +169,28 @@ void eap_send_response(struct eap_state *eap, enum eap_type type,
  		l_put_be32(eap->method->vendor_type, buf + 8);
  	}
 
-	eap->tx_packet(buf, len, eap->user_data);
+	eap_send_packet(eap, EAP_CODE_RESPONSE, eap->last_id, buf, len);
+}
+
+/**
+ * eap_send_packet:

I fixed this to be eap_method_respond

Quoted 9 lines

src/eap.c +9 -0

+ * @eap: EAP state
+ * @buf: Buffer to send
+ * @len: Size of the buffer
+ *
+ * Sends out a response to a received request.  This method first fills
+ * the EAP header in the buffer based on the method's EAP type being
+ * sent.
+ *
+ * If the method uses an expanded type , then the Vendor-Id and

src/eap.c void eap_send_response(struct eap_state *eap, enum eap_type type, · line 195

Quoted 10 lines

src/eap.c +8 -0

+ * Vendor-Type fields are filled in automatically.
+ *
+ * The buffer passed in MUST be at least 12 bytes long if method uses
+ * an expanded type and at least 5 bytes for other cases.
+ **/
+void eap_method_respond(struct eap_state *eap, uint8_t *buf, size_t len)
+{
+	eap_send_response(eap, eap->method->request_type, buf, len);
  }

Applied, thanks.

Regards,
-Denis

Denis Kenzior

2020-08-13 15:31 UTC #10 lore raw

In reply to Andrew Zaborowski

Hi Andrew,

Quoted 6 lines · Andrew Zaborowski

Implement a note from the spec saying that implementations should handle NUL-terminated Network Keys inside credentials structures.
---
  src/wscutil.c | 14 +++++++++++++-
  1 file changed, 13 insertions(+), 1 deletion(-)

Patches 1-5 applied, thanks.

Regards,
-Denis