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);
Re:> Date: Mon, 22 May 2000 00:01:14 -0500 > From: "Kyle Herbert" <kyleh@firstnetimpressions.com> > To: <T.D.Lee@durham.ac.uk>, <samba@samba.org> > Subject: Inherit Permissions addition > > 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.I see your point. Bear in mind throughout that a principal aim of this mode is for home directories at large installations (many thousands of users) using a single [homes] share, where most users need have no real knowledge of UNIX. Thus the UNIX/Samba administrator needs to be able to set up [homes] to get things 98% intuitive for 98% of them 98% of the time. There will always be the remaining 2% who need greater flexibility: let them learn UNIX! The issue can be argued either way. So let me put "the other side". In setting the read/write bits, "inherit permissions" adds new, useful functionality which is not already there. By contrast the principles of "setgid" on directories are already implemented within most UNIX systems. So if "inherit permissions" were to force "setgid" behaviour, regardless of the "setgid" bit itself, this actually removes flexibility (it will have disabled an infrequently used, but useful, switch). One could well envisage the following home directory structure (thousands of users, remember, in [homes]): . : 711 (private as possible, just enough through-access) ./public_html : 755 (public) ./normalgroup : 750 (group-access to my usual group) ./specialgroup : 2750 (owned by non-default group; setgid bit set). All new files and subdirectories (recursively), except those in the three special cases, get 600 (files) or 711 (directories) and the normal group. All new files/subdirectories in "public_html" get 644/755 and the normal group. All new files/subdirectories in "normalgroup" get 640/750 and the normal group. All new files/subdirectories in "specialgroup" get 640/2750 : the group-owner is set via the setgid bit, using the owner of the directory. This gives reasonably full functionality. By contrast, if "inherit permissions" were to force setgid, then we would have lost an aspect of flexibility. So my own view, on balance, is to keep the existing functionality. Perhaps the patch should be a documentation patch describing the various possibilities opened up by this parameter. Indeed, perhaps this should be part of a larger document which describes "force create mode" etc.> 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.Thanks. I hope my apparently negative reaction (I hope it wasn't too negative) hasn't put you off, but rather encouraged you. My guess is that any further discussion should take place on the "samba-technical" list (rather than here on "samba"). -- : David Lee I.T. Service : : Systems Programmer Computer Centre : : University of Durham : : http://www.dur.ac.uk/~dcl0tdl South Road : : Durham : : Phone: +44 191 374 2882 U.K. :