[PATCH 01/16] wscutil: Handle a deprecated network key format

[PATCH 01/16] wscutil: Handle a deprecated network key format

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

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(-)

diff --git a/src/wscutil.c b/src/wscutil.c
index 6b445ce4..a41702e8 100644
--- a/src/wscutil.c
+++ b/src/wscutil.c
@@ -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;
 }
-- 
2.25.1

[PATCH 02/16] eapol: Use the require_handshake flag for FILS

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

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(-)

diff --git a/src/eapol.c b/src/eapol.c
index b0036c10..b982e6b6 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -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);
diff --git a/src/netdev.c b/src/netdev.c
index 9e2690d3..5d3aa862 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -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) {
 			/*
-- 
2.25.1

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

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2078 bytes --]

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(-)

diff --git a/src/eapol.c b/src/eapol.c
index b982e6b6..ae37f6c2 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -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;
-- 
2.25.1

[PATCH 04/16] eapol: Don't try setting protocol_version in eapol_rx_auth_packet

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

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(-)

diff --git a/src/eapol.c b/src/eapol.c
index ae37f6c2..38c7956e 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -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);
-- 
2.25.1

[PATCH 05/16] eapol: Use eapol_sm_write in authenticator mode

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

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(-)

diff --git a/src/eapol.c b/src/eapol.c
index 38c7956e..299de13f 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -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,
-- 
2.25.1

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

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3351 bytes --]

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(-)

diff --git a/src/eapol.c b/src/eapol.c
index 299de13f..06b8a2f6 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -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)
-- 
2.25.1

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

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 13204 bytes --]

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(-)

diff --git a/src/eap-aka.c b/src/eap-aka.c
index 3c815dfd..3ac0b662 100644
--- a/src/eap-aka.c
+++ b/src/eap-aka.c
@@ -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,
diff --git a/src/eap-gtc.c b/src/eap-gtc.c
index f895ec61..d14fa686 100644
--- a/src/eap-gtc.c
+++ b/src/eap-gtc.c
@@ -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);
 
diff --git a/src/eap-md5.c b/src/eap-md5.c
index 1dab7df7..df9f5ab2 100644
--- a/src/eap-md5.c
+++ b/src/eap-md5.c
@@ -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);
diff --git a/src/eap-mschapv2.c b/src/eap-mschapv2.c
index 3ede3498..49b5335c 100644
--- a/src/eap-mschapv2.c
+++ b/src/eap-mschapv2.c
@@ -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);
diff --git a/src/eap-private.h b/src/eap-private.h
index d2e44397..65ee871d 100644
--- a/src/eap-private.h
+++ b/src/eap-private.h
@@ -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);
diff --git a/src/eap-pwd.c b/src/eap-pwd.c
index a77f21b9..070d483a 100644
--- a/src/eap-pwd.c
+++ b/src/eap-pwd.c
@@ -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 */
diff --git a/src/eap-sim.c b/src/eap-sim.c
index b29211f6..93063b7a 100644
--- a/src/eap-sim.c
+++ b/src/eap-sim.c
@@ -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;
diff --git a/src/eap-tls-common.c b/src/eap-tls-common.c
index 28febaf0..c3eb5ab3 100644
--- a/src/eap-tls-common.c
+++ b/src/eap-tls-common.c
@@ -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,
diff --git a/src/eap-wsc.c b/src/eap-wsc.c
index 3e0433e3..0fec29c6 100644
--- a/src/eap-wsc.c
+++ b/src/eap-wsc.c
@@ -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);
 	}
 }
 
diff --git a/src/eap.c b/src/eap.c
index 816b5f9a..57f8d688 100644
--- a/src/eap.c
+++ b/src/eap.c
@@ -149,29 +149,19 @@ const char *eap_get_identity(struct eap_state *eap)
 	return eap->identity;
 }
 
-/**
- * eap_send_response:
- * @eap: EAP state
- * @type: Type of response being sent
- * @buf: Buffer to send
- * @len: Size of the buffer
- *
- * Sends out a response to a received request.  This method first fills the
- * EAP header into the buffer based on the EAP type response being sent.
- *
- * If the response type is EAP_TYPE_EXPANDED, then the Vendor-Id and
- * Vendor-Type fields are filled in based on contents of the eap_method
- * associated with @eap.
- *
- * The buffer passed in MUST be at least 12 bytes long if @type is
- * EAP_TYPE_EXPANDED and at least 5 bytes for other cases.
- **/
-void eap_send_response(struct eap_state *eap, enum eap_type type,
-						uint8_t *buf, size_t len)
+static void eap_send_packet(struct eap_state *eap, enum eap_code code,
+				uint8_t id, uint8_t *buf, size_t len)
 {
-	buf[0] = EAP_CODE_RESPONSE;
-	buf[1] = eap->last_id;
+	buf[0] = code;
+	buf[1] = id;
 	l_put_be16(len, &buf[2]);
+
+	eap->tx_packet(buf, len, eap->user_data);
+}
+
+static void eap_send_response(struct eap_state *eap, enum eap_type type,
+				uint8_t *buf, size_t len)
+{
 	buf[4] = type;
 
 	if (type == EAP_TYPE_EXPANDED) {
@@ -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:
+ * @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
+ * 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);
 }
 
 static void eap_complete_timeout(struct l_timeout *timeout, void *user_data)
diff --git a/src/simutil.c b/src/simutil.c
index eda31a86..8947ad66 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -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,
-- 
2.25.1

Re: [PATCH 01/16] wscutil: Handle a deprecated network key format

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

Hi Andrew,

On 8/12/20 7:50 PM, Andrew Zaborowski wrote:
> 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

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

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]

Hi Andrew,

On 8/12/20 7:50 PM, Andrew Zaborowski wrote:
> 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>

> @@ -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

> + * @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
> + * 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

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

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3282 bytes --]

Hi Andrew,

On 8/12/20 7:50 PM, Andrew Zaborowski wrote:
> 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(-)
> 
> diff --git a/src/eapol.c b/src/eapol.c
> index 299de13f..06b8a2f6 100644
> --- a/src/eapol.c
> +++ b/src/eapol.c
> @@ -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?

> +
> +		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>

> @@ -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;
> +			}

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...

> +		} 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);

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...

>   }
>   
>   bool eapol_start(struct eapol_sm *sm)
> 

Regards,
-Denis

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

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 4457 bytes --]

Hi Denis,

On Thu, 13 Aug 2020 at 18:34, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/12/20 7:50 PM, Andrew Zaborowski wrote:
> > 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(-)
> >
> > diff --git a/src/eapol.c b/src/eapol.c
> > index 299de13f..06b8a2f6 100644
> > --- a/src/eapol.c
> > +++ b/src/eapol.c
> > @@ -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?

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

>
> > +
> > +             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>
>
> > @@ -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;
> > +                     }
>
> 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...
>
> > +             } 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);
>
> 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?

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