Subject: mount_portal credential surrendering
To: None <tech-security@netbsd.org>
From: Brian C. Grayson <bgrayson@marvin.ece.utexas.edu>
List: tech-security
Date: 08/06/1999 01:00:15
  I'm just about done with my improvements to mount_portal, but
my CVS sponsors and I would like to let security folks look at
some of it and see if it is sufficiently safe, but also not overly
pessimistic.

  The problem is, with mount_portal and the "fs" portal
namespace, the mount is running under the uid of the mounter
(say, Aloysius).  An access to the mounted portal fs namespace
is done by, say, Barb.  The mount process forks, and an
Aloysius process tries to service Barb's request.  If we aren't
careful, we're letting Barb have access to anything that
Aloysius can access, 

  Currently, the mount must be done by root, and after the fork,
the process handling the request does a setuid(Barb), thus
avoiding the security hole.  However, this prevents ordinary
users from using mount_portal, as Aloysius can't execute
setuid(Aloysius), so the forked process exits.

  My proposed solution, as discussed on tech-userlevel, is to
only do the setuid if the mounter was root.  Otherwise, exit if
the accessing uid is not the same as the mount-process uid (and
same for gid).  This allows Aloysius to access his fs portal, but
no one else's, and vice versa.

  Here's a patch for pt_file.c that provides this functionality.
The lose_credentials() function will also be used by some new
namespaces, hence the importance behind getting it right.

  The patch also addresses another issue:  the code in the tree
tries to open the file read/write, and gives up if that fails.
With the patch, if the first open attempt fails, it tries to open
the same file read-only.

  If anyone sees a problem with this patch, please let me know!
If I don't hear back negatively within a few days, it's going in.  :)

  TIA.

  Brian Grayson

Index: pt_file.c
===================================================================
RCS file: /cvsroot/basesrc/sbin/mount_portal/pt_file.c,v
retrieving revision 1.10
diff -1 -u -t -p -r1.10 pt_file.c
--- pt_file.c	1997/09/21 02:35:43	1.10
+++ pt_file.c	1999/08/06 05:56:11
@@ -58,3 +58,67 @@ __RCSID("$NetBSD: pt_file.c,v 1.10 1997/
 
+#ifdef DEBUG
+#define DEBUG_SYSLOG    syslog
+#else
+        /*
+         * The "if (0) ..." will be optimized away by the compiler if
+         * DEBUG is not defined.
+         */
+#define DEBUG_SYSLOG    if (0) syslog
+#endif
+
 int
+lose_credentials(pcr)
+        struct portal_cred       *pcr;
+{
+        /*
+         * If we are root, then switch into the caller's credentials.
+         * By the way, we _always_ log these messages, to make
+         * sure questionable activity is noticed.
+         */
+        if (getuid() == 0) {
+                /* Set egid, then groups, then uid. */
+                if (setegid(pcr->pcr_gid) < 0) {
+                        syslog(LOG_ERR, "lose_credentials: setegid(%d) "
+                            "failed (%m)", pcr->pcr_gid);
+                        return (errno);
+                }
+                if (setgroups(pcr->pcr_ngroups, pcr->pcr_groups) < 0) {
+                        syslog(LOG_ERR, "lose_credentials: setgroups() "
+                            "failed (%m)");
+                        return (errno);
+                }
+                if (seteuid(pcr->pcr_uid) < 0) {
+                        syslog(LOG_ERR, "lose_credentials: seteuid(%d) "
+                            "failed (%m)", pcr->pcr_uid);
+                        return (errno);
+                }
+                /* The credential change was successful! */
+                DEBUG_SYSLOG(LOG_ERR, "Root-owned mount process lowered credentials -- returning successfully!\n");
+                return 0;
+        }
+        DEBUG_SYSLOG(LOG_ERR, "Actual/effective/caller's creds are:");
+        DEBUG_SYSLOG(LOG_ERR, "%d/%d %d/%d %d/%d", getuid(),
+                        getgid(), geteuid(), getegid(),
+                        pcr->pcr_uid, pcr->pcr_gid);
+        /*
+         * Else, fail if the uid is neither actual or effective
+         * uid of mount process...
+         */
+        if ((getuid() != pcr->pcr_uid) && (geteuid() != pcr->pcr_uid))
+                return EPERM;
+        /*
+         * ... or the gid is neither the actual or effective
+         * gid of the mount process.
+         */
+        if ((getgid() != pcr->pcr_gid) && (getegid() != pcr->pcr_gid))
+                return EPERM;
+        /*
+         * If we make it here, we have a uid _and_ gid match! Allow the
+         * access.
+         */
+        DEBUG_SYSLOG(LOG_ERR, "Returning successfully!\n");
+        return 0;
+}
+
+int
 portal_file(pcr, key, v, so, fdp)
@@ -69,25 +133,44 @@ portal_file(pcr, key, v, so, fdp)
         int error;
+        int origuid, origgid;
 
+        origuid = getuid();
+        origgid = getgid();
         pbuf[0] = '/';
         strcpy(pbuf+1, key + (v[1] ? strlen(v[1]) : 0));
-
-#ifdef DEBUG
-        printf("path = %s, uid = %d, gid = %d\n", pbuf, pcr->pcr_uid,
-            pcr->pcr_gid);
-#endif
-
-        if (setegid(pcr->pcr_gid) < 0 ||
-            setgroups(pcr->pcr_ngroups, pcr->pcr_groups) < 0)
-                return (errno);
+        DEBUG_SYSLOG(LOG_ERR, "path = %s, uid = %d, gid = %d",
+                        pbuf, pcr->pcr_uid, pcr->pcr_gid);
 
-        if (seteuid(pcr->pcr_uid) < 0)
-                return (errno);
-
+        if ((error = lose_credentials(pcr)) != 0) {
+                DEBUG_SYSLOG(LOG_ERR, "portal_file: Credential err %d", error);
+                return error;
+        }
+        error = 0;
+        /*
+         * Be careful -- only set error to errno if there is an error.
+         * errno could hold an old, uncaught value, from a routine
+         * called long before now.
+         */
         fd = open(pbuf, O_RDWR|O_CREAT, 0666);
-        if (fd < 0)
+        if (fd < 0) {
                 error = errno;
-        else
-                error = 0;
+                if (error == EACCES) {
+                        DEBUG_SYSLOG(LOG_ERR, "Error:  could not open '%s' "
+                            "read/write with create flag.  "
+                            "Trying read-only open...", pbuf);
+                        /* Try opening read-only. */
+                        fd = open(pbuf, O_RDONLY, 0);
+                        if (fd < 0) {
+                                error = errno;
+                                DEBUG_SYSLOG(LOG_ERR, "That failed too!  %m");
+                        } else {
+                                /* Clear the error indicator. */
+                                error = 0;
+                        }
+                } else {
+                        DEBUG_SYSLOG(LOG_ERR, "Error:  could not open '%s': %m", pbuf);
+                }
 
-        if (seteuid((uid_t) 0) < 0) {   /* XXX - should reset gidset too */
+        }
+        if (seteuid((uid_t) origuid) < 0) {     /* XXX - should reset gidset
+                                                 * too */
                 error = errno;
@@ -99,3 +182,2 @@ portal_file(pcr, key, v, so, fdp)
         }
-
         if (error == 0)
@@ -103,7 +185,4 @@ portal_file(pcr, key, v, so, fdp)
 
-#ifdef DEBUG
-        fprintf(stderr, "pt_file returns *fdp = %d, error = %d\n", *fdp,
+        DEBUG_SYSLOG(LOG_ERR, "pt_file returns *fdp = %d, error = %d", *fdp,
             error);
-#endif
-
         return (error);