[v3,08/12] hostapd: fully validate multi-AP IE

Message ID 20181205102401.17810-9-arnout@mind.be
State Accepted
Headers show
Series
  • [v3,01/12] hostapd: Add Multi-AP protocol support
Related show

Commit Message

Arnout Vandecappelle Dec. 5, 2018, 10:23 a.m.
The validation of the multi-AP IE in hostapd is not very complete.

* When the IE is incorrectly constructed, return WLAN_STATUS_INVALID_IE.
This might not be entirely the right thing to do, since that status is
in principle reserved for IEs specified in IEEE 802.11-2016, but it
makes sense to use that for incorrect subelements as well, and it's done
in other places.

* When the AP is configured as fronthaul-only, association requests for
backhaul STA are rejected.

* When the AP is configured as backhaul-only, association requests
without backhaul STA are rejected.

* A multi-AP IE with a value different than MULTI_AP_BACKHAUL_STA is
ignored (i.e. treated the same as if it was missing).

* Rejections use reason WLAN_STATUS_ASSOC_DENIED_UNSPEC instead of
WLAN_STATUS_UNSPECIFIED_FAILURE - that seems a better match with the
IEEE 802.11-2016 specification.

* Set the WLAN_STA_MULTI_AP based on the IE, independent of whether it
is a backhaul or fronthaul AP. This makes sure that the Association
Response contains the IE even if it is a rejection.

None of the above is explicitly specified in the Multi-AP Specification.
However, it is pretty clear that we only want backhaul stations on a
backhaul-only AP and only fronthaul stations on a fronhaul-only AP,
otherwise the distinction is pointless.

Finally, rename hostapd_validate_multi_ap_ie() to check_multi_ap() and
directly give it the multi_ap_ie as argument rather than the
ieee802_11_elems, since that is how most other functions in that file
work.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Cc: Venkateswara Naralasetty <vnaralas@codeaurora.org>
---
 src/ap/ieee802_11.c | 58 +++++++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 18 deletions(-)

Comments

Arnout Vandecappelle Jan. 7, 2019, 1:34 p.m. | #1
On 05/12/2018 11:23, Arnout Vandecappelle (Essensium/Mind) wrote:
> +	if (hapd->conf->multi_ap & FRONTHAUL_BSS) {
> +		if (multi_ap_value == MULTI_AP_BACKHAUL_STA) {
> +			hostapd_logger(hapd, sta->addr,
> +				       HOSTAPD_MODULE_IEEE80211,
> +				       HOSTAPD_LEVEL_INFO,
> +				       "Backhaul STA tries to associate with fronthaul-only BSS");
> +			return WLAN_STATUS_ASSOC_DENIED_UNSPEC;
> +		}

 Turns out that this bit was a mistake. It breaks WPS. In Multi-AP, WPS is done
on the fronthaul BSS. The backhaul STA will associate with the fronthaul BSS and
provide the Multi-AP IE. This will cause the fronthaul BSS to provide
credentials for the backhaul BSS instead of for itself.

 So, if the fronthaul BSS is fronthaul-only, it may indeed get an association
request with a backhaul STA Multi-AP IE...

 Since the specification doesn't say explicitly that a bSTA trying to associate
on a fronthaul-only BSS should be rejected, I think the easiest is to remove
this check again.

 I'll do that as part of my refresh of patches 10-12.

 Regards,
 Arnout
Marianna Carrera Jan. 7, 2019, 2:30 p.m. | #2
> Since the specification doesn't say explicitly that a bSTA trying to associate
> on a fronthaul-only BSS should be rejected, I think the easiest is to remove
> this check again.

Missed it as well. I think a bSTA trying to associate on a "fronthaul-only" BSS is a nominal case, so this check should indeed be removed as there is no reason to reject the bSTA.

However, we may have consider the case where the device has only fronthaul BSSes, no backhaul BSS at all. 
I can't find a way in the Multi-AP specifications for a MAP Controller to provide a MAP Agent with the backhaul credentials, while not actually configuring a backhaul BSS at all.

If that's correct, in theory, we could have a situation where there are one or more frounthaul BSSes configured (i.e., multi_ap=2) but no "multi_ap_backhaul_*" properties configured (cause the device has no mean to know them). When one of these BSSes with multi_ap=2 receives a WPS M1 from a "bSTA", it does not know the backhaul BSS credentials, so what credentials does it provide? 
Providing the credentials of the fronthaul BSS would be wrong per the MAP specs, so maybe rejecting it with an error would be best.

What do you think?

-marianna
Arnout Vandecappelle Jan. 7, 2019, 5:44 p.m. | #3
On 07/01/2019 15:30, Marianna Carrera wrote:
>>  Since the specification doesn't say explicitly that a bSTA trying to associate
>>  on a fronthaul-only BSS should be rejected, I think the easiest is to remove
>>  this check again.
> 
> Missed it as well. I think a bSTA trying to associate on a "fronthaul-only" BSS is a nominal case, so this check should indeed be removed as there is no reason to reject the bSTA.
> 
> However, we may have consider the case where the device has only fronthaul BSSes, no backhaul BSS at all. 
> I can't find a way in the Multi-AP specifications for a MAP Controller to provide a MAP Agent with the backhaul credentials, while not actually configuring a backhaul BSS at all.
> 
> If that's correct, in theory, we could have a situation where there are one or more frounthaul BSSes configured (i.e., multi_ap=2) but no "multi_ap_backhaul_*" properties configured (cause the device has no mean to know them). When one of these BSSes with multi_ap=2 receives a WPS M1 from a "bSTA", it does not know the backhaul BSS credentials, so what credentials does it provide? 
> Providing the credentials of the fronthaul BSS would be wrong per the MAP specs, so maybe rejecting it with an error would be best.

 Good point.

 Patch 11/12 adds:

+       if (wps->peer_dev.multi_ap_ext == MULTI_AP_BACKHAUL_STA &&
+           wps->wps->multi_ap_backhaul_ssid_len)

so that should become something like:

	if (wps->peer_dev.multi_ap_ext == MULTI_AP_BACKHAUL_STA) {
		if (!wps->wps->multi_ap_backhaul_ssid_len) {
			return ERROR;
		}
		...
	}

 That would reject all bSTA requests if no backhaul BSS is configured, even if
hostapd is otherwise multiap-unaware. Not sure if that is approprate... On the
other hand, if a bSTA does WPS to an AP which is not multi-AP, it shouldn't use
those credentials anyway, so it's actually good to reject right away.


 Regards,
 Arnout

> 
> What do you think?
> 
> -marianna
>

Patch

diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
index fec5882e0..63ab27cb3 100644
--- a/src/ap/ieee802_11.c
+++ b/src/ap/ieee802_11.c
@@ -2224,31 +2224,53 @@  static u16 check_wmm(struct hostapd_data *hapd, struct sta_info *sta,
 	return WLAN_STATUS_SUCCESS;
 }
 
-static u16 hostapd_validate_multi_ap_ie(struct hostapd_data *hapd,
-					struct sta_info *sta,
-					struct ieee802_11_elems *elems)
+static u16 check_multi_ap(struct hostapd_data *hapd, struct sta_info *sta,
+					const u8 *multi_ap_ie, size_t multi_ap_len)
 {
-	const u8 *pos, *map_sub_elem;
-	size_t len;
+	u8 multi_ap_value = 0;
 
-	if (!hapd->conf->multi_ap || !elems->multi_ap)
+	sta->flags &= ~WLAN_STA_MULTI_AP;
+
+	if (!hapd->conf->multi_ap)
 		return WLAN_STATUS_SUCCESS;
 
-	pos = elems->multi_ap + 4;/* OUI[3] and OUT_TYPE 1 */
-	len = elems->multi_ap_len - 4;
+	if (multi_ap_ie) {
+		const u8 *multi_ap_subelem = get_ie(multi_ap_ie + 4,
+						    multi_ap_len - 4,
+						    MULTI_AP_SUB_ELEM_TYPE);
+		if (multi_ap_subelem &&
+		    multi_ap_subelem[1] == 1)
+			multi_ap_value = multi_ap_subelem[2];
+		else {
+			hostapd_logger(hapd, sta->addr,
+				       HOSTAPD_MODULE_IEEE80211,
+				       HOSTAPD_LEVEL_INFO,
+				       "Multi-AP IE has missing or invalid Multi-AP subelement");
+			return WLAN_STATUS_INVALID_IE;
+		}
+	}
 
-	sta->flags &= ~WLAN_STA_MULTI_AP;
+	if (multi_ap_value == MULTI_AP_BACKHAUL_STA)
+		sta->flags |= WLAN_STA_MULTI_AP;
 
-	map_sub_elem = get_ie(pos, len, MULTI_AP_SUB_ELEM_TYPE);
+	if (hapd->conf->multi_ap & BACKHAUL_BSS &&
+	    multi_ap_value == MULTI_AP_BACKHAUL_STA)
+		return WLAN_STATUS_SUCCESS;
 
-	if (map_sub_elem) {
-		if (map_sub_elem[1] < 1)
-			return WLAN_STATUS_UNSPECIFIED_FAILURE;
-		if (map_sub_elem[2] & MULTI_AP_BACKHAUL_STA)
-			sta->flags |= WLAN_STA_MULTI_AP;
+	if (hapd->conf->multi_ap & FRONTHAUL_BSS) {
+		if (multi_ap_value == MULTI_AP_BACKHAUL_STA) {
+			hostapd_logger(hapd, sta->addr,
+				       HOSTAPD_MODULE_IEEE80211,
+				       HOSTAPD_LEVEL_INFO,
+				       "Backhaul STA tries to associate with fronthaul-only BSS");
+			return WLAN_STATUS_ASSOC_DENIED_UNSPEC;
+		}
+		return WLAN_STATUS_SUCCESS;
 	}
-
-	return WLAN_STATUS_SUCCESS;
+	hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE80211,
+		       HOSTAPD_LEVEL_INFO,
+		       "Non-Multi-AP STA tries to associate with backhaul-only BSS");
+	return WLAN_STATUS_ASSOC_DENIED_UNSPEC;
 }
 
 static u16 copy_supp_rates(struct hostapd_data *hapd, struct sta_info *sta,
@@ -2507,7 +2529,7 @@  static u16 check_assoc_ies(struct hostapd_data *hapd, struct sta_info *sta,
 	if (resp != WLAN_STATUS_SUCCESS)
 		return resp;
 
-	resp = hostapd_validate_multi_ap_ie(hapd, sta, &elems);
+	resp = check_multi_ap(hapd, sta, elems.multi_ap, elems.multi_ap_len);
 	if (resp != WLAN_STATUS_SUCCESS)
 		return resp;