Re: chroot and setuid [Re: OpenBSD work on Tcpdump privilege separation]

[Andrew Pimlott <andrew () pimlott net>]

A few more comments on the current code.

- You use "getuid() == 0 || geteuid() == 0" to check whether droproot
  will be called.  Currently, they are the same, because we call
  setuid(getuid()).  So the code would be clearer if it just used
  getuid().  Also, it is redundant to check uid before setting
  chroot_dir, username, since it will be checked before they are used.

- It is really not much trouble to drop root in the setuid root case.
  The appended patch does this.  Note that now, geteuid() is the
  appropriate thing to check, above.

- initgroups does not really work after chroot, because it needs to open
  the groups file.  On my (Linux) system, it seems to fall-back to
  setting only the give gid, however it might behave less gracefully on
  other systems.  I think it is better to initgroups before chroot.

Regarding the side-effects of droproot:

- The -C problem argues, perhaps, for detecting when the protocol
  analyzers will not be used, ie, when we are just dumping.  Does anyone
  actually use this?

- The resolver problem appears to be serious.  I doubt there is any
  system that can do name resolution in a chroot, at least without
  somehow preparing beforehand.  My system appears to fall back
  gracefully to printing numbers, but I don't think this regression is
  acceptible.  Is it possible that if you do a gethostbyaddr before the
  chroot, it will read/open all necessary files, so that it will still
  work after the chroot?  If this can't be made to work on all
  platforms, an option not to chroot is required.

Andrew

Index: tcpdump.c
===================================================================
RCS file: /tcpdump/master/tcpdump/tcpdump.c,v
retrieving revision 1.233
diff -u -r1.233 tcpdump.c
--- tcpdump.c   26 Feb 2004 08:47:27 -0000      1.233
+++ tcpdump.c   26 Feb 2004 18:31:33 -0000
@@ -396,6 +396,7 @@
        struct dump_info dumpinfo;
        u_char *pcap_userdata;
        char ebuf[PCAP_ERRBUF_SIZE];
+       uid_t euid;
        char *username = NULL;
        char *chroot_dir = NULL;
 #ifdef HAVE_PCAP_FINDALLDEVS
@@ -716,22 +717,18 @@
        if (tflag > 0)
                thiszone = gmt2local(0);
 
+       euid = geteuid();
+
 #ifdef WITH_CHROOT
-       /* if run as root, prepare for chrooting */
-       if (getuid() == 0 || geteuid() == 0) {
-               /* future extensibility for cmd-line arguments */
-               if (!chroot_dir)
-                       chroot_dir = WITH_CHROOT;
-       }
+       /* future extensibility for cmd-line arguments */
+       if (!chroot_dir)
+               chroot_dir = WITH_CHROOT;
 #endif
 
 #ifdef WITH_USER
-       /* if run as root, prepare for dropping root privileges */
-       if (getuid() == 0 || geteuid() == 0) {
-               /* Run with '-Z root' to restore old behaviour */ 
-               if (!username)
-                       username = WITH_USER;
-       }
+       /* Run with '-Z root' to restore old behaviour */ 
+       if (!username)
+               username = WITH_USER;
 #endif
 
        if (RFileName != NULL) {
@@ -748,8 +745,8 @@
                 * people's trace files (especially if we're set-UID
                 * root).
                 */
-               if (setgid(getgid()) != 0 || setuid(getuid()) != 0 )
-                       fprintf(stderr, "Warning: setgid/setuid failed !\n");
+               if (setgid(getgid()) != 0 || seteuid(getuid()) != 0 )
+                       fprintf(stderr, "Warning: setgid/seteuid failed !\n");
 #endif /* WIN32 */
                pd = pcap_open_offline(RFileName, ebuf);
                if (pd == NULL)
@@ -800,8 +797,8 @@
                 * Let user own process after socket has been opened.
                 */
 #ifndef WIN32
-               if (setgid(getgid()) != 0 || setuid(getuid()) != 0)
-                       fprintf(stderr, "Warning: setgid/setuid failed !\n");
+               if (setgid(getgid()) != 0 || seteuid(getuid()) != 0)
+                       fprintf(stderr, "Warning: setgid/seteuid failed !\n");
 #endif /* WIN32 */
 #ifdef WIN32
                if(UserBufferSize != 1000000)
@@ -906,7 +903,9 @@
         * We cannot do this earlier, because we want to be able to open
         * the file (if done) for writing before giving up permissions.
         */
-       if (getuid() == 0 || geteuid() == 0) {
+       if (euid == 0) {
+               if (seteuid(euid) != 0)
+                       error("seteuid back to %d failed", euid);
                if (username || chroot_dir)
                        droproot(username, chroot_dir);
        }
-
This is the TCPDUMP workers list. It is archived at
http://www.tcpdump.org/lists/workers/index.html
To unsubscribe use mailto:tcpdump-workers-request () tcpdump org?body=unsubscribe