Inherit Permissions addition

[Kyle Herbert]

Gentlemen,

I was delighted to read about and test the "inherit permissions" feature of
version 2.0.7 for myself.  This is a wonderful addition to Samba; thank you,
David Lee, for your fine work.

After initial testing, it became apparent to me that inheriting the 'group'
permissions on a subfolder within a share without also 'forcing' the group
ownership on the new files and directories in that subfolder would quickly
undermine the purpose of the feature and lead to a potential breach in
security.

The documentation for 'inherit permissions' indicates that the systems
administrator can resolve this issue by using the set-gid bit on the share
directory ("New directories inherit the mode of the parent directory,
including bits such as setgid.").

Rather than have this functionality of Samba depend upon correct
administration at the file system level, I thought it better to have Samba
force group id inheritance when the 'inherit permissions' attribute is set.
After all, I could not imagine a situation wherein the group permissions
should be maintained without the group id; and, this saves one step in the
overall administration.

Therefore, I humbly submit this patch for your review.  I've been testing
this in a production environment for a couple weeks now without any issues
(Redhat 6.1).  I've only studied the Samba source code for 14 hours or so --
there may be some efficiencies that your more experienced eyes can add to my
patch (i.e.. is it necessary to build a full path out of conn->connectpath
and conn->dirpath for the dos_stat? or would dos_stat 'ing the conn->dirpath
be sufficient?).  I also have not modified any documentation.

By the way, this is my first contribution to an open source software
project.  Thank you for the opportunity to help advance such a valuable
project.  Any comments or feedback would be very welcome.

Sincerely,
--Kyle


Kyle Herbert
First 'Net' Impressions, LLC

-------------- next part --------------

--- 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);