| Message ID | 20180806224857.14853-1-Mathy.Vanhoef@cs.kuleuven.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: ignore SA Query Requests with unknown payload data | expand |
On Tue, 2018-08-07 at 00:48 +0200, Mathy Vanhoef wrote: > When operating in station mode, ignore SA Query Request frames that > contain extra payload data. The kernel doesn't know how to handle these > frames. Instead, give userspace a chance to handle these frames. > > For example, with Operating Channel Validation, SA Query Requests may > now contain an extra Operating Channel Information (OCI) element as > payload data. The kernel should ignore these frames, since it does not > know how to properly handle them. Instead, let userspace process these > frames. > > Signed-off-by: Mathy Vanhoef <Mathy.Vanhoef@cs.kuleuven.be> > --- > For background on Operating Channel Validation, see: > https://mentor.ieee.org/802.11/dcn/17/11-17-1807-12-000m-defense-against-multi-channel-mitm-attacks-via-operating-channel-validation.docx > > A corresponding patchset was also recently submitted to Hostap, see "Add > support for Operating Channel Validation (OCV)". In a perfect world, this seems fine. However, what if wpa_s doesn't implement this (yet)? In that case, wouldn't it be better (or really required) to still respond to the SA query request in the kernel? Since you change wpa_s to subscribe to the relevant action frame: > + /* SA Query Request */ > + if (nl80211_register_action_frame(bss, (u8 *) "\x08\x00", 2) < 0) > + ret = -1; we could change the logic to be if (!frame_includes_OCV || !cfg80211_rx_mgmt(...)) respond_in_kernel(); I also think we shouldn't necessarily punt too short or otherwise malformed frames to userspace, what's the point? We currently drop/ignore those, and can continue to do so afaict? johannes
On Tue, 2018-08-14 at 13:11 +0200, Johannes Berg wrote: > On Tue, 2018-08-07 at 00:48 +0200, Mathy Vanhoef wrote: > > When operating in station mode, ignore SA Query Request frames that > > contain extra payload data. The kernel doesn't know how to handle these > > frames. Instead, give userspace a chance to handle these frames. > > > > For example, with Operating Channel Validation, SA Query Requests may > > now contain an extra Operating Channel Information (OCI) element as > > payload data. The kernel should ignore these frames, since it does not > > know how to properly handle them. Instead, let userspace process these > > frames. > > > > Signed-off-by: Mathy Vanhoef <Mathy.Vanhoef@cs.kuleuven.be> > > --- > > For background on Operating Channel Validation, see: > > https://mentor.ieee.org/802.11/dcn/17/11-17-1807-12-000m-defense-against-multi-channel-mitm-attacks-via-operating-channel-validation.docx > > > > A corresponding patchset was also recently submitted to Hostap, see "Add > > support for Operating Channel Validation (OCV)". > > In a perfect world, this seems fine. However, what if wpa_s doesn't > implement this (yet)? In that case, wouldn't it be better (or really > required) to still respond to the SA query request in the kernel? > > Since you change wpa_s to subscribe to the relevant action frame: > > > + /* SA Query Request */ > > + if (nl80211_register_action_frame(bss, (u8 *) "\x08\x00", 2) < 0) > > + ret = -1; > > we could change the logic to be > > if (!frame_includes_OCV || !cfg80211_rx_mgmt(...)) > respond_in_kernel(); > An easier alternative might be to push ieee80211_process_sa_query_req() to after ieee80211_rx_h_userspace_mgmt() so it won't see the frames if userspace claimed them, but I'm not sure how that works in AP mode where hostapd claims all frames - though I guess these aren't relevant in AP mode? johannes
On Tue, 2018-08-14 at 13:16 +0200, Johannes Berg wrote: > > > Since you change wpa_s to subscribe to the relevant action frame: > > > > > + /* SA Query Request */ > > > + if (nl80211_register_action_frame(bss, (u8 *) "\x08\x00", 2) < 0) > > > + ret = -1; > > > > we could change the logic to be > > > > if (!frame_includes_OCV || !cfg80211_rx_mgmt(...)) > > respond_in_kernel(); > > > > An easier alternative might be to push ieee80211_process_sa_query_req() > to after ieee80211_rx_h_userspace_mgmt() so it won't see the frames if > userspace claimed them, but I'm not sure how that works in AP mode where > hostapd claims all frames - though I guess these aren't relevant in AP > mode? However, then obviously wpa_s has to be able to handle them if OCV isn't included, which I haven't checked. It probably should be able to anyway though, since the frame might include other elements that aren't OCV, causing the kernel to punt it to wpa_s. johannes
> I also think we shouldn't necessarily punt too short or otherwise > malformed frames to userspace, what's the point? We currently > drop/ignore those, and can continue to do so afaict? Agreed, we should drop too short or malformed frames. > An easier alternative might be to push ieee80211_process_sa_query_req() > to after ieee80211_rx_h_userspace_mgmt() so it won't see the frames if > userspace claimed them, but I'm not sure how that works in AP mode where > hostapd claims all frames - though I guess these aren't relevant in AP > mode? > > However, then obviously wpa_s has to be able to handle them if OCV isn't > included, which I haven't checked. It probably should be able to anyway > though, since the frame might include other elements that aren't OCV, > causing the kernel to punt it to wpa_s. SA Query request frames are also relevant in AP mode. They are fully handled by hostapd currently. With the OCV patch, wpa_s will also be able to handle SA Query requests that don't contain the OCI element. So if userspace registered SA Query request frames, it makes sense to send *all* SA Query requests to userspace. Additionally, older versions of wpa_s won't register SA Query request frames, meaning the kernel will still handle them, and hence everything will still work normally. So to me, it make sense to only let the kernel reply to SA Query requests when operating in station mode *and* when the userspace didn't register SA Query frames. In that case, I suppose we can send a SA Query response as is done currently, i.e., any payload in the SA Query request is ignored? Cheers, Mathy
On Wed, 2018-08-15 at 00:54 -0400, Mathy Vanhoef wrote: > > An easier alternative might be to push ieee80211_process_sa_query_req() > > to after ieee80211_rx_h_userspace_mgmt() so it won't see the frames if > > userspace claimed them, but I'm not sure how that works in AP mode where > > hostapd claims all frames - though I guess these aren't relevant in AP > > mode? > > > > However, then obviously wpa_s has to be able to handle them if OCV isn't > > included, which I haven't checked. It probably should be able to anyway > > though, since the frame might include other elements that aren't OCV, > > causing the kernel to punt it to wpa_s. > > SA Query request frames are also relevant in AP mode. They are fully > handled by hostapd currently. OK. But I see that they've never been handled by the kernel there - we check and do only in STATION mode. > With the OCV patch, wpa_s will also be able to handle SA Query requests > that don't contain the OCI element. So if userspace registered SA Query > request frames, it makes sense to send *all* SA Query requests to > userspace. Additionally, older versions of wpa_s won't register SA > Query request frames, meaning the kernel will still handle them, and > hence everything will still work normally. Right, good. > So to me, it make sense to only let the kernel reply to SA Query > requests when operating in station mode *and* when the userspace didn't > register SA Query frames. In that case, I suppose we can send a SA > Query response as is done currently, i.e., any payload in the SA Query > request is ignored? Sure, I think that's fair. So basically that means we can just move the handling of SA query request to *after* userspace redirection - in anything but station mode nothing changes (hostapd asks for all action frames but handles this one and we don't), and without changes to wpa_s in station mode also nothing changes since it won't ask for the frame, and if it does ask for it then it can as well handle all ... johannes
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 932985ca4e66..9a4fb17bada7 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -2755,7 +2755,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx, struct sk_buff_head *frames) return RX_DROP_MONITOR; } -static void ieee80211_process_sa_query_req(struct ieee80211_sub_if_data *sdata, +static bool ieee80211_process_sa_query_req(struct ieee80211_sub_if_data *sdata, struct ieee80211_mgmt *mgmt, size_t len) { @@ -2765,23 +2765,23 @@ static void ieee80211_process_sa_query_req(struct ieee80211_sub_if_data *sdata, if (!ether_addr_equal(mgmt->da, sdata->vif.addr)) { /* Not to own unicast address */ - return; + return false; } if (!ether_addr_equal(mgmt->sa, sdata->u.mgd.bssid) || !ether_addr_equal(mgmt->bssid, sdata->u.mgd.bssid)) { /* Not from the current AP or not associated yet. */ - return; + return false; } - if (len < 24 + 1 + sizeof(resp->u.action.u.sa_query)) { - /* Too short SA Query request frame */ - return; + if (len != 24 + 1 + sizeof(resp->u.action.u.sa_query)) { + /* Too short SA Query request frame, or one we can't handle */ + return false; } skb = dev_alloc_skb(sizeof(*resp) + local->hw.extra_tx_headroom); if (skb == NULL) - return; + return false; skb_reserve(skb, local->hw.extra_tx_headroom); resp = skb_put_zero(skb, 24); @@ -2798,6 +2798,7 @@ static void ieee80211_process_sa_query_req(struct ieee80211_sub_if_data *sdata, WLAN_SA_QUERY_TR_ID_LEN); ieee80211_tx_skb(sdata, skb); + return true; } static ieee80211_rx_result debug_noinline @@ -3089,7 +3090,8 @@ ieee80211_rx_h_action(struct ieee80211_rx_data *rx) case WLAN_ACTION_SA_QUERY_REQUEST: if (sdata->vif.type != NL80211_IFTYPE_STATION) break; - ieee80211_process_sa_query_req(sdata, mgmt, len); + if (!ieee80211_process_sa_query_req(sdata, mgmt, len)) + break; goto handled; } break;
When operating in station mode, ignore SA Query Request frames that contain extra payload data. The kernel doesn't know how to handle these frames. Instead, give userspace a chance to handle these frames. For example, with Operating Channel Validation, SA Query Requests may now contain an extra Operating Channel Information (OCI) element as payload data. The kernel should ignore these frames, since it does not know how to properly handle them. Instead, let userspace process these frames. Signed-off-by: Mathy Vanhoef <Mathy.Vanhoef@cs.kuleuven.be> --- For background on Operating Channel Validation, see: https://mentor.ieee.org/802.11/dcn/17/11-17-1807-12-000m-defense-against-multi-channel-mitm-attacks-via-operating-channel-validation.docx A corresponding patchset was also recently submitted to Hostap, see "Add support for Operating Channel Validation (OCV)". net/mac80211/rx.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)