[PATCH 01/11] wscutil: Add wsc_build_beacon

[PATCH 01/11] wscutil: Add wsc_build_beacon

[Andrew Zaborowski]

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

---
 src/wscutil.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/wscutil.h |  1 +
 2 files changed, 60 insertions(+)

diff --git a/src/wscutil.c b/src/wscutil.c
index d78a0354..f24e67ed 100644
--- a/src/wscutil.c
+++ b/src/wscutil.c
@@ -2124,6 +2124,65 @@ static void build_credential(struct wsc_attr_builder *builder,
 	l_free(data);
 }
 
+uint8_t *wsc_build_beacon(const struct wsc_beacon *beacon, size_t *out_len)
+{
+	struct wsc_attr_builder *builder;
+	uint8_t *ret;
+
+	builder = wsc_attr_builder_new(512);
+	build_version(builder, 0x10);
+	build_wsc_state(builder, beacon->state);
+
+	if (beacon->ap_setup_locked)
+		build_ap_setup_locked(builder, true);
+
+	if (beacon->selected_registrar) {
+		build_selected_registrar(builder, true);
+		build_device_password_id(builder, beacon->device_password_id);
+		build_selected_registrar_configuration_methods(builder,
+					beacon->selected_reg_config_methods);
+	}
+
+	/* These two "should be provided" if dual-band */
+	if (beacon->rf_bands & (beacon->rf_bands - 1)) {
+		if (beacon->selected_registrar)
+			build_uuid_e(builder, beacon->uuid_e);
+
+		build_rf_bands(builder, beacon->rf_bands);
+	}
+
+	if (!beacon->version2)
+		goto done;
+
+	START_WFA_VENDOR_EXTENSION();
+
+	if (!util_mem_is_zero(beacon->authorized_macs, 30)) {
+		int count;
+
+		for (count = 1; count < 5; count++)
+			if (util_mem_is_zero(beacon->authorized_macs + count * 6,
+						30 - count * 6))
+				break;
+
+		wsc_attr_builder_put_u8(builder,
+					WSC_WFA_EXTENSION_AUTHORIZED_MACS);
+		wsc_attr_builder_put_u8(builder, count * 6);
+		wsc_attr_builder_put_bytes(builder, beacon->authorized_macs,
+						count * 6);
+	}
+
+	if (beacon->reg_config_methods) {
+		wsc_attr_builder_put_u8(builder,
+			WSC_WFA_EXTENSION_REGISTRAR_CONFIGRATION_METHODS);
+		wsc_attr_builder_put_u8(builder, 2);
+		wsc_attr_builder_put_u16(builder, beacon->reg_config_methods);
+	}
+
+done:
+	ret = wsc_attr_builder_free(builder, false, out_len);
+	return ret;
+}
+
 uint8_t *wsc_build_probe_request(const struct wsc_probe_request *probe_request,
 							size_t *out_len)
 {
diff --git a/src/wscutil.h b/src/wscutil.h
index 45d72fbb..5bb26d84 100644
--- a/src/wscutil.h
+++ b/src/wscutil.h
@@ -604,6 +604,7 @@ int wsc_parse_wsc_done(const uint8_t *pdu, uint32_t len, struct wsc_done *out);
 
 uint8_t *wsc_build_credential(const struct wsc_credential *in, size_t *out_len);
 
+uint8_t *wsc_build_beacon(const struct wsc_beacon *beacon, size_t *out_len);
 uint8_t *wsc_build_probe_request(const struct wsc_probe_request *probe_request,
 				size_t *out_len);
 uint8_t *wsc_build_probe_response(
-- 
2.25.1

[PATCH 02/11] eap: Re-send Identity Request on EAPoL-Start

[Andrew Zaborowski]

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

It looks like clients sometimes miss our unsolicited Identity Request
and need a resend.
---
 src/eap.c   | 12 ++++++++++--
 src/eapol.c | 12 ++++++++----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/eap.c b/src/eap.c
index fcc18c77..dea0c3cf 100644
--- a/src/eap.c
+++ b/src/eap.c
@@ -245,10 +245,18 @@ void eap_start(struct eap_state *eap)
 {
 	uint8_t buf[5];
 
-	L_WARN_ON(!eap->method || !eap->authenticator || eap->identity);
+	L_WARN_ON(!eap->method || !eap->authenticator);
 
+	/*
+	 * Until we've received the Identity response we can resend the
+	 * Identity request with a constant ID on EAPoL-Start.
+	 */
+	if (eap->identity)
+		return;
+
+	eap->last_id = 1;
 	buf[4] = EAP_TYPE_IDENTITY;
-	eap_send_packet(eap, EAP_CODE_REQUEST, ++eap->last_id, buf, 5);
+	eap_send_packet(eap, EAP_CODE_REQUEST, eap->last_id, buf, 5);
 }
 
 void __eap_handle_request(struct eap_state *eap, uint16_t id,
diff --git a/src/eapol.c b/src/eapol.c
index 686187c6..2d339163 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -2239,14 +2239,18 @@ static void eapol_rx_auth_packet(uint16_t proto, const uint8_t *from,
 
 	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.
+		 * The supplicant may have sent an EAPoL-Start even before
+		 * we queued our EAP Identity Request or it may have missed our
+		 * early Identity Request and may need a retransmission.  Tell
+		 * sm->eap so it can decide whether to send a new Identity
+		 * Request or ignore this.
 		 *
 		 * TODO: if we're already past the full handshake, send a
 		 * new msg 1/4.
 		 */
+		if (sm->eap)
+			eap_start(sm->eap);
+
 		break;
 
 	case 3: /* EAPOL-Key */
-- 
2.25.1

[PATCH 03/11] eap-wsc: In WSC-R read UUID-E from settings

[Andrew Zaborowski]

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

The intent was to read the UUID-E from the settings rather than generate
it from the enrollee's MAC because it needs to match the UUID-E from
enrolee's Probe Requests, fix this.  The UUID-E supplied in the unit
test was being ignored but the test still passed because the supplied
UUID-E was generated the same way we generated it in eap-wsc.c.
---
 src/eap-wsc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/eap-wsc.c b/src/eap-wsc.c
index bfab9e25..5b87924a 100644
--- a/src/eap-wsc.c
+++ b/src/eap-wsc.c
@@ -1871,9 +1871,6 @@ static struct eap_wsc_state *eap_wsc_new_common(struct l_settings *settings,
 	if (!util_string_to_address(v, wsc->m1->addr))
 		goto err;
 
-	if (!wsc_uuid_from_addr(wsc->m1->addr, wsc->m1->uuid_e))
-		goto err;
-
 	if (!l_settings_get_uint(settings, "WSC", "ConfigurationMethods", &u32))
 		u32 = WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN;
 
@@ -1988,6 +1985,9 @@ static bool eap_wsc_load_settings(struct eap_state *eap,
 						WSC_ENCRYPTION_TYPE_AES_TKIP;
 	wsc->m1->connection_type_flags = WSC_CONNECTION_TYPE_ESS;
 
+	if (!wsc_uuid_from_addr(wsc->m1->addr, wsc->m1->uuid_e))
+		goto err;
+
 	if (!load_device_data(settings,
 				wsc->m1->manufacturer,
 				wsc->m1->model_name,
@@ -2041,6 +2041,9 @@ static bool eap_wsc_r_load_settings(struct eap_state *eap,
 	if (!wsc)
 		return false;
 
+	if (!load_hexencoded(settings, "UUID-E", wsc->m1->uuid_e, 16))
+		goto err;
+
 	if (!load_hexencoded(settings, "UUID-R", wsc->m2->uuid_r, 16))
 		goto err;
 
-- 
2.25.1

[PATCH 04/11] ap: Move AP parameters to a struct

[Andrew Zaborowski]

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

To limit the number of ap_start parameters, group basic AP config
parameters in the ap_config struct that is passed as a pointer and owned
by the ap_state.
---
 src/ap.c | 127 ++++++++++++++++++++++++++++++-------------------------
 src/ap.h |  15 +++++--
 2 files changed, 81 insertions(+), 61 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 51ee23a8..821423ac 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -53,10 +53,7 @@ struct ap_state {
 	ap_event_func_t event_func;
 	ap_stopped_func_t stopped_func;
 	void *user_data;
-	char *ssid;
-	uint8_t channel;
-	uint8_t *authorized_macs;
-	int authorized_macs_num;
+	struct ap_config *config;
 
 	unsigned int ciphers;
 	enum ie_rsn_cipher_suite group_cipher;
@@ -73,7 +70,6 @@ struct ap_state {
 
 	bool started : 1;
 	bool gtk_set : 1;
-	bool no_cck_rates : 1;
 };
 
 struct sta_state {
@@ -94,6 +90,22 @@ struct sta_state {
 
 static uint32_t netdev_watch;
 
+void ap_config_free(struct ap_config *config)
+{
+	if (unlikely(!config))
+		return;
+
+	l_free(config->ssid);
+
+	if (config->psk) {
+		explicit_bzero(config->psk, strlen(config->psk));
+		l_free(config->psk);
+	}
+
+	l_free(config->authorized_macs);
+	l_free(config);
+}
+
 static void ap_sta_free(void *data)
 {
 	struct sta_state *sta = data;
@@ -121,8 +133,6 @@ static void ap_reset(struct ap_state *ap)
 {
 	struct netdev *netdev = ap->netdev;
 
-	l_free(ap->ssid);
-
 	explicit_bzero(ap->pmk, sizeof(ap->pmk));
 
 	if (ap->mlme_watch)
@@ -138,7 +148,9 @@ static void ap_reset(struct ap_state *ap)
 	if (ap->rates)
 		l_uintset_free(ap->rates);
 
-	l_free(ap->authorized_macs);
+	ap_config_free(ap->config);
+	ap->config = NULL;
+
 	ap->started = false;
 }
 
@@ -318,7 +330,8 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 
 	/* SSID IE */
 	ie_tlv_builder_next(&builder, IE_TYPE_SSID);
-	ie_tlv_builder_set_data(&builder, ap->ssid, strlen(ap->ssid));
+	ie_tlv_builder_set_data(&builder, ap->config->ssid,
+				strlen(ap->config->ssid));
 
 	/* Supported Rates IE */
 	ie_tlv_builder_next(&builder, IE_TYPE_SUPPORTED_RATES);
@@ -343,7 +356,7 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 
 	/* DSSS Parameter Set IE for DSSS, HR, ERP and HT PHY rates */
 	ie_tlv_builder_next(&builder, IE_TYPE_DSSS_PARAMETER_SET);
-	ie_tlv_builder_set_data(&builder, &ap->channel, 1);
+	ie_tlv_builder_set_data(&builder, &ap->config->channel, 1);
 
 	ie_tlv_builder_finalize(&builder, &len);
 	return 36 + len;
@@ -375,7 +388,8 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 	struct l_genl_msg *msg;
 	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
 	uint32_t id;
-	uint32_t ch_freq = scan_channel_to_freq(ap->channel, SCAN_BAND_2_4_GHZ);
+	uint32_t ch_freq = scan_channel_to_freq(ap->config->channel,
+						SCAN_BAND_2_4_GHZ);
 
 	msg = l_genl_msg_new_sized(NL80211_CMD_FRAME, 128 + frame_len);
 
@@ -386,7 +400,7 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 		l_genl_msg_append_attr(msg, NL80211_ATTR_DONT_WAIT_FOR_ACK,
 					0, NULL);
 
-	if (ap->no_cck_rates)
+	if (ap->config->no_cck_rates)
 		l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0,
 					NULL);
 
@@ -446,7 +460,8 @@ static void ap_start_rsna(struct sta_state *sta, const uint8_t *gtk_rsc)
 	sta->hs = netdev_handshake_state_new(netdev);
 
 	handshake_state_set_event_func(sta->hs, ap_handshake_event, sta);
-	handshake_state_set_ssid(sta->hs, (void *)ap->ssid, strlen(ap->ssid));
+	handshake_state_set_ssid(sta->hs, (void *) ap->config->ssid,
+					strlen(ap->config->ssid));
 	handshake_state_set_authenticator(sta->hs, true);
 	handshake_state_set_authenticator_ie(sta->hs, bss_rsne);
 	handshake_state_set_supplicant_ie(sta->hs, sta->assoc_rsne);
@@ -900,8 +915,8 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 			break;
 		}
 
-	if (!rates || !ssid || !rsn || ssid_len != strlen(ap->ssid) ||
-			memcmp(ssid, ap->ssid, ssid_len)) {
+	if (!rates || !ssid || !rsn || ssid_len != strlen(ap->config->ssid) ||
+			memcmp(ssid, ap->config->ssid, ssid_len)) {
 		err = MMPDU_REASON_CODE_INVALID_IE;
 		goto bad_frame;
 	}
@@ -1125,8 +1140,8 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 
 	if (!ssid || ssid_len == 0) /* Wildcard SSID */
 		match = true;
-	else if (ssid && ssid_len == strlen(ap->ssid) && /* Specific SSID */
-			!memcmp(ssid, ap->ssid, ssid_len))
+	else if (ssid && ssid_len == strlen(ap->config->ssid) && /* One SSID */
+			!memcmp(ssid, ap->config->ssid, ssid_len))
 		match = true;
 	else if (ssid_list) { /* SSID List */
 		ie_tlv_iter_init(&iter, ssid_list, ssid_list_len);
@@ -1138,15 +1153,16 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 			ssid = (const char *) ie_tlv_iter_get_data(&iter);
 			ssid_len = ie_tlv_iter_get_length(&iter);
 
-			if (ssid_len == strlen(ap->ssid) &&
-					!memcmp(ssid, ap->ssid, ssid_len)) {
+			if (ssid_len == strlen(ap->config->ssid) &&
+					!memcmp(ssid, ap->config->ssid,
+						ssid_len)) {
 				match = true;
 				break;
 			}
 		}
 	}
 
-	if (dsss_channel != 0 && dsss_channel != ap->channel)
+	if (dsss_channel != 0 && dsss_channel != ap->config->channel)
 		match = false;
 
 	if (!match)
@@ -1247,14 +1263,15 @@ static void ap_auth_cb(const struct mmpdu_header *hdr, const void *body,
 			memcmp(hdr->address_3, bssid, 6))
 		return;
 
-	if (ap->authorized_macs) {
-		int i;
+	if (ap->config->authorized_macs_num) {
+		unsigned int i;
 
-		for (i = 0; i < ap->authorized_macs_num; i++)
-			if (!memcmp(from, ap->authorized_macs + i * 6, 6))
+		for (i = 0; i < ap->config->authorized_macs_num; i++)
+			if (!memcmp(from, ap->config->authorized_macs + i * 6,
+					6))
 				break;
 
-		if (i == ap->authorized_macs_num) {
+		if (i == ap->config->authorized_macs_num) {
 			ap_auth_reply(ap, from, MMPDU_REASON_CODE_UNSPECIFIED);
 			return;
 		}
@@ -1374,7 +1391,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	uint32_t nl_akm = CRYPTO_AKM_PSK;
 	uint32_t wpa_version = NL80211_WPA_VERSION_2;
 	uint32_t auth_type = NL80211_AUTHTYPE_OPEN_SYSTEM;
-	uint32_t ch_freq = scan_channel_to_freq(ap->channel, SCAN_BAND_2_4_GHZ);
+	uint32_t ch_freq = scan_channel_to_freq(ap->config->channel,
+						SCAN_BAND_2_4_GHZ);
 	uint32_t ch_width = NL80211_CHAN_WIDTH_20;
 
 	static const uint8_t bcast_addr[6] = {
@@ -1389,7 +1407,7 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 		return NULL;
 
 	cmd = l_genl_msg_new_sized(NL80211_CMD_START_AP, 256 + head_len +
-					tail_len + strlen(ap->ssid));
+					tail_len + strlen(ap->config->ssid));
 
 	/* SET_BEACON attrs */
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_BEACON_HEAD, head_len, head);
@@ -1403,8 +1421,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 				&ap->beacon_interval);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_DTIM_PERIOD, 4, &dtim_period);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_IFINDEX, 4, &ifindex);
-	l_genl_msg_append_attr(cmd, NL80211_ATTR_SSID, strlen(ap->ssid),
-				ap->ssid);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_SSID, strlen(ap->config->ssid),
+				ap->config->ssid);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_HIDDEN_SSID, 4,
 				&hidden_ssid);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_CIPHER_SUITES_PAIRWISE, 4,
@@ -1462,11 +1480,14 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
  * @event_func is required and must react to AP_EVENT_START_FAILED
  * and AP_EVENT_STOPPING by forgetting the ap_state struct, which
  * is going to be freed automatically.
- * @channel is optional.
+ * In the @config struct only .ssid and .psk need to be non-NUL,
+ * other fields are optional.  If @ap_start succeeds, the returned
+ * ap_state takes ownership of @config and the caller shouldn't
+ * free it or any of the memory pointed to by its members (they
+ * also can't be static).  In principle there's no reason that the
+ * caller shouldn't read/update some of the members though.
  */
-struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
-				const char *psk, int channel, bool no_cck_rates,
-				const uint8_t **authorized_macs,
+struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 				ap_event_func_t event_func, void *user_data)
 {
 	struct ap_state *ap;
@@ -1476,30 +1497,14 @@ struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
 
 	ap = l_new(struct ap_state, 1);
 	ap->nl80211 = l_genl_family_new(iwd_get_genl(), NL80211_GENL_NAME);
-	ap->ssid = l_strdup(ssid);
+	ap->config = config;
 	ap->netdev = netdev;
-	ap->no_cck_rates = no_cck_rates;
 	ap->event_func = event_func;
 	ap->user_data = user_data;
 
-	if (channel)
-		ap->channel = channel;
-	else {
+	if (!config->channel)
 		/* TODO: Start a Get Survey to decide the channel */
-		ap->channel = 6;
-	}
-
-	if (authorized_macs) {
-		int i;
-
-		for (i = 0; authorized_macs[i]; i++);
-		ap->authorized_macs = l_malloc(i * 6);
-		ap->authorized_macs_num = i;
-
-		for (i = 0; authorized_macs[i]; i++)
-			memcpy(ap->authorized_macs + i * 6, authorized_macs[i],
-				6);
-	}
+		config->channel = 6;
 
 	/* TODO: Add all ciphers supported by wiphy */
 	ap->ciphers = wiphy_select_cipher(wiphy, 0xffff);
@@ -1507,7 +1512,7 @@ struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
 	ap->beacon_interval = 100;
 
 	/* TODO: Pick from actual supported rates */
-	if (no_cck_rates) {
+	if (config->no_cck_rates) {
 		l_uintset_put(ap->rates, 12); /* 6 Mbps*/
 		l_uintset_put(ap->rates, 18); /* 9 Mbps*/
 		l_uintset_put(ap->rates, 24); /* 12 Mbps*/
@@ -1523,8 +1528,8 @@ struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
 		l_uintset_put(ap->rates, 22); /* 11 Mbps*/
 	}
 
-	if (crypto_psk_from_passphrase(psk, (uint8_t *) ssid, strlen(ssid),
-					ap->pmk) < 0)
+	if (crypto_psk_from_passphrase(config->psk, (uint8_t *) config->ssid,
+					strlen(config->ssid), ap->pmk) < 0)
 		goto error;
 
 	if (!frame_watch_add(wdev_id, 0, 0x0000 |
@@ -1757,6 +1762,7 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 {
 	struct ap_if_data *ap_if = user_data;
 	const char *ssid, *wpa2_psk;
+	struct ap_config *config;
 
 	if (ap_if->ap && ap_if->ap->started)
 		return dbus_error_already_exists(message);
@@ -1767,10 +1773,15 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 	if (!l_dbus_message_get_arguments(message, "ss", &ssid, &wpa2_psk))
 		return dbus_error_invalid_args(message);
 
-	ap_if->ap = ap_start(ap_if->netdev, ssid, wpa2_psk, 0, false, NULL,
-				ap_if_event_func, ap_if);
-	if (!ap_if->ap)
+	config = l_new(struct ap_config, 1);
+	config->ssid = l_strdup(ssid);
+	config->psk = l_strdup(wpa2_psk);
+
+	ap_if->ap = ap_start(ap_if->netdev, config, ap_if_event_func, ap_if);
+	if (!ap_if->ap) {
+		ap_config_free(config);
 		return dbus_error_invalid_args(message);
+	}
 
 	ap_if->pending = l_dbus_message_ref(message);
 	return NULL;
diff --git a/src/ap.h b/src/ap.h
index b007b657..330ae011 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -45,9 +45,18 @@ typedef void (*ap_event_func_t)(enum ap_event_type type, const void *event_data,
 				void *user_data);
 typedef void (*ap_stopped_func_t)(void *user_data);
 
-struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
-				const char *psk, int channel, bool no_cck_rates,
-				const uint8_t **authorized_macs,
+struct ap_config {
+	char *ssid;
+	char *psk;
+	uint8_t channel;
+	uint8_t *authorized_macs;
+	unsigned int authorized_macs_num;
+	bool no_cck_rates : 1;
+};
+
+void ap_config_free(struct ap_config *config);
+
+struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 				ap_event_func_t event_func, void *user_data);
 void ap_shutdown(struct ap_state *ap, ap_stopped_func_t stopped_func,
 			void *user_data);
-- 
2.25.1

[PATCH 05/11] ap: Drop unused variable

[Andrew Zaborowski]

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

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

diff --git a/src/ap.c b/src/ap.c
index 821423ac..1267e254 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -443,12 +443,9 @@ static void ap_start_rsna(struct sta_state *sta, const uint8_t *gtk_rsc)
 	struct ap_state *ap = sta->ap;
 	struct netdev *netdev = sta->ap->netdev;
 	const uint8_t *own_addr = netdev_get_address(netdev);
-	struct scan_bss bss;
 	struct ie_rsn_info rsn;
 	uint8_t bss_rsne[24];
 
-	memset(&bss, 0, sizeof(bss));
-
 	ap_set_rsn_info(ap, &rsn);
 	/*
 	 * TODO: This assumes the length that ap_set_rsn_info() requires. If
-- 
2.25.1

[PATCH 06/11] ap: Fix incoming Probe Request BSSID check

[Andrew Zaborowski]

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

Setting 'match' false wouldn't do anything because it was already false.
If the frame is addressed to some other non-broadcast address ignore it
directly and exit ap_probe_req_cb.
---
 src/ap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 1267e254..aef2e1dd 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -1129,11 +1129,11 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 
 	if (memcmp(hdr->address_1, bssid, 6) &&
 			!util_is_broadcast_address(hdr->address_1))
-		match = false;
+		return;
 
 	if (memcmp(hdr->address_3, bssid, 6) &&
 			!util_is_broadcast_address(hdr->address_3))
-		match = false;
+		return;
 
 	if (!ssid || ssid_len == 0) /* Wildcard SSID */
 		match = true;
-- 
2.25.1

[PATCH 07/11] ap: Stop ongoing handshake on reassociation

[Andrew Zaborowski]

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

On a new association or re-association, in addition to forgetting a
complete RSN Association, also stop the EAPoL SM to stop any ongoing
handshake.

Do this in a new function ap_stop_handshake that is now used in a few
places that had copies of the same few lines.  I'll be adding some more
lines to this function for WSC support.
---
 src/ap.c | 51 +++++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index aef2e1dd..5cd2b717 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -106,6 +106,19 @@ void ap_config_free(struct ap_config *config)
 	l_free(config);
 }
 
+static void ap_stop_handshake(struct sta_state *sta)
+{
+	if (sta->sm) {
+		eapol_sm_free(sta->sm);
+		sta->sm = NULL;
+	}
+
+	if (sta->hs) {
+		handshake_state_free(sta->hs);
+		sta->hs = NULL;
+	}
+}
+
 static void ap_sta_free(void *data)
 {
 	struct sta_state *sta = data;
@@ -120,11 +133,7 @@ static void ap_sta_free(void *data)
 	if (sta->gtk_query_cmd_id)
 		l_genl_family_cancel(ap->nl80211, sta->gtk_query_cmd_id);
 
-	if (sta->sm)
-		eapol_sm_free(sta->sm);
-
-	if (sta->hs)
-		handshake_state_free(sta->hs);
+	ap_stop_handshake(sta);
 
 	l_free(sta);
 }
@@ -180,14 +189,7 @@ static void ap_del_station(struct sta_state *sta, uint16_t reason,
 		sta->gtk_query_cmd_id = 0;
 	}
 
-	if (sta->sm)
-		eapol_sm_free(sta->sm);
-
-	if (sta->hs)
-		handshake_state_free(sta->hs);
-
-	sta->hs = NULL;
-	sta->sm = NULL;
+	ap_stop_handshake(sta);
 
 	if (send_event)
 		ap->event_func(AP_EVENT_STATION_REMOVED, &event_data,
@@ -268,14 +270,7 @@ static void ap_drop_rsna(struct sta_state *sta)
 		l_error("Issuing DEL_KEY failed");
 	}
 
-	if (sta->sm)
-		eapol_sm_free(sta->sm);
-
-	if (sta->hs)
-		handshake_state_free(sta->hs);
-
-	sta->hs = NULL;
-	sta->sm = NULL;
+	ap_stop_handshake(sta);
 
 	if (ap->event_func) {
 		struct ap_event_station_removed_data event_data = {};
@@ -938,6 +933,12 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 		goto unsupported;
 	}
 
+	/* 802.11-2016 11.3.5.3 j) */
+	if (sta->rsna)
+		ap_drop_rsna(sta);
+
+	ap_stop_handshake(sta);
+
 	if (!sta->associated) {
 		/*
 		 * Everything fine so far, assign an AID, send response.
@@ -961,10 +962,6 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 
 	sta->assoc_rsne = l_memdup(rsn, rsn[1] + 2);
 
-	/* 802.11-2016 11.3.5.3 j) */
-	if (sta->rsna)
-		ap_drop_rsna(sta);
-
 	sta->assoc_resp_cmd_id = ap_assoc_resp(ap, sta, sta->addr, sta->aid, 0,
 						reassoc,
 						ap_success_assoc_resp_cb);
@@ -987,8 +984,10 @@ bad_frame:
 	 *
 	 * For now, we need to drop the RSNA.
 	 */
-	if (sta->associated && sta->rsna)
+	if (sta->associated && sta->rsna) {
 		ap_drop_rsna(sta);
+		ap_stop_handshake(sta);
+	}
 
 	if (rates)
 		l_uintset_free(rates);
-- 
2.25.1

[PATCH 08/11] ap: Push Button mode API and beacon changes

[Andrew Zaborowski]

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

This adds the API for putting the AP in Push Button mode, which we'll
need to P2P GO side but may be useful on its own too.  A WSC IE is added
to our beacons and probe responses indicating whether the PBC mode is
active.
---
 src/ap.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/ap.h |  15 ++++
 2 files changed, 219 insertions(+), 3 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 5cd2b717..b1e57a23 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -45,6 +45,7 @@
 #include "src/dbus.h"
 #include "src/nl80211util.h"
 #include "src/frame-xchg.h"
+#include "src/wscutil.h"
 #include "src/ap.h"
 
 struct ap_state {
@@ -64,6 +65,9 @@ struct ap_state {
 	uint32_t mlme_watch;
 	uint8_t gtk[CRYPTO_MAX_GTK_LEN];
 	uint8_t gtk_index;
+	struct l_timeout *wsc_pbc_timeout;
+	uint16_t wsc_dpid;
+	uint8_t wsc_uuid_r[16];
 
 	uint16_t last_aid;
 	struct l_queue *sta_states;
@@ -103,6 +107,7 @@ void ap_config_free(struct ap_config *config)
 	}
 
 	l_free(config->authorized_macs);
+	l_free(config->wsc_name);
 	l_free(config);
 }
 
@@ -358,10 +363,15 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 }
 
 /* Beacon / Probe Response frame portion after the TIM IE */
-static size_t ap_build_beacon_pr_tail(struct ap_state *ap, uint8_t *out_buf)
+static size_t ap_build_beacon_pr_tail(struct ap_state *ap, bool pr,
+					uint8_t *out_buf)
 {
 	size_t len;
 	struct ie_rsn_info rsn;
+	uint8_t *wsc_data;
+	size_t wsc_data_size;
+	uint8_t *wsc_ie;
+	size_t wsc_ie_size;
 
 	/* TODO: Country IE between TIM IE and RSNE */
 
@@ -371,9 +381,142 @@ static size_t ap_build_beacon_pr_tail(struct ap_state *ap, uint8_t *out_buf)
 		return 0;
 	len = 2 + out_buf[1];
 
+	/* WSC IE */
+	if (pr) {
+		struct wsc_probe_response wsc_pr = {};
+
+		wsc_pr.state = WSC_STATE_CONFIGURED;
+
+		if (ap->wsc_pbc_timeout) {
+			wsc_pr.selected_registrar = true;
+			wsc_pr.device_password_id = ap->wsc_dpid;
+			wsc_pr.selected_reg_config_methods =
+				WSC_CONFIGURATION_METHOD_PUSH_BUTTON;
+		}
+
+		wsc_pr.response_type = WSC_RESPONSE_TYPE_AP;
+		memcpy(wsc_pr.uuid_e, ap->wsc_uuid_r, sizeof(wsc_pr.uuid_e));
+		wsc_pr.primary_device_type =
+			ap->config->wsc_primary_device_type;
+
+		if (ap->config->wsc_name)
+			l_strlcpy(wsc_pr.device_name, ap->config->wsc_name,
+					sizeof(wsc_pr.device_name));
+
+		wsc_pr.config_methods =
+			WSC_CONFIGURATION_METHOD_PUSH_BUTTON;
+
+		if (ap->config->authorized_macs_num) {
+			size_t len;
+
+			wsc_pr.version2 = true;
+
+			len = ap->config->authorized_macs_num * 6;
+			if (len > sizeof(wsc_pr.authorized_macs))
+				len = sizeof(wsc_pr.authorized_macs);
+
+			memcpy(wsc_pr.authorized_macs,
+				ap->config->authorized_macs, len);
+		}
+
+		wsc_data = wsc_build_probe_response(&wsc_pr, &wsc_data_size);
+	} else {
+		struct wsc_beacon wsc_beacon = {};
+
+		wsc_beacon.state = WSC_STATE_CONFIGURED;
+
+		if (ap->wsc_pbc_timeout) {
+			wsc_beacon.selected_registrar = true;
+			wsc_beacon.device_password_id = ap->wsc_dpid;
+			wsc_beacon.selected_reg_config_methods =
+				WSC_CONFIGURATION_METHOD_PUSH_BUTTON;
+		}
+
+		if (ap->config->authorized_macs_num) {
+			size_t len;
+
+			wsc_beacon.version2 = true;
+
+			len = ap->config->authorized_macs_num * 6;
+			if (len > sizeof(wsc_beacon.authorized_macs))
+				len = sizeof(wsc_beacon.authorized_macs);
+
+			memcpy(wsc_beacon.authorized_macs,
+				ap->config->authorized_macs, len);
+		}
+
+		wsc_data = wsc_build_beacon(&wsc_beacon, &wsc_data_size);
+	}
+
+	if (!wsc_data)
+		return 0;
+
+	wsc_ie = ie_tlv_encapsulate_wsc_payload(wsc_data, wsc_data_size,
+						&wsc_ie_size);
+	l_free(wsc_data);
+
+	if (!wsc_ie)
+		return 0;
+
+	memcpy(out_buf + len, wsc_ie, wsc_ie_size);
+	len += wsc_ie_size;
+	l_free(wsc_ie);
+
 	return len;
 }
 
+static void ap_set_beacon_cb(struct l_genl_msg *msg, void *user_data)
+{
+	int error = l_genl_msg_get_error(msg);
+
+	if (error < 0)
+		l_error("SET_BEACON failed: %s (%i)", strerror(-error), -error);
+}
+
+static void ap_update_beacon(struct ap_state *ap)
+{
+	struct l_genl_msg *cmd;
+	uint8_t head[256], tail[256];
+	size_t head_len, tail_len;
+	uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
+	static const uint8_t bcast_addr[6] = {
+		0xff, 0xff, 0xff, 0xff, 0xff, 0xff
+	};
+
+	head_len = ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
+						bcast_addr, head, sizeof(head));
+	tail_len = ap_build_beacon_pr_tail(ap, false, tail);
+	if (L_WARN_ON(!head_len || !tail_len))
+		return;
+
+	cmd = l_genl_msg_new_sized(NL80211_CMD_SET_BEACON,
+					32 + head_len + tail_len);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_WDEV, 8, &wdev_id);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_BEACON_HEAD, head_len, head);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_BEACON_TAIL, tail_len, tail);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_IE, 0, "");
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_PROBE_RESP, 0, "");
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_ASSOC_RESP, 0, "");
+
+	if (l_genl_family_send(ap->nl80211, cmd, ap_set_beacon_cb, NULL, NULL))
+		return;
+
+	l_genl_msg_unref(cmd);
+	l_error("Issuing SET_BEACON failed");
+}
+
+static void ap_wsc_exit_pbc(struct ap_state *ap)
+{
+	if (!ap->wsc_pbc_timeout)
+		return;
+
+	l_timeout_remove(ap->wsc_pbc_timeout);
+	ap->wsc_dpid = 0;
+	ap_update_beacon(ap);
+
+	ap->event_func(AP_EVENT_PBC_MODE_EXIT, NULL, ap->user_data);
+}
+
 static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 					const struct mmpdu_header *frame,
 					size_t frame_len, bool wait_ack,
@@ -1069,6 +1212,9 @@ bad_frame:
 		l_error("Sending error Reassociation Response failed");
 }
 
+#define AP_WSC_PBC_MONITOR_TIME	120
+#define AP_WSC_PBC_WALK_TIME	120
+
 static void ap_probe_resp_cb(struct l_genl_msg *msg, void *user_data)
 {
 	if (l_genl_msg_get_error(msg) < 0)
@@ -1167,7 +1313,7 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 	len = ap_build_beacon_pr_head(ap,
 					MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE,
 					hdr->address_2, resp, sizeof(resp));
-	len += ap_build_beacon_pr_tail(ap, resp + len);
+	len += ap_build_beacon_pr_tail(ap, true, resp + len);
 
 	ap_send_mgmt_frame(ap, (struct mmpdu_header *) resp, len, false,
 				ap_probe_resp_cb, NULL);
@@ -1397,7 +1543,7 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 
 	head_len = ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
 						bcast_addr, head, sizeof(head));
-	tail_len = ap_build_beacon_pr_tail(ap, tail);
+	tail_len = ap_build_beacon_pr_tail(ap, false, tail);
 
 	if (!head_len || !tail_len)
 		return NULL;
@@ -1502,6 +1648,17 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 		/* TODO: Start a Get Survey to decide the channel */
 		config->channel = 6;
 
+	if (!config->wsc_name)
+		config->wsc_name = l_strdup(config->ssid);
+
+	if (!config->wsc_primary_device_type.category) {
+		/* Make ourselves a WFA standard PC by default */
+		config->wsc_primary_device_type.category = 1;
+		memcpy(config->wsc_primary_device_type.oui, wsc_wfa_oui, 3);
+		config->wsc_primary_device_type.oui_type = 0x04;
+		config->wsc_primary_device_type.subcategory = 1;
+	}
+
 	/* TODO: Add all ciphers supported by wiphy */
 	ap->ciphers = wiphy_select_cipher(wiphy, 0xffff);
 	ap->group_cipher = wiphy_select_cipher(wiphy, 0xffff);
@@ -1524,6 +1681,8 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 		l_uintset_put(ap->rates, 22); /* 11 Mbps*/
 	}
 
+	wsc_uuid_from_addr(netdev_get_address(netdev), ap->wsc_uuid_r);
+
 	if (crypto_psk_from_passphrase(config->psk, (uint8_t *) config->ssid,
 					strlen(config->ssid), ap->pmk) < 0)
 		goto error;
@@ -1703,6 +1862,45 @@ bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
 	return true;
 }
 
+static void ap_wsc_pbc_timeout_cb(struct l_timeout *timeout, void *user_data)
+{
+	struct ap_state *ap = user_data;
+
+	l_debug("PBC mode timeout");
+	ap_wsc_exit_pbc(ap);
+}
+
+static void ap_wsc_pbc_timeout_destroy(void *user_data)
+{
+	struct ap_state *ap = user_data;
+
+	ap->wsc_pbc_timeout = NULL;
+}
+
+bool ap_push_button(struct ap_state *ap)
+{
+	if (!ap->started)
+		return false;
+
+	/*
+	 * WSC v2.0.5 Section 11.3: "Multiple presses of the button are
+	 * permitted.  If a PBC button on an Enrollee or Registrar is
+	 * pressed again during Walk Time, the timers for that device are
+	 * restarted at that time [...]"
+	 */
+	if (ap->wsc_pbc_timeout) {
+		l_timeout_modify(ap->wsc_pbc_timeout, AP_WSC_PBC_WALK_TIME);
+		return true;
+	}
+
+	ap->wsc_pbc_timeout = l_timeout_create(AP_WSC_PBC_WALK_TIME,
+						ap_wsc_pbc_timeout_cb, ap,
+						ap_wsc_pbc_timeout_destroy);
+	ap->wsc_dpid = WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON;
+	ap_update_beacon(ap);
+	return true;
+}
+
 struct ap_if_data {
 	struct netdev *netdev;
 	struct ap_state *ap;
@@ -1748,6 +1946,9 @@ static void ap_if_event_func(enum ap_event_type type, const void *event_data,
 
 	case AP_EVENT_STATION_ADDED:
 	case AP_EVENT_STATION_REMOVED:
+	case AP_EVENT_REGISTRATION_START:
+	case AP_EVENT_REGISTRATION_SUCCESS:
+	case AP_EVENT_PBC_MODE_EXIT:
 		/* Ignored */
 		break;
 	}
diff --git a/src/ap.h b/src/ap.h
index 330ae011..4351fdc5 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -29,6 +29,9 @@ enum ap_event_type {
 	AP_EVENT_STOPPING,
 	AP_EVENT_STATION_ADDED,
 	AP_EVENT_STATION_REMOVED,
+	AP_EVENT_REGISTRATION_START,
+	AP_EVENT_REGISTRATION_SUCCESS,
+	AP_EVENT_PBC_MODE_EXIT,
 };
 
 struct ap_event_station_added_data {
@@ -41,6 +44,14 @@ struct ap_event_station_removed_data {
 	enum mmpdu_reason_code reason;
 };
 
+struct ap_event_registration_started_data {
+	const uint8_t *mac;
+};
+
+struct ap_event_registration_success_data {
+	const uint8_t *mac;
+};
+
 typedef void (*ap_event_func_t)(enum ap_event_type type, const void *event_data,
 				void *user_data);
 typedef void (*ap_stopped_func_t)(void *user_data);
@@ -51,6 +62,8 @@ struct ap_config {
 	uint8_t channel;
 	uint8_t *authorized_macs;
 	unsigned int authorized_macs_num;
+	char *wsc_name;
+	struct wsc_primary_device_type wsc_primary_device_type;
 	bool no_cck_rates : 1;
 };
 
@@ -64,3 +77,5 @@ void ap_free(struct ap_state *ap);
 
 bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
 				enum mmpdu_reason_code reason);
+
+bool ap_push_button(struct ap_state *ap);
-- 
2.25.1

[PATCH 09/11] ap: WSC Probe Request processing logic

[Andrew Zaborowski]

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

Implement the caching of WSC probe requests -- when an Enrollee later
associates to start registration we need to have its Probe Request on
file.  Also use this cache for PBC "Session Overlap" detection.
---
 src/ap.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/src/ap.c b/src/ap.c
index b1e57a23..df29a2fb 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -65,6 +65,7 @@ struct ap_state {
 	uint32_t mlme_watch;
 	uint8_t gtk[CRYPTO_MAX_GTK_LEN];
 	uint8_t gtk_index;
+	struct l_queue *wsc_pbc_probes;
 	struct l_timeout *wsc_pbc_timeout;
 	uint16_t wsc_dpid;
 	uint8_t wsc_uuid_r[16];
@@ -92,6 +93,12 @@ struct sta_state {
 	uint32_t gtk_query_cmd_id;
 };
 
+struct ap_wsc_pbc_probe_record {
+	uint8_t mac[6];
+	uint8_t uuid_e[16];
+	uint64_t timestamp;
+};
+
 static uint32_t netdev_watch;
 
 void ap_config_free(struct ap_config *config)
@@ -165,6 +172,8 @@ static void ap_reset(struct ap_state *ap)
 	ap_config_free(ap->config);
 	ap->config = NULL;
 
+	l_queue_destroy(ap->wsc_pbc_probes, l_free);
+
 	ap->started = false;
 }
 
@@ -645,6 +654,24 @@ error:
 	ap_del_station(sta, MMPDU_REASON_CODE_UNSPECIFIED, true);
 }
 
+struct ap_pbc_record_expiry_data {
+	uint64_t min_time;
+	const uint8_t *mac;
+};
+
+static bool ap_wsc_pbc_record_expire(void *data, void *user_data)
+{
+	struct ap_wsc_pbc_probe_record *record = data;
+	const struct ap_pbc_record_expiry_data *expiry_data = user_data;
+
+	if (record->timestamp > expiry_data->min_time &&
+			memcmp(record->mac, expiry_data->mac, 6))
+		return false;
+
+	l_free(record);
+	return true;
+}
+
 static struct l_genl_msg *ap_build_cmd_del_key(struct ap_state *ap)
 {
 	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
@@ -1215,6 +1242,115 @@ bad_frame:
 #define AP_WSC_PBC_MONITOR_TIME	120
 #define AP_WSC_PBC_WALK_TIME	120
 
+static void ap_process_wsc_probe_req(struct ap_state *ap, const uint8_t *from,
+					const uint8_t *wsc_data,
+					size_t wsc_data_len)
+{
+	struct wsc_probe_request req;
+	struct ap_pbc_record_expiry_data expiry_data;
+	struct ap_wsc_pbc_probe_record *record;
+	uint64_t now;
+	bool empty;
+	uint8_t first_sta_addr[6] = {};
+	const struct l_queue_entry *entry;
+
+	if (wsc_parse_probe_request(wsc_data, wsc_data_len, &req) < 0)
+		return;
+
+	if (!(req.config_methods & WSC_CONFIGURATION_METHOD_PUSH_BUTTON))
+		return;
+
+	if (req.device_password_id != WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON)
+		return;
+
+	/* Save the address of the first enrollee record */
+	record = l_queue_peek_head(ap->wsc_pbc_probes);
+	if (record)
+		memcpy(first_sta_addr, record->mac, 6);
+
+	now = l_time_now();
+
+	/*
+	 * Expire entries older than PBC Monitor Time.  While there also drop
+	 * older entries from the same Enrollee that sent us this new Probe
+	 * Request.  It's unclear whether we should also match by the UUID-E.
+	 */
+	expiry_data.min_time = now - AP_WSC_PBC_MONITOR_TIME * 1000000;
+	expiry_data.mac = from;
+	l_queue_foreach_remove(ap->wsc_pbc_probes, ap_wsc_pbc_record_expire,
+				&expiry_data);
+
+	empty = l_queue_isempty(ap->wsc_pbc_probes);
+
+	if (!ap->wsc_pbc_probes)
+		ap->wsc_pbc_probes = l_queue_new();
+
+	/* Add new record */
+	record = l_new(struct ap_wsc_pbc_probe_record, 1);
+	memcpy(record->mac, from, 6);
+	memcpy(record->uuid_e, req.uuid_e, sizeof(record->uuid_e));
+	record->timestamp = now;
+	l_queue_push_tail(ap->wsc_pbc_probes, record);
+
+	/*
+	 * If queue was non-empty and we've added one more record then we
+	 * now have seen more than one PBC enrollee during the PBC Monitor
+	 * Time and must exit "active PBC mode" due to "session overlap".
+	 * WSC v2.0.5 Section 11.3:
+	 * "Within the PBC Monitor Time, if the Registrar receives PBC
+	 * probe requests from more than one Enrollee [...] then the
+	 * Registrar SHALL signal a "session overlap" error.  As a result,
+	 * the Registrar shall refuse to enter active PBC mode and shall
+	 * also refuse to perform a PBC-based Registration Protocol
+	 * exchange [...]"
+	 */
+	if (empty)
+		return;
+
+	if (ap->wsc_pbc_timeout) {
+		l_debug("Exiting PBC mode due to Session Overlap");
+		ap_wsc_exit_pbc(ap);
+	}
+
+	/*
+	 * "If the Registrar is engaged in PBC Registration Protocol
+	 * exchange with an Enrollee and receives a Probe Request or M1
+	 * Message from another Enrollee, then the Registrar should
+	 * signal a "session overlap" error".
+	 *
+	 * For simplicity just interrupt the handshake with that enrollee.
+	 */
+	for (entry = l_queue_get_entries(ap->sta_states); entry;
+			entry = entry->next) {
+		struct sta_state *sta = entry->data;
+
+		if (!sta->associated || sta->assoc_rsne)
+			continue;
+
+		/*
+		 * Check whether this enrollee is in PBC Registration
+		 * Protocol by comparing its mac with the first (and only)
+		 * record we had in ap->wsc_pbc_probes.  If we had more
+		 * than one record we wouldn't have been in
+		 * "active PBC mode".
+		 */
+		if (memcmp(sta->addr, first_sta_addr, 6) ||
+				!memcmp(sta->addr, from, 6))
+			continue;
+
+		l_debug("Interrupting handshake with %s due to Session Overlap",
+			util_address_to_string(sta->addr));
+
+		if (sta->hs) {
+			netdev_handshake_failed(sta->hs,
+					MMPDU_REASON_CODE_DISASSOC_AP_BUSY);
+			sta->sm = NULL;
+		}
+
+		ap_remove_sta(sta);
+	}
+}
+
 static void ap_probe_resp_cb(struct l_genl_msg *msg, void *user_data)
 {
 	if (l_genl_msg_get_error(msg) < 0)
@@ -1241,6 +1377,8 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 	const uint8_t *bssid = netdev_get_address(ap->netdev);
 	bool match = false;
 	uint8_t resp[512];
+	uint8_t *wsc_data;
+	ssize_t wsc_data_len;
 
 	l_info("AP Probe Request from %s",
 		util_address_to_string(hdr->address_2));
@@ -1310,6 +1448,18 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 	if (!match)
 		return;
 
+	/*
+	 * Process the WSC IE first as it may cause us to exit "active PBC
+	 * mode" and that can be immediately reflected in our Probe Response.
+	 */
+	wsc_data = ie_tlv_extract_wsc_payload(req->ies, body_len - sizeof(*req),
+						&wsc_data_len);
+	if (wsc_data) {
+		ap_process_wsc_probe_req(ap, hdr->address_2,
+						wsc_data, wsc_data_len);
+		l_free(wsc_data);
+	}
+
 	len = ap_build_beacon_pr_head(ap,
 					MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE,
 					hdr->address_2, resp, sizeof(resp));
@@ -1882,6 +2032,11 @@ bool ap_push_button(struct ap_state *ap)
 	if (!ap->started)
 		return false;
 
+	if (l_queue_length(ap->wsc_pbc_probes) > 1) {
+		l_debug("Can't start PBC mode due to Session Overlap");
+		return false;
+	}
+
 	/*
 	 * WSC v2.0.5 Section 11.3: "Multiple presses of the button are
 	 * permitted.  If a PBC button on an Enrollee or Registrar is
-- 
2.25.1

[PATCH 10/11] ap: Parse WSC PBC association request and build response

[Andrew Zaborowski]

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

Check the conditions for PBC enrollee registration when we receive the
Association Request with WSC IE and indicate to the enrollee whether we
accept the association using a WSC IE in the Association Response.
After this, a NULL sta->assoc_rsne indicates that the station is not
establishing the RSNA and is a WSC enrollee.
---
 src/ap.c | 183 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 148 insertions(+), 35 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index df29a2fb..0876dc01 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -91,6 +91,7 @@ struct sta_state {
 	struct eapol_sm *sm;
 	struct handshake_state *hs;
 	uint32_t gtk_query_cmd_id;
+	uint8_t wsc_uuid_e[16];
 };
 
 struct ap_wsc_pbc_probe_record {
@@ -908,7 +909,7 @@ static void ap_fail_assoc_resp_cb(struct l_genl_msg *msg, void *user_data)
 }
 
 static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
-				const uint8_t *dest, uint16_t aid,
+				const uint8_t *dest,
 				enum mmpdu_reason_code status_code,
 				bool reassoc, l_genl_msg_func_t callback)
 {
@@ -936,7 +937,7 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 	resp = (void *) mmpdu_body(mpdu);
 	l_put_le16(capability, &resp->capability);
 	resp->status_code = L_CPU_TO_LE16(status_code);
-	resp->aid = L_CPU_TO_LE16(aid | 0xc000);
+	resp->aid = sta ? L_CPU_TO_LE16(sta->aid | 0xc000) : 0;
 
 	/* Supported Rates IE */
 	resp->ies[ies_len++] = IE_TYPE_SUPPORTED_RATES;
@@ -958,6 +959,37 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 	resp->ies[ies_len++] = count;
 	ies_len += count;
 
+	if (sta && !sta->assoc_rsne) {
+		struct wsc_association_response wsc_resp = {};
+		uint8_t *wsc_data;
+		size_t wsc_data_len;
+		uint8_t *wsc_ie;
+		size_t wsc_ie_len;
+
+		wsc_resp.response_type = WSC_RESPONSE_TYPE_AP;
+
+		wsc_data = wsc_build_association_response(&wsc_resp,
+								&wsc_data_len);
+		if (!wsc_data) {
+			l_error("wsc_build_beacon error");
+			goto send_frame;
+		}
+
+		wsc_ie = ie_tlv_encapsulate_wsc_payload(wsc_data, wsc_data_len,
+							&wsc_ie_len);
+		l_free(wsc_data);
+
+		if (!wsc_ie) {
+			l_error("ie_tlv_encapsulate_wsc_payload error");
+			goto send_frame;
+		}
+
+		memcpy(resp->ies + ies_len, wsc_ie, wsc_ie_len);
+		ies_len += wsc_ie_len;
+		l_free(wsc_ie);
+	}
+
+send_frame:
 	return ap_send_mgmt_frame(ap, mpdu, resp->ies + ies_len - mpdu_buf,
 					true, callback, sta);
 }
@@ -1032,8 +1064,8 @@ static int ap_parse_supported_rates(struct ie_tlv_iter *iter,
  */
 static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 				const struct mmpdu_field_capability *capability,
-				uint16_t listen_interval,
-				struct ie_tlv_iter *ies)
+				uint16_t listen_interval, const uint8_t *ies,
+				size_t ies_len)
 {
 	struct ap_state *ap = sta->ap;
 	const char *ssid = NULL;
@@ -1042,6 +1074,9 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 	struct l_uintset *rates = NULL;
 	struct ie_rsn_info rsn_info;
 	int err;
+	struct ie_tlv_iter iter;
+	uint8_t *wsc_data = NULL;
+	ssize_t wsc_data_len;
 
 	if (sta->assoc_resp_cmd_id)
 		return;
@@ -1051,16 +1086,20 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 		goto unsupported;
 	}
 
-	while (ie_tlv_iter_next(ies))
-		switch (ie_tlv_iter_get_tag(ies)) {
+	wsc_data = ie_tlv_extract_wsc_payload(ies, ies_len, &wsc_data_len);
+
+	ie_tlv_iter_init(&iter, ies, ies_len);
+
+	while (ie_tlv_iter_next(&iter))
+		switch (ie_tlv_iter_get_tag(&iter)) {
 		case IE_TYPE_SSID:
-			ssid = (const char *) ie_tlv_iter_get_data(ies);
-			ssid_len = ie_tlv_iter_get_length(ies);
+			ssid = (const char *) ie_tlv_iter_get_data(&iter);
+			ssid_len = ie_tlv_iter_get_length(&iter);
 			break;
 
 		case IE_TYPE_SUPPORTED_RATES:
 		case IE_TYPE_EXTENDED_SUPPORTED_RATES:
-			if (ap_parse_supported_rates(ies, &rates) < 0) {
+			if (ap_parse_supported_rates(&iter, &rates) < 0) {
 				err = MMPDU_REASON_CODE_INVALID_IE;
 				goto bad_frame;
 			}
@@ -1068,16 +1107,26 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 			break;
 
 		case IE_TYPE_RSN:
-			if (ie_parse_rsne(ies, &rsn_info) < 0) {
+			/*
+			 * WSC v2.0.5 Section 8.2:
+			 * "Note that during the WSC association [...] the
+			 * RSN IE and the WPA IE are irrelevant and shall be
+			 * ignored by both the station and AP."
+			 */
+			if (wsc_data)
+				break;
+
+			if (ie_parse_rsne(&iter, &rsn_info) < 0) {
 				err = MMPDU_REASON_CODE_INVALID_IE;
 				goto bad_frame;
 			}
 
-			rsn = (const uint8_t *) ie_tlv_iter_get_data(ies) - 2;
+			rsn = (const uint8_t *) ie_tlv_iter_get_data(&iter) - 2;
 			break;
 		}
 
-	if (!rates || !ssid || !rsn || ssid_len != strlen(ap->config->ssid) ||
+	if (!rates || !ssid || (!wsc_data && !rsn) ||
+			ssid_len != strlen(ap->config->ssid) ||
 			memcmp(ssid, ap->config->ssid, ssid_len)) {
 		err = MMPDU_REASON_CODE_INVALID_IE;
 		goto bad_frame;
@@ -1088,19 +1137,81 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 		goto unsupported;
 	}
 
-	if (rsn_info.mfpr && rsn_info.spp_a_msdu_required) {
-		err = MMPDU_REASON_CODE_UNSPECIFIED;
-		goto unsupported;
-	}
+	/* Is the client requesting RSNA establishment or WSC registration */
+	if (!rsn) {
+		struct wsc_association_request wsc_req;
+		struct ap_event_registration_started_data event_data;
+		struct ap_wsc_pbc_probe_record *record;
 
-	if (!(rsn_info.pairwise_ciphers & ap->ciphers)) {
-		err = MMPDU_REASON_CODE_INVALID_PAIRWISE_CIPHER;
-		goto unsupported;
-	}
+		if (wsc_parse_association_request(wsc_data, wsc_data_len,
+							&wsc_req) < 0) {
+			err = MMPDU_REASON_CODE_INVALID_IE;
+			goto bad_frame;
+		}
 
-	if (rsn_info.akm_suites != IE_RSN_AKM_SUITE_PSK) {
-		err = MMPDU_REASON_CODE_INVALID_AKMP;
-		goto unsupported;
+		l_free(wsc_data);
+		wsc_data = NULL;
+
+		if (wsc_req.request_type !=
+				WSC_REQUEST_TYPE_ENROLLEE_OPEN_8021X) {
+			err = MMPDU_REASON_CODE_INVALID_IE;
+			goto bad_frame;
+		}
+
+		if (!ap->wsc_pbc_timeout) {
+			l_debug("WSC association from %s but we're not in "
+				"PBC mode", util_address_to_string(sta->addr));
+			err = MMPDU_REASON_CODE_UNSPECIFIED;
+			goto bad_frame;
+		}
+
+		if (l_queue_isempty(ap->wsc_pbc_probes)) {
+			l_debug("%s tried to register as enrollee but we "
+				"don't have their Probe Request record",
+				util_address_to_string(sta->addr));
+			err = MMPDU_REASON_CODE_UNSPECIFIED;
+			goto bad_frame;
+		}
+
+		/*
+		 * For PBC, the Enrollee must have sent the only PBC Probe
+		 * Request within the monitor time and walk time.
+		 */
+		record = l_queue_peek_head(ap->wsc_pbc_probes);
+		if (memcmp(sta->addr, record->mac, 6)) {
+			l_debug("Session overlap during %s's attempt to "
+				"register as WSC enrollee",
+				util_address_to_string(sta->addr));
+			err = MMPDU_REASON_CODE_UNSPECIFIED;
+			goto bad_frame;
+		}
+
+		memcpy(sta->wsc_uuid_e, record->uuid_e, 16);
+
+		event_data.mac = sta->addr;
+		ap->event_func(AP_EVENT_REGISTRATION_START, &event_data,
+				ap->user_data);
+
+		/*
+		 * Since we're starting the PBC Registration Protocol
+		 * we can now exit the "active PBC mode".
+		 */
+		ap_wsc_exit_pbc(ap);
+	} else {
+		if (rsn_info.mfpr && rsn_info.spp_a_msdu_required) {
+			err = MMPDU_REASON_CODE_UNSPECIFIED;
+			goto unsupported;
+		}
+
+		if (!(rsn_info.pairwise_ciphers & ap->ciphers)) {
+			err = MMPDU_REASON_CODE_INVALID_PAIRWISE_CIPHER;
+			goto unsupported;
+		}
+
+		if (rsn_info.akm_suites != IE_RSN_AKM_SUITE_PSK) {
+			err = MMPDU_REASON_CODE_INVALID_AKMP;
+			goto unsupported;
+		}
 	}
 
 	/* 802.11-2016 11.3.5.3 j) */
@@ -1130,10 +1241,12 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 	if (sta->assoc_rsne)
 		l_free(sta->assoc_rsne);
 
-	sta->assoc_rsne = l_memdup(rsn, rsn[1] + 2);
+	if (rsn)
+		sta->assoc_rsne = l_memdup(rsn, rsn[1] + 2);
+	else
+		sta->assoc_rsne = NULL;
 
-	sta->assoc_resp_cmd_id = ap_assoc_resp(ap, sta, sta->addr, sta->aid, 0,
-						reassoc,
+	sta->assoc_resp_cmd_id = ap_assoc_resp(ap, sta, sta->addr, 0, reassoc,
 						ap_success_assoc_resp_cb);
 	if (!sta->assoc_resp_cmd_id)
 		l_error("Sending success (Re)Association Response failed");
@@ -1162,7 +1275,9 @@ bad_frame:
 	if (rates)
 		l_uintset_free(rates);
 
-	if (!ap_assoc_resp(ap, NULL, sta->addr, 0, err, reassoc,
+	l_free(wsc_data);
+
+	if (!ap_assoc_resp(ap, sta, sta->addr, err, reassoc,
 				ap_fail_assoc_resp_cb))
 		l_error("Sending error (Re)Association Response failed");
 }
@@ -1176,7 +1291,6 @@ static void ap_assoc_req_cb(const struct mmpdu_header *hdr, const void *body,
 	const uint8_t *from = hdr->address_2;
 	const struct mmpdu_association_request *req = body;
 	const uint8_t *bssid = netdev_get_address(ap->netdev);
-	struct ie_tlv_iter iter;
 
 	l_info("AP Association Request from %s", util_address_to_string(from));
 
@@ -1186,7 +1300,7 @@ static void ap_assoc_req_cb(const struct mmpdu_header *hdr, const void *body,
 
 	sta = l_queue_find(ap->sta_states, ap_sta_match_addr, from);
 	if (!sta) {
-		if (!ap_assoc_resp(ap, NULL, from, 0,
+		if (!ap_assoc_resp(ap, NULL, from,
 				MMPDU_REASON_CODE_STA_REQ_ASSOC_WITHOUT_AUTH,
 				false, ap_fail_assoc_resp_cb))
 			l_error("Sending error Association Response failed");
@@ -1194,9 +1308,9 @@ static void ap_assoc_req_cb(const struct mmpdu_header *hdr, const void *body,
 		return;
 	}
 
-	ie_tlv_iter_init(&iter, req->ies, body_len - sizeof(*req));
 	ap_assoc_reassoc(sta, false, &req->capability,
-				L_LE16_TO_CPU(req->listen_interval), &iter);
+				L_LE16_TO_CPU(req->listen_interval),
+				req->ies, body_len - sizeof(*req));
 }
 
 /* 802.11-2016 9.3.3.8 */
@@ -1208,7 +1322,6 @@ static void ap_reassoc_req_cb(const struct mmpdu_header *hdr, const void *body,
 	const uint8_t *from = hdr->address_2;
 	const struct mmpdu_reassociation_request *req = body;
 	const uint8_t *bssid = netdev_get_address(ap->netdev);
-	struct ie_tlv_iter iter;
 	int err;
 
 	l_info("AP Reassociation Request from %s",
@@ -1229,13 +1342,13 @@ static void ap_reassoc_req_cb(const struct mmpdu_header *hdr, const void *body,
 		goto bad_frame;
 	}
 
-	ie_tlv_iter_init(&iter, req->ies, body_len - sizeof(*req));
 	ap_assoc_reassoc(sta, true, &req->capability,
-				L_LE16_TO_CPU(req->listen_interval), &iter);
+				L_LE16_TO_CPU(req->listen_interval),
+				req->ies, body_len - sizeof(*req));
 	return;
 
 bad_frame:
-	if (!ap_assoc_resp(ap, NULL, from, 0, err, true, ap_fail_assoc_resp_cb))
+	if (!ap_assoc_resp(ap, NULL, from, err, true, ap_fail_assoc_resp_cb))
 		l_error("Sending error Reassociation Response failed");
 }
 
-- 
2.25.1

[PATCH 11/11] ap: Start EAP-WSC authentication with WSC enrollees

[Andrew Zaborowski]

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

After association and sending the SET_STATION commands, set up the
handshake_state and eapol_sm for EAP-WSC and start the handshake.
---
 src/ap.c | 217 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 176 insertions(+), 41 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 0876dc01..26dddfea 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -46,6 +46,7 @@
 #include "src/nl80211util.h"
 #include "src/frame-xchg.h"
 #include "src/wscutil.h"
+#include "src/eap-wsc.h"
 #include "src/ap.h"
 
 struct ap_state {
@@ -91,6 +92,8 @@ struct sta_state {
 	struct eapol_sm *sm;
 	struct handshake_state *hs;
 	uint32_t gtk_query_cmd_id;
+	struct l_idle *stop_handshake_work;
+	struct l_settings *wsc_settings;
 	uint8_t wsc_uuid_e[16];
 };
 
@@ -130,6 +133,23 @@ static void ap_stop_handshake(struct sta_state *sta)
 		handshake_state_free(sta->hs);
 		sta->hs = NULL;
 	}
+
+	if (sta->wsc_settings) {
+		l_settings_free(sta->wsc_settings);
+		sta->wsc_settings = NULL;
+	}
+
+	if (sta->stop_handshake_work) {
+		l_idle_remove(sta->stop_handshake_work);
+		sta->stop_handshake_work = NULL;
+	}
+}
+
+static void ap_stop_handshake_work(struct l_idle *idle, void *user_data)
+{
+	struct sta_state *sta = user_data;
+
+	ap_stop_handshake(sta);
 }
 
 static void ap_sta_free(void *data)
@@ -561,6 +581,44 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 	return id;
 }
 
+static void ap_start_handshake(struct sta_state *sta)
+{
+	struct ap_state *ap = sta->ap;
+	const uint8_t *own_addr = netdev_get_address(ap->netdev);
+	struct ie_rsn_info rsn;
+	uint8_t bss_rsne[24];
+
+	handshake_state_set_ssid(sta->hs, (void *) ap->config->ssid,
+					strlen(ap->config->ssid));
+	handshake_state_set_authenticator_address(sta->hs, own_addr);
+	handshake_state_set_supplicant_address(sta->hs, sta->addr);
+
+	ap_set_rsn_info(ap, &rsn);
+	/*
+	 * Note: This assumes the length that ap_set_rsn_info() requires. If
+	 * ap_set_rsn_info() changes then this will need to be updated.
+	 */
+	ie_build_rsne(&rsn, bss_rsne);
+	handshake_state_set_authenticator_ie(sta->hs, bss_rsne);
+
+	sta->sm = eapol_sm_new(sta->hs);
+	if (!sta->sm) {
+		ap_stop_handshake(sta);
+		l_error("could not create sm object");
+		goto error;
+	}
+
+	eapol_sm_set_listen_interval(sta->sm, sta->listen_interval);
+
+	eapol_register(sta->sm);
+	eapol_start(sta->sm);
+
+	return;
+
+error:
+	ap_del_station(sta, MMPDU_REASON_CODE_UNSPECIFIED, true);
+}
+
 static void ap_handshake_event(struct handshake_state *hs,
 		enum handshake_event event, void *user_data, ...)
 {
@@ -588,53 +646,18 @@ static void ap_handshake_event(struct handshake_state *hs,
 
 static void ap_start_rsna(struct sta_state *sta, const uint8_t *gtk_rsc)
 {
-	struct ap_state *ap = sta->ap;
-	struct netdev *netdev = sta->ap->netdev;
-	const uint8_t *own_addr = netdev_get_address(netdev);
-	struct ie_rsn_info rsn;
-	uint8_t bss_rsne[24];
-
-	ap_set_rsn_info(ap, &rsn);
-	/*
-	 * TODO: This assumes the length that ap_set_rsn_info() requires. If
-	 * ap_set_rsn_info() changes then this will need to be updated.
-	 */
-	ie_build_rsne(&rsn, bss_rsne);
-
 	/* this handshake setup assumes PSK network */
-	sta->hs = netdev_handshake_state_new(netdev);
-
-	handshake_state_set_event_func(sta->hs, ap_handshake_event, sta);
-	handshake_state_set_ssid(sta->hs, (void *) ap->config->ssid,
-					strlen(ap->config->ssid));
+	sta->hs = netdev_handshake_state_new(sta->ap->netdev);
 	handshake_state_set_authenticator(sta->hs, true);
-	handshake_state_set_authenticator_ie(sta->hs, bss_rsne);
+	handshake_state_set_event_func(sta->hs, ap_handshake_event, sta);
 	handshake_state_set_supplicant_ie(sta->hs, sta->assoc_rsne);
-	handshake_state_set_pmk(sta->hs, ap->pmk, 32);
-	handshake_state_set_authenticator_address(sta->hs, own_addr);
-	handshake_state_set_supplicant_address(sta->hs, sta->addr);
+	handshake_state_set_pmk(sta->hs, sta->ap->pmk, 32);
 
 	if (gtk_rsc)
-		handshake_state_set_gtk(sta->hs, ap->gtk, ap->gtk_index,
-					gtk_rsc);
-
-	sta->sm = eapol_sm_new(sta->hs);
-	if (!sta->sm) {
-		handshake_state_free(sta->hs);
-		sta->hs = NULL;
-		l_error("could not create sm object");
-		goto error;
-	}
-
-	eapol_sm_set_listen_interval(sta->sm, sta->listen_interval);
+		handshake_state_set_gtk(sta->hs, sta->ap->gtk,
+					sta->ap->gtk_index, gtk_rsc);
 
-	eapol_register(sta->sm);
-	eapol_start(sta->sm);
-
-	return;
-
-error:
-	ap_del_station(sta, MMPDU_REASON_CODE_UNSPECIFIED, true);
+	ap_start_handshake(sta);
 }
 
 static void ap_gtk_query_cb(struct l_genl_msg *msg, void *user_data)
@@ -673,6 +696,105 @@ static bool ap_wsc_pbc_record_expire(void *data, void *user_data)
 	return true;
 }
 
+static void ap_stop_handshake_schedule(struct sta_state *sta)
+{
+	if (sta->stop_handshake_work)
+		return;
+
+	sta->stop_handshake_work = l_idle_create(ap_stop_handshake_work,
+							sta, NULL);
+}
+
+static void ap_wsc_handshake_event(struct handshake_state *hs,
+		enum handshake_event event, void *user_data, ...)
+{
+	struct sta_state *sta = user_data;
+	va_list args;
+	struct ap_event_registration_success_data event_data;
+	struct ap_pbc_record_expiry_data expiry_data;
+
+	va_start(args, user_data);
+
+	switch (event) {
+	case HANDSHAKE_EVENT_FAILED:
+		sta->sm = NULL;
+		ap_stop_handshake_schedule(sta);
+		/*
+		 * Some diagrams in WSC v2.0.5 indicate we should
+		 * automatically deauthenticate the Enrollee.  The text
+		 * generally indicates the Enrollee may disassociate
+		 * meaning that we should neither deauthenticate nor
+		 * disassociate it automatically.  Some places indicate
+		 * that the enrollee can send a new EAPoL-Start right away
+		 * on an unsuccessful registration, we don't implement
+		 * this for now.  STA remains associated but not authorized
+		 * and basically has no other option than to re-associate
+		 * or disassociate/deauthenticate.
+		 */
+		break;
+	case HANDSHAKE_EVENT_EAP_NOTIFY:
+		if (va_arg(args, unsigned int) != EAP_WSC_EVENT_CREDENTIAL_SENT)
+			break;
+
+		/*
+		 * WSC v2.0.5 Section 11.3:
+		 * "If the Registrar successfully runs the PBC method to
+		 * completion with an Enrollee, that Enrollee's probe requests
+		 * are removed from the Monitor Time check the next time the
+		 * Registrar's PBC button is pressed."
+		 */
+		expiry_data.min_time = 0;
+		expiry_data.mac = sta->addr;
+		l_queue_foreach_remove(sta->ap->wsc_pbc_probes,
+					ap_wsc_pbc_record_expire,
+					&expiry_data);
+
+		event_data.mac = sta->addr;
+		sta->ap->event_func(AP_EVENT_REGISTRATION_SUCCESS, &event_data,
+					sta->ap->user_data);
+		break;
+	default:
+		break;
+	}
+
+	va_end(args);
+}
+
+static void ap_start_eap_wsc(struct sta_state *sta)
+{
+	struct ap_state *ap = sta->ap;
+	L_AUTO_FREE_VAR(char *, uuid_r_str) = NULL;
+	L_AUTO_FREE_VAR(char *, uuid_e_str) = NULL;
+
+	uuid_r_str = l_util_hexstring(ap->wsc_uuid_r, 16);
+	uuid_e_str = l_util_hexstring(sta->wsc_uuid_e, 16);
+
+	sta->wsc_settings = l_settings_new();
+	l_settings_set_string(sta->wsc_settings, "Security", "EAP-Method",
+				"WSC-R");
+	l_settings_set_string(sta->wsc_settings, "WSC", "EnrolleeMAC",
+				util_address_to_string(sta->addr));
+	l_settings_set_string(sta->wsc_settings, "WSC", "UUID-R",
+				uuid_r_str);
+	l_settings_set_string(sta->wsc_settings, "WSC", "UUID-E",
+				uuid_e_str);
+	l_settings_set_uint(sta->wsc_settings, "WSC", "RFBand",
+				WSC_RF_BAND_2_4_GHZ);
+	l_settings_set_uint(sta->wsc_settings, "WSC", "ConfigurationMethods",
+				WSC_CONFIGURATION_METHOD_PUSH_BUTTON);
+	l_settings_set_string(sta->wsc_settings, "WSC", "WPA2-SSID",
+				ap->config->ssid);
+	l_settings_set_string(sta->wsc_settings, "WSC", "WPA2-Passphrase",
+				ap->config->psk);
+
+	sta->hs = netdev_handshake_state_new(ap->netdev);
+	handshake_state_set_authenticator(sta->hs, true);
+	handshake_state_set_event_func(sta->hs, ap_wsc_handshake_event, sta);
+	handshake_state_set_8021x_config(sta->hs, sta->wsc_settings);
+
+	ap_start_handshake(sta);
+}
+
 static struct l_genl_msg *ap_build_cmd_del_key(struct ap_state *ap)
 {
 	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
@@ -739,6 +861,19 @@ static void ap_associate_sta_cb(struct l_genl_msg *msg, void *user_data)
 		return;
 	}
 
+	/*
+	 * WSC v2.0.5 Section 8.2:
+	 * "Therefore if a WSC IE is present in the (re)association request,
+	 * the AP shall engage in EAP-WSC with the station and shall not
+	 * attempt any other security handshake."
+	 *
+	 * So no need for group traffic, skip the GTK setup below.
+	 */
+	if (!sta->assoc_rsne) {
+		ap_start_eap_wsc(sta);
+		return;
+	}
+
 	/*
 	 * Set up the group key.  If this is our first STA then we have
 	 * to add the new GTK to the kernel.  In theory we should be
-- 
2.25.1

Re: [PATCH 02/11] eap: Re-send Identity Request on EAPoL-Start

[Denis Kenzior]

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

On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> It looks like clients sometimes miss our unsolicited Identity Request
> and need a resend.
> ---
>   src/eap.c   | 12 ++++++++++--
>   src/eapol.c | 12 ++++++++----
>   2 files changed, 18 insertions(+), 6 deletions(-)
> 

Applied.

By the way, there are several implementations (Apple AirPort for example) that 
do not start WSC until an EAPoL-Start is sent.  I don't recall that EAPoL-Start 
behavior was specified in WSC 1.0.  I know that one of my routers that uses WSC 
2.0 doesn't do this and sends a RequestIdentity right away.  So that might 
explain what you're seeing here.

Regards,
-Denis

Re: [PATCH 04/11] ap: Move AP parameters to a struct

[Denis Kenzior]

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

Hi Andrew,


On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> To limit the number of ap_start parameters, group basic AP config
> parameters in the ap_config struct that is passed as a pointer and owned
> by the ap_state.
> ---
>   src/ap.c | 127 ++++++++++++++++++++++++++++++-------------------------
>   src/ap.h |  15 +++++--
>   2 files changed, 81 insertions(+), 61 deletions(-)
> 

<snip>

> @@ -1462,11 +1480,14 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
>    * @event_func is required and must react to AP_EVENT_START_FAILED
>    * and AP_EVENT_STOPPING by forgetting the ap_state struct, which
>    * is going to be freed automatically.
> - * @channel is optional.
> + * In the @config struct only .ssid and .psk need to be non-NUL,
> + * other fields are optional.  If @ap_start succeeds, the returned
> + * ap_state takes ownership of @config and the caller shouldn't
> + * free it or any of the memory pointed to by its members (they
> + * also can't be static).  In principle there's no reason that the
> + * caller shouldn't read/update some of the members though.

Yeah, no ;)  That last sentence should not happen (and definitely should not be 
explicitly encouraged) for reasons already discussed.  I amended this comment to 
leave this out and applied.

>    */
> -struct ap_state *ap_start(struct netdev *netdev, const char *ssid,
> -				const char *psk, int channel, bool no_cck_rates,
> -				const uint8_t **authorized_macs,
> +struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   				ap_event_func_t event_func, void *user_data)
>   {
>   	struct ap_state *ap;

Regards,
-Denis

Re: [PATCH 03/11] eap-wsc: In WSC-R read UUID-E from settings

[Denis Kenzior]

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

Hi Andrew,

On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> The intent was to read the UUID-E from the settings rather than generate
> it from the enrollee's MAC because it needs to match the UUID-E from
> enrolee's Probe Requests, fix this.  The UUID-E supplied in the unit
> test was being ignored but the test still passed because the supplied
> UUID-E was generated the same way we generated it in eap-wsc.c.
> ---
>   src/eap-wsc.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 

Patch 3, 5, 6 applied, thanks.

Regards,
-Denis

Re: [PATCH 07/11] ap: Stop ongoing handshake on reassociation

[Denis Kenzior]

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

Hi Andrew,

On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> On a new association or re-association, in addition to forgetting a
> complete RSN Association, also stop the EAPoL SM to stop any ongoing
> handshake.
> 
> Do this in a new function ap_stop_handshake that is now used in a few
> places that had copies of the same few lines.  I'll be adding some more
> lines to this function for WSC support.
> ---
>   src/ap.c | 51 +++++++++++++++++++++++++--------------------------
>   1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ap.c b/src/ap.c
> index aef2e1dd..5cd2b717 100644
> --- a/src/ap.c
> +++ b/src/ap.c

No real concerns on the first three uses, but...

<snip>

> @@ -938,6 +933,12 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
>   		goto unsupported;
>   	}
>   
> +	/* 802.11-2016 11.3.5.3 j) */
> +	if (sta->rsna)
> +		ap_drop_rsna(sta);
> +
> +	ap_stop_handshake(sta);
> +

ap_drop_rsna also destroys eapol_sm and the handshake object.  So perhaps you 
need to track the authorized state to choose the right cleanup action?

>   	if (!sta->associated) {
>   		/*
>   		 * Everything fine so far, assign an AID, send response.
> @@ -961,10 +962,6 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
>   
>   	sta->assoc_rsne = l_memdup(rsn, rsn[1] + 2);
>   
> -	/* 802.11-2016 11.3.5.3 j) */
> -	if (sta->rsna)
> -		ap_drop_rsna(sta);
> -
>   	sta->assoc_resp_cmd_id = ap_assoc_resp(ap, sta, sta->addr, sta->aid, 0,
>   						reassoc,
>   						ap_success_assoc_resp_cb);
> @@ -987,8 +984,10 @@ bad_frame:
>   	 *
>   	 * For now, we need to drop the RSNA.
>   	 */
> -	if (sta->associated && sta->rsna)
> +	if (sta->associated && sta->rsna) {
>   		ap_drop_rsna(sta);
> +		ap_stop_handshake(sta);
> +	}

Same question here?

>   
>   	if (rates)
>   		l_uintset_free(rates);
> 

Regards,
-Denis

Re: [PATCH 01/11] wscutil: Add wsc_build_beacon

[Denis Kenzior]

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

Hi Andrew,

On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> ---
>   src/wscutil.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/wscutil.h |  1 +
>   2 files changed, 60 insertions(+)
> 

<snip>

> +
> +	/* These two "should be provided" if dual-band */
> +	if (beacon->rf_bands & (beacon->rf_bands - 1)) {

I probably missed this in the earlier review, but would it be more clear to 
check rf_bands & ~WSC_RF_BAND_2_4_GHZ here instead?

> +		if (beacon->selected_registrar)
> +			build_uuid_e(builder, beacon->uuid_e);
> +
> +		build_rf_bands(builder, beacon->rf_bands);
> +	}
> +
> +	if (!beacon->version2)
> +		goto done;
> +
> +	START_WFA_VENDOR_EXTENSION();
> +
> +	if (!util_mem_is_zero(beacon->authorized_macs, 30)) {
> +		int count;
> +
> +		for (count = 1; count < 5; count++)
> +			if (util_mem_is_zero(beacon->authorized_macs + count * 6,
> +						30 - count * 6))
> +				break;
> +
> +		wsc_attr_builder_put_u8(builder,
> +					WSC_WFA_EXTENSION_AUTHORIZED_MACS);
> +		wsc_attr_builder_put_u8(builder, count * 6);
> +		wsc_attr_builder_put_bytes(builder, beacon->authorized_macs,
> +						count * 6);
> +	}

I'm guilty of copy-pasting as much as anyone, but could we make a utility for 
this?  Also, wouldn't it make more sense to run util_mem_is_zero backwards 6 
bytes at a time instead of pounding the array 6 times?

Regards,
-Denis

Re: [PATCH 08/11] ap: Push Button mode API and beacon changes

[Denis Kenzior]

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

Hi Andrew,

On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> This adds the API for putting the AP in Push Button mode, which we'll
> need to P2P GO side but may be useful on its own too.  A WSC IE is added
> to our beacons and probe responses indicating whether the PBC mode is
> active.
> ---
>   src/ap.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   src/ap.h |  15 ++++
>   2 files changed, 219 insertions(+), 3 deletions(-)
> 

<snip>

So just some general comments since this patch doesn't compile without patch 1:

> diff --git a/src/ap.h b/src/ap.h
> index 330ae011..4351fdc5 100644
> --- a/src/ap.h
> +++ b/src/ap.h
> @@ -29,6 +29,9 @@ enum ap_event_type {
>   	AP_EVENT_STOPPING,
>   	AP_EVENT_STATION_ADDED,
>   	AP_EVENT_STATION_REMOVED,
> +	AP_EVENT_REGISTRATION_START,
> +	AP_EVENT_REGISTRATION_SUCCESS,

These two are not used in this patch, but ok..

> +	AP_EVENT_PBC_MODE_EXIT,
>   };
>   
>   struct ap_event_station_added_data {
> @@ -41,6 +44,14 @@ struct ap_event_station_removed_data {
>   	enum mmpdu_reason_code reason;
>   };
>   
> +struct ap_event_registration_started_data {
> +	const uint8_t *mac;
> +};
> +
> +struct ap_event_registration_success_data {
> +	const uint8_t *mac;
> +};
> +

As above

>   typedef void (*ap_event_func_t)(enum ap_event_type type, const void *event_data,
>   				void *user_data);
>   typedef void (*ap_stopped_func_t)(void *user_data);
> @@ -51,6 +62,8 @@ struct ap_config {
>   	uint8_t channel;
>   	uint8_t *authorized_macs;
>   	unsigned int authorized_macs_num;
> +	char *wsc_name;
> +	struct wsc_primary_device_type wsc_primary_device_type;
>   	bool no_cck_rates : 1;
>   };
>   
> @@ -64,3 +77,5 @@ void ap_free(struct ap_state *ap);
>   
>   bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
>   				enum mmpdu_reason_code reason);
> +
> +bool ap_push_button(struct ap_state *ap);
> 

So correct me if I'm wrong, but the intent here seems to be:

Have P2P-GO start AP in PBC mode by setting authorized_macs to whatever, then 
run ap_push_button().  Then the AP has to be shut down and restarted (or maybe 
not?)  But regardless, it seems that PBC cannot be restarted with AP running 
after the previous PBC transaction has finished.

This seems like an unnecessary limitation.  Why not implement PBC (and in the 
future PIN) as a transitory state instead?  This way we could have our AP (non 
P2P-GO) mode support push buttons / PINs almost for free...

So something like:

ap_push_button(struct ap_state *ap, struct wsc_config *wsc);

Regards,
-Denis

Re: [PATCH 02/11] eap: Re-send Identity Request on EAPoL-Start

[Denis Kenzior]

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

Hi Andrew,

On 8/27/20 3:47 PM, Andrew Zaborowski wrote:
> On Thu, 27 Aug 2020 at 20:55, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
>>> It looks like clients sometimes miss our unsolicited Identity Request
>>> and need a resend.
>>> ---
>>>    src/eap.c   | 12 ++++++++++--
>>>    src/eapol.c | 12 ++++++++----
>>>    2 files changed, 18 insertions(+), 6 deletions(-)
>>>
>>
>> Applied.
>>
>> By the way, there are several implementations (Apple AirPort for example) that
>> do not start WSC until an EAPoL-Start is sent.  I don't recall that EAPoL-Start
>> behavior was specified in WSC 1.0.  I know that one of my routers that uses WSC
>> 2.0 doesn't do this and sends a RequestIdentity right away.  So that might
>> explain what you're seeing here.
> 
> So I intended to mention this separately but the spec recommends that
> we send the EAP Identity request automatically if the enrollee is WSC
> 2.0 and it prohibits us doing this if it's WSC 1.0.  I think there's
> probably no harm trying it until we see it throws off some device.

Okay.  So if the spec recommends this, it probably is intentional.  May be a 
good idea to follow its recommendation.  Do you recall where this is stated?

> 
> In my case an Android device replied to my initial Identity Request in
> three attempts, but on one occasion it apparently missed the request,
> sent the EAPoL-Start after a timeout and waited for a new Identity
> Request.
> 

Yeah, iwd (and I think wpa_s as well) starts a timer and sends an EAPoL-Start if 
they don't receive the Request-Identity after some time.  But that doesn't mean 
others do.

Regards,
-Denis

Re: [PATCH 07/11] ap: Stop ongoing handshake on reassociation

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 27 Aug 2020 at 21:15, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> > On a new association or re-association, in addition to forgetting a
> > complete RSN Association, also stop the EAPoL SM to stop any ongoing
> > handshake.
> >
> > Do this in a new function ap_stop_handshake that is now used in a few
> > places that had copies of the same few lines.  I'll be adding some more
> > lines to this function for WSC support.
> > ---
> >   src/ap.c | 51 +++++++++++++++++++++++++--------------------------
> >   1 file changed, 25 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/ap.c b/src/ap.c
> > index aef2e1dd..5cd2b717 100644
> > --- a/src/ap.c
> > +++ b/src/ap.c
>
> No real concerns on the first three uses, but...
>
> <snip>
>
> > @@ -938,6 +933,12 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
> >               goto unsupported;
> >       }
> >
> > +     /* 802.11-2016 11.3.5.3 j) */
> > +     if (sta->rsna)
> > +             ap_drop_rsna(sta);
> > +
> > +     ap_stop_handshake(sta);
> > +
>
> ap_drop_rsna also destroys eapol_sm and the handshake object.  So perhaps you
> need to track the authorized state to choose the right cleanup action?

Ok, good catch, in fact it calls ap_stop_handshake() after this patch.
I didn't see this because conceptually ap_drop_rsna is a transition
from State 4 to State 3 while ap_stop_handshake sort of stops an
ongoing transition State 4.

I guess this will make sense:
if (sta->rsna)
    ap_drop_rsna(sta);
else
    ap_stop_handshake(sta);

Best regards

Re: [PATCH 02/11] eap: Re-send Identity Request on EAPoL-Start

[Andrew Zaborowski]

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

On Thu, 27 Aug 2020 at 20:55, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> > It looks like clients sometimes miss our unsolicited Identity Request
> > and need a resend.
> > ---
> >   src/eap.c   | 12 ++++++++++--
> >   src/eapol.c | 12 ++++++++----
> >   2 files changed, 18 insertions(+), 6 deletions(-)
> >
>
> Applied.
>
> By the way, there are several implementations (Apple AirPort for example) that
> do not start WSC until an EAPoL-Start is sent.  I don't recall that EAPoL-Start
> behavior was specified in WSC 1.0.  I know that one of my routers that uses WSC
> 2.0 doesn't do this and sends a RequestIdentity right away.  So that might
> explain what you're seeing here.

So I intended to mention this separately but the spec recommends that
we send the EAP Identity request automatically if the enrollee is WSC
2.0 and it prohibits us doing this if it's WSC 1.0.  I think there's
probably no harm trying it until we see it throws off some device.

In my case an Android device replied to my initial Identity Request in
three attempts, but on one occasion it apparently missed the request,
sent the EAPoL-Start after a timeout and waited for a new Identity
Request.

Best regards

Re: [PATCH 08/11] ap: Push Button mode API and beacon changes

[Denis Kenzior]

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

Hi Andrew,

>>> +
>>> +bool ap_push_button(struct ap_state *ap);
>>>
>>
>> So correct me if I'm wrong, but the intent here seems to be:
>>
>> Have P2P-GO start AP in PBC mode by setting authorized_macs to whatever, then
>> run ap_push_button().  Then the AP has to be shut down and restarted (or maybe
>> not?)  But regardless, it seems that PBC cannot be restarted with AP running
>> after the previous PBC transaction has finished.
>>
>> This seems like an unnecessary limitation.  Why not implement PBC (and in the
>> future PIN) as a transitory state instead?  This way we could have our AP (non
>> P2P-GO) mode support push buttons / PINs almost for free...
> 
> So the idea was that this function corresponds to a physical button
> press, i.e. whenever the AP UI senses button press it calls this, and
> perhaps lights up the "WPS" LED until it receives
> AP_EVENT_PBC_MODE_EXIT.

Right

> 
> So in P2P we'd first wait for the AP_EVENT_STARTED event, call this,
> and from there we should receive REGISTRATION_START -> PBC_MODE_EXIT
> -> REGISTRATION_SUCCESS -> STATION_ADDED without doing much, except
> perhaps restarting a timeout on each step.  The PBC mode doesn't
> preclude an RSN connection or multiple connections from different
> STAs, so the client can reassociate and do the 4-Way handshake as soon
> as it receives the credentials.
> 
>>
>> So something like:
>>
>> ap_push_button(struct ap_state *ap, struct wsc_config *wsc);
> 
> What information would go in wsc_config?
> 

Its been a while since I looked at WSC, so I could be wrong, but wouldn't 
authorized_macs belong to the wsc_config and not ap_config?

I suppose you could have them in both, but it seemed strange to see it in 
ap_config given you're trying to make wsc work and not configuring mac filtering.

Regards,
-Denis

Re: [PATCH 08/11] ap: Push Button mode API and beacon changes

[Andrew Zaborowski]

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

On Thu, 27 Aug 2020 at 21:50, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> > @@ -29,6 +29,9 @@ enum ap_event_type {
> >       AP_EVENT_STOPPING,
> >       AP_EVENT_STATION_ADDED,
> >       AP_EVENT_STATION_REMOVED,
> > +     AP_EVENT_REGISTRATION_START,
> > +     AP_EVENT_REGISTRATION_SUCCESS,
>
> These two are not used in this patch, but ok..
>
> > +     AP_EVENT_PBC_MODE_EXIT,
> >   };
> >
> >   struct ap_event_station_added_data {
> > @@ -41,6 +44,14 @@ struct ap_event_station_removed_data {
> >       enum mmpdu_reason_code reason;
> >   };
> >
> > +struct ap_event_registration_started_data {
> > +     const uint8_t *mac;
> > +};
> > +
> > +struct ap_event_registration_success_data {
> > +     const uint8_t *mac;
> > +};
> > +
>
> As above

Right, I thought it would be easier to see the idea with the API
defined in one patch.

>
> >   typedef void (*ap_event_func_t)(enum ap_event_type type, const void *event_data,
> >                               void *user_data);
> >   typedef void (*ap_stopped_func_t)(void *user_data);
> > @@ -51,6 +62,8 @@ struct ap_config {
> >       uint8_t channel;
> >       uint8_t *authorized_macs;
> >       unsigned int authorized_macs_num;
> > +     char *wsc_name;
> > +     struct wsc_primary_device_type wsc_primary_device_type;
> >       bool no_cck_rates : 1;
> >   };
> >
> > @@ -64,3 +77,5 @@ void ap_free(struct ap_state *ap);
> >
> >   bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
> >                               enum mmpdu_reason_code reason);
> > +
> > +bool ap_push_button(struct ap_state *ap);
> >
>
> So correct me if I'm wrong, but the intent here seems to be:
>
> Have P2P-GO start AP in PBC mode by setting authorized_macs to whatever, then
> run ap_push_button().  Then the AP has to be shut down and restarted (or maybe
> not?)  But regardless, it seems that PBC cannot be restarted with AP running
> after the previous PBC transaction has finished.
>
> This seems like an unnecessary limitation.  Why not implement PBC (and in the
> future PIN) as a transitory state instead?  This way we could have our AP (non
> P2P-GO) mode support push buttons / PINs almost for free...

So the idea was that this function corresponds to a physical button
press, i.e. whenever the AP UI senses button press it calls this, and
perhaps lights up the "WPS" LED until it receives
AP_EVENT_PBC_MODE_EXIT.

So in P2P we'd first wait for the AP_EVENT_STARTED event, call this,
and from there we should receive REGISTRATION_START -> PBC_MODE_EXIT
-> REGISTRATION_SUCCESS -> STATION_ADDED without doing much, except
perhaps restarting a timeout on each step.  The PBC mode doesn't
preclude an RSN connection or multiple connections from different
STAs, so the client can reassociate and do the 4-Way handshake as soon
as it receives the credentials.

>
> So something like:
>
> ap_push_button(struct ap_state *ap, struct wsc_config *wsc);

What information would go in wsc_config?

Best regards

Re: [PATCH 01/11] wscutil: Add wsc_build_beacon

[Andrew Zaborowski]

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

On Thu, 27 Aug 2020 at 21:21, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> On 8/27/20 1:14 PM, Andrew Zaborowski wrote:
> > ---
> >   src/wscutil.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   src/wscutil.h |  1 +
> >   2 files changed, 60 insertions(+)
> >
>
> <snip>
>
> > +
> > +     /* These two "should be provided" if dual-band */
> > +     if (beacon->rf_bands & (beacon->rf_bands - 1)) {
>
> I probably missed this in the earlier review, but would it be more clear to
> check rf_bands & ~WSC_RF_BAND_2_4_GHZ here instead?

That might be falsely triggered on a 5GHz-only AP.  I was gonna try
something like:

  (rf_bands & 2_4_AND_5_MASK) == 2_4_AND_5_MASK

as the condition, but ended up copying this from
wsc_build_probe_response, I can change it in both places.
>
> > +             if (beacon->selected_registrar)
> > +                     build_uuid_e(builder, beacon->uuid_e);
> > +
> > +             build_rf_bands(builder, beacon->rf_bands);
> > +     }
> > +
> > +     if (!beacon->version2)
> > +             goto done;
> > +
> > +     START_WFA_VENDOR_EXTENSION();
> > +
> > +     if (!util_mem_is_zero(beacon->authorized_macs, 30)) {
> > +             int count;
> > +
> > +             for (count = 1; count < 5; count++)
> > +                     if (util_mem_is_zero(beacon->authorized_macs + count * 6,
> > +                                             30 - count * 6))
> > +                             break;
> > +
> > +             wsc_attr_builder_put_u8(builder,
> > +                                     WSC_WFA_EXTENSION_AUTHORIZED_MACS);
> > +             wsc_attr_builder_put_u8(builder, count * 6);
> > +             wsc_attr_builder_put_bytes(builder, beacon->authorized_macs,
> > +                                             count * 6);
> > +     }
>
> I'm guilty of copy-pasting as much as anyone, but could we make a utility for
> this?  Also, wouldn't it make more sense to run util_mem_is_zero backwards 6
> bytes at a time instead of pounding the array 6 times?

Oops, I didn't realize the code was doing this.  I think elsewhere we
were assuming the first 6-zeros address in the array was the end of
the array, maybe it's enough here as well.

Best regards

Re: [PATCH 02/11] eap: Re-send Identity Request on EAPoL-Start

[Andrew Zaborowski]

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

On Thu, 27 Aug 2020 at 22:52, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/27/20 3:47 PM, Andrew Zaborowski wrote:
> > On Thu, 27 Aug 2020 at 20:55, Denis Kenzior <denkenz@gmail.com> wrote:
> >> By the way, there are several implementations (Apple AirPort for example) that
> >> do not start WSC until an EAPoL-Start is sent.  I don't recall that EAPoL-Start
> >> behavior was specified in WSC 1.0.  I know that one of my routers that uses WSC
> >> 2.0 doesn't do this and sends a RequestIdentity right away.  So that might
> >> explain what you're seeing here.
> >
> > So I intended to mention this separately but the spec recommends that
> > we send the EAP Identity request automatically if the enrollee is WSC
> > 2.0 and it prohibits us doing this if it's WSC 1.0.  I think there's
> > probably no harm trying it until we see it throws off some device.
>
> Okay.  So if the spec recommends this, it probably is intentional.  May be a
> good idea to follow its recommendation.

Ok.

> Do you recall where this is stated?

In 8.2:
"On successful association, the station will then send an EAPOL-Start
to the AP and wait
for EAP-Request/Identity. The AP is allowed to send
EAP-Request/Identity to the station
before EAPOL-Start is received if a WSC IE is included in the
(re)association request
and the WSC IE is version 2.0 or higher."

Also here's the other compatibility bit I mentioned, which I'm not
honoring for the moment:
"Note: For backwards compatibility with version 1.0 implementations an
AP hosting a
WPA2-Personal network and supporting WSC would need to permit the association
exchange with a station intending to engage in EAP-WSC where there is
no RSN IE or
WPA IE in the association request frames even if there is no WSC IE
either. The AP
shall only permit the exchange of EAP-WSC messages with a station that
associates in
this manner and only after receiving the EAPOL-Start frame. The WSC IE
may or may
not be present in the association requests from WSC version 1.0 devices."

Best regards

Re: [PATCH 01/11] wscutil: Add wsc_build_beacon

[Denis Kenzior]

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

Hi Andrew,

>>> +     /* These two "should be provided" if dual-band */
>>> +     if (beacon->rf_bands & (beacon->rf_bands - 1)) {
>>
>> I probably missed this in the earlier review, but would it be more clear to
>> check rf_bands & ~WSC_RF_BAND_2_4_GHZ here instead?
> 
> That might be falsely triggered on a 5GHz-only AP.  I was gonna try
> something like:
> 
>    (rf_bands & 2_4_AND_5_MASK) == 2_4_AND_5_MASK
> 
> as the condition, but ended up copying this from
> wsc_build_probe_response, I can change it in both places.

So whats the intent here? Only including RF bands if multiple bits are set?  If 
so then

__builtin_popcount(rf_bands) > 1 is what you want.

Regards,
-Denis