Bug 4211 - Samba 3.0.23c posix_acl bug
Summary: Samba 3.0.23c posix_acl bug
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.0
Classification: Unclassified
Component: File Services (show other bugs)
Version: 3.0.9
Hardware: Other Linux
: P3 normal
Target Milestone: none
Assignee: Samba Bugzilla Account
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-11-06 21:56 UTC by David Daugherty
Modified: 2007-03-23 08:01 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Daugherty 2006-11-06 21:56:38 UTC
Thanks to Jim Wang of Wesoft for his good eyes...

Line 2260 of posix_acl() (function acl_group_override()) reads:

...
 &&(lp_acl_group_control(SNUM(conn) || ...

note the missing ) after SNUM(conn)

It should read:
 &&(lp_acl_group_control(SNUM(conn)) || ...

and get rid of a ')' at the end
Comment 1 Jeremy Allison 2006-11-07 12:34:41 UTC
Well spotted - will be in 3.0.23d - thanks !
Jeremy.
Comment 2 David Daugherty 2006-11-15 12:46:40 UTC
We confirmed with our customer that in fact after making the change users who belonged to the file's primary group can now modify the ACLs.  However reading smb.conf documentation about dos filemode = yes, it seems that this is still too restrictive (and the customer says it is now more restrictive than it used to be in 3.0.20b).  

According to dos filemode documentation:

"Enabling this parameter allows a user who has write access to the file (by whatever means) to modify the permissions (including ACL)"

We have not tested this change yet ourselves, but this is what we plan to try:

...

if (errno EACCESS || errno == EPERM)
{

  if (!CAN_WRITE(conn))
   return False;

  if (lp_acl_group_control(SNUM(conn) && current_user_in_group(prim_gid))
    return True;
 
  if (lp_dos_filemode(SNUM(conn)) {
    SMB_STRUCT_STAT st;
    files_struct *fsp = open_file_fchmod(conn, fname, &st);
    if (fsp) {
      close_file_fchmod(fsp);
      return True;
     }
  }
  return False;
}

If you think this is bad just close the bug again :)

Customer's example:
example, the directory dept
root@kashyyyk:/export/shared$ getfacl dept
 
# file: dept
# owner: cens_owner
# group: 11
user::rwx
group::rwx              #effective:rwx
group:cens_it-staff:rwx         #effective:rwx
mask:rwx
other:r-x
default:user::rwx
default:group::rwx
default:group:cens_it-staff:rwx
default:mask:rwx
default:other:r-x
 
The group cens_it-staff cannot update permissions, even with a group and default
group acl set. 
 
If we chgrp the directory so that cens_it-staff owns it...
root@kashyyyk:/export/shared$ getfacl dept
 
# file: dept
# owner: cens_owner
# group: cens_it-staff
user::rwx
group::rwx              #effective:--x
other:r-x
default:user::rwx
default:group::rwx
default:group:cens_it-staff:rwx
default:mask:rwx
default:other:r-x
 
Now members of the group cens_it-staff change change acl's from windows.
 
 
This also appears to be a regression from the previously working samba release.
Any thoughts?
 
 
Comment 3 David Daugherty 2006-11-30 12:32:27 UTC
Here is how Jim Yang modified the code to conform to our understanding of the "dos filemode" directive.

2257c2257
< static BOOL acl_group_override(connection_struct *conn, gid_t prim_gid,char *fname)
---
> static BOOL acl_group_override(connection_struct *conn, gid_t prim_gid)
2259c2259,2261
< 	if (errno==EACCES || errno == EPERM)
---
> 	if ( (errno == EACCES || errno == EPERM) 
> 		&& (lp_acl_group_control(SNUM(conn) || lp_dos_filemode(SNUM(conn)))) 
> 		&& current_user_in_group(prim_gid) ) 
2260a2263,2264
> 		return True;
> 	} 
2262,2285c2266
<   		if (!CAN_WRITE(conn))
<    			return False;
< 
<  		 /* This make sure that when acl group control = yes, only user in the group can change the ownership */ 
<   		if (lp_acl_group_control(SNUM(conn)) && current_user_in_group(prim_gid))
<   			return True;
< 
< 		/* When dos filemode = yes, we need to check if the current credential has write permission
< 		   if yes, then user can change the group permission
< 		   if no, then user does not have right to change permission */ 
<   		if (lp_dos_filemode(SNUM(conn))) 
<   		{
<     			SMB_STRUCT_STAT st;
<     			files_struct *fsp;
<     			BOOL bacc = False;
<     			
<     			bacc = can_access_file(conn,fname,&st,FILE_WRITE_DATA);
< 
<     			if (bacc == True)
<     				return bacc;
<   		}
<   	return False;
< 	}
< 
---
> 	return False;
2482c2463
< 			if (acl_group_override(conn, prim_gid,fsp->fsp_name)) {
---
> 			if (acl_group_override(conn, prim_gid)) {
2513c2494
< 			if (acl_group_override(conn, prim_gid,fsp->fsp_name)) {
---
> 			if (acl_group_override(conn, prim_gid)) {
3252c3233
< 						if (acl_group_override(conn, sbuf.st_gid,fsp->fsp_name)) {
---
> 						if (acl_group_override(conn, sbuf.st_gid)) {
3299c3280
< 						if (acl_group_override(conn, sbuf.st_gid,fsp->fsp_name)) {
---
> 						if (acl_group_override(conn, sbuf.st_gid)) {
Comment 4 Gerald (Jerry) Carter (dead mail address) 2007-03-23 08:01:51 UTC
fixed.