"Inherit Permissions" request for comments

[Kyle Herbert]

Hello everyone,

Thanks for the encouragement, David!  As you suggested, I'd like to start a
thread and get some input regarding the "inherit permissions" feature you
introduced in version 2.0.7.  As much needed an addition as it is, I still
believe there is a fundamental security issue.

My premise is simple:  to inherit group permissions for a new file or
sub-directory from the parent directory without also inheriting the group
ownership of the new file or sub-directory from the parent directory is an
exploitable flaw in security.  (i.e.  Inheriting the write attribute for the
group-owner looses its meaning when the group-owner is altered.)

The current implementation of the "inherit permissions" feature in Samba
2.0.7 is 'flawed' (or 'featured' depending upon your point of view ;-) in
this way.  I devised the attached patch to correct this in hopes that in
some form it would be included in the next Samba release.

Note:  this patch does not require that the GID and permissions for the new
file or sub-directory match the GID and permissions of the root of the file
share; it only requires that a match be made to the parent directory.
Furthermore, by administratively changing the ownership and permissions of
sub-directories within the share, the desired behavior David describes in
his post to 'Samba' is still achieved (without setgid bits set at the file
system level).  This flexibility, in fact, would be instrumental in creating
a "Public" share containing a sub-folder for each department in an
organization, with each sub-folder writable only by the members of its
respective department.  Flexibility with no expense to security!

I've been using this patch in a production environment (a plastics firm)
without issue.

I'd like to hear other people's opinions.  Rather than patching Samba,
closing this security hole can be achieved by requiring the sysadmin to use
setgid on the share directory at the operating system level.  This
requirement, however, can only be enforced via documentation, which is poor
enforcement at best and requires an extra step in administration.  Perhaps
there is a better solution?  How can the behavior of an NT file server be
most closely approximated?

I think I've got the best solution nailed, but ...   Anyone care to comment?

--Kyle


Kyle Herbert
Information Technology Director
First 'Net' Impressions, LLC



--- uid.c.original      Thu May 11 04:57:19 2000
+++ uid.c       Sat May 13 12:17:30 2000
@@ -160,9 +160,11 @@
 {
        user_struct *vuser = get_valid_user_struct(vuid);
        int snum;
-    gid_t gid;
+       gid_t gid;
        uid_t uid;
        char group_c;
+       int inheritnewgid;
+       gid_t inheritedgid;

        if (!conn) {
                DEBUG(2,("Connection not open\n"));
@@ -170,6 +172,43 @@
        }

        /*
+        * See if we are inheriting permissions for this service.
+        * Inheriting permissions requires group synchronization.
+        * This will override forced group settings.
+        * Hence, the 'inherit permissions' option should be used
+        * on a share-by-share basis and not as a global setting.
+        */
+
+       inheritnewgid = 0;
+       if(lp_inherit_perms(SNUM(conn)))
+       {
+               pstring path;
+               SMB_STRUCT_STAT sbuf;
+
+               DEBUG(3, ("connectpath (%s), dirpath (%s)\n",
conn->connectpath, conn->dirpath));
+
+               safe_strcpy(path, conn->connectpath, sizeof(path));
+               if(*(conn->dirpath) != '\0')
+               {
+                       safe_strcat(path, "/", sizeof(path));
+                       safe_strcat(path, conn->dirpath, sizeof(path));
+               }
+
+               DEBUG(3, ("path to stat is (%s)\n", path));
+
+               if(dos_stat(path, &sbuf) != 0)
+               {
+                       DEBUG(3, ("could not stat path\n"));
+               }
+               else
+               {
+                       DEBUG(3, ("gid of path = %d\n",(int)(sbuf.st_gid)));
+                       inheritnewgid = ((gid_t)sbuf.st_gid !=
current_user.gid);
+                       inheritedgid = (gid_t)sbuf.st_gid;
+               }
+       }
+
+       /*
         * We need a separate check in security=share mode due to vuid
         * always being UID_FIELD_INVALID. If we don't do this then
         * in share mode security we are *always* changing uid's between
@@ -177,12 +216,12 @@
         */

        if((lp_security() == SEC_SHARE) && (current_user.conn == conn) &&
-          (current_user.uid == conn->uid)) {
+          (current_user.uid == conn->uid) && (!inheritnewgid)) {
                DEBUG(4,("Skipping become_user - already user\n"));
                return(True);
        } else if ((current_user.conn == conn) &&
                   (vuser != 0) && (current_user.vuid == vuid) &&
-                  (current_user.uid == vuser->uid)) {
+                  (current_user.uid == vuser->uid) && (!inheritnewgid)) {
                DEBUG(4,("Skipping become_user - already user\n"));
                return(True);
        }
@@ -239,7 +278,16 @@
                        gid = conn->gid;
                }
        }
-
+
+       /*
+        * Inherited GID takes precedence over forced options
+        */
+
+       if(inheritnewgid)
+       {
+               gid = inheritedgid;
+       }
+
        if (!become_gid(gid))
                return(False);

@@ -260,7 +308,7 @@
        current_user.conn = conn;
        current_user.vuid = vuid;

-       DEBUG(5,("become_user uid=(%d,%d) gid=(%d,%d)\n",
+       DEBUG(3,("become_user uid=(%d,%d) gid=(%d,%d)\n",

(int)getuid(),(int)geteuid(),(int)getgid(),(int)getegid()));

        return(True);