When receiving a response from the radius server, check the source of the response.

[Pxlatn]

Hi, Please accept my apologies if this is not a suitable addition. I am primarily a sysadmin, so deal with configuration more than code, but I understood the problem well enough, and hope I understand sufficiently how to address it. I welcome any changes if the naming or structure could be improved.

What

Check that the source of a received response matches the expected radius server

Why

If multiple radius servers are configured, and one takes longer than the timeout to reply (but does reply), the reply causes a log of:

packet from RADIUS server $current_server failed verification: The shared secret is probably incorrect.

and the listening socket is closed before the reply actually from the current server is received.

This is particularly a problem when due to additional checks (in our case, certain MFA methods with an NPS server) delay the response.

How

• Add the function ipaddr_eq to check for the equality of two sockaddrs

Testing

I have tested this with a pam authenticated sshd, configured to talk to an NPS radius server.

Without this change:

• ✅ a correct reply from the correct server is accepted.
• ❌ a correct reply from a previously timed out server causes the port to be closed.
• ❌ a completely incorrect packet from any source to the port socket causes the port to be closed.

With this change:

• ✅ a correct reply from the correct server is accepted.
• ✅ a correct reply from a previously timed out server is discarded, and the port continues listening.
• ✅ a completely incorrect packet from any other source is discarded, and the port continues listening.

I have also tested the ipaddr_eq function by itself, to hopefully sufficiently check it with IPv4, IPv6 and a mix (with IPv4-mapped IPv6 addresses).

[alandekok]

Thanks. That's a patch which should have gone in when the module was first written. :(

I've pushed a slightly different fix. And then another one which ignores more bad packets, instead of treating them as errors.

Please let me know if this doesn't work.

[Pxlatn]

Thank you for the change, it looks good, but I'm having trouble testing it as it seems to die ~before the reply can arrive~ when the reply arrives. I'll see if I can work out why.

[Pxlatn]

Okay, it appears to be this line https://github.com/FreeRADIUS/pam_radius/blob/2751218c5cd822b4ca8783f1fc03df15500db3c6/src/pam_radius_auth.c#L971-L973 The comparison using memcmp with int types doesn't appear to work. It's okay if I change that to an == comparison.

[alandekok]

I've pushed a fix, thanks.

[Pxlatn]

That's perfect. Thanks for your help.