On Thu, 02 Mar 2023 17:28:01 +0100, "Peter J. Philipp" wrote:
> I just looked up RADIUS in RFC 2865 and on page 15 it reads:
>
> --------->
> Length
>
> The Length field is two octets. It indicates the length of the
> packet including the Code, Identifier, Length, Authenticator and
> Attribute fields. Octets outside the range of the Length field
> MUST be treated as padding and ignored on reception. If the
> packet is shorter than the Length field indicates, it MUST be
> silently discarded. The minimum length is 20 and maximum length
> is 4096.
>
> <---------
>
> Notice the silent discard, so perhaps we should try again? I adjusted
> the patch for this, but I can't test.
I think it is simplest to just retry if rad_recv() returns -1 until
we hit the max retries. Here's a diff relative to the earlier one
I just committed that simplifies the retry logic a bit by removing
the use of timedout in raddauth().
- todd
Index: libexec/login_radius/raddauth.c
===================================================================
RCS file: /cvs/src/libexec/login_radius/raddauth.c,v
retrieving revision 1.31
diff -u -p -u -r1.31 raddauth.c
--- libexec/login_radius/raddauth.c 2 Mar 2023 16:13:57 -0000 1.31
+++ libexec/login_radius/raddauth.c 2 Mar 2023 16:50:27 -0000
@@ -114,13 +114,10 @@
char *radius_dir = RADIUS_DIR;
char auth_secret[MAXSECRETLEN+1];
volatile sig_atomic_t timedout;
-int alt_retries;
-int retries;
+in_port_t radius_port;
+in_addr_t auth_server;
int sockfd;
int timeout;
-in_addr_t alt_server;
-in_addr_t auth_server;
-in_port_t radius_port;
typedef struct {
u_char code;
@@ -158,6 +155,8 @@ raddauth(char *username, char *class, ch
struct servent *svp;
struct sockaddr_in sin;
struct sigaction sa;
+ int alt_retries, retries;
+ in_addr_t alt_server;
const char *errstr;
memset(_pwstate, 0, sizeof(_pwstate));
@@ -268,24 +267,20 @@ raddauth(char *username, char *class, ch
sa.sa_flags = 0; /* don't restart system calls */
(void)sigaction(SIGALRM, &sa, NULL);
retry:
- if (timedout) {
- timedout = 0;
- if (--retries <= 0) {
- /*
- * If we ran out of tries but there is an alternate
- * server, switch to it and try again.
- */
- if (alt_retries) {
- auth_server = alt_server;
- retries = alt_retries;
- alt_retries = 0;
- getsecret();
- } else
- warnx("no response from authentication server");
- }
+ if (retries-- <= 0) {
+ /*
+ * If we ran out of tries but there is an alternate
+ * server, switch to it and try again.
+ */
+ if (alt_retries) {
+ auth_server = alt_server;
+ retries = alt_retries;
+ alt_retries = 0;
+ getsecret();
+ } else
+ warnx("no response from authentication server");
}
-
- if (retries > 0) {
+ if (retries >= 0) {
rad_request(req_id, userstyle, passwd, auth_port, vector,
pwstate);
@@ -324,9 +319,11 @@ retry:
passwd = "";
break;
+ case -1:
+ /* Timeout or bad packet, retry. */
+ goto retry;
+
default:
- if (timedout)
- goto retry;
snprintf(_pwstate, sizeof(_pwstate),
"invalid response type %d\n", i);
*emsg = _pwstate;
@@ -460,12 +457,16 @@ rad_recv(char *state, char *challenge, u
(struct sockaddr *)&sin, &salen);
alarm(0);
if (total_length < AUTH_HDR_LEN) {
- if (timedout)
+ if (timedout) {
+ timedout = 0;
return(-1);
+ }
errx(1, "bogus auth packet from server");
}
- if (ntohs(auth.length) > total_length)
- errx(1, "bogus auth packet from server");
+ if (ntohs(auth.length) > total_length) {
+ /* RFC 2865 says to silently discard short packets. */
+ return(-1);
+ }
if (sin.sin_addr.s_addr != auth_server)
errx(1, "bogus authentication server");
