system/5390: authpf removes pidfile without owning the lock

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

system/5390: authpf removes pidfile without owning the lock

Stefan Krah
>Number:         5390
>Category:       system
>Synopsis:       authpf removes pidfile without owning the lock
>Confidential:   yes
>Severity:       serious
>Priority:       medium
>Responsible:    bugs
>State:          open
>Quarter:        
>Keywords:      
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Feb 22 19:40:01 GMT 2007
>Closed-Date:
>Last-Modified:
>Originator:     Stefan Krah
>Release:        OpenBSD 4.0
>Organization:
net
>Environment:
        <machine, os, target, libraries (multiple lines)>
        System      : OpenBSD 4.0
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:

When a second authpf process tries to kill a previous one and the maximum
lockcnt is reached, it removes the pidfile of the first process without
ever having obtained a lock on the pidfile.

As a result, multiple logins from the same ip address are possible.

>How-To-Repeat:

I haven't encountered a real world case where killing a previous
authpf process failed, but of course it might happen.

>Fix:

Reset pidfp to NULL and check for it later. Since the pidfp effectively
indicates if an authpf process has the lock, it might be more readable to
introduce a new variable like have_lock instead.


Index: authpf.c
===================================================================
RCS file: /cvs/src/usr.sbin/authpf/authpf.c,v
retrieving revision 1.99
diff -u -r1.99 authpf.c
--- authpf.c    9 Aug 2006 16:21:39 -0000       1.99
+++ authpf.c    22 Feb 2007 18:50:05 -0000
@@ -246,6 +246,8 @@
                if (++lockcnt > 10) {
                        syslog(LOG_ERR, "cannot kill previous authpf (pid %d)",
                            otherpid);
+                       fclose(pidfp);
+                       pidfp = NULL;
                        goto dogdeath;
                }
                sleep(1);
@@ -255,6 +257,7 @@
                 * it's lock, giving us a chance to get it now
                 */
                fclose(pidfp);
+               pidfp = NULL;
        } while (1);

        /* whack the group list */
@@ -835,10 +838,11 @@
                authpf_kill_states();
                remove_stale_rulesets();
        }
-       if (pidfp)
+       if (pidfp) {
                ftruncate(fileno(pidfp), 0);
-       if (pidfile[0])
-               if (unlink(pidfile) == -1)
-                       syslog(LOG_ERR, "cannot unlink %s (%m)", pidfile);
+               if (pidfile[0])
+                       if (unlink(pidfile) == -1)
+                               syslog(LOG_ERR, "cannot unlink %s (%m)", pidfile);
+       }
        exit(ret);
 }


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