Bug 10489 - Unable to set permissions through windows on fs with no posix acls (noacl)
Unable to set permissions through windows on fs with no posix acls (noacl)
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.1.5
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-07 21:12 UTC by John Mulligan
Modified: 2016-02-17 08:22 UTC (History)
3 users (show)

See Also:


Attachments
samba 3.6 log (10.52 KB, application/x-gzip)
2014-03-07 21:19 UTC, John Mulligan
no flags Details
Samba 4.1 log (20.39 KB, application/x-gzip)
2014-03-07 21:19 UTC, John Mulligan
no flags Details
git-am fix for master. (2.54 KB, patch)
2016-02-09 20:55 UTC, Jeremy Allison
no flags Details
git-am fix for master. (2.57 KB, patch)
2016-02-10 00:25 UTC, Jeremy Allison
no flags Details
git-am fix for 4.4.0, 4.3.x, 4.2.x (2.80 KB, patch)
2016-02-12 23:29 UTC, Jeremy Allison
uri: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Mulligan 2014-03-07 21:12:16 UTC
Using Samba version 3.6 a file system without posix acl support Samba would manipulate the traditional unix permissions instead. When trying the same operation with samba 4.1.5 the operation fails with an "Access is Denied" dialog box.

The specific test I ran was the following: using a Linux server with Samba 3.6.10 installed, I created a ext4 file system and mounted it with the noacl mount option. I created a world writable dir for the windows test system to write into.

$ lvcreate --name srv --size 4G  foo
$ mkfs.ext4 /dev/mapper/foo-srv
$ mount /dev/mapper/foo-srv  /srv/ -o noatime,noacl
$ mkdir /srv/abc
$ chmod ugo+rwx /srv/abc

I set up samba with the following config:

[global]
        workgroup = ENGWIN2K8
        realm = ENGWIN2K8.FOOBAR.NET
        server string = SMB Server
        security = ADS
        allow trusted domains = No
        syslog = 0
        server max protocol = SMB2
        min receivefile size = 131072
        load printers = No
        printcap name = /dev/null
        disable spoolss = Yes
        show add printer wizard = No
        winbind enum users = Yes
        winbind enum groups = Yes
        idmap config *: read only = yes
        idmap config *: range = 200000-210000
        idmap config ENGWIN2K8: read only = no
        idmap config ENGWIN2K8: backend = rid
        idmap config ENGWIN2K8: range = 400000-8000000
        idmap config * : backend = tdb
        force unknown acl user = Yes

[testsrv]
        path = /srv/
        read only = No
        inherit permissions = Yes
        inherit acls = Yes
        map acl inherit = Yes
        hide unreadable = Yes
        veto files = /.snapshot/
        store dos attributes = Yes
        delete readonly = Yes
        dos filemode = Yes

Using a Windows 2008 Server system as a client I connected to the testsrv share and created file one.txt. Following that I changed the permissions of the group and then both the group and other (everyone) using the windows security tab.

$ getfacl  /srv/abc/one.txt 
getfacl: Removing leading '/' from absolute path names
# file: srv/abc/one.txt
# owner: ENGWIN2K8\134johnm
# group: ENGWIN2K8\134domain\040users
user::rwx
group::rw-
other::rw-

$ ls -l  /srv/abc/one.txt
-rwxrw-rw- 1 ENGWIN2K8\johnm ENGWIN2K8\domain users 0 Mar  7 15:14 /srv/abc/one.txt


$ getfacl  /srv/abc/one.txt 
getfacl: Removing leading '/' from absolute path names
# file: srv/abc/one.txt
# owner: ENGWIN2K8\134johnm
# group: ENGWIN2K8\134domain\040users
user::rwx
group::r--
other::rw-

$ ls -l  /srv/abc/one.txt
-rwxr--rw- 1 ENGWIN2K8\johnm ENGWIN2K8\domain users 0 Mar  7 15:14 /srv/abc/one.txt


$ getfacl  /srv/abc/one.txt 
getfacl: Removing leading '/' from absolute path names
# file: srv/abc/one.txt
# owner: ENGWIN2K8\134johnm
# group: ENGWIN2K8\134domain\040users
user::rwx
group::---
other::---

$ ls -l  /srv/abc/one.txt 
-rwx------ 1 ENGWIN2K8\johnm ENGWIN2K8\domain users 0 Mar  7 15:14 /srv/abc/one.txt


After verifying that Samba 3.6 worked as expected I upgraded to Samba 4.1.5.
Once upgraded and restarted I tried changing the permissions of the group again (same client). Now the permissions change fails and the following appears in the samba log:

[2014/03/07 15:35:19.306498,  2] ../source3/smbd/open.c:972(open_file)
  ENGWIN2K8\johnm opened file abc/one.txt read=No write=No (numopen=2)
[2014/03/07 15:35:19.307430,  2] ../source3/smbd/posix_acls.c:3014(set_canon_ace_list)
  set_canon_ace_list: sys_acl_set_file type file failed for file abc/one.txt (Operation not supported).
[2014/03/07 15:35:19.307576,  3] ../source3/smbd/posix_acls.c:3099(convert_canon_ace_to_posix_perms)
  convert_canon_ace_to_posix_perms: Too many ACE entries for file abc/one.txt to convert to posix perms.
[2014/03/07 15:35:19.307831,  3] ../source3/smbd/posix_acls.c:3923(set_nt_acl)
  set_nt_acl: failed to convert file acl to posix permissions for file abc/one.txt.
[2014/03/07 15:35:19.308384,  2] ../source3/smbd/close.c:780(close_normal_file)
  ENGWIN2K8\johnm closed file abc/one.txt (numopen=1) NT_STATUS_OK
[2014/03/07 15:35:20.759663,  3] ../source3/smbd/service.c:1122(close_cnum)


If I enable posix acls for the file system and try again Windows saves the acls without error and the posix acls are updated on the file system.

$ mount   /srv/  -o remount,noatime,acl
$ getfacl  /srv/abc/one.txt 
getfacl: Removing leading '/' from absolute path names
# file: srv/abc/one.txt
# owner: ENGWIN2K8\134johnm
# group: ENGWIN2K8\134domain\040users
user::rwx
user:ENGWIN2K8\134domain\040users:r--
group::r--
group:ENGWIN2K8\134domain\040users:r--
group:ENGWIN2K8\134johnm:rwx
mask::rwx
other::---


I did search samba bugzilla and did not see this specific issue already reported. I apologize if I missed something and this has been previously reported. Please let me know if there is any additional information I can provide. I am attaching logs from both logs from the 3.6 and 4.1 tests.
Comment 1 John Mulligan 2014-03-07 21:19:24 UTC
Created attachment 9759 [details]
samba 3.6 log
Comment 2 John Mulligan 2014-03-07 21:19:56 UTC
Created attachment 9760 [details]
Samba 4.1 log
Comment 3 Jeremy Allison 2014-08-05 20:24:12 UTC
More info needed. A wireshark capture showing the ACL sent by the client would help I think.

Jeremy.
Comment 4 Jeremy Allison 2014-08-05 20:43:28 UTC
Ignore the previous comment, it was meant for a different bug (#10072). I'll take a look at this, but to be honest it's going to be a low priority as making smbd work without any underlying ACLs on the file system is getting to be a very minor use-case.

Jeremy.
Comment 5 Tim L 2016-02-08 19:53:23 UTC
Hello there.  I think I ran across this same but in Samba 4.2.8 and think I have a patch. 

The bug is in source3/smbd/posix_acls.c.  Trying to change permissions call set_nt_acl.  From what I can gather the code calls unpack_canon_ace, then ensure_canon_entry_valid_on_set.  That code creates an ACL list from the unix permissions.  It creates 3 ACEs based on the owner, group & other permssions.  But the as the comments at line 1505 says:
        /* Ensure when setting a POSIX ACL, that the uid for a
           SMB_ACL_USER_OBJ ACE (the owner ACE entry) has a duplicate
           permission entry as an SMB_ACL_USER, and a gid for a
           SMB_ACL_GROUP_OBJ ACE (the primary group ACE entry) also has
           a duplicate permission entry as an SMB_ACL_GROUP. If not,
           then if the ownership or group ownership of this file or
           directory gets changed, the user or group can lose their
           access. */

So then there are 5 ACEs. They get passed back and eventually the code calls convert_canon_ace_to_posix_perms and it fails at line 3088:
        if (ace_count != 3) {
                DEBUG(3,("convert_canon_ace_to_posix_perms: Too many ACE "
                         "entries for file %s to convert to posix perms.\n",
                         fsp_str_dbg(fsp)));
                return False;
        }

So basically it appears that if you don't have real ACLs and just use unix permissions it will always fail since the code will always create 5 ACEs in the ACL and then fail since it only wants 3.

I'll attach a proposed patch. Please contact me if you have questions
Comment 6 Tim L 2016-02-08 19:54:11 UTC
Actually the patch is so small I'll just put it here.

*** source3/smbd/posix_acls.c.orig      Mon Feb  8 07:45:38 2016
--- source3/smbd/posix_acls.c   Mon Feb  8 07:45:49 2016
***************
*** 3085,3091 ****
        canon_ace *group_ace = NULL;
        canon_ace *other_ace = NULL;

!       if (ace_count != 3) {
                DEBUG(3,("convert_canon_ace_to_posix_perms: Too many ACE "
                         "entries for file %s to convert to posix perms.\n",
                         fsp_str_dbg(fsp)));
--- 3085,3091 ----
        canon_ace *group_ace = NULL;
        canon_ace *other_ace = NULL;

!       if (ace_count != 5) {
                DEBUG(3,("convert_canon_ace_to_posix_perms: Too many ACE "
                         "entries for file %s to convert to posix perms.\n",
                         fsp_str_dbg(fsp)));
Comment 7 Jeremy Allison 2016-02-09 20:55:05 UTC
Created attachment 11822 [details]
git-am fix for master.

Can you test this fix ? I'm afraid your fix is a little too simple to catch all cases, and I think this might do the trick.

If you can confirm it works, I'll push to master and get it back-ported to all release branches.

Thanks a *lot* for your help on this !

Jeremy.
Comment 8 Tim L 2016-02-09 23:17:03 UTC
Thanks Jeremy.  Unfortunately that didn't quite work.  The log has:

[2016/02/09 14:20:33.256915,  3, pid=55888, effective(165, 20), real(0, 0), class=acls] ../source3/smbd/posix_acls.c:3141(convert_canon_ace_to_posix_perms)
  Invalid type 1, uid 20 in ACE for file tim test.

I went to loglevel 10 so it would print the ACL.  
[2016/02/09 14:20:33.256370, 10, pid=55888, effective(165, 20), real(0, 0), class=acls] ../source3/smbd/posix_acls.c:2935(set_canon_ace_list)
  canon_ace index 0. Type = allow SID = S-1-5-21-1062074158-754817265-4250498807-513 gid 20 (users) SMB_ACL_GROUP ace_flags = 0x0 perms r-x
[2016/02/09 14:20:33.256473, 10, pid=55888, effective(165, 20), real(0, 0), class=acls] ../source3/smbd/posix_acls.c:2935(set_canon_ace_list)
  canon_ace index 1. Type = allow SID = S-1-5-21-1062074158-754817265-4250498807-1330 uid 165 (temp15) SMB_ACL_USER ace_flags = 0x0 perms rwx
[2016/02/09 14:20:33.256585, 10, pid=55888, effective(165, 20), real(0, 0), class=acls] ../source3/smbd/posix_acls.c:2935(set_canon_ace_list)
  canon_ace index 2. Type = allow SID = S-1-5-21-1062074158-754817265-4250498807-1330 uid 165 (temp15) SMB_ACL_USER_OBJ ace_flags =0x0 perms rwx
[2016/02/09 14:20:33.256699, 10, pid=55888, effective(165, 20), real(0, 0), class=acls] ../source3/smbd/posix_acls.c:2935(set_canon_ace_list)
  canon_ace index 3. Type = allow SID = S-1-5-21-1062074158-754817265-4250498807-513 gid 20 (users) SMB_ACL_GROUP_OBJ ace_flags = 0x0 perms r-x
[2016/02/09 14:20:33.256802, 10, pid=55888, effective(165, 20), real(0, 0), class=acls] ../source3/smbd/posix_acls.c:2935(set_canon_ace_list)
  canon_ace index 4. Type = allow SID = S-1-1-0 other SMB_ACL_OTHER ace_flags = 0x0 perms rwx


I think you need to include the last bit about the WORLD_ACE in an else.  Otherwise you will always hit it even if ace_p->owner_type is UID_ACE or GID_ACE.  Here's the updated patch.  It seems to work for me.

*** source3/smbd/posix_acls.c.orig      Tue Jul 14 03:54:24 2015
--- source3/smbd/posix_acls.c   Tue Feb  9 14:59:10 2016
***************
*** 3085,3091 ****
        canon_ace *group_ace = NULL;
        canon_ace *other_ace = NULL;

!       if (ace_count != 3) {
                DEBUG(3,("convert_canon_ace_to_posix_perms: Too many ACE "
                         "entries for file %s to convert to posix perms.\n",
                         fsp_str_dbg(fsp)));
--- 3085,3091 ----
        canon_ace *group_ace = NULL;
        canon_ace *other_ace = NULL;

!       if (ace_count != 5) {
                DEBUG(3,("convert_canon_ace_to_posix_perms: Too many ACE "
                         "entries for file %s to convert to posix perms.\n",
                         fsp_str_dbg(fsp)));
***************
*** 3107,3112 ****
--- 3107,3149 ----
                return False;
        }

+       /*
+        * Ensure all ACE entries are owner, group or other.
+        * We can't set if there are any other SIDs.
+        */
+       for (ace_p = file_ace_list; ace_p; ace_p = ace_p->next) {
+               if (ace_p == owner_ace || ace_p == group_ace ||
+                               ace_p == other_ace) {
+                       continue;
+               }
+               if (ace_p->owner_type == UID_ACE) {
+                       if (ace_p->unix_ug.id != owner_ace->unix_ug.id) {
+                               DEBUG(3,("Invalid uid %u in ACE for file %s.\n",
+                                       (unsigned int)ace_p->unix_ug.id,
+                                       fsp_str_dbg(fsp)));
+                               return false;
+                       }
+               } else if (ace_p->owner_type == GID_ACE) {
+                       if (ace_p->unix_ug.id != group_ace->unix_ug.id) {
+                               DEBUG(3,("Invalid gid %u in ACE for file %s.\n",
+                                       (unsigned int)ace_p->unix_ug.id,
+                                       fsp_str_dbg(fsp)));
+                               return false;
+                       }
+               } else {
+
+                       /*
+                       * There should be no duplicate WORLD_ACE entries.
+                       */
+
+                       DEBUG(3,("Invalid type %u, uid %u in ACE for file %s.\n",
+                               (unsigned int)ace_p->owner_type,
+                               (unsigned int)ace_p->unix_ug.id,
+                               fsp_str_dbg(fsp)));
+                       return false;
+               }
+       }
+
        *posix_perms = (mode_t)0;

        *posix_perms |= owner_ace->perms;
Comment 9 Jeremy Allison 2016-02-10 00:25:23 UTC
Created attachment 11823 [details]
git-am fix for master.

Doh ! Of course - great catch - thanks !

Can you confirm this, then I'll push to master ?
Comment 10 Tim L 2016-02-10 16:52:31 UTC
Hi Jeremy - that patch seemed to do the trick.  Thanks for the fix and all your work on Samba over the years.  We've been using it ~20 years now and love it.
Comment 11 Jeremy Allison 2016-02-12 23:29:19 UTC
Created attachment 11835 [details]
git-am fix for 4.4.0, 4.3.x, 4.2.x

Cherry-pick from master. Applies cleanly to 4.4.0, 4.3.next, 4.2.next.
Comment 12 Uri Simchoni 2016-02-13 20:04:18 UTC
Assigning to Karolin for inclusion in 4.4.0, 4.3.next, 4.2.next.
Comment 13 Karolin Seeger 2016-02-15 09:32:23 UTC
(In reply to Uri Simchoni from comment #12)
Pushed to autobuild-v4-[4|3|2]-test.
Comment 14 Karolin Seeger 2016-02-17 08:22:23 UTC
(In reply to Karolin Seeger from comment #13)
Pushed to all branches.
Closing out bug report.

Thanks!