RADIUS: Drop pending request only when accepting the response

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>

diff --git a/src/radius/radius_client.c b/src/radius/radius_client.c
index 2a7f361702641a208bbbc5acd19ff7190a18e1b9..7909b29a7d377cd1eeea3ad1b6b131ab8467fdd8 100644 (file)
--- 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);