[PATCH] Two attempts in fixing default ACL in vfs_acl_xattr
Ralph Boehme
slow at samba.org
Fri Jul 15 16:46:20 UTC 2016
Hi!
For files or directories without an ACL xattr we build an ACL from the
filesytem mode including user, group and other ACE, plus an ACE for
system.
This can lead to unexpected ACLs and doesn't match Windows behaviour,
eg:
- a directory with the following ACL:
$ ./bin/smbcacls -Uslow%pass //localhost/normal ""
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\root
ACL:SLOW\slow:ALLOWED/0x0/FULL
So only one non-inheritable(!) ACE.
- creating a subdirectory:
$ ./bin/smbclient -Uslow%pass //localhost/normal -c "mkdir dir1"
- checking whether there's an ACL xattr:
$ getfattr -m "" /Volumes/normal/dir1
getfattr: Removing leading '/' from absolute path names
system.posix_acl_access
system.posix_acl_default
user.DOSATTRIB
So there isn't an ACL xattr, because there where no inheritable ACEs
on the parent folder.
- reading the new subdirectories ACL:
$ ./bin/smbcacls -Uslow%pass //localhost/normal "dir1"
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\slow
ACL:SLOW\slow:ALLOWED/0x0/FULL
ACL:Unix Group\slow:ALLOWED/0x0/READ
ACL:Everyone:ALLOWED/0x0/READ
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
The ACES for "SLOW\slow", "Unix Group\slow" and "Everyone" are coming
from make_default_filesystem_acl(). This is the problem.
- Windows assigns the following ACL in this situation:
$ ./bin/smbcacls -UAdministrator%Passw0rd //10.10.10.14/data "dir"
REVISION:1
CONTROL:SR|PD|DI|DP
OWNER:VORDEFINIERT\Administratoren
GROUP:WIN2008R2\Domänen-Benutzer
ACL:WIN2008R2\Administrator:ALLOWED/0x0/FULL
$ ./bin/smbclient -UAdministrator%Passw0rd //10.10.10.14/data -c "mkdir dir\dir1"
$ ./bin/smbcacls -UAdministrator%Passw0rd //10.10.10.14/data "dir\dir1"
REVISION:1
CONTROL:SR|DI|DP
OWNER:VORDEFINIERT\Administratoren
GROUP:WIN2008R2\Domänen-Benutzer
ACL:VORDEFINIERT\Administratoren:ALLOWED/0x0/FULL
ACL:NT-AUTORITÄT\SYSTEM:ALLOWED/0x0/FULL
By changing make_default_filesystem_acl() to only adds user and system
ACE to the ACL of objects that lack an ACL xattr, we match Windows
behaviour:
$ ./bin/smbclient -Uslow%pass //localhost/normal -c "mkdir dir2"
$ ./bin/smbcacls -Uslow%pass //localhost/normal "dir2"
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\slow
ACL:SLOW\slow:ALLOWED/0x0/FULL
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
You can get the desired behaviour by setting all of the following
parameters:
acl_xattr:ignore system acls = yes
directory mask = 0700
create mask = 0600
but this is rather unobvious.
I have a patch (acl-default.patch) that fixes
make_default_filesystem_acl() to only return user and system ACE.
For completeness, I've attached another patch (acl-mask.patch) that
automatically sets the last two parameters in case "ignore system
acls" is enabled when the acl_xattr module is loaded. This was the
first attempt to fix this and is probably not what we want.
Review and comments appreciated. If ok, please don't push yet, I'd
like to create a bugreport first and add the bug link to the commit
messages.
Cheerio!
-slow
-------------- next part --------------
From 268ab6d380f7401ac65745ff2c426521b99be657 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 15 Jul 2016 17:56:02 +0200
Subject: [PATCH 1/2] s3/smbd: move make_default_filesystem_acl() to
vfs_acl_common.c
This function is only used in vfs_acl_common.c and will be modified in
the next commit.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/modules/vfs_acl_common.c | 108 ++++++++++++++++++++++++++++++++++++++
source3/smbd/posix_acls.c | 110 ---------------------------------------
source3/smbd/proto.h | 4 --
3 files changed, 108 insertions(+), 114 deletions(-)
diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index 98e1b8e..f3d6c0d 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -358,6 +358,114 @@ static NTSTATUS add_directory_inheritable_components(vfs_handle_struct *handle,
return NT_STATUS_OK;
}
+static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
+ const char *name,
+ SMB_STRUCT_STAT *psbuf,
+ struct security_descriptor **ppdesc)
+{
+ struct dom_sid owner_sid, group_sid;
+ size_t size = 0;
+ struct security_ace aces[4];
+ uint32_t access_mask = 0;
+ mode_t mode = psbuf->st_ex_mode;
+ struct security_acl *new_dacl = NULL;
+ int idx = 0;
+
+ DEBUG(10,("make_default_filesystem_acl: file %s mode = 0%o\n",
+ name, (int)mode ));
+
+ uid_to_sid(&owner_sid, psbuf->st_ex_uid);
+ gid_to_sid(&group_sid, psbuf->st_ex_gid);
+
+ /*
+ We provide up to 4 ACEs
+ - Owner
+ - Group
+ - Everyone
+ - NT System
+ */
+
+ if (mode & S_IRUSR) {
+ if (mode & S_IWUSR) {
+ access_mask |= SEC_RIGHTS_FILE_ALL;
+ } else {
+ access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
+ }
+ }
+ if (mode & S_IWUSR) {
+ access_mask |= SEC_RIGHTS_FILE_WRITE | SEC_STD_DELETE;
+ }
+
+ init_sec_ace(&aces[idx],
+ &owner_sid,
+ SEC_ACE_TYPE_ACCESS_ALLOWED,
+ access_mask,
+ 0);
+ idx++;
+
+ access_mask = 0;
+ if (mode & S_IRGRP) {
+ access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
+ }
+ if (mode & S_IWGRP) {
+ /* note that delete is not granted - this matches posix behaviour */
+ access_mask |= SEC_RIGHTS_FILE_WRITE;
+ }
+ if (access_mask) {
+ init_sec_ace(&aces[idx],
+ &group_sid,
+ SEC_ACE_TYPE_ACCESS_ALLOWED,
+ access_mask,
+ 0);
+ idx++;
+ }
+
+ access_mask = 0;
+ if (mode & S_IROTH) {
+ access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
+ }
+ if (mode & S_IWOTH) {
+ access_mask |= SEC_RIGHTS_FILE_WRITE;
+ }
+ if (access_mask) {
+ init_sec_ace(&aces[idx],
+ &global_sid_World,
+ SEC_ACE_TYPE_ACCESS_ALLOWED,
+ access_mask,
+ 0);
+ idx++;
+ }
+
+ init_sec_ace(&aces[idx],
+ &global_sid_System,
+ SEC_ACE_TYPE_ACCESS_ALLOWED,
+ SEC_RIGHTS_FILE_ALL,
+ 0);
+ idx++;
+
+ new_dacl = make_sec_acl(ctx,
+ NT4_ACL_REVISION,
+ idx,
+ aces);
+
+ if (!new_dacl) {
+ return NT_STATUS_NO_MEMORY;
+ }
+
+ *ppdesc = make_sec_desc(ctx,
+ SECURITY_DESCRIPTOR_REVISION_1,
+ SEC_DESC_SELF_RELATIVE|SEC_DESC_DACL_PRESENT,
+ &owner_sid,
+ &group_sid,
+ NULL,
+ new_dacl,
+ &size);
+ if (!*ppdesc) {
+ return NT_STATUS_NO_MEMORY;
+ }
+ return NT_STATUS_OK;
+}
+
/*******************************************************************
Pull a DATA_BLOB from an xattr given a pathname.
If the hash doesn't match, or doesn't exist - return the underlying
diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index 2bb90c4..c575568 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -4688,116 +4688,6 @@ NTSTATUS get_nt_acl_no_snum(TALLOC_CTX *ctx, const char *fname,
return status;
}
-/* Stolen shamelessly from pvfs_default_acl() in source4 :-). */
-
-NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
- const char *name,
- SMB_STRUCT_STAT *psbuf,
- struct security_descriptor **ppdesc)
-{
- struct dom_sid owner_sid, group_sid;
- size_t size = 0;
- struct security_ace aces[4];
- uint32_t access_mask = 0;
- mode_t mode = psbuf->st_ex_mode;
- struct security_acl *new_dacl = NULL;
- int idx = 0;
-
- DEBUG(10,("make_default_filesystem_acl: file %s mode = 0%o\n",
- name, (int)mode ));
-
- uid_to_sid(&owner_sid, psbuf->st_ex_uid);
- gid_to_sid(&group_sid, psbuf->st_ex_gid);
-
- /*
- We provide up to 4 ACEs
- - Owner
- - Group
- - Everyone
- - NT System
- */
-
- if (mode & S_IRUSR) {
- if (mode & S_IWUSR) {
- access_mask |= SEC_RIGHTS_FILE_ALL;
- } else {
- access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
- }
- }
- if (mode & S_IWUSR) {
- access_mask |= SEC_RIGHTS_FILE_WRITE | SEC_STD_DELETE;
- }
-
- init_sec_ace(&aces[idx],
- &owner_sid,
- SEC_ACE_TYPE_ACCESS_ALLOWED,
- access_mask,
- 0);
- idx++;
-
- access_mask = 0;
- if (mode & S_IRGRP) {
- access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
- }
- if (mode & S_IWGRP) {
- /* note that delete is not granted - this matches posix behaviour */
- access_mask |= SEC_RIGHTS_FILE_WRITE;
- }
- if (access_mask) {
- init_sec_ace(&aces[idx],
- &group_sid,
- SEC_ACE_TYPE_ACCESS_ALLOWED,
- access_mask,
- 0);
- idx++;
- }
-
- access_mask = 0;
- if (mode & S_IROTH) {
- access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
- }
- if (mode & S_IWOTH) {
- access_mask |= SEC_RIGHTS_FILE_WRITE;
- }
- if (access_mask) {
- init_sec_ace(&aces[idx],
- &global_sid_World,
- SEC_ACE_TYPE_ACCESS_ALLOWED,
- access_mask,
- 0);
- idx++;
- }
-
- init_sec_ace(&aces[idx],
- &global_sid_System,
- SEC_ACE_TYPE_ACCESS_ALLOWED,
- SEC_RIGHTS_FILE_ALL,
- 0);
- idx++;
-
- new_dacl = make_sec_acl(ctx,
- NT4_ACL_REVISION,
- idx,
- aces);
-
- if (!new_dacl) {
- return NT_STATUS_NO_MEMORY;
- }
-
- *ppdesc = make_sec_desc(ctx,
- SECURITY_DESCRIPTOR_REVISION_1,
- SEC_DESC_SELF_RELATIVE|SEC_DESC_DACL_PRESENT,
- &owner_sid,
- &group_sid,
- NULL,
- new_dacl,
- &size);
- if (!*ppdesc) {
- return NT_STATUS_NO_MEMORY;
- }
- return NT_STATUS_OK;
-}
-
int posix_sys_acl_blob_get_file(vfs_handle_struct *handle,
const char *path_p,
TALLOC_CTX *mem_ctx,
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 81bdc87..cd1d825 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -778,10 +778,6 @@ bool set_unix_posix_acl(connection_struct *conn, files_struct *fsp, const char *
NTSTATUS get_nt_acl_no_snum( TALLOC_CTX *ctx, const char *fname,
uint32_t security_info_wanted,
struct security_descriptor **sd);
-NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
- const char *name,
- SMB_STRUCT_STAT *psbuf,
- struct security_descriptor **ppdesc);
int posix_sys_acl_blob_get_file(vfs_handle_struct *handle,
const char *path_p,
TALLOC_CTX *mem_ctx,
--
2.5.0
From d29c60879eec241478735163fc1af12421e94623 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 15 Jul 2016 17:48:19 +0200
Subject: [PATCH 2/2] vfs_acl_xattr: objects without NT ACL xattr
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Even with "ignore system acls" set to "yes", for objects without NT ACL
xattr we use the underlying filesystem permissions to construct an NT
ACL. This can result in *very* unexpected permissions, eg:
- a directory with the following ACL:
$ ./bin/smbcacls -Uslow%pass //localhost/normal ""
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\root
ACL:SLOW\slow:ALLOWED/0x0/FULL
So only one non-inheritable(!) ACE.
- creating a subdirectory:
$ ./bin/smbclient -Uslow%pass //localhost/normal -c "mkdir dir1"
- checking whether there's an ACL xattr:
$ getfattr -m "" /Volumes/normal/dir1
getfattr: Removing leading '/' from absolute path names
system.posix_acl_access
system.posix_acl_default
user.DOSATTRIB
So there isn't an ACL xattr, because there where no inheritable ACEs on
the parent folder.
- reading the new subdirectories ACL:
$ ./bin/smbcacls -Uslow%pass //localhost/normal "dir1"
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\slow
ACL:SLOW\slow:ALLOWED/0x0/FULL
ACL:Unix Group\slow:ALLOWED/0x0/READ
ACL:Everyone:ALLOWED/0x0/READ
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
The ACES for "SLOW\slow", "Unix Group\slow" and "Everyone" are coming
from the underlying filesystem. This is the problem.
- Windows assigns the following ACL in this situation:
$ ./bin/smbcacls -UAdministrator%Passw0rd //10.10.10.14/data "dir"
REVISION:1
CONTROL:SR|PD|DI|DP
OWNER:VORDEFINIERT\Administratoren
GROUP:WIN2008R2\Domänen-Benutzer
ACL:WIN2008R2\Administrator:ALLOWED/0x0/FULL
$ ./bin/smbclient -UAdministrator%Passw0rd //10.10.10.14/data -c "mkdir dir\dir1"
$ ./bin/smbcacls -UAdministrator%Passw0rd //10.10.10.14/data "dir\dir1"
REVISION:1
CONTROL:SR|DI|DP
OWNER:VORDEFINIERT\Administratoren
GROUP:WIN2008R2\Domänen-Benutzer
ACL:VORDEFINIERT\Administratoren:ALLOWED/0x0/FULL
ACL:NT-AUTORITÄT\SYSTEM:ALLOWED/0x0/FULL
By changing make_default_filesystem_acl() to only adds user and system
ACE to the ACL of objects that lack an ACL xattr, we match Windows
behaviour:
$ ./bin/smbclient -Uslow%pass //localhost/normal -c "mkdir dir2"
$ ./bin/smbcacls -Uslow%pass //localhost/normal "dir2"
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\slow
ACL:SLOW\slow:ALLOWED/0x0/FULL
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/modules/vfs_acl_common.c | 44 +++++-----------------------------------
1 file changed, 5 insertions(+), 39 deletions(-)
diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index f3d6c0d..2fda938e 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -24,6 +24,7 @@
#include "../libcli/security/security.h"
#include "../librpc/gen_ndr/ndr_security.h"
#include "../lib/util/bitmap.h"
+#include "passdb/lookup_sid.h"
static NTSTATUS create_acl_blob(const struct security_descriptor *psd,
DATA_BLOB *pblob,
@@ -378,12 +379,10 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
gid_to_sid(&group_sid, psbuf->st_ex_gid);
/*
- We provide up to 4 ACEs
- - Owner
- - Group
- - Everyone
- - NT System
- */
+ * We provide 2 ACEs:
+ * - Owner
+ * - NT System
+ */
if (mode & S_IRUSR) {
if (mode & S_IWUSR) {
@@ -403,39 +402,6 @@ static NTSTATUS make_default_filesystem_acl(TALLOC_CTX *ctx,
0);
idx++;
- access_mask = 0;
- if (mode & S_IRGRP) {
- access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
- }
- if (mode & S_IWGRP) {
- /* note that delete is not granted - this matches posix behaviour */
- access_mask |= SEC_RIGHTS_FILE_WRITE;
- }
- if (access_mask) {
- init_sec_ace(&aces[idx],
- &group_sid,
- SEC_ACE_TYPE_ACCESS_ALLOWED,
- access_mask,
- 0);
- idx++;
- }
-
- access_mask = 0;
- if (mode & S_IROTH) {
- access_mask |= SEC_RIGHTS_FILE_READ | SEC_FILE_EXECUTE;
- }
- if (mode & S_IWOTH) {
- access_mask |= SEC_RIGHTS_FILE_WRITE;
- }
- if (access_mask) {
- init_sec_ace(&aces[idx],
- &global_sid_World,
- SEC_ACE_TYPE_ACCESS_ALLOWED,
- access_mask,
- 0);
- idx++;
- }
-
init_sec_ace(&aces[idx],
&global_sid_System,
SEC_ACE_TYPE_ACCESS_ALLOWED,
--
2.5.0
-------------- next part --------------
From d01162ed18cfb6c995a60ded8392e447be0b24d6 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 15 Jul 2016 16:24:05 +0200
Subject: [PATCH] HACK: vfs_acl_xattr: objects without NT ACL xattr
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Even with "ignore system acls" set to "yes", for objects without NT ACL
xattr we use the underlying filesystem permissions to construct an NT
ACL. This can result in *very* unexpected permissions, eg:
- a directory with the following ACL:
$ ./bin/smbcacls -Uslow%pass //localhost/normal ""
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\root
ACL:SLOW\slow:ALLOWED/0x0/FULL
So only one non-inheritable(!) ACE.
- creating a subdirectory:
$ ./bin/smbclient -Uslow%pass //localhost/normal -c "mkdir dir1"
- checking whether there's an ACL xattr:
$ getfattr -m "" /Volumes/normal/dir1
getfattr: Removing leading '/' from absolute path names
system.posix_acl_access
system.posix_acl_default
user.DOSATTRIB
So there isn't an ACL xattr, because there where no inheritable ACEs on
the parent folder.
- reading the new subdirectories ACL:
$ ./bin/smbcacls -Uslow%pass //localhost/normal "dir1"
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\slow
ACL:SLOW\slow:ALLOWED/0x0/FULL
ACL:Unix Group\slow:ALLOWED/0x0/READ
ACL:Everyone:ALLOWED/0x0/READ
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
The ACES for "SLOW\slow", "Unix Group\slow" and "Everyone" are coming
from the underlying filesystem. This is the problem.
- Windows assigns the following ACL in this situation:
$ ./bin/smbcacls -UAdministrator%Passw0rd //10.10.10.14/data "dir"
REVISION:1
CONTROL:SR|PD|DI|DP
OWNER:VORDEFINIERT\Administratoren
GROUP:WIN2008R2\Domänen-Benutzer
ACL:WIN2008R2\Administrator:ALLOWED/0x0/FULL
$ ./bin/smbclient -UAdministrator%Passw0rd //10.10.10.14/data -c "mkdir dir\dir1"
$ ./bin/smbcacls -UAdministrator%Passw0rd //10.10.10.14/data "dir\dir1"
REVISION:1
CONTROL:SR|DI|DP
OWNER:VORDEFINIERT\Administratoren
GROUP:WIN2008R2\Domänen-Benutzer
ACL:VORDEFINIERT\Administratoren:ALLOWED/0x0/FULL
ACL:NT-AUTORITÄT\SYSTEM:ALLOWED/0x0/FULL
Samba can be configured to get the same behaviour, but this is rather
unobvious, as it requires changing POSIX permissions related settings:
directory mask = 0700
create mask = 0600
With these setting we get:
$ ./bin/smbclient -Uslow%pass //localhost/normal -c "mkdir dir2"
$ ./bin/smbcacls -Uslow%pass //localhost/normal "dir2"
REVISION:1
CONTROL:SR|DP
OWNER:SLOW\slow
GROUP:Unix Group\slow
ACL:SLOW\slow:ALLOWED/0x0/FULL
ACL:NT Authority\SYSTEM:ALLOWED/0x0/FULL
This means we still munge POSIX perms and NT ACL xattr in code, but at
least in an improved way, matching Windows behaviour.
By setting both values at runtime, with a dependency on "ignore system
acls = yes", we get more correct behaviour out of the box.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/modules/vfs_acl_xattr.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c
index d311c57..2a6c99f 100644
--- a/source3/modules/vfs_acl_xattr.c
+++ b/source3/modules/vfs_acl_xattr.c
@@ -180,6 +180,10 @@ static int connect_acl_xattr(struct vfs_handle_struct *handle,
const char *user)
{
int ret = SMB_VFS_NEXT_CONNECT(handle, service, user);
+ bool ignore_file_system_acl = lp_parm_bool(SNUM(handle->conn),
+ ACL_MODULE_NAME,
+ "ignore system acls",
+ false);
if (ret < 0) {
return ret;
@@ -196,6 +200,11 @@ static int connect_acl_xattr(struct vfs_handle_struct *handle,
lp_do_parameter(SNUM(handle->conn), "dos filemode", "true");
lp_do_parameter(SNUM(handle->conn), "force unknown acl user", "true");
+ if (ignore_file_system_acl) {
+ lp_do_parameter(SNUM(handle->conn), "create mask", "0600");
+ lp_do_parameter(SNUM(handle->conn), "directory mask", "0700");
+ }
+
return 0;
}
--
2.5.0
More information about the samba-technical
mailing list