RADIUS: Use more likely unique accounting Acct-{,Multi-}Session-Id

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@lugatech.com>

diff --git a/src/ap/accounting.c b/src/ap/accounting.c
index abf3cf1..962a869 100644 (file)
--- a/src/ap/accounting.c
+++ b/src/ap/accounting.c
@@ -55,10 +55,10 @@ 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",
+                                   (long unsigned int)
+                                   sta->eapol_sm->acct_multi_session_id);
                        if (!radius_msg_add_attr(
                                    msg, RADIUS_ATTR_ACCT_MULTI_SESSION_ID,
                                    (u8 *) buf, os_strlen(buf))) {
@@ -236,8 +236,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",
+                      (long unsigned int) sta->acct_session_id);
 
        os_get_reltime(&sta->acct_session_start);
        sta->last_rx_bytes = sta->last_tx_bytes = 0;
@@ -386,22 +386,22 @@ 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",
+                              (long unsigned int) sta->acct_session_id);
                sta->acct_session_started = 0;
        }
 }
 
 
-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)
 {
-       sta->acct_session_id_lo = hapd->acct_session_id_lo++;
-       if (hapd->acct_session_id_lo == 0) {
-               hapd->acct_session_id_hi++;
-       }
-       sta->acct_session_id_hi = 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.
+        */
+       return os_get_random((u8 *) &sta->acct_session_id,
+                            sizeof(sta->acct_session_id));
 }
 
 
@@ -460,17 +460,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..de5a33f 100644 (file)
--- 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,
-                                        struct sta_info *sta)
+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..a848f35 100644 (file)
--- 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",
+                                   (long unsigned int) 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,14 @@ 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",
+                                   (long unsigned int)
+                                   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 (file)
--- 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 d09267a..0c25f49 100644 (file)
--- 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",
+                           (long unsigned int) 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");
@@ -2493,12 +2493,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,
+                         (long unsigned int) sta->acct_session_id,
                          (wpa_key_mgmt_wpa_ieee8021x(
                                   wpa_auth_sta_key_mgmt(sta->wpa_sm))) ?
                          1 : 2,
@@ -2508,11 +2508,11 @@ 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",
+                                 (long unsigned int)
+                                 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..eb37c78 100644 (file)
--- 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",
+                           (long unsigned int) 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 (file)
--- 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..0583a31 100644 (file)
--- a/src/ap/sta_info.c
+++ b/src/ap/sta_info.c
@@ -625,7 +625,10 @@ 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) {
+               os_free(sta);
+               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 (file)
--- 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 cdbec4e..62db368 100644 (file)
--- a/src/eapol_auth/eapol_auth_sm.c
+++ b/src/eapol_auth/eapol_auth_sm.c
@@ -866,10 +866,16 @@ 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) {
+               eapol_auth_free(sm);
+               return NULL;
+       }
 
        return sm;
 }
@@ -1274,7 +1280,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)
@@ -1303,12 +1308,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 (file)
--- 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 */