* [PATCH 1/4] cifs: Fix getting and setting SACLs over SMB1
2024-12-22 15:10 [PATCH 0/4] cifs: Fix gettting and setting parts of security descriptor Pali Rohár
@ 2024-12-22 15:10 ` Pali Rohár
2024-12-27 14:43 ` Pali Rohár
2024-12-22 15:10 ` [PATCH 2/4] cifs: Change ->get_acl() API callback to request only for asked info Pali Rohár
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Pali Rohár @ 2024-12-22 15:10 UTC (permalink / raw)
To: Steve French, Paulo Alcantara, Ronnie Sahlberg; +Cc: linux-cifs, linux-kernel
SMB1 callback get_cifs_acl_by_fid() currently ignores its last argument and
therefore ignores request for SACL_SECINFO. Fix this issue by correctly
propagating info argument from get_cifs_acl() and get_cifs_acl_by_fid() to
CIFSSMBGetCIFSACL() function and pass SACL_SECINFO when requested.
For accessing SACLs it is needed to open object with SYSTEM_SECURITY
access. Pass this flag when trying to get or set SACLs.
Same logic is in the SMB2+ code path.
Fixes: 3970acf7ddb9 ("SMB3: Add support for getting and setting SACLs")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
fs/smb/client/cifsacl.c | 23 ++++++++++++++---------
fs/smb/client/cifsproto.h | 2 +-
fs/smb/client/cifssmb.c | 4 ++--
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index ba79aa2107cc..b07f3609adec 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -1395,7 +1395,7 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
#ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
struct smb_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb,
const struct cifs_fid *cifsfid, u32 *pacllen,
- u32 __maybe_unused unused)
+ u32 info)
{
struct smb_ntsd *pntsd = NULL;
unsigned int xid;
@@ -1407,7 +1407,7 @@ struct smb_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb,
xid = get_xid();
rc = CIFSSMBGetCIFSACL(xid, tlink_tcon(tlink), cifsfid->netfid, &pntsd,
- pacllen);
+ pacllen, info);
free_xid(xid);
cifs_put_tlink(tlink);
@@ -1419,7 +1419,7 @@ struct smb_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb,
}
static struct smb_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb,
- const char *path, u32 *pacllen)
+ const char *path, u32 *pacllen, u32 info)
{
struct smb_ntsd *pntsd = NULL;
int oplock = 0;
@@ -1446,9 +1446,12 @@ static struct smb_ntsd *get_cifs_acl_by_path(struct cifs_sb_info *cifs_sb,
.fid = &fid,
};
+ if (info & SACL_SECINFO)
+ oparms.desired_access |= SYSTEM_SECURITY;
+
rc = CIFS_open(xid, &oparms, &oplock, NULL);
if (!rc) {
- rc = CIFSSMBGetCIFSACL(xid, tcon, fid.netfid, &pntsd, pacllen);
+ rc = CIFSSMBGetCIFSACL(xid, tcon, fid.netfid, &pntsd, pacllen, info);
CIFSSMBClose(xid, tcon, fid.netfid);
}
@@ -1472,7 +1475,7 @@ struct smb_ntsd *get_cifs_acl(struct cifs_sb_info *cifs_sb,
if (inode)
open_file = find_readable_file(CIFS_I(inode), true);
if (!open_file)
- return get_cifs_acl_by_path(cifs_sb, path, pacllen);
+ return get_cifs_acl_by_path(cifs_sb, path, pacllen, info);
pntsd = get_cifs_acl_by_fid(cifs_sb, &open_file->fid, pacllen, info);
cifsFileInfo_put(open_file);
@@ -1498,10 +1501,12 @@ int set_cifs_acl(struct smb_ntsd *pnntsd, __u32 acllen,
tcon = tlink_tcon(tlink);
xid = get_xid();
- if (aclflag == CIFS_ACL_OWNER || aclflag == CIFS_ACL_GROUP)
- access_flags = WRITE_OWNER;
- else
- access_flags = WRITE_DAC;
+ if (aclflag & CIFS_ACL_OWNER || aclflag & CIFS_ACL_GROUP)
+ access_flags |= WRITE_OWNER;
+ if (aclflag & CIFS_ACL_SACL)
+ access_flags |= SYSTEM_SECURITY;
+ if (aclflag & CIFS_ACL_DACL)
+ access_flags |= WRITE_DAC;
oparms = (struct cifs_open_parms) {
.tcon = tcon,
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 467378e23bd7..5aa2769a42f3 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -560,7 +560,7 @@ extern int CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon,
const struct nls_table *nls_codepage,
struct cifs_sb_info *cifs_sb);
extern int CIFSSMBGetCIFSACL(const unsigned int xid, struct cifs_tcon *tcon,
- __u16 fid, struct smb_ntsd **acl_inf, __u32 *buflen);
+ __u16 fid, struct smb_ntsd **acl_inf, __u32 *buflen, __u32 info);
extern int CIFSSMBSetCIFSACL(const unsigned int, struct cifs_tcon *, __u16,
struct smb_ntsd *pntsd, __u32 len, int aclflag);
extern int cifs_do_get_acl(const unsigned int xid, struct cifs_tcon *tcon,
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 4ca6022d2cd5..2791c1cbadfa 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3364,7 +3364,7 @@ validate_ntransact(char *buf, char **ppparm, char **ppdata,
/* Get Security Descriptor (by handle) from remote server for a file or dir */
int
CIFSSMBGetCIFSACL(const unsigned int xid, struct cifs_tcon *tcon, __u16 fid,
- struct smb_ntsd **acl_inf, __u32 *pbuflen)
+ struct smb_ntsd **acl_inf, __u32 *pbuflen, __u32 info)
{
int rc = 0;
int buf_type = 0;
@@ -3387,7 +3387,7 @@ CIFSSMBGetCIFSACL(const unsigned int xid, struct cifs_tcon *tcon, __u16 fid,
pSMB->MaxSetupCount = 0;
pSMB->Fid = fid; /* file handle always le */
pSMB->AclFlags = cpu_to_le32(CIFS_ACL_OWNER | CIFS_ACL_GROUP |
- CIFS_ACL_DACL);
+ CIFS_ACL_DACL | info);
pSMB->ByteCount = cpu_to_le16(11); /* 3 bytes pad + 8 bytes parm */
inc_rfc1001_len(pSMB, 11);
iov[0].iov_base = (char *)pSMB;
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/4] cifs: Fix getting and setting SACLs over SMB1
2024-12-22 15:10 ` [PATCH 1/4] cifs: Fix getting and setting SACLs over SMB1 Pali Rohár
@ 2024-12-27 14:43 ` Pali Rohár
0 siblings, 0 replies; 7+ messages in thread
From: Pali Rohár @ 2024-12-27 14:43 UTC (permalink / raw)
To: Steve French, Paulo Alcantara, Ronnie Sahlberg; +Cc: linux-cifs, linux-kernel
On Sunday 22 December 2024 16:10:48 Pali Rohár wrote:
> diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
> index ba79aa2107cc..b07f3609adec 100644
> --- a/fs/smb/client/cifsacl.c
> +++ b/fs/smb/client/cifsacl.c
> @@ -1498,10 +1501,12 @@ int set_cifs_acl(struct smb_ntsd *pnntsd, __u32 acllen,
> tcon = tlink_tcon(tlink);
> xid = get_xid();
>
> - if (aclflag == CIFS_ACL_OWNER || aclflag == CIFS_ACL_GROUP)
> - access_flags = WRITE_OWNER;
> - else
> - access_flags = WRITE_DAC;
> + if (aclflag & CIFS_ACL_OWNER || aclflag & CIFS_ACL_GROUP)
> + access_flags |= WRITE_OWNER;
> + if (aclflag & CIFS_ACL_SACL)
> + access_flags |= SYSTEM_SECURITY;
> + if (aclflag & CIFS_ACL_DACL)
> + access_flags |= WRITE_DAC;
>
> oparms = (struct cifs_open_parms) {
> .tcon = tcon,
In this function is missing initialization of access_flags value after my change.
I can fix it by this simple fixup change:
diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 1054c62ade6c..b3e2f1dad175 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -1488,7 +1488,7 @@ int set_cifs_acl(struct smb_ntsd *pnntsd, __u32 acllen,
{
int oplock = 0;
unsigned int xid;
- int rc, access_flags;
+ int rc, access_flags = 0;
struct cifs_tcon *tcon;
struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/4] cifs: Change ->get_acl() API callback to request only for asked info
2024-12-22 15:10 [PATCH 0/4] cifs: Fix gettting and setting parts of security descriptor Pali Rohár
2024-12-22 15:10 ` [PATCH 1/4] cifs: Fix getting and setting SACLs over SMB1 Pali Rohár
@ 2024-12-22 15:10 ` Pali Rohár
2024-12-22 15:10 ` [PATCH 3/4] cifs: Add a new xattr system.smb3_ntsd_sacl for getting or setting SACLs Pali Rohár
2024-12-22 15:10 ` [PATCH 4/4] cifs: Add a new xattr system.smb3_ntsd_owner for getting or setting owner Pali Rohár
3 siblings, 0 replies; 7+ messages in thread
From: Pali Rohár @ 2024-12-22 15:10 UTC (permalink / raw)
To: Steve French, Paulo Alcantara, Ronnie Sahlberg; +Cc: linux-cifs, linux-kernel
Currently ->get_acl() always request for ONWER, GROUP and DACL, even when
only DACLs was requested. Change API callback to request only information
for which the caller asked. Therefore when only DACLs requested, then SMB
client send DACL-only request.
Signed-off-by: Pali Rohár <pali@kernel.org>
---
fs/smb/client/cifsacl.c | 4 ++--
fs/smb/client/cifssmb.c | 3 +--
fs/smb/client/smb2pdu.c | 4 +---
fs/smb/client/xattr.c | 15 +++++++++++----
4 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index b07f3609adec..1054c62ade6c 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -1546,7 +1546,7 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr,
int rc = 0;
struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
struct smb_version_operations *ops;
- const u32 info = 0;
+ const u32 info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO;
cifs_dbg(NOISY, "converting ACL to mode for %s\n", path);
@@ -1600,7 +1600,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
struct tcon_link *tlink;
struct smb_version_operations *ops;
bool mode_from_sid, id_from_sid;
- const u32 info = 0;
+ const u32 info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO;
bool posix;
tlink = cifs_sb_tlink(cifs_sb);
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 2791c1cbadfa..2763db49b155 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3386,8 +3386,7 @@ CIFSSMBGetCIFSACL(const unsigned int xid, struct cifs_tcon *tcon, __u16 fid,
/* BB TEST with big acls that might need to be e.g. larger than 16K */
pSMB->MaxSetupCount = 0;
pSMB->Fid = fid; /* file handle always le */
- pSMB->AclFlags = cpu_to_le32(CIFS_ACL_OWNER | CIFS_ACL_GROUP |
- CIFS_ACL_DACL | info);
+ pSMB->AclFlags = cpu_to_le32(info);
pSMB->ByteCount = cpu_to_le16(11); /* 3 bytes pad + 8 bytes parm */
inc_rfc1001_len(pSMB, 11);
iov[0].iov_base = (char *)pSMB;
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 010eae9d6c47..ccded987f82b 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -3928,12 +3928,10 @@ SMB2_query_acl(const unsigned int xid, struct cifs_tcon *tcon,
u64 persistent_fid, u64 volatile_fid,
void **data, u32 *plen, u32 extra_info)
{
- __u32 additional_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO |
- extra_info;
*plen = 0;
return query_info(xid, tcon, persistent_fid, volatile_fid,
- 0, SMB2_O_INFO_SECURITY, additional_info,
+ 0, SMB2_O_INFO_SECURITY, extra_info,
SMB2_MAX_BUFFER_SIZE, MIN_SEC_DESC_LEN, data, plen);
}
diff --git a/fs/smb/client/xattr.c b/fs/smb/client/xattr.c
index 58a584f0b27e..7d49f38f01f3 100644
--- a/fs/smb/client/xattr.c
+++ b/fs/smb/client/xattr.c
@@ -320,10 +320,17 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
if (pTcon->ses->server->ops->get_acl == NULL)
goto out; /* rc already EOPNOTSUPP */
- if (handler->flags == XATTR_CIFS_NTSD_FULL) {
- extra_info = SACL_SECINFO;
- } else {
- extra_info = 0;
+ switch (handler->flags) {
+ case XATTR_CIFS_NTSD_FULL:
+ extra_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO | SACL_SECINFO;
+ break;
+ case XATTR_CIFS_NTSD:
+ extra_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO;
+ break;
+ case XATTR_CIFS_ACL:
+ default:
+ extra_info = DACL_SECINFO;
+ break;
}
pacl = pTcon->ses->server->ops->get_acl(cifs_sb,
inode, full_path, &acllen, extra_info);
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] cifs: Add a new xattr system.smb3_ntsd_sacl for getting or setting SACLs
2024-12-22 15:10 [PATCH 0/4] cifs: Fix gettting and setting parts of security descriptor Pali Rohár
2024-12-22 15:10 ` [PATCH 1/4] cifs: Fix getting and setting SACLs over SMB1 Pali Rohár
2024-12-22 15:10 ` [PATCH 2/4] cifs: Change ->get_acl() API callback to request only for asked info Pali Rohár
@ 2024-12-22 15:10 ` Pali Rohár
2024-12-22 15:10 ` [PATCH 4/4] cifs: Add a new xattr system.smb3_ntsd_owner for getting or setting owner Pali Rohár
3 siblings, 0 replies; 7+ messages in thread
From: Pali Rohár @ 2024-12-22 15:10 UTC (permalink / raw)
To: Steve French, Paulo Alcantara, Ronnie Sahlberg; +Cc: linux-cifs, linux-kernel
Access to SACL part of SMB security descriptor is granted by SACL privilege
which by default is accessible only for local administrator. But it can be
granted to any other user by local GPO or AD. SACL access is not granted by
DACL permissions and therefore is it possible that some user would not have
access to DACLs of some file, but would have access to SACLs of all files.
So it means that for accessing SACLs (either getting or setting) in some
cases requires not touching or asking for DACLs.
Currently Linux SMB client does not allow to get or set SACLs without
touching DACLs. Which means that user without DACL access is not able to
get or set SACLs even if it has access to SACLs.
Fix this problem by introducing a new xattr for accessing only SACLs
(without DACLs and OWNER/GROUP).
Signed-off-by: Pali Rohár <pali@kernel.org>
---
fs/smb/client/xattr.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/fs/smb/client/xattr.c b/fs/smb/client/xattr.c
index 7d49f38f01f3..95b8269851f3 100644
--- a/fs/smb/client/xattr.c
+++ b/fs/smb/client/xattr.c
@@ -31,6 +31,7 @@
* secure, replaced by SMB2 (then even more highly secure SMB3) many years ago
*/
#define SMB3_XATTR_CIFS_ACL "system.smb3_acl" /* DACL only */
+#define SMB3_XATTR_CIFS_NTSD_SACL "system.smb3_ntsd_sacl" /* SACL only */
#define SMB3_XATTR_CIFS_NTSD "system.smb3_ntsd" /* owner plus DACL */
#define SMB3_XATTR_CIFS_NTSD_FULL "system.smb3_ntsd_full" /* owner/DACL/SACL */
#define SMB3_XATTR_ATTRIB "smb3.dosattrib" /* full name: user.smb3.dosattrib */
@@ -38,6 +39,7 @@
/* BB need to add server (Samba e.g) support for security and trusted prefix */
enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT,
+ XATTR_CIFS_NTSD_SACL,
XATTR_CIFS_NTSD, XATTR_CIFS_NTSD_FULL };
static int cifs_attrib_set(unsigned int xid, struct cifs_tcon *pTcon,
@@ -160,6 +162,7 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
break;
case XATTR_CIFS_ACL:
+ case XATTR_CIFS_NTSD_SACL:
case XATTR_CIFS_NTSD:
case XATTR_CIFS_NTSD_FULL: {
struct smb_ntsd *pacl;
@@ -187,6 +190,9 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
CIFS_ACL_GROUP |
CIFS_ACL_DACL);
break;
+ case XATTR_CIFS_NTSD_SACL:
+ aclflags = CIFS_ACL_SACL;
+ break;
case XATTR_CIFS_ACL:
default:
aclflags = CIFS_ACL_DACL;
@@ -308,6 +314,7 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
break;
case XATTR_CIFS_ACL:
+ case XATTR_CIFS_NTSD_SACL:
case XATTR_CIFS_NTSD:
case XATTR_CIFS_NTSD_FULL: {
/*
@@ -327,6 +334,9 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
case XATTR_CIFS_NTSD:
extra_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO;
break;
+ case XATTR_CIFS_NTSD_SACL:
+ extra_info = SACL_SECINFO;
+ break;
case XATTR_CIFS_ACL:
default:
extra_info = DACL_SECINFO;
@@ -448,6 +458,13 @@ static const struct xattr_handler smb3_acl_xattr_handler = {
.set = cifs_xattr_set,
};
+static const struct xattr_handler smb3_ntsd_sacl_xattr_handler = {
+ .name = SMB3_XATTR_CIFS_NTSD_SACL,
+ .flags = XATTR_CIFS_NTSD_SACL,
+ .get = cifs_xattr_get,
+ .set = cifs_xattr_set,
+};
+
static const struct xattr_handler cifs_cifs_ntsd_xattr_handler = {
.name = CIFS_XATTR_CIFS_NTSD,
.flags = XATTR_CIFS_NTSD,
@@ -493,6 +510,7 @@ const struct xattr_handler * const cifs_xattr_handlers[] = {
&cifs_os2_xattr_handler,
&cifs_cifs_acl_xattr_handler,
&smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
+ &smb3_ntsd_sacl_xattr_handler,
&cifs_cifs_ntsd_xattr_handler,
&smb3_ntsd_xattr_handler, /* alias for above since avoiding "cifs" */
&cifs_cifs_ntsd_full_xattr_handler,
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/4] cifs: Add a new xattr system.smb3_ntsd_owner for getting or setting owner
2024-12-22 15:10 [PATCH 0/4] cifs: Fix gettting and setting parts of security descriptor Pali Rohár
` (2 preceding siblings ...)
2024-12-22 15:10 ` [PATCH 3/4] cifs: Add a new xattr system.smb3_ntsd_sacl for getting or setting SACLs Pali Rohár
@ 2024-12-22 15:10 ` Pali Rohár
2024-12-23 10:33 ` Pali Rohár
3 siblings, 1 reply; 7+ messages in thread
From: Pali Rohár @ 2024-12-22 15:10 UTC (permalink / raw)
To: Steve French, Paulo Alcantara, Ronnie Sahlberg; +Cc: linux-cifs, linux-kernel
Changing owner is controlled by DACL permission WRITE_OWNER. Changing DACL
itself is controlled by DACL permisssion WRITE_DAC. Owner of the file has
implicit WRITE_DAC permission even when it is not explicitly granted for
owner by DACL.
Reading DACL or owner is controlled only by one permission READ_CONTROL.
WRITE_OWNER permission can be bypassed by the SeTakeOwnershipPrivilege,
which is by default available for local administrators.
So if the local administrator wants to access some file to which does not
have access, it is required to first change owner to ourself and then
change DACL permissions.
Currently Linux SMB client does not support to do this, because it does not
provide a way to change owner without touching DACL permissions.
Fix this problem by introducing a new xattr for setting only owner part of
security descriptor.
Signed-off-by: Pali Rohár <pali@kernel.org>
---
fs/smb/client/xattr.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/fs/smb/client/xattr.c b/fs/smb/client/xattr.c
index 95b8269851f3..b88fa04f5792 100644
--- a/fs/smb/client/xattr.c
+++ b/fs/smb/client/xattr.c
@@ -32,6 +32,7 @@
*/
#define SMB3_XATTR_CIFS_ACL "system.smb3_acl" /* DACL only */
#define SMB3_XATTR_CIFS_NTSD_SACL "system.smb3_ntsd_sacl" /* SACL only */
+#define SMB3_XATTR_CIFS_NTSD_OWNER "system.smb3_ntsd_owner" /* owner only */
#define SMB3_XATTR_CIFS_NTSD "system.smb3_ntsd" /* owner plus DACL */
#define SMB3_XATTR_CIFS_NTSD_FULL "system.smb3_ntsd_full" /* owner/DACL/SACL */
#define SMB3_XATTR_ATTRIB "smb3.dosattrib" /* full name: user.smb3.dosattrib */
@@ -39,7 +40,7 @@
/* BB need to add server (Samba e.g) support for security and trusted prefix */
enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT,
- XATTR_CIFS_NTSD_SACL,
+ XATTR_CIFS_NTSD_SACL, XATTR_CIFS_NTSD_OWNER,
XATTR_CIFS_NTSD, XATTR_CIFS_NTSD_FULL };
static int cifs_attrib_set(unsigned int xid, struct cifs_tcon *pTcon,
@@ -163,6 +164,7 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
case XATTR_CIFS_ACL:
case XATTR_CIFS_NTSD_SACL:
+ case XATTR_CIFS_NTSD_OWNER:
case XATTR_CIFS_NTSD:
case XATTR_CIFS_NTSD_FULL: {
struct smb_ntsd *pacl;
@@ -190,6 +192,10 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
CIFS_ACL_GROUP |
CIFS_ACL_DACL);
break;
+ case XATTR_CIFS_NTSD_OWNER:
+ aclflags = (CIFS_ACL_OWNER |
+ CIFS_ACL_GROUP);
+ break;
case XATTR_CIFS_NTSD_SACL:
aclflags = CIFS_ACL_SACL;
break;
@@ -315,6 +321,7 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
case XATTR_CIFS_ACL:
case XATTR_CIFS_NTSD_SACL:
+ case XATTR_CIFS_NTSD_OWNER:
case XATTR_CIFS_NTSD:
case XATTR_CIFS_NTSD_FULL: {
/*
@@ -334,6 +341,9 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
case XATTR_CIFS_NTSD:
extra_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO;
break;
+ case XATTR_CIFS_NTSD_OWNER:
+ extra_info = OWNER_SECINFO | GROUP_SECINFO;
+ break;
case XATTR_CIFS_NTSD_SACL:
extra_info = SACL_SECINFO;
break;
@@ -465,6 +475,13 @@ static const struct xattr_handler smb3_ntsd_sacl_xattr_handler = {
.set = cifs_xattr_set,
};
+static const struct xattr_handler smb3_ntsd_owner_xattr_handler = {
+ .name = SMB3_XATTR_CIFS_NTSD_OWNER,
+ .flags = XATTR_CIFS_NTSD_OWNER,
+ .get = cifs_xattr_get,
+ .set = cifs_xattr_set,
+};
+
static const struct xattr_handler cifs_cifs_ntsd_xattr_handler = {
.name = CIFS_XATTR_CIFS_NTSD,
.flags = XATTR_CIFS_NTSD,
@@ -511,6 +528,7 @@ const struct xattr_handler * const cifs_xattr_handlers[] = {
&cifs_cifs_acl_xattr_handler,
&smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
&smb3_ntsd_sacl_xattr_handler,
+ &smb3_ntsd_owner_xattr_handler,
&cifs_cifs_ntsd_xattr_handler,
&smb3_ntsd_xattr_handler, /* alias for above since avoiding "cifs" */
&cifs_cifs_ntsd_full_xattr_handler,
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 4/4] cifs: Add a new xattr system.smb3_ntsd_owner for getting or setting owner
2024-12-22 15:10 ` [PATCH 4/4] cifs: Add a new xattr system.smb3_ntsd_owner for getting or setting owner Pali Rohár
@ 2024-12-23 10:33 ` Pali Rohár
0 siblings, 0 replies; 7+ messages in thread
From: Pali Rohár @ 2024-12-23 10:33 UTC (permalink / raw)
To: Steve French, Paulo Alcantara, Ronnie Sahlberg; +Cc: linux-cifs, linux-kernel
I'm thinking about this change...
Currently this change via system.smb3_ntsd_owner changes both user owner
and group owner. Would not it be rather better to have separate xattrs
for changing just user owner and separate for changing just group owner?
Example scenario: If you are logged in Administrator and have granted
zero access rights for particular file, then you do not have permission
to neither read DACL, not read user and group owner, but as you are
Administrator, you have privilege to change user and group owners.
So for example, in this scenario you as Administrator could want to just
change group owner to yourself, without touching user owner, so the
original user owner would still have all access to that file. But with
this change, this is not possible yet, as it is required to change both
user and group owners.
On Sunday 22 December 2024 16:10:51 Pali Rohár wrote:
> Changing owner is controlled by DACL permission WRITE_OWNER. Changing DACL
> itself is controlled by DACL permisssion WRITE_DAC. Owner of the file has
> implicit WRITE_DAC permission even when it is not explicitly granted for
> owner by DACL.
>
> Reading DACL or owner is controlled only by one permission READ_CONTROL.
> WRITE_OWNER permission can be bypassed by the SeTakeOwnershipPrivilege,
> which is by default available for local administrators.
>
> So if the local administrator wants to access some file to which does not
> have access, it is required to first change owner to ourself and then
> change DACL permissions.
>
> Currently Linux SMB client does not support to do this, because it does not
> provide a way to change owner without touching DACL permissions.
>
> Fix this problem by introducing a new xattr for setting only owner part of
> security descriptor.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> fs/smb/client/xattr.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/xattr.c b/fs/smb/client/xattr.c
> index 95b8269851f3..b88fa04f5792 100644
> --- a/fs/smb/client/xattr.c
> +++ b/fs/smb/client/xattr.c
> @@ -32,6 +32,7 @@
> */
> #define SMB3_XATTR_CIFS_ACL "system.smb3_acl" /* DACL only */
> #define SMB3_XATTR_CIFS_NTSD_SACL "system.smb3_ntsd_sacl" /* SACL only */
> +#define SMB3_XATTR_CIFS_NTSD_OWNER "system.smb3_ntsd_owner" /* owner only */
> #define SMB3_XATTR_CIFS_NTSD "system.smb3_ntsd" /* owner plus DACL */
> #define SMB3_XATTR_CIFS_NTSD_FULL "system.smb3_ntsd_full" /* owner/DACL/SACL */
> #define SMB3_XATTR_ATTRIB "smb3.dosattrib" /* full name: user.smb3.dosattrib */
> @@ -39,7 +40,7 @@
> /* BB need to add server (Samba e.g) support for security and trusted prefix */
>
> enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT,
> - XATTR_CIFS_NTSD_SACL,
> + XATTR_CIFS_NTSD_SACL, XATTR_CIFS_NTSD_OWNER,
> XATTR_CIFS_NTSD, XATTR_CIFS_NTSD_FULL };
>
> static int cifs_attrib_set(unsigned int xid, struct cifs_tcon *pTcon,
> @@ -163,6 +164,7 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
>
> case XATTR_CIFS_ACL:
> case XATTR_CIFS_NTSD_SACL:
> + case XATTR_CIFS_NTSD_OWNER:
> case XATTR_CIFS_NTSD:
> case XATTR_CIFS_NTSD_FULL: {
> struct smb_ntsd *pacl;
> @@ -190,6 +192,10 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
> CIFS_ACL_GROUP |
> CIFS_ACL_DACL);
> break;
> + case XATTR_CIFS_NTSD_OWNER:
> + aclflags = (CIFS_ACL_OWNER |
> + CIFS_ACL_GROUP);
> + break;
> case XATTR_CIFS_NTSD_SACL:
> aclflags = CIFS_ACL_SACL;
> break;
> @@ -315,6 +321,7 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
>
> case XATTR_CIFS_ACL:
> case XATTR_CIFS_NTSD_SACL:
> + case XATTR_CIFS_NTSD_OWNER:
> case XATTR_CIFS_NTSD:
> case XATTR_CIFS_NTSD_FULL: {
> /*
> @@ -334,6 +341,9 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
> case XATTR_CIFS_NTSD:
> extra_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO;
> break;
> + case XATTR_CIFS_NTSD_OWNER:
> + extra_info = OWNER_SECINFO | GROUP_SECINFO;
> + break;
> case XATTR_CIFS_NTSD_SACL:
> extra_info = SACL_SECINFO;
> break;
> @@ -465,6 +475,13 @@ static const struct xattr_handler smb3_ntsd_sacl_xattr_handler = {
> .set = cifs_xattr_set,
> };
>
> +static const struct xattr_handler smb3_ntsd_owner_xattr_handler = {
> + .name = SMB3_XATTR_CIFS_NTSD_OWNER,
> + .flags = XATTR_CIFS_NTSD_OWNER,
> + .get = cifs_xattr_get,
> + .set = cifs_xattr_set,
> +};
> +
> static const struct xattr_handler cifs_cifs_ntsd_xattr_handler = {
> .name = CIFS_XATTR_CIFS_NTSD,
> .flags = XATTR_CIFS_NTSD,
> @@ -511,6 +528,7 @@ const struct xattr_handler * const cifs_xattr_handlers[] = {
> &cifs_cifs_acl_xattr_handler,
> &smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
> &smb3_ntsd_sacl_xattr_handler,
> + &smb3_ntsd_owner_xattr_handler,
> &cifs_cifs_ntsd_xattr_handler,
> &smb3_ntsd_xattr_handler, /* alias for above since avoiding "cifs" */
> &cifs_cifs_ntsd_full_xattr_handler,
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread