[PATCH] Rework the Acct-Session-Id and Acct-Multi-Session-Id implementation to give better global and temporal uniqueness.

Nick Lowe nick.lowe at lugatech.com
Sat Jan 23 17:28:03 PST 2016


From: Nick Lowe <nick.lowe at lugatech.com>

Rework the Acct-Session-Id and Acct-Multi-Session-Id implementation to
give better global and temporal uniqueness.
Previously, only 32-bits of the Acct-Session-Id would contain random
data, the other 32-bits would be incremented.
Previously, the Acct-Multi-Session-id would not use random data.
Switch from two u32 variables to a single u64 for the Acct-Session-Id
and Acct-Multi-Session-Id.
Do not increment, this serves no legitimate purpose.
Exclusively use os_get_random to get quality random numbers, do not
use or mix in the time. Inherently take a dependency on /dev/urandom
working properly therefore.
Remove the global Acct-Session-Id and Acct-Multi-Session-Id values
that serve no legitimate purpose.

Signed-off-by: Nick Lowe <nick.lowe at lugatech.com>
---
 src/ap/accounting.c              | 41 ++++++++++++++++------------------------
 src/ap/accounting.h              |  5 +++--
 src/ap/hostapd.c                 | 20 +++++++++-----------
 src/ap/hostapd.h                 |  1 -
 src/ap/ieee802_1x.c              | 17 ++++++++---------
 src/ap/pmksa_cache_auth.c        | 15 ++++++---------
 src/ap/pmksa_cache_auth.h        |  3 +--
 src/ap/sta_info.c                |  3 ++-
 src/ap/sta_info.h                |  3 +--
 src/eapol_auth/eapol_auth_sm.c   | 17 ++++++-----------
 src/eapol_auth/eapol_auth_sm_i.h |  6 +-----
 11 files changed, 53 insertions(+), 78 deletions(-)

diff --git a/src/ap/accounting.c b/src/ap/accounting.c
index c60b3a6..95dc366 100644
--- a/src/ap/accounting.c
+++ b/src/ap/accounting.c
@@ -54,10 +54,9 @@ static struct radius_msg * accounting_msg(struct
hostapd_data *hapd,

  if ((hapd->conf->wpa & 2) &&
     !hapd->conf->disable_pmksa_caching &&
-    sta->eapol_sm && sta->eapol_sm->acct_multi_session_id_hi) {
- os_snprintf(buf, sizeof(buf), "%08X+%08X",
-    sta->eapol_sm->acct_multi_session_id_hi,
-    sta->eapol_sm->acct_multi_session_id_lo);
+    sta->eapol_sm && sta->eapol_sm->acct_multi_session_id) {
+ os_snprintf(buf, sizeof(buf), "%016lX",
+    sta->eapol_sm->acct_multi_session_id);
  if (!radius_msg_add_attr(
     msg, RADIUS_ATTR_ACCT_MULTI_SESSION_ID,
     (u8 *) buf, os_strlen(buf))) {
@@ -226,8 +225,8 @@ void accounting_sta_start(struct hostapd_data
*hapd, struct sta_info *sta)

  hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_RADIUS,
        HOSTAPD_LEVEL_INFO,
-       "starting accounting session %08X-%08X",
-       sta->acct_session_id_hi, sta->acct_session_id_lo);
+       "starting accounting session %016lX",
+       sta->acct_session_id);

  os_get_reltime(&sta->acct_session_start);
  sta->last_rx_bytes = sta->last_tx_bytes = 0;
@@ -384,22 +383,25 @@ void accounting_sta_stop(struct hostapd_data
*hapd, struct sta_info *sta)
  eloop_cancel_timeout(accounting_interim_update, hapd, sta);
  hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_RADIUS,
        HOSTAPD_LEVEL_INFO,
-       "stopped accounting session %08X-%08X",
-       sta->acct_session_id_hi,
-       sta->acct_session_id_lo);
+       "stopped accounting session %016lX",
+       sta->acct_session_id);
  sta->acct_session_started = 0;
  }
 }


-void accounting_sta_get_id(struct hostapd_data *hapd,
+int accounting_sta_get_id(struct hostapd_data *hapd,
   struct sta_info *sta)
 {
- sta->acct_session_id_lo = hapd->acct_session_id_lo++;
- if (hapd->acct_session_id_lo == 0) {
- hapd->acct_session_id_hi++;
+ /* Acct-Session-Id should be globally and temporarily unique.
+ * A high quality random number is required therefore.
+ * This could be be improved by switching to a GUID. */
+ if (os_get_random((u8 *) &sta->acct_session_id,
+  sizeof(sta->acct_session_id)) < 0) {
+ return -1;
  }
- sta->acct_session_id_hi = hapd->acct_session_id_hi;
+
+ return 0;
 }


@@ -466,17 +468,6 @@ static void accounting_report_state(struct
hostapd_data *hapd, int on)
  */
 int accounting_init(struct hostapd_data *hapd)
 {
- struct os_time now;
-
- /* Acct-Session-Id should be unique over reboots. Using a random number
- * is preferred. If that is not available, take the current time. Mix
- * in microseconds to make this more likely to be unique. */
- os_get_time(&now);
- if (os_get_random((u8 *) &hapd->acct_session_id_hi,
-  sizeof(hapd->acct_session_id_hi)) < 0)
- hapd->acct_session_id_hi = now.sec;
- hapd->acct_session_id_hi ^= now.usec;
-
  if (radius_client_register(hapd->radius, RADIUS_ACCT,
    accounting_receive, hapd))
  return -1;
diff --git a/src/ap/accounting.h b/src/ap/accounting.h
index dcc54ee..59c3dd1 100644
--- a/src/ap/accounting.h
+++ b/src/ap/accounting.h
@@ -10,9 +10,10 @@
 #define ACCOUNTING_H

 #ifdef CONFIG_NO_ACCOUNTING
-static inline void accounting_sta_get_id(struct hostapd_data *hapd,
+static inline int accounting_sta_get_id(struct hostapd_data *hapd,
  struct sta_info *sta)
 {
+ return 0;
 }

 static inline void accounting_sta_start(struct hostapd_data *hapd,
@@ -34,7 +35,7 @@ static inline void accounting_deinit(struct
hostapd_data *hapd)
 {
 }
 #else /* CONFIG_NO_ACCOUNTING */
-void accounting_sta_get_id(struct hostapd_data *hapd, struct sta_info *sta);
+int accounting_sta_get_id(struct hostapd_data *hapd, struct sta_info *sta);
 void accounting_sta_start(struct hostapd_data *hapd, struct sta_info *sta);
 void accounting_sta_stop(struct hostapd_data *hapd, struct sta_info *sta);
 int accounting_init(struct hostapd_data *hapd);
diff --git a/src/ap/hostapd.c b/src/ap/hostapd.c
index 2aa4f8c..3428697 100644
--- a/src/ap/hostapd.c
+++ b/src/ap/hostapd.c
@@ -673,7 +673,7 @@ static struct sta_info *
hostapd_das_find_sta(struct hostapd_data *hapd,

  if (attr->acct_session_id) {
  num_attr++;
- if (attr->acct_session_id_len != 17) {
+ if (attr->acct_session_id_len != 16) {
  wpa_printf(MSG_DEBUG,
    "RADIUS DAS: Acct-Session-Id cannot match");
  return NULL;
@@ -683,10 +683,9 @@ static struct sta_info *
hostapd_das_find_sta(struct hostapd_data *hapd,
  for (sta = hapd->sta_list; sta; sta = sta->next) {
  if (!sta->radius_das_match)
  continue;
- os_snprintf(buf, sizeof(buf), "%08X-%08X",
-    sta->acct_session_id_hi,
-    sta->acct_session_id_lo);
- if (os_memcmp(attr->acct_session_id, buf, 17) != 0)
+ os_snprintf(buf, sizeof(buf), "%016lX",
+    sta->acct_session_id);
+ if (os_memcmp(attr->acct_session_id, buf, 16) != 0)
  sta->radius_das_match = 0;
  else
  count++;
@@ -702,7 +701,7 @@ static struct sta_info *
hostapd_das_find_sta(struct hostapd_data *hapd,

  if (attr->acct_multi_session_id) {
  num_attr++;
- if (attr->acct_multi_session_id_len != 17) {
+ if (attr->acct_multi_session_id_len != 16) {
  wpa_printf(MSG_DEBUG,
    "RADIUS DAS: Acct-Multi-Session-Id cannot match");
  return NULL;
@@ -713,14 +712,13 @@ static struct sta_info *
hostapd_das_find_sta(struct hostapd_data *hapd,
  if (!sta->radius_das_match)
  continue;
  if (!sta->eapol_sm ||
-    !sta->eapol_sm->acct_multi_session_id_hi) {
+    !sta->eapol_sm->acct_multi_session_id) {
  sta->radius_das_match = 0;
  continue;
  }
- os_snprintf(buf, sizeof(buf), "%08X+%08X",
-    sta->eapol_sm->acct_multi_session_id_hi,
-    sta->eapol_sm->acct_multi_session_id_lo);
- if (os_memcmp(attr->acct_multi_session_id, buf, 17) !=
+ os_snprintf(buf, sizeof(buf), "%016lX",
+    sta->eapol_sm->acct_multi_session_id);
+ if (os_memcmp(attr->acct_multi_session_id, buf, 16) !=
     0)
  sta->radius_das_match = 0;
  else
diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
index 7b59f80..72f8252 100644
--- a/src/ap/hostapd.h
+++ b/src/ap/hostapd.h
@@ -138,7 +138,6 @@ struct hostapd_data {
  void *msg_ctx_parent; /* parent interface ctx for wpa_msg() calls */

  struct radius_client_data *radius;
- u32 acct_session_id_hi, acct_session_id_lo;
  struct radius_das_data *radius_das;

  struct iapp_data *iapp;
diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c
index 607f941..e153f08 100644
--- a/src/ap/ieee802_1x.c
+++ b/src/ap/ieee802_1x.c
@@ -438,9 +438,9 @@ static int add_common_radius_sta_attr(struct
hostapd_data *hapd,
  return -1;
  }

- if (sta->acct_session_id_hi || sta->acct_session_id_lo) {
- os_snprintf(buf, sizeof(buf), "%08X-%08X",
-    sta->acct_session_id_hi, sta->acct_session_id_lo);
+ if (sta->acct_session_id) {
+ os_snprintf(buf, sizeof(buf), "%016lX",
+    sta->acct_session_id);
  if (!radius_msg_add_attr(msg, RADIUS_ATTR_ACCT_SESSION_ID,
  (u8 *) buf, os_strlen(buf))) {
  wpa_printf(MSG_ERROR, "Could not add Acct-Session-Id");
@@ -2495,12 +2495,12 @@ int ieee802_1x_get_mib_sta(struct hostapd_data
*hapd, struct sta_info *sta,
   /* TODO: dot1xAuthSessionOctetsTx */
   /* TODO: dot1xAuthSessionFramesRx */
   /* TODO: dot1xAuthSessionFramesTx */
-  "dot1xAuthSessionId=%08X-%08X\n"
+  "dot1xAuthSessionId=%016lX\n"
   "dot1xAuthSessionAuthenticMethod=%d\n"
   "dot1xAuthSessionTime=%u\n"
   "dot1xAuthSessionTerminateCause=999\n"
   "dot1xAuthSessionUserName=%s\n",
-  sta->acct_session_id_hi, sta->acct_session_id_lo,
+  sta->acct_session_id,
   (wpa_key_mgmt_wpa_ieee8021x(
    wpa_auth_sta_key_mgmt(sta->wpa_sm))) ?
   1 : 2,
@@ -2510,11 +2510,10 @@ int ieee802_1x_get_mib_sta(struct hostapd_data
*hapd, struct sta_info *sta,
  return len;
  len += ret;

- if (sm->acct_multi_session_id_hi) {
+ if (sm->acct_multi_session_id) {
  ret = os_snprintf(buf + len, buflen - len,
-  "authMultiSessionId=%08X+%08X\n",
-  sm->acct_multi_session_id_hi,
-  sm->acct_multi_session_id_lo);
+  "authMultiSessionId=%016lX\n",
+  sm->acct_multi_session_id);
  if (os_snprintf_error(buflen - len, ret))
  return len;
  len += ret;
diff --git a/src/ap/pmksa_cache_auth.c b/src/ap/pmksa_cache_auth.c
index 83e4bda..b191e1c 100644
--- a/src/ap/pmksa_cache_auth.c
+++ b/src/ap/pmksa_cache_auth.c
@@ -148,8 +148,7 @@ static void pmksa_cache_from_eapol_data(struct
rsn_pmksa_cache_entry *entry,
  entry->eap_type_authsrv = eapol->eap_type_authsrv;
  entry->vlan_id = ((struct sta_info *) eapol->sta)->vlan_id;

- entry->acct_multi_session_id_hi = eapol->acct_multi_session_id_hi;
- entry->acct_multi_session_id_lo = eapol->acct_multi_session_id_lo;
+ entry->acct_multi_session_id = eapol->acct_multi_session_id;
 }


@@ -188,8 +187,7 @@ void pmksa_cache_to_eapol_data(struct
rsn_pmksa_cache_entry *entry,
  eapol->eap_type_authsrv = entry->eap_type_authsrv;
  ((struct sta_info *) eapol->sta)->vlan_id = entry->vlan_id;

- eapol->acct_multi_session_id_hi = entry->acct_multi_session_id_hi;
- eapol->acct_multi_session_id_lo = entry->acct_multi_session_id_lo;
+ eapol->acct_multi_session_id = entry->acct_multi_session_id;
 }


@@ -471,12 +469,11 @@ static int das_attr_match(struct
rsn_pmksa_cache_entry *entry,
  if (attr->acct_multi_session_id) {
  char buf[20];

- if (attr->acct_multi_session_id_len != 17)
+ if (attr->acct_multi_session_id_len != 16)
  return 0;
- os_snprintf(buf, sizeof(buf), "%08X+%08X",
-    entry->acct_multi_session_id_hi,
-    entry->acct_multi_session_id_lo);
- if (os_memcmp(attr->acct_multi_session_id, buf, 17) != 0)
+ os_snprintf(buf, sizeof(buf), "%016lX",
+    entry->acct_multi_session_id);
+ if (os_memcmp(attr->acct_multi_session_id, buf, 16) != 0)
  return 0;
  match++;
  }
diff --git a/src/ap/pmksa_cache_auth.h b/src/ap/pmksa_cache_auth.h
index b2da379..4dc841f 100644
--- a/src/ap/pmksa_cache_auth.h
+++ b/src/ap/pmksa_cache_auth.h
@@ -31,8 +31,7 @@ struct rsn_pmksa_cache_entry {
  int vlan_id;
  int opportunistic;

- u32 acct_multi_session_id_hi;
- u32 acct_multi_session_id_lo;
+ u64 acct_multi_session_id;
 };

 struct rsn_pmksa_cache;
diff --git a/src/ap/sta_info.c b/src/ap/sta_info.c
index 8bba73c..39b9d12 100644
--- a/src/ap/sta_info.c
+++ b/src/ap/sta_info.c
@@ -625,7 +625,8 @@ struct sta_info * ap_sta_add(struct hostapd_data
*hapd, const u8 *addr)
  return NULL;
  }
  sta->acct_interim_interval = hapd->conf->acct_interim_interval;
- accounting_sta_get_id(hapd, sta);
+ if (accounting_sta_get_id(hapd, sta) < 0)
+ return NULL;

  if (!(hapd->iface->drv_flags & WPA_DRIVER_FLAGS_INACTIVITY_TIMER)) {
  wpa_printf(MSG_DEBUG, "%s: register ap_handle_timer timeout "
diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h
index e3b4915..359f480 100644
--- a/src/ap/sta_info.h
+++ b/src/ap/sta_info.h
@@ -101,8 +101,7 @@ struct sta_info {
  /* IEEE 802.1X related data */
  struct eapol_state_machine *eapol_sm;

- u32 acct_session_id_hi;
- u32 acct_session_id_lo;
+ u64 acct_session_id;
  struct os_reltime acct_session_start;
  int acct_session_started;
  int acct_terminate_cause; /* Acct-Terminate-Cause */
diff --git a/src/eapol_auth/eapol_auth_sm.c b/src/eapol_auth/eapol_auth_sm.c
index ff33d28..771ee23 100644
--- a/src/eapol_auth/eapol_auth_sm.c
+++ b/src/eapol_auth/eapol_auth_sm.c
@@ -866,10 +866,12 @@ eapol_auth_alloc(struct eapol_authenticator
*eapol, const u8 *addr,
  sm->radius_cui = wpabuf_alloc_copy(radius_cui,
    os_strlen(radius_cui));

- sm->acct_multi_session_id_lo = eapol->acct_multi_session_id_lo++;
- if (eapol->acct_multi_session_id_lo == 0)
- eapol->acct_multi_session_id_hi++;
- sm->acct_multi_session_id_hi = eapol->acct_multi_session_id_hi;
+ /* Acct-Multi-Session-Id should be globally and temporarily unique.
+ * A high quality random number is required therefore.
+ * This could be be improved by switching to a GUID. */
+ if (os_get_random((u8 *) &sm->acct_multi_session_id,
+ sizeof(sm->acct_multi_session_id)) < 0)
+ return NULL;

  return sm;
 }
@@ -1271,7 +1273,6 @@ struct eapol_authenticator *
eapol_auth_init(struct eapol_auth_config *conf,
      struct eapol_auth_cb *cb)
 {
  struct eapol_authenticator *eapol;
- struct os_time now;

  eapol = os_zalloc(sizeof(*eapol));
  if (eapol == NULL)
@@ -1300,12 +1301,6 @@ struct eapol_authenticator *
eapol_auth_init(struct eapol_auth_config *conf,
  eapol->cb.erp_get_key = cb->erp_get_key;
  eapol->cb.erp_add_key = cb->erp_add_key;

- /* Acct-Multi-Session-Id should be unique over reboots. If reliable
- * clock is not available, this could be replaced with reboot counter,
- * etc. */
- os_get_time(&now);
- eapol->acct_multi_session_id_hi = now.sec;
-
  return eapol;
 }

diff --git a/src/eapol_auth/eapol_auth_sm_i.h b/src/eapol_auth/eapol_auth_sm_i.h
index aa3e117..04386b2 100644
--- a/src/eapol_auth/eapol_auth_sm_i.h
+++ b/src/eapol_auth/eapol_auth_sm_i.h
@@ -30,9 +30,6 @@ struct eapol_authenticator {

  u8 *default_wep_key;
  u8 default_wep_key_idx;
-
- u32 acct_multi_session_id_hi;
- u32 acct_multi_session_id_lo;
 };


@@ -173,8 +170,7 @@ struct eapol_state_machine {

  int remediation;

- u32 acct_multi_session_id_hi;
- u32 acct_multi_session_id_lo;
+ u64 acct_multi_session_id;
 };

 #endif /* EAPOL_AUTH_SM_I_H */
-- 
2.5.0



More information about the Hostap mailing list