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>

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;

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);

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

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;

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;

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++;
 	}

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;

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 "

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 */

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;
 }
 

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 */