Radius plugin bug.

Radius plugin bug.

[Ray Van Dolson]

I have a busy Linux-based PPTP concentrator (backed by pppd) using Radius
authentication and am seeing some situations where a user connects and gets
no /var/run/radattr file.

I've determined the situation is occuring as a result of User A connecting
and being assigned interface ppp0; disconnecting, and while disconnecting
user B connects and is assigned interface ppp0 (it apparently is made
available before User A's disconnection process or plugins complete
running).  User B's radattr script is written before user A's disconnect
process is completed.  User A's process removes the radattr file (which is
actually for user B!).

Here is an example from my logs of this type of situation occurring:

Jun 28 23:08:50 chico-pptp1 pppd[22494]: Plugin radius.so loaded.
Jun 28 23:08:50 chico-pptp1 pppd[22494]: RADIUS plugin initialized.
Jun 28 23:08:50 chico-pptp1 pppd[22494]: Plugin radattr.so loaded.
Jun 28 23:08:50 chico-pptp1 pppd[22494]: RADATTR plugin initialized.
Jun 28 23:08:50 chico-pptp1 pppd[22494]: pppd 2.4.3 started by root, uid 0
Jun 28 23:08:50 chico-pptp1 pppd[22494]: Using interface ppp29
Jun 28 23:08:50 chico-pptp1 pppd[22494]: Connect: ppp29 <--> /dev/pts/113
Jun 28 23:08:53 chico-pptp1 pppd[22494]: Peer jewell@cncnet.us CHAP authentication succeeded
Jun 28 23:08:53 chico-pptp1 pppd[22494]: MPPE 128-bit stateless compression enabled
Jun 28 23:08:55 chico-pptp1 pppd[22494]: found interface eth0 for proxy arp
Jun 28 23:08:55 chico-pptp1 pppd[22494]: local  IP address 208.53.80.9
Jun 28 23:08:55 chico-pptp1 pppd[22494]: remote IP address 208.53.80.189
Jun 28 23:09:49 chico-pptp1 pppd[22494]: LCP terminated by peer (^D~^SM-a^@<M-Mt^@^@^@^@)
Jun 28 23:09:50 chico-pptp1 pppd[22494]: Connect time 1.0 minutes.
Jun 28 23:09:50 chico-pptp1 pppd[22494]: Sent 504 bytes, received 1294 bytes.
Jun 28 23:09:50 chico-pptp1 pppd[22494]: Modem hangup
Jun 28 23:09:50 chico-pptp1 pppd[22494]: Connection terminated.
Jun 28 23:09:51 chico-pptp1 pppd[23135]: Plugin radius.so loaded.
Jun 28 23:09:51 chico-pptp1 pppd[23135]: RADIUS plugin initialized.
Jun 28 23:09:51 chico-pptp1 pppd[23135]: Plugin radattr.so loaded.
Jun 28 23:09:51 chico-pptp1 pppd[23135]: RADATTR plugin initialized.
Jun 28 23:09:51 chico-pptp1 pppd[23135]: pppd 2.4.3 started by root, uid 0
Jun 28 23:09:51 chico-pptp1 pppd[23135]: Using interface ppp29
Jun 28 23:09:51 chico-pptp1 pppd[23135]: Connect: ppp29 <--> /dev/pts/268
Jun 28 23:09:51 chico-pptp1 pppd[23135]: Peer frems21@myswi.net CHAP authentication succeeded
Jun 28 23:09:51 chico-pptp1 pppd[23135]: MPPE 128-bit stateless compression enabled
Jun 28 23:09:51 chico-pptp1 pppd[23135]: found interface eth0 for proxy arp
Jun 28 23:09:51 chico-pptp1 pppd[23135]: local  IP address 208.53.80.9
Jun 28 23:09:51 chico-pptp1 pppd[23135]: remote IP address 208.53.81.169
Jun 28 23:09:51 chico-pptp1 pppd[22494]: Exit.

As you can see, process 22494 doesn't exit until after 23135 completes its
plugin processing (you'll have to take my word for it since timestamp
accuracy is in seconds).

I hacky fix idea I had in mind was to have cleanup() in radattr.c check the
file modification time on the file in question to ensure it hasn't been
written to by another process.  If so, don't remove it.

Maybe the proper fix would be to do some file locking or to reserve the
interface from being assigned to another pppd until the previous connection
has completed?

Suggestions?

Ray

Re: Radius plugin bug.

[Ben McKeegan]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

Ray Van Dolson wrote:
> I have a busy Linux-based PPTP concentrator (backed by pppd) using Radius
> authentication and am seeing some situations where a user connects and gets
> no /var/run/radattr file.
> 
> I've determined the situation is occuring as a result of User A connecting
> and being assigned interface ppp0; disconnecting, and while disconnecting
> user B connects and is assigned interface ppp0 (it apparently is made
> available before User A's disconnection process or plugins complete
> running).  User B's radattr script is written before user A's disconnect
> process is completed.  User A's process removes the radattr file (which is
> actually for user B!).
> 
> Here is an example from my logs of this type of situation occurring:
> ...
> I hacky fix idea I had in mind was to have cleanup() in radattr.c check the
> file modification time on the file in question to ensure it hasn't been
> written to by another process.  If so, don't remove it.
> 
> Maybe the proper fix would be to do some file locking or to reserve the
> interface from being assigned to another pppd until the previous connection
> has completed?
> 

Hi Ray,

That's not the only problem with the way radattr handles its files. 
With multilink enabled it doesn't work at all as it always misses the 
interface name out of the filename, so all pppd processes write to 
exactly the same file!

Attached is a patch I put together recently to work around the multilink 
issue. (This is against the 2.4.4b1 package from Debian Unstable.) It 
delays creating the file if the interface name is unavailable at the 
time of authentication.  Perhaps you could modify this to always delay. 
  Also, there's a section of radattr:

#if 0
     /* calling cleanup() on link down is problematic because 
print_attributes()
        is called only after PAP or CHAP authentication, but not when 
the link
        should go up again for any other reason */
     add_notifier(&link_down_notifier, cleanup, NULL);
#endif

     /* Just in case... */
     add_notifier(&exitnotify, cleanup, NULL);

As you can see until someone macro'd out a chunk of code it originally 
removed the file on link down, and again on exit (long after the ppp 
device has been reassigned).  Perhaps by storing a copy of the 
attributes (as I am doing in my multilink patch) and rewriting the file 
whenever the link goes up, we can work around the issue that prompted 
someone to macro out the first call to cleanup(), and get rid of the 
second call.

Ben.

-- 
Ben McKeegan
Netservers Limited


[-- Attachment #2: radattr.c.patch --]
[-- Type: text/x-patch, Size: 3000 bytes --]

--- ppp-2.4.4b1/pppd/plugins/radius/radattr.c.orig	2004-10-28 01:24:40.000000000 +0100
+++ ppp-2.4.4b1/pppd/plugins/radius/radattr.c	2006-06-27 16:21:39.000000000 +0100
@@ -22,11 +22,17 @@
 #include <stdio.h>
 
 extern void (*radius_attributes_hook)(VALUE_PAIR *);
+void (*radattr_attributes_oldhook)(VALUE_PAIR *) = NULL;
+void (*radattr_ip_choose_oldhook) __P((u_int32_t *)) = NULL;
 static void print_attributes(VALUE_PAIR *);
 static void cleanup(void *opaque, int arg);
 
 char pppd_version[] = VERSION;
 
+char fname[512];
+VALUE_PAIR *saved_vp=NULL;
+static radattr_ip_choose_hooked=0;
+
 /**********************************************************************
 * %FUNCTION: plugin_init
 * %ARGUMENTS:
@@ -39,8 +45,11 @@
 void
 plugin_init(void)
 {
+    radattr_attributes_oldhook = radius_attributes_hook;
     radius_attributes_hook = print_attributes;
 
+    fname[0]='\0';
+
 #if 0
     /* calling cleanup() on link down is problematic because print_attributes()
        is called only after PAP or CHAP authentication, but not when the link
@@ -63,11 +72,8 @@
 *  Prints the attribute pairs to /var/run/radattr.pppN.  Each line of the
 *  file contains "name value" pairs.
 ***********************************************************************/
-static void
-print_attributes(VALUE_PAIR *vp)
-{
+void print_attributes_to_file(VALUE_PAIR *vp) {
     FILE *fp;
-    char fname[512];
     char name[2048];
     char value[2048];
     int cnt = 0;
@@ -90,6 +96,47 @@
     dbglog("RADATTR plugin wrote %d line(s) to file %s.", cnt, fname);
 }
 
+
+static void radattr_ip_choose_hook(u_int32_t *addrp) {
+  if (saved_vp && ifname[0]) {
+    /* If multilink is enabled this is first available
+       hook after setting ifname.
+       Thankfully IPCP is first network protocol to be started
+       and this hook will get called before any other
+       protocols are started.
+       We're assuming IP is always enabled if multilink is!  */
+
+    print_attributes_to_file(saved_vp);
+    rc_avpair_free(saved_vp);
+    saved_vp=NULL;
+  }
+  if (radattr_ip_choose_oldhook) {
+    radattr_ip_choose_oldhook(addrp);
+  }
+}
+
+static void
+print_attributes(VALUE_PAIR *vp)
+{
+
+    if (radattr_attributes_oldhook) {
+      radattr_attributes_oldhook(vp);
+    }
+
+    if (ifname[0]) {
+      print_attributes_to_file(vp);
+    } else {
+      if (saved_vp) 
+	rc_avpair_free(saved_vp);
+      saved_vp=rc_avpair_copy(vp);
+      if (saved_vp && ! radattr_ip_choose_hooked) {
+	radattr_ip_choose_oldhook=ip_choose_hook;
+	ip_choose_hook=radattr_ip_choose_hook;
+	radattr_ip_choose_hooked=1;
+      }
+    }
+}
+
 /**********************************************************************
 * %FUNCTION: cleanup
 * %ARGUMENTS:
@@ -103,9 +150,8 @@
 static void
 cleanup(void *opaque, int arg)
 {
-    char fname[512];
-
-    slprintf(fname, sizeof(fname), "/var/run/radattr.%s", ifname);
+  if (fname[0]) {
     (void) remove(fname);
     dbglog("RADATTR plugin removed file %s.", fname);
+  }
 }

Re: Radius plugin bug.

[Ray Van Dolson]

On Thu, Jun 29, 2006 at 10:37:30AM +0100, Ben McKeegan wrote:
> Hi Ray,
> 
> That's not the only problem with the way radattr handles its files. 
> With multilink enabled it doesn't work at all as it always misses the 
> interface name out of the filename, so all pppd processes write to 
> exactly the same file!

Thanks for the reply Ben.  I may indeed try to make use of your patch, but
it seems to me the best way to handle this would be to not release the ppp
unit number until the plugins have finished their cleanup() functions.

I'm by no means a C master and am trying to find out where in the code the
"lock" on the device name pppXXX is released at.  I'm trying to trace
backwards from the sifdown() call in sys-linux.c but getting a little lost.

Anyone have any pointers on how this works?  Is it a matter of just putting
the sys_cleanup() call in cleanup() in main.c after the cleanup calls for
the channels?

Ray

Re: Radius plugin bug.

[Paul Mackerras]

Ray Van Dolson writes:

> I'm by no means a C master and am trying to find out where in the code the
> "lock" on the device name pppXXX is released at.  I'm trying to trace
> backwards from the sifdown() call in sys-linux.c but getting a little lost.

The device name gets released in generic_disestablish_ppp() in
sys-linux.c, called (usually) from tty_disestablish_ppp().

Paul.

Re: Radius plugin bug.

[Ray Van Dolson]

Well have been working on this a little, and may have found a potential
solution that works.

Feedback would be appreciate from real programmers as to negative
consequences of reorganizing the code a little...

Basically, I am removing the calls to free the ppp interface from the
link_terminated function and relocating them to later in the process --
after the plugin cleanup functions have been called removing the radius
attribute file.

Seems to be working OK so far on our server...

diff -uNrp ppp-2.4.3.orig/pppd/auth.c ppp-2.4.3/pppd/auth.c
--- ppp-2.4.3.orig/pppd/auth.c	2004-11-12 02:30:51.000000000 -0800
+++ ppp-2.4.3/pppd/auth.c	2006-07-05 12:03:47.000000000 -0700
@@ -83,6 +83,7 @@
 #include <sys/socket.h>
 #include <utmp.h>
 #include <fcntl.h>
 #if defined(_PATH_LASTLOG) && defined(__linux__)
 #include <lastlog.h>
 #endif
@@ -621,6 +622,7 @@ link_terminated(unit)
      * the ppp unit back to the loopback.  Set the
      * real serial device back to its normal mode of operation.
      */
+    /*
     if (fd_ppp >= 0) {
 	remove_fd(fd_ppp);
 	clean_check();
@@ -629,6 +631,8 @@ link_terminated(unit)
 	    mp_exit_bundle();
 	fd_ppp = -1;
     }
+    */
     if (!hungup)
 	lcp_lowerdown(0);
     if (!doing_multilink && !demand)
@@ -638,10 +642,14 @@ link_terminated(unit)
      * Run disconnector script, if requested.
      * XXX we may not be able to do this if the line has hung up!
      */
+    /*
     if (devfd >= 0 && the_channel->disconnect) {
 	the_channel->disconnect();
 	devfd = -1;
     }
+    */
 
     if (doing_multilink && multilink_master) {
 	if (!bundle_terminating)
diff -uNrp ppp-2.4.3.orig/pppd/main.c ppp-2.4.3/pppd/main.c
--- ppp-2.4.3.orig/pppd/main.c	2004-11-13 04:05:48.000000000 -0800
+++ ppp-2.4.3/pppd/main.c	2006-07-05 12:03:52.000000000 -0700
@@ -1098,8 +1108,9 @@ die(status)
 {
     if (!doing_multilink || multilink_master)
 	print_link_stats();
-    cleanup();
     notify(exitnotify, status);
+    cleanup();
     syslog(LOG_INFO, "Exit.");
     exit(status);
 }
@@ -1113,10 +1124,46 @@ cleanup()
 {
     sys_cleanup();
 
-    if (fd_ppp >= 0)
+    /*
+    if (fd_ppp >= 0) {
+        syslog(LOG_INFO, "Calling disestablish_ppp from cleanup()");
 	the_channel->disestablish_ppp(devfd);
-    if (the_channel->cleanup)
+    }
+    */
+
+    /*
+     * If we may want to bring the link up again, transfer
+     * the ppp unit back to the loopback.  Set the
+     * real serial device back to its normal mode of operation.
+     */
+
+    if (fd_ppp >= 0) {
+	remove_fd(fd_ppp);
+	clean_check();
+        syslog(LOG_INFO, "Calling disestablish_ppp from cleanup()");
+	the_channel->disestablish_ppp(devfd);
+	if (doing_multilink)
+	    mp_exit_bundle();
+	fd_ppp = -1;
+    }
+
+    /*
+     * Run disconnector script, if requested.
+     * XXX we may not be able to do this if the line has hung up!
+     */
+    if (devfd >= 0 && the_channel->disconnect) {
+	syslog(LOG_INFO, "Running disconnector scripts...");
+	the_channel->disconnect();
+	devfd = -1;
+	syslog(LOG_INFO, "Finished running disconnector scripts...");
+    }
+
+    if (the_channel->cleanup) {
+        syslog(LOG_INFO, "Calling cleanup from cleanup()");
 	(*the_channel->cleanup)();
+    }
+
+    syslog(LOG_INFO, "Removing pidfiles.");
     remove_pidfiles();
 
 #ifdef USE_TDB

Re: Radius plugin bug.

[Ray Van Dolson]

On Wed, Jul 05, 2006 at 12:40:21PM -0700, Ray Van Dolson wrote:
> Well have been working on this a little, and may have found a potential
> solution that works.
> 
> Feedback would be appreciate from real programmers as to negative
> consequences of reorganizing the code a little...
> 
> Basically, I am removing the calls to free the ppp interface from the
> link_terminated function and relocating them to later in the process --
> after the plugin cleanup functions have been called removing the radius
> attribute file.
> 
> Seems to be working OK so far on our server...

Been a few days, and still having no problems with this patch.  Problems
still arise if a customer connects and then disconnects before the ip-up
scripts have completed their initial run, but this isn't as much of an
issue. :)

Ray