[PATCH] RADIUS client: Do not free pending messages when allocating an identifier

[alepacheco99 at gmail.com]

radius_client_get_id() removed and freed any pending message that used
the newly allocated identifier. This function is also called from
radius_client_retransmit() to assign a new identifier when updating
Acct-Delay-Time and that is called while radius_client_timer() and
radius_client_process_tls_handshake() iterate over the retransmit list.
Freeing list entries there could leave the callers using freed entries,
including the entry being retransmitted. In addition, num_msgs was not
updated for the removed entries.

The retransmit list is limited to RADIUS_CLIENT_MAX_ENTRIES messages, so
there is always an identifier that is not used by any pending message.
Skip identifiers that are in use instead of removing the pending
messages. This keeps the protection against matching a new reply with an
old request without modifying the list.

Add a module test for the identifier allocation.

Fixes: debde14b5 ("RADIUS: Add Acct-Delay-Time into accounting messages")
Signed-off-by: Alejandro Pacheco <alepacheco99 at gmail.com>
---
 hostapd/hapd_module_tests.c | 79 +++++++++++++++++++++++++++++++++++++
 src/radius/radius_client.c  | 44 ++++++++-------------
 2 files changed, 96 insertions(+), 27 deletions(-)

diff --git a/hostapd/hapd_module_tests.c b/hostapd/hapd_module_tests.c
index a5016f22b..6da08ac63 100644
--- a/hostapd/hapd_module_tests.c
+++ b/hostapd/hapd_module_tests.c
@@ -10,9 +10,88 @@

 #include "utils/common.h"
 #include "utils/module_tests.h"
+#include "radius/radius.h"
+#include "radius/radius_client.h"
+
+
+#ifndef CONFIG_NO_RADIUS
+static int radius_client_get_id_tests(void)
+{
+	struct hostapd_radius_servers conf;
+	struct hostapd_radius_server serv;
+	struct radius_client_data *radius;
+	struct radius_msg *msg;
+	u8 secret[] = "secret";
+	const u8 addr[ETH_ALEN] = { 0x02, 0x00, 0x00, 0x00, 0x00, 0x01 };
+	u8 ids[3];
+	char buf[2000];
+	unsigned int i, j;
+	int ret = -1;
+
+	wpa_printf(MSG_INFO, "radius_client_get_id() tests");
+
+	os_memset(&serv, 0, sizeof(serv));
+	if (hostapd_parse_ip_addr("127.0.0.1", &serv.addr) < 0)
+		return -1;
+	serv.port = 1;
+	serv.shared_secret = secret;
+	serv.shared_secret_len = os_strlen((char *) secret);
+
+	os_memset(&conf, 0, sizeof(conf));
+	conf.auth_servers = conf.auth_server = &serv;
+	conf.num_auth_servers = 1;
+
+	radius = radius_client_init(NULL, &conf);
+	if (!radius)
+		return -1;
+
+	for (i = 0; i < ARRAY_SIZE(ids); i++) {
+		ids[i] = radius_client_get_id(radius);
+		msg = radius_msg_new(RADIUS_CODE_ACCESS_REQUEST, ids[i]);
+		if (!msg)
+			goto fail;
+		if (radius_client_send(radius, msg, RADIUS_AUTH, addr) < 0) {
+			radius_msg_free(msg);
+			goto fail;
+		}
+	}
+
+	/* Identifiers of pending requests are not handed out again and the
+	 * pending requests remain queued. */
+	for (i = 0; i < 2 * 256; i++) {
+		u8 id = radius_client_get_id(radius);
+
+		for (j = 0; j < ARRAY_SIZE(ids); j++) {
+			if (id == ids[j]) {
+				wpa_printf(MSG_INFO,
+					   "radius_client_get_id() returned pending id %u",
+					   id);
+				goto fail;
+			}
+		}
+	}
+
+	if (radius_client_get_mib(radius, buf, sizeof(buf)) < 0 ||
+	    !os_strstr(buf, "radiusAuthClientPendingRequests=3\n")) {
+		wpa_printf(MSG_INFO,
+			   "radius_client_get_id() removed pending requests");
+		goto fail;
+	}
+
+	ret = 0;
+fail:
+	radius_client_deinit(radius);
+	return ret;
+}
+#endif /* CONFIG_NO_RADIUS */
+

 int hapd_module_tests(void)
 {
 	wpa_printf(MSG_INFO, "hostapd module tests");
+#ifndef CONFIG_NO_RADIUS
+	if (radius_client_get_id_tests() < 0)
+		return -1;
+#endif /* CONFIG_NO_RADIUS */
 	return 0;
 }
diff --git a/src/radius/radius_client.c b/src/radius/radius_client.c
index 16284e1fe..021fc34df 100644
--- a/src/radius/radius_client.c
+++ b/src/radius/radius_client.c
@@ -1522,37 +1522,27 @@ fail:
  * Returns: Allocated identifier
  *
  * This function is used to fetch a unique (among pending requests) identifier
- * for a new RADIUS message.
+ * for a new RADIUS message. Pending messages are not modified, so this can be
+ * called while iterating over the retransmit list.
  */
 u8 radius_client_get_id(struct radius_client_data *radius)
 {
-	struct radius_msg_list *entry, *prev, *_remove;
-	u8 id = radius->next_radius_identifier++;
-
-	/* remove entries with matching id from retransmit list to avoid
-	 * using new reply from the RADIUS server with an old request */
-	entry = radius->msgs;
-	prev = NULL;
-	while (entry) {
-		if (radius_msg_get_hdr(entry->msg)->identifier == id) {
-			hostapd_logger(radius->ctx, entry->addr,
-				       HOSTAPD_MODULE_RADIUS,
-				       HOSTAPD_LEVEL_DEBUG,
-				       "Removing pending RADIUS message, "
-				       "since its id (%d) is reused", id);
-			if (prev)
-				prev->next = entry->next;
-			else
-				radius->msgs = entry->next;
-			_remove = entry;
-		} else {
-			_remove = NULL;
-			prev = entry;
+	struct radius_msg_list *entry;
+	unsigned int i;
+	u8 id;
+
+	/* Skip identifiers that are in use in the retransmit list to avoid
+	 * using new reply from the RADIUS server with an old request. The list
+	 * is limited to RADIUS_CLIENT_MAX_ENTRIES (< 256) messages, so a free
+	 * identifier is always found. */
+	for (i = 0; i < 256; i++) {
+		id = radius->next_radius_identifier++;
+		for (entry = radius->msgs; entry; entry = entry->next) {
+			if (radius_msg_get_hdr(entry->msg)->identifier == id)
+				break;
 		}
-		entry = entry->next;
-
-		if (_remove)
-			radius_client_msg_free(_remove);
+		if (!entry)
+			break;
 	}

 	return id;
-- 
2.48.1