Bug 6802 - acl_xattr.c module: A created folder does not properly inherit permissions from parent
Summary: acl_xattr.c module: A created folder does not properly inherit permissions fr...
Status: ASSIGNED
Alias: None
Product: Samba 3.4
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 3.4.0
Hardware: Other Windows XP
: P3 normal
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-12 18:54 UTC by Barry Sabsevitz (mail address dead)
Modified: 2011-04-08 15:49 UTC (History)
1 user (show)

See Also:


Attachments
Possible fix for inheritance problem in acl_xattr module (2.19 KB, patch)
2009-10-20 22:58 UTC, Barry Sabsevitz (mail address dead)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Barry Sabsevitz (mail address dead) 2009-10-12 18:54:29 UTC
We are using the acl_xattr.c vfs module on samba 3.4.0. When we try the following experiment, we receive an error:

1. Mount a CIFS share on a Windows Server 2003 client being exported from a Linux system running Samba 3.4.0. This Linux system is using the acl_xattr vfs module.
2. Create a folder called "test1" on the CIFS share via the Windows client. When I click on properties on this folder and then click security I see it shows Administrator and also has an entry for SYSTEM. 
3. right click on the test1 folder and click properties. Click Security. Click add and add a new user to the folder, e.g. testuser. Now it lists Administrator, SYSTEM, and testuser.
4. Now enter the test1 folder and right click to create a new folder within the test1 folder. 

The new folder will be created initially as "New Folder", but when we try to name the folder as something else we get access denied and the folder remains "New Folder". We right click on properties on the "New Folder" and then click security we see that the folder only has an entry for testuser. 

This is likely why the rename is failing because I am logged into the Windows Client as administrator but yet don't have the rights to rename the folder as it only has rights for testuser. It looks like the acl_xattr module is not inheriting the administrator and SYSTEM entries from the parent folder for the new folder that is being created.

Looking in the Samba code, it looks like when the test1 folder is created, it uses the default ace's created in default_file_sd(). And these ACES are created in such a way that they are not marked as inheritable, such that when later the routine se_create_child_secdesc() is invoked in the path where I create the subfolder in test1, it calls is_inheritable_ace(), and fails to inherit the aces to the newly created folder.

I'm wondering if the fix is that in default_file_sd(), the call to init_sec_ace() should mark those aces as inheritable? It may be more complicated than this but I'm not an expert on Windows ACls so don't really know.
Comment 1 Barry Sabsevitz (mail address dead) 2009-10-12 21:07:05 UTC
The last argument to init_sec_ace() is the one that defines whether the ACE is inheritable.
Comment 2 Jeremy Allison 2009-10-14 19:36:24 UTC
Hi Jeremy,

I'm looking into Bug # 6802 that we filed on the acl_xattr module. I can get the "proper" behavior by modifying the code in the function default_file_sd() to do the following:

OLD CODE:
        ....
        init_sec_ace(&pace[0], &owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0);
        init_sec_ace(&pace[1], &owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, 0);
        .....


NEW CODE:
                Function has new argument indicating whether we are creating a container.

        ....
                init_sec_ace(&pace[0], &owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, (container ?                                                                                  (SEC_ACE_FLAG_OBJECT_INHERIT|SEC_ACE_FLAG_CONTAINER_INHERIT): 0));

                init_sec_ace(&pace[1], &owner_sid, SEC_ACE_TYPE_ACCESS_ALLOWED, SEC_RIGHTS_FILE_ALL, (container ?                                                                                  (SEC_ACE_FLAG_OBJECT_INHERIT|SEC_ACE_FLAG_CONTAINER_INHERIT): 0));
        .....


Basically my proposed fix is that if we are creating a container that does not inherit anything from a parent (i.e. we end up in default_file_sd()), the default descriptor ACE's should be marked as inheritable for the owner and system.

This seems to work when I try the experiment mentioned in the defect report. But I don't have the warm and fuzzy's because when I run smbtorture on the share with this fix, it complains about the ACE's having these bits set. I'm a bit new at understanding Windows ACLs and how they work. Do you think the above fix would be a valid one or have I misunderstood the Samba code?

The original problem reported was that:
1. When a folder is created, Samba invokes default_file_sd() and creates ACE's for owner and for system but marks the flags as 0.
2. If I add an ACE for a different user to that folder, we now have the following:
                owner -> with flags 0 (not inheritable)
                system -> with flags 0 (not inheritable)
                newuser -> with flags set (inheritable)

3. When I create a subfolder, the subfolder is created ONLY with an ACE for newuser as he/she was the only inheritable ACE from the parent since owner and system are marked as not inheritable. If I had never created the user in the first place, the newly created subfolder would have gone through the default_file_sd() code and we would have ended up with owner and system being marked as not inheritable.

It seems like the fix should be that owner and system are marked inheritable but I can't be 100% sure this is the right fix.
Comment 3 Jeremy Allison 2009-10-14 19:37:02 UTC
Ok, this is a very interesting observation. The test code in
smbtorture4 RAW-ACLS test_inheritance() sets a number of very
specific security descriptors on the containing directory -
one of which that doesn't inherit to children, and then creates
files and directories within it and sees what comes back.

The current code in the acl_xattr module passes that test,
so we know it's correct for a default created non-inherited
SD.

The problem is in a real NTFS filesystem there is *never* a
directory created with no inherited ACE's. The very top level
of all drives in a SD with inherited ACE's, so the situation in
Samba where usually on a fresh share or share that hasn't had
an NT-ACL applied to the root directory we have *no* parent
SD is a very unusual case.

The fix is probably to do something like you're proposing
in the case where the parent dir has *no* ACL, but not in
the case where there already is a specific ACL on the parent
dir (that should take care of the RAW-ACLS test_inheritance()
case I think). Let me dig into this more. I'm planning
to add this text to the bug report as it helps a lot.

Thanks !

Jeremy.
Comment 4 Barry Sabsevitz (mail address dead) 2009-10-20 22:58:50 UTC
Created attachment 4874 [details]
Possible fix for inheritance problem in acl_xattr module

Possible fix for inheritence problem in vfs_acl_xattr module. Needs review. Based on conversation that I had with Jeremy.
Comment 5 Barry Sabsevitz (mail address dead) 2009-10-20 23:00:06 UTC
Jeremy, I uploaded a possible patch to this bug based on the discussion we had earlier. I think this is what you were proposing as a possible fix. It passes smbtorture and fixes the problem we are seeing. 
Comment 6 Jeremy Allison 2009-10-23 13:52:14 UTC
Applied to 3.5.0 and master - thanks ! This is a little too late for 3.4.3, but I'll investigate it for 3.4.4.
Jeremy.
Comment 7 Jeremy Allison 2009-11-20 16:07:07 UTC
Can you confirm this fix in 3.5.0 please ? If so I'll close this bug.
Jeremy.
Comment 8 Jeremy Allison 2009-12-02 16:42:21 UTC
Ok, I've removed this change as I'm going to address the problem a different way when a SD doesn't exist on the parent directory. Changes to follow.
Jeremy.
Comment 9 Karolin Seeger 2010-04-08 04:20:35 UTC
Jeremy, is there a chance to fix this one for 3.4.8 (scheduled for May 11)?