[4.3 PATCH] locate fails when run as root

[Nix]

locate's new slocate support has broken locate when run as root, because
of an assumption in drop_privs() that failure to drop privileges is
caused by attackers running locate without CAP_SETUID. This is obviously
not always true: you can't drop privileges if your real uid is already
root, and in that situation setuid(0) will obviously still return 0
because we are still root, yet there is no attack. (The code is odd in
this area: are there really systems on which setuid() can fail without
returning nonzero and setting errno? That seems a gross violation of
POSIX to me.)

(As an aside, it's strange and rather inefficient that the secure DB is
opened even if we later decide that we don't need it, perhaps because it
doesn't exist.)

Combined with the non-resetting of errno before setuid() is called,
this can lead to comical error messages:

hades:~# locate Errno.pm
locate: Failed to drop privileges: No such file or directory

Er, I don't think so :) that was an open() failure for the secure_db!

Thus this patch, against findutils-4.3.2, with a free additional
gratuitous comment typo fix:

2007-01-15  Nix  <address@hidden>

        * locate/locate.c (drop_privs): Don't try to drop privileges
        if our real uid is root.

Index: findutils/locate/locate.c
===================================================================
--- findutils.orig/locate/locate.c      2006-12-16 10:46:28.000000000 +0000
+++ findutils/locate/locate.c   2007-01-15 22:59:10.000000000 +0000
@@ -1345,7 +1345,7 @@
   const char * what = "failed";
   uid_t orig_euid = geteuid();
 
-  /* Use of setgroups() is restrcted to root only. */
+  /* Use of setgroups() is restricted to root only. */
   if (0 == orig_euid)
     {
       gid_t groups[1];
@@ -1356,22 +1356,28 @@
          goto fail;
        }
     }
-  
-  if (0 != setuid(getuid()))
+
+  /* If we are not run with a real uid of root, try to drop our
+     excess privileges. */
+
+  if (getuid() != 0)
     {
-      what = _("failed to drop setuid privileges");
-      goto fail;
-    }
+      if (0 != setuid(getuid()))
+        {
+          what = _("failed to drop setuid privileges");
+          goto fail;
+        }
   
-  /* Defend against the case where the attacker runs us with the
-   * capability to call setuid() turned off, which on some systems
-   * will cause the above attempt to drop privileges fail (leaving us
-   * privileged).
-   */
-  if (0 == setuid(0))
-    {
-      what = _("Failed to drop privileges");
-      goto fail;
+      /* Defend against the case where the attacker runs us with the
+       * capability to call setuid() turned off, which on some systems
+       * will cause the above attempt to drop privileges fail (leaving us
+       * privileged).
+       */
+      if (0 == setuid(0))
+        {
+          what = _("Failed to drop privileges");
+          goto fail;
+        }
     }
 
   /* success. */


-- 
`The serial comma, however, is correct and proper, and abandoning it will
surely lead to chaos, anarchy, rioting in the streets, the Terrorists
taking over, and possibly the complete collapse of Human Civilization.'