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@xxxxxxxxxxxx>
---
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
From 53aaa132412f7e05799ea46df7f5685aa22d820e Mon Sep 17 00:00:00 2001
From: Nick Lowe <nick.lowe@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
---
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
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
