From patchwork Tue Nov 18 18:54:50 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jouni Malinen X-Patchwork-Id: 412128 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from maxx.maxx.shmoo.com (maxx.shmoo.com [205.134.188.171]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44F3F14014D for ; Wed, 19 Nov 2014 05:55:17 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id B1DDE17C130; Tue, 18 Nov 2014 13:55:13 -0500 (EST) X-Virus-Scanned: amavisd-new at maxx.shmoo.com Received: from maxx.maxx.shmoo.com ([127.0.0.1]) by localhost (maxx.shmoo.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qA5Igk3BH1zw; Tue, 18 Nov 2014 13:55:13 -0500 (EST) Received: from maxx.shmoo.com (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id 695B417C168; Tue, 18 Nov 2014 13:55:07 -0500 (EST) X-Original-To: mailman-post+hostap@maxx.shmoo.com Delivered-To: mailman-post+hostap@maxx.shmoo.com Received: from localhost (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id 83BE517C130 for ; Tue, 18 Nov 2014 13:55:05 -0500 (EST) X-Virus-Scanned: amavisd-new at maxx.shmoo.com Received: from maxx.maxx.shmoo.com ([127.0.0.1]) by localhost (maxx.shmoo.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JQ9fj-+AImP2 for ; Tue, 18 Nov 2014 13:54:58 -0500 (EST) Received: from li674-96.members.linode.com (mail.w1.fi [212.71.239.96]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id 3684717C179 for ; Tue, 18 Nov 2014 13:54:55 -0500 (EST) Received: from jm (188-67-125-155.bb.dnainternet.fi [188.67.125.155]) by li674-96.members.linode.com (Postfix) with ESMTPSA id CF378112A6 for ; Tue, 18 Nov 2014 18:54:50 +0000 (UTC) Received: by jm (sSMTP sendmail emulation); Tue, 18 Nov 2014 20:54:50 +0200 Date: Tue, 18 Nov 2014 20:54:50 +0200 From: Jouni Malinen To: hostap@lists.shmoo.com Subject: Re: [PATCH] AP: Drop retransmitted auth/assoc frames Message-ID: <20141118185450.GA6887@w1.fi> Mail-Followup-To: hostap@lists.shmoo.com References: <1415177437-11016-1-git-send-email-ilan.peer@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1415177437-11016-1-git-send-email-ilan.peer@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: hostap@lists.shmoo.com X-Mailman-Version: 2.1.11 Precedence: list List-Id: HostAP Project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: hostap-bounces@lists.shmoo.com Errors-To: hostap-bounces@lists.shmoo.com On Wed, Nov 05, 2014 at 03:50:34AM -0500, Ilan Peer wrote: > It is possible that a station device might miss an ACK for > an authentication or association frame, and thus retransmit the > same frame although the frame is already being processed in the stack. > > In such a case, the local AP will process the retransmitted frame although > it has already handled the request, which might cause the station to get > confused and as a result disconnect from the AP, blacklist it etc. > > To avoid such a case, save the sequence control of the last processed > management frame and in case of retransmissions drop them. Thanks. This should really be the responsibility of the driver or well, kernel code (mac80211) that takes care of duplicate detection in general. Anyway, I'm thinking of adding this into hostapd taken into account mac80211 does not handle the pre-association cases today and it is unclear whether that gets added there in the short term. However, there are couple of things in this specific version that do not look desirable: > @@ -1945,6 +1979,8 @@ static void handle_assoc_cb(struct hostapd_data *hapd, > + sta->last_seq_ctrl = WLAN_INVALID_MGMT_SEQ; This would prevent duplicate detection for a case where the duplicated (re)association request frame gets delivered to hostapd after (re)association response frame TX status is processed. Apparently that can happen in some cases. Same for authentication and well, there is is actually even more complex with different number of roundtrips getting used. Resetting last_seq_ctrl to WLAN_INVALID_MGMT_SEQ could potentially be done "some time" after completion of association processing, but I'm not sure whether it is really worth the effort to reduce the already pretty minimal likelihood of hitting false duplicates with the addition I describe below. > diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h > + /* last auth/assoc seq. control */ > + u16 last_seq_ctrl; Since this is not perfect in the sense of not seeing all management frames from the STA, it would likely be useful to track the last management frame subtype as well to reduce likelihood of dropping non-duplicates in some unexpected cases. I would also like to get Action frames covered (well for parts where there is an actual STA entry present or added like GAS). I've been testing with the following version and it seems to be working pretty well, so I'm planning on committing it shortly unless someone comes up with concerns. [PATCH] AP: Drop retransmitted auth/assoc/action frames It is possible that a station device might miss an ACK for an authentication, association, or action frame, and thus retransmit the same frame although the frame is already being processed in the stack. While the duplicated frame should really be dropped in the kernel or firmware code where duplicate detection is implemented for data frames, it is possible that pre-association cases are not fully addressed (which is the case at least with mac80211 today) and the frame may be delivered to upper layer stack. In such a case, the local AP will process the retransmitted frame although it has already handled the request, which might cause the station to get confused and as a result disconnect from the AP, blacklist it, etc. To avoid such a case, save the sequence control of the last processed management frame and in case of retransmissions drop them. Signed-off-by: Ilan Peer --- src/ap/ieee802_11.c | 88 +++++++++++++++++++++++++++++++++++++------- src/ap/sta_info.c | 3 ++ src/ap/sta_info.h | 5 +++ src/common/ieee802_11_defs.h | 2 + 4 files changed, 85 insertions(+), 13 deletions(-) diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c index 4e389d0..8b903b3 100644 --- a/src/ap/ieee802_11.c +++ b/src/ap/ieee802_11.c @@ -709,6 +709,7 @@ static void handle_auth(struct hostapd_data *hapd, size_t resp_ies_len = 0; char *identity = NULL; char *radius_cui = NULL; + u16 seq_ctrl; if (len < IEEE80211_HDRLEN + sizeof(mgmt->u.auth)) { wpa_printf(MSG_INFO, "handle_auth - too short payload (len=%lu)", @@ -730,6 +731,7 @@ static void handle_auth(struct hostapd_data *hapd, auth_transaction = le_to_host16(mgmt->u.auth.auth_transaction); status_code = le_to_host16(mgmt->u.auth.status_code); fc = le_to_host16(mgmt->frame_control); + seq_ctrl = le_to_host16(mgmt->seq_ctrl); if (len >= IEEE80211_HDRLEN + sizeof(mgmt->u.auth) + 2 + WLAN_AUTH_CHALLENGE_LEN && @@ -738,10 +740,12 @@ static void handle_auth(struct hostapd_data *hapd, challenge = &mgmt->u.auth.variable[2]; wpa_printf(MSG_DEBUG, "authentication: STA=" MACSTR " auth_alg=%d " - "auth_transaction=%d status_code=%d wep=%d%s", + "auth_transaction=%d status_code=%d wep=%d%s " + "seq_ctrl=0x%x%s", MAC2STR(mgmt->sa), auth_alg, auth_transaction, status_code, !!(fc & WLAN_FC_ISWEP), - challenge ? " challenge" : ""); + challenge ? " challenge" : "", + seq_ctrl, (fc & WLAN_FC_RETRY) ? " retry" : ""); if (hapd->tkip_countermeasures) { resp = WLAN_REASON_MICHAEL_MIC_FAILURE; @@ -802,21 +806,36 @@ static void handle_auth(struct hostapd_data *hapd, return; } + sta = ap_get_sta(hapd, mgmt->sa); + if (sta) { + if ((fc & WLAN_FC_RETRY) && + sta->last_seq_ctrl != WLAN_INVALID_MGMT_SEQ && + sta->last_seq_ctrl == seq_ctrl && + sta->last_subtype == WLAN_FC_STYPE_AUTH) { + hostapd_logger(hapd, sta->addr, + HOSTAPD_MODULE_IEEE80211, + HOSTAPD_LEVEL_DEBUG, + "Drop repeated authentication frame seq_ctrl=0x%x", + seq_ctrl); + return; + } + } else { #ifdef CONFIG_MESH - if (hapd->conf->mesh & MESH_ENABLED) { - /* if the mesh peer is not available, we don't do auth. */ - sta = ap_get_sta(hapd, mgmt->sa); - if (!sta) + if (hapd->conf->mesh & MESH_ENABLED) { + /* if the mesh peer is not available, we don't do auth. + */ return; - } else + } #endif /* CONFIG_MESH */ - { + sta = ap_sta_add(hapd, mgmt->sa); if (!sta) { resp = WLAN_STATUS_AP_UNABLE_TO_HANDLE_NEW_STA; goto fail; } } + sta->last_seq_ctrl = seq_ctrl; + sta->last_subtype = WLAN_FC_STYPE_AUTH; if (vlan_id > 0) { if (!hostapd_vlan_id_valid(hapd->conf->vlan, vlan_id)) { @@ -1464,7 +1483,7 @@ static void handle_assoc(struct hostapd_data *hapd, const struct ieee80211_mgmt *mgmt, size_t len, int reassoc) { - u16 capab_info, listen_interval; + u16 capab_info, listen_interval, seq_ctrl, fc; u16 resp = WLAN_STATUS_SUCCESS; const u8 *pos; int left, i; @@ -1497,15 +1516,19 @@ static void handle_assoc(struct hostapd_data *hapd, } #endif /* CONFIG_TESTING_OPTIONS */ + fc = le_to_host16(mgmt->frame_control); + seq_ctrl = le_to_host16(mgmt->seq_ctrl); + if (reassoc) { capab_info = le_to_host16(mgmt->u.reassoc_req.capab_info); listen_interval = le_to_host16( mgmt->u.reassoc_req.listen_interval); wpa_printf(MSG_DEBUG, "reassociation request: STA=" MACSTR " capab_info=0x%02x listen_interval=%d current_ap=" - MACSTR, + MACSTR " seq_ctrl=0x%x%s", MAC2STR(mgmt->sa), capab_info, listen_interval, - MAC2STR(mgmt->u.reassoc_req.current_ap)); + MAC2STR(mgmt->u.reassoc_req.current_ap), + seq_ctrl, (fc & WLAN_FC_RETRY) ? " retry" : ""); left = len - (IEEE80211_HDRLEN + sizeof(mgmt->u.reassoc_req)); pos = mgmt->u.reassoc_req.variable; } else { @@ -1513,8 +1536,10 @@ static void handle_assoc(struct hostapd_data *hapd, listen_interval = le_to_host16( mgmt->u.assoc_req.listen_interval); wpa_printf(MSG_DEBUG, "association request: STA=" MACSTR - " capab_info=0x%02x listen_interval=%d", - MAC2STR(mgmt->sa), capab_info, listen_interval); + " capab_info=0x%02x listen_interval=%d " + "seq_ctrl=0x%x%s", + MAC2STR(mgmt->sa), capab_info, listen_interval, + seq_ctrl, (fc & WLAN_FC_RETRY) ? " retry" : ""); left = len - (IEEE80211_HDRLEN + sizeof(mgmt->u.assoc_req)); pos = mgmt->u.assoc_req.variable; } @@ -1540,6 +1565,21 @@ static void handle_assoc(struct hostapd_data *hapd, return; } + if ((fc & WLAN_FC_RETRY) && + sta->last_seq_ctrl != WLAN_INVALID_MGMT_SEQ && + sta->last_seq_ctrl == seq_ctrl && + sta->last_subtype == reassoc ? WLAN_FC_STYPE_REASSOC_REQ : + WLAN_FC_STYPE_ASSOC_REQ) { + hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE80211, + HOSTAPD_LEVEL_DEBUG, + "Drop repeated association frame seq_ctrl=0x%x", + seq_ctrl); + return; + } + sta->last_seq_ctrl = seq_ctrl; + sta->last_subtype = reassoc ? WLAN_FC_STYPE_REASSOC_REQ : + WLAN_FC_STYPE_ASSOC_REQ; + if (hapd->tkip_countermeasures) { resp = WLAN_REASON_MICHAEL_MIC_FAILURE; goto fail; @@ -1665,6 +1705,7 @@ static void handle_disassoc(struct hostapd_data *hapd, } ap_sta_set_authorized(hapd, sta, 0); + sta->last_seq_ctrl = WLAN_INVALID_MGMT_SEQ; sta->flags &= ~(WLAN_STA_ASSOC | WLAN_STA_ASSOC_REQ_OK); wpa_auth_sm_event(sta->wpa_sm, WPA_DISASSOC); hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE80211, @@ -1716,6 +1757,7 @@ static void handle_deauth(struct hostapd_data *hapd, } ap_sta_set_authorized(hapd, sta, 0); + sta->last_seq_ctrl = WLAN_INVALID_MGMT_SEQ; sta->flags &= ~(WLAN_STA_AUTH | WLAN_STA_ASSOC | WLAN_STA_ASSOC_REQ_OK); wpa_auth_sm_event(sta->wpa_sm, WPA_DEAUTH); @@ -1815,6 +1857,26 @@ static int handle_action(struct hostapd_data *hapd, } #endif /* CONFIG_IEEE80211W */ + if (sta) { + u16 fc = le_to_host16(mgmt->frame_control); + u16 seq_ctrl = le_to_host16(mgmt->seq_ctrl); + + if ((fc & WLAN_FC_RETRY) && + sta->last_seq_ctrl != WLAN_INVALID_MGMT_SEQ && + sta->last_seq_ctrl == seq_ctrl && + sta->last_subtype == WLAN_FC_STYPE_ACTION) { + hostapd_logger(hapd, sta->addr, + HOSTAPD_MODULE_IEEE80211, + HOSTAPD_LEVEL_DEBUG, + "Drop repeated action frame seq_ctrl=0x%x", + seq_ctrl); + return 1; + } + + sta->last_seq_ctrl = seq_ctrl; + sta->last_subtype = WLAN_FC_STYPE_ACTION; + } + switch (mgmt->u.action.category) { #ifdef CONFIG_IEEE80211R case WLAN_ACTION_FT: diff --git a/src/ap/sta_info.c b/src/ap/sta_info.c index c48222e..19292a4 100644 --- a/src/ap/sta_info.c +++ b/src/ap/sta_info.c @@ -604,6 +604,7 @@ struct sta_info * ap_sta_add(struct hostapd_data *hapd, const u8 *addr) ap_sta_hash_add(hapd, sta); sta->ssid = &hapd->conf->ssid; ap_sta_remove_in_other_bss(hapd, sta); + sta->last_seq_ctrl = WLAN_INVALID_MGMT_SEQ; return sta; } @@ -668,6 +669,7 @@ void ap_sta_disassociate(struct hostapd_data *hapd, struct sta_info *sta, { wpa_printf(MSG_DEBUG, "%s: disassociate STA " MACSTR, hapd->conf->iface, MAC2STR(sta->addr)); + sta->last_seq_ctrl = WLAN_INVALID_MGMT_SEQ; sta->flags &= ~(WLAN_STA_ASSOC | WLAN_STA_ASSOC_REQ_OK); ap_sta_set_authorized(hapd, sta, 0); sta->timeout_next = STA_DEAUTH; @@ -706,6 +708,7 @@ void ap_sta_deauthenticate(struct hostapd_data *hapd, struct sta_info *sta, { wpa_printf(MSG_DEBUG, "%s: deauthenticate STA " MACSTR, hapd->conf->iface, MAC2STR(sta->addr)); + sta->last_seq_ctrl = WLAN_INVALID_MGMT_SEQ; sta->flags &= ~(WLAN_STA_AUTH | WLAN_STA_ASSOC); ap_sta_set_authorized(hapd, sta, 0); sta->timeout_next = STA_REMOVE; diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h index 76e34e4..89f0ef8 100644 --- a/src/ap/sta_info.h +++ b/src/ap/sta_info.h @@ -158,6 +158,11 @@ struct sta_info { #endif /* CONFIG_SAE */ u32 session_timeout; /* valid only if session_timeout_set == 1 */ + + /* Last Authentication/(Re)Association Request frame seuence control */ + u16 last_seq_ctrl; + /* Last Authentication/(Re)Association Request frame subtype */ + u8 last_subtype; }; diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h index 14a0ddc..10ad2ee 100644 --- a/src/common/ieee802_11_defs.h +++ b/src/common/ieee802_11_defs.h @@ -25,6 +25,8 @@ #define WLAN_FC_GET_TYPE(fc) (((fc) & 0x000c) >> 2) #define WLAN_FC_GET_STYPE(fc) (((fc) & 0x00f0) >> 4) +#define WLAN_INVALID_MGMT_SEQ 0xFFFF + #define WLAN_GET_SEQ_FRAG(seq) ((seq) & (BIT(3) | BIT(2) | BIT(1) | BIT(0))) #define WLAN_GET_SEQ_SEQ(seq) \ (((seq) & (~(BIT(3) | BIT(2) | BIT(1) | BIT(0)))) >> 4)