linux-kernel - Re: [PATCH 4/4] cifs: Add a new xattr system.smb3_ntsd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241223103350.v5lmcgd3mzpyn2yc@pali>
Date: Mon, 23 Dec 2024 11:33:50 +0100
From: Pali Rohár <pali@...nel.org>
To: Steve French <sfrench@...ba.org>, Paulo Alcantara <pc@...guebit.com>,
	Ronnie Sahlberg <ronniesahlberg@...il.com>
Cc: linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] cifs: Add a new xattr system.smb3_ntsd_owner for
 getting or setting owner

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@...nel.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
>