vfs_fruit bug in ACL get/set - [PATCH] attached.
Jeremy Allison
jra at samba.org
Fri Mar 2 22:14:04 UTC 2018
On Fri, Mar 02, 2018 at 10:05:37PM +0100, Ralph Böhme wrote:
> Hi Jeremy,
>
> On Fri, Mar 02, 2018 at 09:14:59AM -0800, Jeremy Allison wrote:
> > I just created:
> >
> > https://bugzilla.samba.org/show_bug.cgi?id=13319
> >
> > ------------------------------------------------
> > In fruit_fget_nt_acl() the 3 extra ACE entries
> > corresponding to mode/uid/gid are always added
> > to the returned ACL as virtual ACE entries that
> > are not expected to be stored in the file ACL on disk.
> >
> > In fruit_fset_nt_acl() the client-sent ACL is applied
> > to the file, then the mode entry (if sent) is used to
> > do the CHMOD - but the client-sent ACL is applied
> > without removing any virtual ACE entries.
> >
> > If a naive client just round-trips get/set/get/set,
> > on every set the 'virtual' ACE entries will be stored
> > in the xattr.
> >
> > I think the correct action on fruit_fset_nt_acl()
> > is to check for - then *remove* any virtual ACE
> > entries sent by the client before passing down
> > to the underlying SET_NT_ACL call.
>
> hm, I guess the idea was that the NFS ACE *could* be of interest to lower layers
> as well, so I chose not to filter them out.
No, that's the wrong thing to do (IMHO). You're
always generating them yourself in the fruit layer on
get, so you can't then pass them down in set.
There's nothing the lower layer can do with these,
and if it really wants them it can generate them
itself.
> If we're starting to use NFS ACEs more broadly (by coincidence just today metze
> mentioned that he's thinkin about using them in s3/auth iirc), maybe we should
> move the functionality to a lower layer and out of fruit.
Let's but that is a patch for another day :-).
In the meantime, here's a patch that fixes fruit
and the bug:
https://bugzilla.samba.org/show_bug.cgi?id=13319
Please review and let me know if you're happy with it !
Cheers,
Jeremy.
-------------- next part --------------
From b81aad4d9eeb367fbcb199c6a2d1a76d12271967 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 2 Mar 2018 13:07:48 -0800
Subject: [PATCH 1/4] s3: vfs_fruit. Ensure we only return one set of the
'virtual' UNIX ACE entries.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13319
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/modules/vfs_fruit.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index ec76f718c37..1a0586cdc33 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -5687,6 +5687,7 @@ static NTSTATUS fruit_fget_nt_acl(vfs_handle_struct *handle,
struct security_ace ace;
struct dom_sid sid;
struct fruit_config_data *config;
+ bool remove_ok = false;
SMB_VFS_HANDLE_GET_DATA(handle, config,
struct fruit_config_data,
@@ -5711,6 +5712,15 @@ static NTSTATUS fruit_fget_nt_acl(vfs_handle_struct *handle,
/* MS NFS style mode */
sid_compose(&sid, &global_sid_Unix_NFS_Mode, fsp->fsp_name->st.st_ex_mode);
init_sec_ace(&ace, &sid, SEC_ACE_TYPE_ACCESS_DENIED, 0, 0);
+ /* First remove any existing ACE's with this SID. */
+ status = security_descriptor_dacl_del(*ppdesc, &sid);
+ remove_ok = (NT_STATUS_IS_OK(status) ||
+ NT_STATUS_EQUAL(status,
+ NT_STATUS_OBJECT_NAME_NOT_FOUND));
+ if (!remove_ok) {
+ DBG_WARNING("failed to remove MS NFS_mode style ACE\n");
+ return status;
+ }
status = security_descriptor_dacl_add(*ppdesc, &ace);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(1,("failed to add MS NFS style ACE\n"));
@@ -5720,6 +5730,15 @@ static NTSTATUS fruit_fget_nt_acl(vfs_handle_struct *handle,
/* MS NFS style uid */
sid_compose(&sid, &global_sid_Unix_NFS_Users, fsp->fsp_name->st.st_ex_uid);
init_sec_ace(&ace, &sid, SEC_ACE_TYPE_ACCESS_DENIED, 0, 0);
+ /* First remove any existing ACE's with this SID. */
+ status = security_descriptor_dacl_del(*ppdesc, &sid);
+ remove_ok = (NT_STATUS_IS_OK(status) ||
+ NT_STATUS_EQUAL(status,
+ NT_STATUS_OBJECT_NAME_NOT_FOUND));
+ if (!remove_ok) {
+ DBG_WARNING("failed to remove MS NFS_users style ACE\n");
+ return status;
+ }
status = security_descriptor_dacl_add(*ppdesc, &ace);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(1,("failed to add MS NFS style ACE\n"));
@@ -5729,6 +5748,15 @@ static NTSTATUS fruit_fget_nt_acl(vfs_handle_struct *handle,
/* MS NFS style gid */
sid_compose(&sid, &global_sid_Unix_NFS_Groups, fsp->fsp_name->st.st_ex_gid);
init_sec_ace(&ace, &sid, SEC_ACE_TYPE_ACCESS_DENIED, 0, 0);
+ /* First remove any existing ACE's with this SID. */
+ status = security_descriptor_dacl_del(*ppdesc, &sid);
+ remove_ok = (NT_STATUS_IS_OK(status) ||
+ NT_STATUS_EQUAL(status,
+ NT_STATUS_OBJECT_NAME_NOT_FOUND));
+ if (!remove_ok) {
+ DBG_WARNING("failed to remove MS NFS_groups style ACE\n");
+ return status;
+ }
status = security_descriptor_dacl_add(*ppdesc, &ace);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(1,("failed to add MS NFS style ACE\n"));
--
2.16.2.395.g2e18187dfd-goog
From 679bcb4d8bb6636a523cbc5359d50b3dccf681e8 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 2 Mar 2018 13:21:37 -0800
Subject: [PATCH 2/4] s3: vfs_fruit: Ensure we operate on a copy of the
incoming security descriptor.
This will allow us to modify it in the next commit.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13319
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/modules/vfs_fruit.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 1a0586cdc33..2f9798cad78 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -5769,24 +5769,32 @@ static NTSTATUS fruit_fget_nt_acl(vfs_handle_struct *handle,
static NTSTATUS fruit_fset_nt_acl(vfs_handle_struct *handle,
files_struct *fsp,
uint32_t security_info_sent,
- const struct security_descriptor *psd)
+ const struct security_descriptor *orig_psd)
{
NTSTATUS status;
bool do_chmod;
mode_t ms_nfs_mode = 0;
int result;
+ struct security_descriptor *psd = security_descriptor_copy(talloc_tos(),
+ orig_psd);
+
+ if (psd == NULL) {
+ return NT_STATUS_NO_MEMORY;
+ }
DBG_DEBUG("fruit_fset_nt_acl: %s\n", fsp_str_dbg(fsp));
status = check_ms_nfs(handle, fsp, psd, &ms_nfs_mode, &do_chmod);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(1, ("fruit_fset_nt_acl: check_ms_nfs failed%s\n", fsp_str_dbg(fsp)));
+ TALLOC_FREE(psd);
return status;
}
status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(1, ("fruit_fset_nt_acl: SMB_VFS_NEXT_FSET_NT_ACL failed%s\n", fsp_str_dbg(fsp)));
+ TALLOC_FREE(psd);
return status;
}
@@ -5804,10 +5812,12 @@ static NTSTATUS fruit_fset_nt_acl(vfs_handle_struct *handle,
result, (unsigned)ms_nfs_mode,
strerror(errno)));
status = map_nt_error_from_unix(errno);
+ TALLOC_FREE(psd);
return status;
}
}
+ TALLOC_FREE(psd);
return NT_STATUS_OK;
}
--
2.16.2.395.g2e18187dfd-goog
From e65af60f5c5d6ee5231c88eb6f41e1f25a1f9208 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 2 Mar 2018 13:51:54 -0800
Subject: [PATCH 3/4] s3: vfs_fruit. If the security descriptor was modified,
ensure we set the flags correctly to reflect the ACE's left.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13319
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/modules/vfs_fruit.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index 2f9798cad78..c56f80b2745 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -5775,6 +5775,7 @@ static NTSTATUS fruit_fset_nt_acl(vfs_handle_struct *handle,
bool do_chmod;
mode_t ms_nfs_mode = 0;
int result;
+ uint32_t orig_num_aces = orig_psd->dacl ? orig_psd->dacl->num_aces : 0;
struct security_descriptor *psd = security_descriptor_copy(talloc_tos(),
orig_psd);
@@ -5791,6 +5792,22 @@ static NTSTATUS fruit_fset_nt_acl(vfs_handle_struct *handle,
return status;
}
+ /*
+ * If only ms_nfs ACE entries were sent, ensure we set the DACL
+ * sent/present flags correctly now we've removed them.
+ */
+
+ if (orig_num_aces != 0) {
+ /*
+ * Are there any ACE's left ?
+ */
+ if (psd->dacl->num_aces == 0) {
+ /* No - clear the DACL sent/present flags. */
+ security_info_sent &= ~SECINFO_DACL;
+ psd->type &= ~SEC_DESC_DACL_PRESENT;
+ }
+ }
+
status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(1, ("fruit_fset_nt_acl: SMB_VFS_NEXT_FSET_NT_ACL failed%s\n", fsp_str_dbg(fsp)));
--
2.16.2.395.g2e18187dfd-goog
From 0fa074dd4d31f8845242ab972d7ebf88d6a0a347 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 2 Mar 2018 13:53:55 -0800
Subject: [PATCH 4/4] s3: vfs_fruit. Change check_ms_nfs() to remove the
virtual ACE's generated by fruit_fget_nt_acl().
Ensures they don't get stored in the underlying ACL.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13319
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/modules/vfs_fruit.c | 45 ++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index c56f80b2745..f276040d79f 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -2957,12 +2957,15 @@ static NTSTATUS readdir_attr_macmeta(struct vfs_handle_struct *handle,
/* Search MS NFS style ACE with UNIX mode */
static NTSTATUS check_ms_nfs(vfs_handle_struct *handle,
files_struct *fsp,
- const struct security_descriptor *psd,
+ struct security_descriptor *psd,
mode_t *pmode,
bool *pdo_chmod)
{
uint32_t i;
struct fruit_config_data *config = NULL;
+ struct dom_sid sid;
+ NTSTATUS status = NT_STATUS_OK;
+ bool remove_ok = false;
*pdo_chmod = false;
@@ -2991,6 +2994,46 @@ static NTSTATUS check_ms_nfs(vfs_handle_struct *handle,
}
}
+ /*
+ * Remove any incoming virtual ACE entries generated by
+ * fruit_fget_nt_acl().
+ */
+ /* MS NFS style mode */
+ sid_compose(&sid, &global_sid_Unix_NFS_Mode,
+ fsp->fsp_name->st.st_ex_mode);
+ status = security_descriptor_dacl_del(psd, &sid);
+ remove_ok = (NT_STATUS_IS_OK(status) ||
+ NT_STATUS_EQUAL(status,
+ NT_STATUS_OBJECT_NAME_NOT_FOUND));
+ if (!remove_ok) {
+ DBG_WARNING("failed to remove MS NFS_mode style ACE\n");
+ return status;
+ }
+
+ /* MS NFS style uid */
+ sid_compose(&sid, &global_sid_Unix_NFS_Users,
+ fsp->fsp_name->st.st_ex_uid);
+ status = security_descriptor_dacl_del(psd, &sid);
+ remove_ok = (NT_STATUS_IS_OK(status) ||
+ NT_STATUS_EQUAL(status,
+ NT_STATUS_OBJECT_NAME_NOT_FOUND));
+ if (!remove_ok) {
+ DBG_WARNING("failed to remove MS NFS_users style ACE\n");
+ return status;
+ }
+
+ /* MS NFS style gid */
+ sid_compose(&sid, &global_sid_Unix_NFS_Groups,
+ fsp->fsp_name->st.st_ex_gid);
+ status = security_descriptor_dacl_del(psd, &sid);
+ remove_ok = (NT_STATUS_IS_OK(status) ||
+ NT_STATUS_EQUAL(status,
+ NT_STATUS_OBJECT_NAME_NOT_FOUND));
+ if (!remove_ok) {
+ DBG_WARNING("failed to remove MS NFS_groups style ACE\n");
+ return status;
+ }
+
return NT_STATUS_OK;
}
--
2.16.2.395.g2e18187dfd-goog
More information about the samba-technical
mailing list