Bug 4929 - Adding a user to an ACL via Windows XP GUI gives away read access for owning group
Adding a user to an ACL via Windows XP GUI gives away read access for owning ...
Status: RESOLVED FIXED
Product: Samba 3.0
Classification: Unclassified
Component: File Services
3.0.25c
Other Linux
: P3 normal
: none
Assigned To: Jim McDonough
Samba QA Contact
:
: 5094 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-29 11:17 UTC by John Janosik
Modified: 2008-02-19 06:54 UTC (History)
5 users (show)

See Also:


Attachments
main smb.conf file (2.06 KB, text/plain)
2007-08-29 11:19 UTC, John Janosik
no flags Details
Included smb.conf file (242 bytes, text/plain)
2007-08-29 11:20 UTC, John Janosik
no flags Details
Level 10 log of the nttrans set security desc operation (30.11 KB, text/plain)
2007-08-29 11:26 UTC, John Janosik
no flags Details
wireshark capture of adding a user to the acl (97.03 KB, application/octet-stream)
2008-01-24 13:33 UTC, Jim McDonough
no flags Details
Patch (3.12 KB, patch)
2008-01-24 14:09 UTC, Jeremy Allison
no flags Details
Replacement patch (4.49 KB, patch)
2008-01-24 15:03 UTC, Jeremy Allison
no flags Details
Second part of the patch. (1.05 KB, patch)
2008-01-24 15:04 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Janosik 2007-08-29 11:17:09 UTC
Running Samba 3.0.25c compiled with ACL support on RHEL4 Linux.  The server is a member server of a Samba domain.  I'll attach a complete smb.conf later but the smb.conf options I think are relevant are here:

   map archive = no
   map acl inherit = no
   inherit acls = yes
   store dos attributes = yes

To reproduce:

1.  Create a directory with an ACL like the following.  The important point is that the owning group has just execute permission and the default acl also grants only execute permission to the owning group:
 
   [root@rchs5bld samba-3.0.25c]# getfacl /home/group/xlstest/jpjtest/
   getfacl: Removing leading '/' from absolute path names
   # file: home/group/xlstest/jpjtest
   # owner: jpjanosi
   # group: domain\040users
   user::rwx
   group::--x
   group:ltest:rwx
   mask::rwx  
   other::---
   default:user::rwx
   default:group::--x
   default:group:ltest:rwx
   default:mask::rwx
   default:other::---

2.  Create a file in that directory via a Windows client as a user with a primary group of domain users.  The ACL matches the default acl as expected and the ownership is correct:

   [root@rchs5bld samba-3.0.25c]# getfacl /home/group/xlstest/jpjtest/testfile8
   getfacl: Removing leading '/' from absolute path names
   # file: home/group/xlstest/jpjtest/testfile8
   # owner: jpjanosi
   # group: domain\040users
   user::rwx
   group::--x
   group:ltest:rwx
   mask::rwx
   other::---

3.  User the security tab of the file properties window in explorer on a Windows XP client to add a user to ACL for the file.  Only a user was added, none of the check boxes for existing ACEs were clicked or anything done in the advanced tab.  The ACL now has rx for the owning group and r for other which means any domain user can read the file now since x is set down the tree for domain users:
   [root@rchs5bld samba-3.0.25c]# getfacl /home/group/xlstest/jpjtest/testfile8
   getfacl: Removing leading '/' from absolute path names
   # file: home/group/xlstest/jpjtest/testfile8
   # owner: jpjanosi
   # group: domain\040users
   user::rwx
   user:ronda:r-x
   group::r-x
   group:ltest:rwx
   mask::rwx
   other::r--

This happens because smbd/posix_acls.c:set_nt_acl calls smbd/posix_acls.c:append_parent_acl, which then calls smbd/dosmode.c:unixmode.  This function starts with 666 for the mode it returns as its result:
	mode_t result = (S_IRUSR | S_IRGRP | S_IROTH | S_IWUSR | S_IWGRP | S_IWOTH);
With our config the these bits do not get masked off during this function and we the set_net_acl call gives the owning group and other read permission.
Comment 1 John Janosik 2007-08-29 11:19:40 UTC
Created attachment 2900 [details]
main smb.conf file
Comment 2 John Janosik 2007-08-29 11:20:07 UTC
Created attachment 2901 [details]
Included smb.conf file
Comment 3 John Janosik 2007-08-29 11:26:09 UTC
Created attachment 2902 [details]
Level 10 log of the nttrans set security desc operation
Comment 4 John Janosik 2007-12-20 08:46:07 UTC
bmarsh was trying to duplicate this problem on 3.0.25b so we could report this to RedHat.  I looked at this again and have the following comments:

This was introduced in 3.0.25c so bmarsh could not duplicate it on 3.0.25b to report to RedHat for assistance.  Here is the change to smbd/posix_acls.c that introduces the problem.

***************
*** 3188,3193 ****
--- 3403,3420 ----

        create_file_sids(&sbuf, &file_owner_sid, &file_grp_sid);

+       if ((security_info_sent & DACL_SECURITY_INFORMATION) &&
+               psd->dacl != NULL &&
+               (psd->type & (SE_DESC_DACL_AUTO_INHERITED|
+                             SE_DESC_DACL_AUTO_INHERIT_REQ))==
+                       (SE_DESC_DACL_AUTO_INHERITED|
+                        SE_DESC_DACL_AUTO_INHERIT_REQ) ) {
+               NTSTATUS status = append_parent_acl(fsp, &sbuf, psd, &psd);
+               if (!NT_STATUS_IS_OK(status)) {
+                       return False;
+               }
+       }
+
        acl_perms = unpack_canon_ace( fsp, &sbuf, &file_owner_sid, &file_grp_sid,
                                        &file_ace_list, &dir_ace_list, security_info_sent, psd);


So I think there are two problem with this new code.  First since we have "map acl inherit = no" we should not being trying to do win2k like inheritance of ACLs and so the if should fail and we shouldn't call append_parent_acl.  The second bug is that append_parent_acl starts off with 666 for mode bits and builds from there because it calls smbd/dosmode.c:unixmode at the start.
Comment 5 Jim McDonough 2008-01-21 09:48:10 UTC
I'm not convinced yet that we shouldn't be doing _any_ attempt of acl inheritance.  I'm trying to think about this on systems where we simply don't have any xattrs to use for the mapping.  The "map acl inherit" parm is merely about storing those inheritance bits to get a more accurate inheritance.  In the "no" case we are inferring these bits...and perhaps we need to revisit this.
Comment 6 Jim McDonough 2008-01-21 10:00:34 UTC
I should just add that the group mode bits still shouldn't be changing in this case, so your second point is still correct.
Comment 7 Jim McDonough 2008-01-24 13:05:08 UTC
Ok, so as John pointed out to me, the key here is that the SE_DESC_DACL_PROTECTED flag is on, so the ACE for CREATOR_GROUP that has been set on the child shouldn't be overwritten by the inherited ACE for the same.

Perhaps this points to a merge logic error instead?
Comment 8 Jim McDonough 2008-01-24 13:33:38 UTC
Created attachment 3118 [details]
wireshark capture of adding a user to the acl

frame 510
Comment 9 Jeremy Allison 2008-01-24 14:09:01 UTC
Created attachment 3119 [details]
Patch

I cannot tell a lie :-). Jim told me how to fix this bug and was gracious enough to let me write the patch :-).
Thanks Jim !
Let me know if this fixes it.
Jeremy.
Comment 10 Jeremy Allison 2008-01-24 15:03:51 UTC
Created attachment 3120 [details]
Replacement patch

This is a commented version of the first patch.
Jeremy.
Comment 11 Jeremy Allison 2008-01-24 15:04:13 UTC
Created attachment 3121 [details]
Second part of the patch.

Second part of the patch.
Comment 12 Jim McDonough 2008-01-25 16:07:19 UTC
John, when you get a chance, can you verify?  If you want to build yourself with just patches, you'll want:
0e7886a3ceb8406c5e331a66c0e6fb6ab4493a3e
485cedadb0e61775e6cb152f42f4dfdf17e82666
6b594996a8dff0c6c663752f06a994c95020d869
7a529c43181eb9b3926b214b2fe84aea06be7a3c
938f78546a4706f25d7b07efbca97a6b2d12d4b9
b83dfaf09679b0bbd7341230e1e96b53ae5289cb
fc0508922417e9ef9a4450067d29d15121b52902

or just pull the latest.
Comment 13 John Janosik 2008-01-28 20:45:55 UTC
Jim -

Looks good.  I pulled the latest 3.0 last night, the version is reported as 3.0.28a-GIT-f04810e-test.  It worked properly when setting ACLs from Windows XP in our setup.  I also tested with Excel and was unable to duplicate the problems we had seen prior to 3.0.25 with files going read-only when opened back and forth between multiple users.
Comment 14 Jim McDonough 2008-01-28 22:00:00 UTC
Closing as fixed 
Comment 15 Roel van Meer 2008-01-30 10:39:02 UTC
*** Bug 5094 has been marked as a duplicate of this bug. ***
Comment 16 mvanraaij 2008-02-19 06:27:52 UTC
The problem is still here. I also tried the patches with no luck. This problem
is not fixed in every situation.
Comment 17 Jim McDonough 2008-02-19 06:54:06 UTC
First, what version have you tested?  the attached patches, the mentioned git patches, latest git, etc.?  

Second, you'll need to give more details on what part is not working, as the original reported case is working for the reporter.  You may need to provide instructions to reproduce, smb.conf, log files...