[Samba] still ACL bug in 3.0.14a

[Jeremy Allison]

On Sat, Apr 16, 2005 at 11:42:33PM -0400, Stewart, Eric wrote:
> 	If someone has this working on Red Hat Enterprise Linux 3, I'd
> like a few pointers.
> 	I've changed "defaults" in /etc/fstab for the affected partition
> to "defaults,acl,user_xattr" and rebooted the box.  I've gone so far as
> to make sure all processes were killed, remove the samba sbin, bin, lib,
> and include directories, checked to make sure ACL support is being
> compiled in (ldd even shows libacl.so.1 linked).  I've even gotten
> desperate and and added "delete readonly = yes" and even "nt acl support
> = no" (in all sorts of combinations) to the junk share in the config
> below, and yet I still get access denied when attempting to delete a
> file.  ls -laF shows:

Ok, I'd like to explain why we didn't catch this in 3.0.14a
testing.

We get the group array in our current_user process
token with a getgroups() call. Under Linux, the getgroups()
call returns the current effective group-id of the process
in this list, so the ACL code only needed to check the
group list, not the effective-gid separately in order to
correctly check all permissions. Both Jerry and I do
primary testing on Linux, under which this code would
work fine.

If Solaris does this differently then this explains the
problem people saw on Solaris not being solved with --with-acl-support.

It's a problem, I hate that this bug is there, and it was
in my code so it's my fault. I'm going to discuss with
Jerry how we could have caught this earlier for future
releases.

Suggestions from users on how we can improve our testing
before release are also most welcome - except for ones
which ask people to do more testing, as the usually say
they will but don't (and I'm as guilty of that as anyone :-).

In case you missed my earlier message, I'm attaching the
patch for 3.0.14a to ensure effective gid's get checked
on delete requests also.

Jeremy.
-------------- next part --------------
Index: smbd/posix_acls.c
===================================================================
--- smbd/posix_acls.c	(revision 6363)
+++ smbd/posix_acls.c	(working copy)
@@ -3867,6 +3867,23 @@
 				if (pgid == NULL) {
 					goto check_stat;
 				}
+
+				/* Does it match the current effective group ? */
+				if (current_user.gid == *pgid) {
+					ret = have_write;
+					DEBUG(10,("check_posix_acl_group_write: file %s \
+match on group %u -> can write.\n", fname, (unsigned int)*pgid ));
+
+					/* If we don't have write permission this entry doesn't
+					 * prevent the subsequent enumeration of the supplementary
+					 * groups.
+					 */
+					if (have_write) {
+						goto done;
+					}
+				}
+
+				/* Continue with the supplementary groups. */
 				for (i = 0; i < current_user.ngroups; i++) {
 					if (current_user.groups[i] == *pgid) {
 						ret = have_write;
@@ -3898,6 +3915,15 @@
 
 	/* Do we match on the owning group entry ? */
 
+	/* First, does it match the current effective group ? */
+	if (current_user.gid == psbuf->st_gid) {
+		ret = (psbuf->st_mode & S_IWGRP) ? 1 : 0;
+		DEBUG(10,("check_posix_acl_group_write: file %s \
+match on owning group %u -> %s.\n", fname, (unsigned int)psbuf->st_gid, ret ? "can write" : "cannot write"));
+		goto done;
+	}
+
+	/* If not look at the supplementary groups. */
 	for (i = 0; i < current_user.ngroups; i++) {
 		if (current_user.groups[i] == psbuf->st_gid) {
 			ret = (psbuf->st_mode & S_IWGRP) ? 1 : 0;