149 lines
5.5 KiB
Diff
149 lines
5.5 KiB
Diff
From 726432d7622cc0088ac353d073b59628b590ea44 Mon Sep 17 00:00:00 2001
|
|
From: Jouni Malinen <j@w1.fi>
|
|
Date: Sat, 25 Jan 2025 11:21:16 +0200
|
|
Subject: [PATCH] RADIUS: Drop pending request only when accepting the response
|
|
References: bsc#1239461, CVE-2025-24912
|
|
|
|
The case of an invalid authenticator in a RADIUS response could imply
|
|
that the response is not from the correct RADIUS server and as such,
|
|
such a response should be discarded without changing internal state for
|
|
the pending request. The case of an unknown response (RADIUS_RX_UNKNOWN)
|
|
is somewhat more complex since it could have been indicated before
|
|
validating the authenticator. In any case, it seems better to change the
|
|
state for the pending request only when we have fully accepted the
|
|
response.
|
|
|
|
Allowing the internal state of pending RADIUS request to change based on
|
|
responses that are not fully validation could have allow at least a
|
|
theoretical DoS attack if an attacker were to have means for injecting
|
|
RADIUS messages to the network using the IP address of the real RADIUS
|
|
server and being able to do so more quickly than the real server and
|
|
with the matching identifier from the request header (i.e., either by
|
|
flooding 256 responses quickly or by having means to capture the RADIUS
|
|
request). These should not really be realistic options in a properly
|
|
protected deployment, but nevertheless it is good to be more careful in
|
|
processing RADIUS responses.
|
|
|
|
Remove a pending RADIUS request from the internal list only when having
|
|
fully accepted a matching RADIUS response, i.e., after one of the
|
|
registered handlers has confirmed that the authenticator is valid and
|
|
processing of the response has succeeded.
|
|
|
|
Signed-off-by: Jouni Malinen <j@w1.fi>
|
|
---
|
|
src/radius/radius_client.c | 15 +++++++--------
|
|
1 file changed, 7 insertions(+), 8 deletions(-)
|
|
|
|
diff --git a/src/radius/radius_client.c b/src/radius/radius_client.c
|
|
index 2a7f36170..7909b29a7 100644
|
|
--- a/src/radius/radius_client.c
|
|
+++ b/src/radius/radius_client.c
|
|
@@ -1259,13 +1259,6 @@ static void radius_client_receive(int sock, void *eloop_ctx, void *sock_ctx)
|
|
roundtrip / 100, roundtrip % 100);
|
|
rconf->round_trip_time = roundtrip;
|
|
|
|
- /* Remove ACKed RADIUS packet from retransmit list */
|
|
- if (prev_req)
|
|
- prev_req->next = req->next;
|
|
- else
|
|
- radius->msgs = req->next;
|
|
- radius->num_msgs--;
|
|
-
|
|
for (i = 0; i < num_handlers; i++) {
|
|
RadiusRxResult res;
|
|
res = handlers[i].handler(msg, req->msg, req->shared_secret,
|
|
@@ -1276,6 +1269,13 @@ static void radius_client_receive(int sock, void *eloop_ctx, void *sock_ctx)
|
|
radius_msg_free(msg);
|
|
/* fall through */
|
|
case RADIUS_RX_QUEUED:
|
|
+ /* Remove ACKed RADIUS packet from retransmit list */
|
|
+ if (prev_req)
|
|
+ prev_req->next = req->next;
|
|
+ else
|
|
+ radius->msgs = req->next;
|
|
+ radius->num_msgs--;
|
|
+
|
|
radius_client_msg_free(req);
|
|
return;
|
|
case RADIUS_RX_INVALID_AUTHENTICATOR:
|
|
@@ -1297,7 +1297,6 @@ static void radius_client_receive(int sock, void *eloop_ctx, void *sock_ctx)
|
|
msg_type, hdr->code, hdr->identifier,
|
|
invalid_authenticator ? " [INVALID AUTHENTICATOR]" :
|
|
"");
|
|
- radius_client_msg_free(req);
|
|
|
|
fail:
|
|
radius_msg_free(msg);
|
|
--
|
|
2.43.0
|
|
|
|
From 339a334551ca911187cc870f4f97ef08e11db109 Mon Sep 17 00:00:00 2001
|
|
From: Jouni Malinen <quic_jouni@quicinc.com>
|
|
Date: Wed, 5 Feb 2025 19:23:39 +0200
|
|
Subject: [PATCH] RADIUS: Fix pending request dropping
|
|
|
|
A recent change to this moved the place where the processed RADIUS
|
|
request was removed from the pending list to happen after the message
|
|
handler had been called. This did not take into account possibility of
|
|
the handler adding a new pending request in the list and the prev_req
|
|
pointer not necessarily pointing to the correct entry anymore. As such,
|
|
some of the pending requests could have been lost and that would result
|
|
in not being able to process responses to those requests and also, to a
|
|
memory leak.
|
|
|
|
Fix this by determining prev_req at the point when the pending request
|
|
is being removed, i.e., after the handler function has already added a
|
|
new entry.
|
|
|
|
Fixes: 726432d7622c ("RADIUS: Drop pending request only when accepting the response")
|
|
Signed-off-by: Jouni Malinen <quic_jouni@quicinc.com>
|
|
---
|
|
src/radius/radius_client.c | 10 +++++++---
|
|
1 file changed, 7 insertions(+), 3 deletions(-)
|
|
|
|
diff --git a/src/radius/radius_client.c b/src/radius/radius_client.c
|
|
index 7909b29a7..d4faa7936 100644
|
|
--- a/src/radius/radius_client.c
|
|
+++ b/src/radius/radius_client.c
|
|
@@ -1099,7 +1099,7 @@ static void radius_client_receive(int sock, void *eloop_ctx, void *sock_ctx)
|
|
struct radius_hdr *hdr;
|
|
struct radius_rx_handler *handlers;
|
|
size_t num_handlers, i;
|
|
- struct radius_msg_list *req, *prev_req;
|
|
+ struct radius_msg_list *req, *prev_req, *r;
|
|
struct os_reltime now;
|
|
struct hostapd_radius_server *rconf;
|
|
int invalid_authenticator = 0;
|
|
@@ -1224,7 +1224,6 @@ static void radius_client_receive(int sock, void *eloop_ctx, void *sock_ctx)
|
|
break;
|
|
}
|
|
|
|
- prev_req = NULL;
|
|
req = radius->msgs;
|
|
while (req) {
|
|
/* TODO: also match by src addr:port of the packet when using
|
|
@@ -1236,7 +1235,6 @@ static void radius_client_receive(int sock, void *eloop_ctx, void *sock_ctx)
|
|
hdr->identifier)
|
|
break;
|
|
|
|
- prev_req = req;
|
|
req = req->next;
|
|
}
|
|
|
|
@@ -1270,6 +1268,12 @@ static void radius_client_receive(int sock, void *eloop_ctx, void *sock_ctx)
|
|
/* fall through */
|
|
case RADIUS_RX_QUEUED:
|
|
/* Remove ACKed RADIUS packet from retransmit list */
|
|
+ prev_req = NULL;
|
|
+ for (r = radius->msgs; r; r = r->next) {
|
|
+ if (r == req)
|
|
+ break;
|
|
+ prev_req = r;
|
|
+ }
|
|
if (prev_req)
|
|
prev_req->next = req->next;
|
|
else
|
|
--
|
|
2.43.0
|
|
|