| Message ID | CAGnO3dp6U6ixE8vmP6SDYWyomPbxCBG-M9fcG4nB32sFJuweZw@mail.gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Sun, Mar 06, 2016 at 04:16:06PM +0000, Nick Lowe wrote: > The current behaviour of hostapd_das_find_sta() is undesirable > as it can result in over broad, potentially insecure matching. Could you please describe in detail why this is undesirable and what exactly would be "potential insecure"? > It is best is to define an order of precedence for session identifying > attributes, based on their specificity, and to match only by the most > specific attribute that is present in a CoA-Request or > Disconnect-Request packet. What is this based on? RFC 5176 is pretty clear on mandating _all_ the specified attributes matching. This patch would not be compliant with that. RFC 5176, Chapter 3. Attributes: In Disconnect-Request and CoA-Request packets, certain attributes are used to uniquely identify the NAS as well as user session(s) on the NAS. The combination of NAS and session identification attributes included in a CoA-Request or Disconnect-Request packet MUST match at least one session in order for a Request to be successful; otherwise a Disconnect-NAK or CoA-NAK MUST be sent. If all NAS identification attributes match, and more than one session matches all of the session identification attributes, then a CoA-Request or Disconnect- Request MUST apply to all matching sessions. > This order of precedence should be: > > Acct-Session-Id (Session) > Acct-Multi-Session-Id (Session) > Calling-Station-Id (Station) > Chargeable-User-Identity (User) > User-Name (User) It is up to the DAC to decide which filtering rules (and these are ANDed together) to use. If it knows Acct-Session-Id and Acct-Multi-Session-Id are supported, those should really be used. > Of particular concern is that the EAP outer identity, typically used > to populate the User-Name can often be anonymised in a way that spoofs > another active user. Sure, I would not a DAC to use User-Name with EAP authentication. Still, I see no reason to change DAS implementation for this, i.e., this is something to guide on the DAC side.. > Where we are given a specific CoA-Request or Disconnect-Request > packet, we should handle it as being such. As far as I can tell, the current implementation complies with the RFC 5176 requirements. You would need to provide quite a bit more justification to change that to something non-compliant.
Hi Jouni, Requiring a match against all the session identifying attributes supplied would be fine and, of course, an order of precedence would be not applicable and meaningless at this point. That would be stricter that what the that patch I submitted does. Currently hostapd implements faulty logic such that any session identifying attribute that matches is acceptable. Herein lies the fault in the implementation. In the case that more than one session is matched, hostapd currently elects to do nothing. If this was changed in the future to permit more than one session to be matched, this could result in unexpected sessions being changed or disconnected. At present, this may result in expected sessions not being changed or disconnected due to multiple sessions being matched. Where the User-Name is being sent as a session identifying attribute alongside others, this can be manipulated for to cause deliberate malfunction of CoA-Request and Disconnect-Request by stations. Cheers, Nick On Sun, Mar 6, 2016 at 6:34 PM, Jouni Malinen <j@w1.fi> wrote: > On Sun, Mar 06, 2016 at 04:16:06PM +0000, Nick Lowe wrote: >> The current behaviour of hostapd_das_find_sta() is undesirable >> as it can result in over broad, potentially insecure matching. > > Could you please describe in detail why this is undesirable and what > exactly would be "potential insecure"? > >> It is best is to define an order of precedence for session identifying >> attributes, based on their specificity, and to match only by the most >> specific attribute that is present in a CoA-Request or >> Disconnect-Request packet. > > What is this based on? RFC 5176 is pretty clear on mandating _all_ the > specified attributes matching. This patch would not be compliant with > that. > > RFC 5176, Chapter 3. Attributes: > > In Disconnect-Request and CoA-Request packets, certain attributes are > used to uniquely identify the NAS as well as user session(s) on the > NAS. The combination of NAS and session identification attributes > included in a CoA-Request or Disconnect-Request packet MUST match at > least one session in order for a Request to be successful; otherwise > a Disconnect-NAK or CoA-NAK MUST be sent. If all NAS identification > attributes match, and more than one session matches all of the > session identification attributes, then a CoA-Request or Disconnect- > Request MUST apply to all matching sessions. > >> This order of precedence should be: >> >> Acct-Session-Id (Session) >> Acct-Multi-Session-Id (Session) >> Calling-Station-Id (Station) >> Chargeable-User-Identity (User) >> User-Name (User) > > It is up to the DAC to decide which filtering rules (and these are ANDed > together) to use. If it knows Acct-Session-Id and Acct-Multi-Session-Id > are supported, those should really be used. > >> Of particular concern is that the EAP outer identity, typically used >> to populate the User-Name can often be anonymised in a way that spoofs >> another active user. > > Sure, I would not a DAC to use User-Name with EAP authentication. Still, > I see no reason to change DAS implementation for this, i.e., this is > something to guide on the DAC side.. > >> Where we are given a specific CoA-Request or Disconnect-Request >> packet, we should handle it as being such. > > As far as I can tell, the current implementation complies with the RFC > 5176 requirements. You would need to provide quite a bit more > justification to change that to something non-compliant. > > -- > Jouni Malinen PGP id EFC895FA
On Sun, Mar 06, 2016 at 08:23:01PM +0000, Nick Lowe wrote: > Requiring a match against all the session identifying attributes > supplied would be fine and, of course, an order of precedence would be > not applicable and meaningless at this point. > That would be stricter that what the that patch I submitted does. > > Currently hostapd implements faulty logic such that any session > identifying attribute that matches is acceptable. > Herein lies the fault in the implementation. Could you please be more specific here? The current implementation matches all the session identifying attributes and requires all of them to match. > In the case that more than one session is matched, hostapd currently > elects to do nothing. Does nothing is somewhat inaccurate. hostapd rejects the request in such a case with Error-Cause 508 (Multiple Session Selection Unsupported). > If this was changed in the future to permit more than one session to > be matched, this could result in unexpected sessions being changed or > disconnected. What would be unexpected? DAC better know what it is doing and if it does not use specific enough attributes, it'll get what it asks for.. > At present, this may result in expected sessions not being changed or > disconnected due to multiple sessions being matched. Only if DAC specified overly flexible identifying attributes. Or do you have a specific example of attributes where more than a single match were to be expected? > Where the User-Name is being sent as a session identifying attribute > alongside others, this can be manipulated for to cause deliberate > malfunction of CoA-Request and Disconnect-Request by stations. How would User-Name alongside others do anything here if the other attributes are specific enough to find a single match? Even if that User-Name were to match multiple sessions, only the one also matching the other, more specific, attributes would be identified.
From 53aaa132412f7e05799ea46df7f5685aa22d820e Mon Sep 17 00:00:00 2001 From: Nick Lowe <nick.lowe@lugatech.com> Date: Sun, 6 Mar 2016 16:12:44 +0000 Subject: [PATCH] The current behaviour of hostapd_das_find_sta() is undesirable as it can result in over broad, potentially insecure matching. It is best is to define an order of precedence for session identifying attributes, based on their specificity, and to match only by the most specific attribute that is present in a CoA-Request or Disconnect-Request packet. This order of precedence should be: Acct-Session-Id (Session) Acct-Multi-Session-Id (Session) Calling-Station-Id (Station) Chargeable-User-Identity (User) User-Name (User) Of particular concern is that the EAP outer identity, typically used to populate the User-Name can often be anonymised in a way that spoofs another active user. Where we are given a specific CoA-Request or Disconnect-Request packet, we should handle it as being such. Signed-off-by: Nick Lowe <nick.lowe@lugatech.com> --- src/ap/hostapd.c | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c index 9aaa9a6..6c2900e 100644 --- a/src/ap/hostapd.c +++ b/src/ap/hostapd.c @@ -654,23 +654,6 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, for (sta = hapd->sta_list; sta; sta = sta->next) sta->radius_das_match = 1; - if (attr->sta_addr) { - num_attr++; - sta = ap_get_sta(hapd, attr->sta_addr); - if (!sta) { - wpa_printf(MSG_DEBUG, - "RADIUS DAS: No Calling-Station-Id match"); - return NULL; - } - - selected = sta; - for (sta = hapd->sta_list; sta; sta = sta->next) { - if (sta != selected) - sta->radius_das_match = 0; - } - wpa_printf(MSG_DEBUG, "RADIUS DAS: Calling-Station-Id match"); - } - if (attr->acct_session_id) { num_attr++; if (attr->acct_session_id_len != 16) { @@ -698,8 +681,7 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, } wpa_printf(MSG_DEBUG, "RADIUS DAS: Acct-Session-Id match"); } - - if (attr->acct_multi_session_id) { + else if (attr->acct_multi_session_id) { num_attr++; if (attr->acct_multi_session_id_len != 16) { wpa_printf(MSG_DEBUG, @@ -734,8 +716,23 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, wpa_printf(MSG_DEBUG, "RADIUS DAS: Acct-Multi-Session-Id match"); } + else if (attr->sta_addr) { + num_attr++; + sta = ap_get_sta(hapd, attr->sta_addr); + if (!sta) { + wpa_printf(MSG_DEBUG, + "RADIUS DAS: No Calling-Station-Id match"); + return NULL; + } - if (attr->cui) { + selected = sta; + for (sta = hapd->sta_list; sta; sta = sta->next) { + if (sta != selected) + sta->radius_das_match = 0; + } + wpa_printf(MSG_DEBUG, "RADIUS DAS: Calling-Station-Id match"); + } + else if (attr->cui) { num_attr++; count = 0; @@ -761,8 +758,7 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, wpa_printf(MSG_DEBUG, "RADIUS DAS: Chargeable-User-Identity match"); } - - if (attr->user_name) { + else if (attr->user_name) { num_attr++; count = 0; @@ -791,8 +787,7 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, wpa_printf(MSG_DEBUG, "RADIUS DAS: User-Name match"); } - - if (num_attr == 0) { + else { /* * In theory, we could match all current associations, but it * seems safer to just reject requests that do not include any -- 2.7.2
v2 including a minor optimisation. The current behaviour of hostapd_das_find_sta() is undesirable as it can result in over broad, potentially insecure matching. It is best is to define an order of precedence for session identifying attributes, based on their specificity, and to match only by the most specific attribute that is present in a CoA-Request or Disconnect-Request packet. This order of precedence should be: Acct-Session-Id (Session) Acct-Multi-Session-Id (Session) Calling-Station-Id (Station) Chargeable-User-Identity (User) User-Name (User) Of particular concern is that the EAP outer identity, typically used to populate the User-Name can often be anonymised in a way that spoofs another active user. Where we are given a specific CoA-Request or Disconnect-Request packet, we should handle it as being such. Signed-off-by: Nick Lowe <nick.lowe@lugatech.com> --- src/ap/hostapd.c | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-) if (attr->acct_session_id_len != 16) { @@ -698,8 +681,7 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, } wpa_printf(MSG_DEBUG, "RADIUS DAS: Acct-Session-Id match"); } - - if (attr->acct_multi_session_id) { + else if (attr->acct_multi_session_id) { num_attr++; if (attr->acct_multi_session_id_len != 16) { wpa_printf(MSG_DEBUG, @@ -734,8 +716,23 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, wpa_printf(MSG_DEBUG, "RADIUS DAS: Acct-Multi-Session-Id match"); } + else if (attr->sta_addr) { + num_attr++; + sta = ap_get_sta(hapd, attr->sta_addr); + if (!sta) { + wpa_printf(MSG_DEBUG, + "RADIUS DAS: No Calling-Station-Id match"); + return NULL; + } - if (attr->cui) { + selected = sta; + for (sta = hapd->sta_list; sta; sta = sta->next) { + if (sta != selected) + sta->radius_das_match = 0; + } + wpa_printf(MSG_DEBUG, "RADIUS DAS: Calling-Station-Id match"); + } + else if (attr->cui) { num_attr++; count = 0; @@ -761,8 +758,7 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, wpa_printf(MSG_DEBUG, "RADIUS DAS: Chargeable-User-Identity match"); } - - if (attr->user_name) { + else if (attr->user_name) { num_attr++; count = 0; @@ -791,8 +787,7 @@ static struct sta_info * hostapd_das_find_sta(struct hostapd_data *hapd, wpa_printf(MSG_DEBUG, "RADIUS DAS: User-Name match"); } - - if (num_attr == 0) { + else { /* * In theory, we could match all current associations, but it * seems safer to just reject requests that do not include any