bin/103764: libradius aborts server processing prematurely

[kevin brintnall]

>Number:         103764
>Category:       bin
>Synopsis:       libradius aborts server processing prematurely
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Sep 28 16:10:24 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator:     kevin brintnall
>Release:        FreeBSD 6.1-RELEASE-p8 i386
>Organization:
>Environment:
System: FreeBSD hamachi.rufus.net 6.1-RELEASE-p8 FreeBSD 6.1-RELEASE-p8 #3: Thu Sep 28 09:30:09 CDT 2006 root at hamachi.rufus.net:/usr/obj/usr/src/sys/RUFUS i386

Problem exists in -CURRENT.  Patch is based on the following file:

    $FreeBSD: src/lib/libradius/radlib.c,v 1.12 2004/06/14 20:55:30 stefanf Exp $

>Description:

In rad_continue_send_request(), anything that causes sendto() to return -1
will cause the function to exit prematurely.  This return value is
propagated all the way up the call tree:

	rad_continue_send_request() returns -1 at line 577
	rad_init_send_request() returns -1 at line 739
	rad_send_request() returns -1 at line 924

However, there are several conditions where sendto() may return -1, but
the search should not abort.  For example, errno = EHOSTUNREACH or EPERM.
If the first server is unreachable due to no route, the most sensible
action is to skip that server altogether and try any remaining servers.

This bug causes the pam_radius.so module to return immediate failure when
sendto() errors.

>How-To-Repeat:

Configure pam_radius to point to two different servers.  Try to
authenticate.

Verify that libradius fails over to the second server when the frist one
doesn't respond.

Create a firewall rule on the client which will block the RADIUS packet.
In the radius library, this will cause an immedaite error return on the
sendto() system call (with errno = EPERM).

	echo "block return out proto udp from any to $SERVER port = radius" | pfctl -f -

Try to authenticate again.  Error return is immediate.  Verify that
libradius did NOT fail over to the second server after the first failure.

The following will show up in /var/log/messages:

	sshd[50002]: rad_send_request: sendto: Operation not permitted

>Fix:

The following patch changes the behavior of libradius when sendto()
returns an error.  Instead of aborting altogether, it will proceed onto
the next server immediately.

In the case of a failure, the patch causes rad_continue_send_request() to
increment the try count for the failed server and the rad_handle exactly
as though all attempts to that server have timed out.  Then, the timeout
value is set to zero.  This will cause the calling function to re-enter
rad_continue_send_request() immediately; the function will start with the
next server in the list.

Also, I patched the exit condition for "No valid RADIUS responses
received" to use >= instead of =.  This avoids the case where "h->try >
h->total_tries", which results in an infinite loop.  This should never
happen (because of the way that h->total_tries is built), but it's safer
than the existing code.

--- radlib.patch begins here ---
--- radlib.c.orig	Thu Sep 28 04:10:58 2006
+++ radlib.c	Thu Sep 28 10:18:46 2006
@@ -541,7 +541,7 @@
 		}
 	}
 
-	if (h->try == h->total_tries) {
+	if (h->try >= h->total_tries) {
 		generr(h, "No valid RADIUS responses received");
 		return -1;
 	}
@@ -569,17 +569,24 @@
 	n = sendto(h->fd, h->request, h->req_len, 0,
 	    (const struct sockaddr *)&h->servers[h->srv].addr,
 	    sizeof h->servers[h->srv].addr);
-	if (n != h->req_len) {
+
+	if (n == h->req_len) {
+		h->try++;
+		h->servers[h->srv].num_tries++;
+		tv->tv_sec = h->servers[h->srv].timeout;
+	} else {
 		if (n == -1)
 			generr(h, "sendto: %s", strerror(errno));
 		else
 			generr(h, "sendto: short write");
-		return -1;
+
+		/* do not try this server again */
+		h->try += h->servers[h->srv].max_tries
+			- h->servers[h->srv].num_tries;
+		h->servers[h->srv].num_tries = h->servers[h->srv].max_tries;
+		tv->tv_sec = 0;
 	}
 
-	h->try++;
-	h->servers[h->srv].num_tries++;
-	tv->tv_sec = h->servers[h->srv].timeout;
 	tv->tv_usec = 0;
 	*fd = h->fd;
 
--- radlib.patch ends here ---

begin 644 radlib.patch.sig
MB#\#!0!%&^I=R7#7 at V6>O3X1`JE:`*"<'WIVR)7G<26N93HZ=WO(;#.&DP"@
4E`S(6ID13\$,OR)S!UDC1-*(/*\`
`
end


>Release-Note:
>Audit-Trail:
>Unformatted: