Re: [RFC PATCH v2 39/41] ksmbd: log in UUID and GUID users and map them to SIDs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ksmbd: sid_to_id() bypasses make_kuid_r() validation for UUID/GUID SIDs
- [High] ksmbd: ksmbd_alloc_user() falls back to resp uid/gid when resp_ext is NULL
- [Low] ksmbd: unused uid and gid fields and helpers in struct ksmbd_user
--

commit a1ea85f35f595431966b4e9bd815708cd849c5bc
Author: Jan Koester <jan.koester@tuxist.de>

ksmbd: log in UUID and GUID users and map them to SIDs

This commit adds support for logging in UUID and GUID users and groups via
ksmbd_login_response_ext and mapping their kuid_t and kgid_t values to and
from S-1-22-<kind>-w0-w1-w2-w3 SIDs in security descriptors, POSIX ACLs,
and durable handle owners.

[ ... ]

> diff --git a/fs/smb/server/mgmt/user_config.c b/fs/smb/server/mgmt/user_config.c
> index 5efc3d7455b99..cb175a4c78d83 100644
> --- a/fs/smb/server/mgmt/user_config.c
> +++ b/fs/smb/server/mgmt/user_config.c
> @@ -56,6 +56,8 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp,
>  	user->flags = resp->status;
>  	user->gid = resp->gid;
>  	user->uid = resp->uid;
> +	user->kuid = make_kuid(&init_user_ns, resp->uid);
> +	user->kgid = make_kgid(&init_user_ns, resp->gid);
>  	user->passkey_sz = resp->hash_sz;
>  	user->passkey = kmalloc(resp->hash_sz, KSMBD_DEFAULT_GFP);
>  	if (user->passkey)
> @@ -68,12 +70,38 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp,
>  		goto err_free;
>  
>  	if (resp_ext) {
> -		user->sgid = kmemdup(resp_ext->____payload,
> -				     resp_ext->ngroups * sizeof(gid_t),
> +		int i;
> +
> +		if (resp_ext->flags & KSMBD_LOGIN_EXT_ID_R) {
> +			/* the user's ids may be UUIDs or GUIDs */
> +			user->kuid = make_kuid_r(&init_user_ns, &resp_ext->uid_r);
> +			user->kgid = make_kgid_r(&init_user_ns, &resp_ext->gid_r);
> +			if (!uid_valid(user->kuid) || !gid_valid(user->kgid))
> +				goto err_free;
> +			user->uid = from_kuid_munged(&init_user_ns, user->kuid);
> +			user->gid = from_kgid_munged(&init_user_ns, user->kgid);
> +		}

[Severity: High]
What happens in ksmbd_alloc_user() in fs/smb/server/mgmt/user_config.c if
resp->status has KSMBD_USER_FLAG_EXTENSION set, but fetching the login
extension via ksmbd_ipc_login_request_ext() fails and returns NULL?

Both callers pass resp_ext directly to ksmbd_alloc_user() without checking
whether the second IPC request succeeded:

fs/smb/server/mgmt/user_config.c:ksmbd_login_user() {
	if (resp->status & KSMBD_USER_FLAG_EXTENSION)
		resp_ext = ksmbd_ipc_login_request_ext(account);

	user = ksmbd_alloc_user(resp, resp_ext);
}

fs/smb/server/auth.c:ksmbd_krb5_authenticate() {
	if (resp->login_response.status & KSMBD_USER_FLAG_EXTENSION)
		resp_ext = ksmbd_ipc_login_request_ext(resp->login_response.account);

	user = ksmbd_alloc_user(&resp->login_response, resp_ext);
}

Per the comment on struct ksmbd_login_response_ext in
fs/smb/server/ksmbd_netlink.h, resp->uid and resp->gid in the base login
response are ignored when KSMBD_LOGIN_EXT_ID_R is used. However, if
ksmbd_ipc_login_request_ext() returns NULL (for example on an IPC error,
allocation failure, or validation failure in ipc_validate_msg()), the
if (resp_ext) block is skipped and ksmbd_alloc_user() still succeeds using
user->kuid and user->kgid initialized from resp->uid and resp->gid.

If userspace left resp->uid and resp->gid zeroed in the base response, the
session will log in as GLOBAL_ROOT_UID and GLOBAL_ROOT_GID (keeping
filesystem capabilities in __ksmbd_override_fsids()). If resp->uid or
resp->gid is -1 (or when KSMBD_LOGIN_EXT_ID_R is not set), user->kuid or
user->kgid becomes INVALID_UID or INVALID_GID because uid_valid() and
gid_valid() are only checked inside the KSMBD_LOGIN_EXT_ID_R branch.

Should ksmbd_alloc_user() reject the login when
resp->status & KSMBD_USER_FLAG_EXTENSION is set but resp_ext is NULL, and
also check uid_valid(user->kuid) and gid_valid(user->kgid) when
KSMBD_LOGIN_EXT_ID_R is not set?

[ ... ]

> diff --git a/fs/smb/server/mgmt/user_config.h b/fs/smb/server/mgmt/user_config.h
> index cc460b4ff7d39..e55470e008f50 100644
> --- a/fs/smb/server/mgmt/user_config.h
> +++ b/fs/smb/server/mgmt/user_config.h
> @@ -13,13 +13,15 @@ struct ksmbd_user {
>  
>  	unsigned int		uid;
>  	unsigned int		gid;
> +	kuid_t			kuid;	/* the user: uid, or a UUID or GUID */
> +	kgid_t			kgid;
>  
>  	char			*name;
>  
>  	size_t			passkey_sz;
>  	char			*passkey;
>  	int			ngroups;
> -	gid_t			*sgid;
> +	kgid_t			*sgid;
>  };
>  
>  static inline bool user_guest(struct ksmbd_user *user)

[Severity: Low]
This isn't a bug, but now that smb2_open(), process_query_dir_entries(),
__ksmbd_override_fsids(), ksmbd_vfs_set_durable_owner(), and
ksmbd_vfs_compare_durable_owner() all use user->kuid and user->kgid, are
the 32-bit uid and gid fields in struct ksmbd_user (and the user_uid() and
user_gid() helpers in fs/smb/server/mgmt/user_config.h) still needed, or
could they be dropped now that they have no remaining readers?

[ ... ]

> diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
> index 1fad6ccf3a72a..126264cc0734e 100644
> --- a/fs/smb/server/smbacl.c
> +++ b/fs/smb/server/smbacl.c

[ ... ]

> @@ -279,6 +341,42 @@ static int sid_to_id(struct mnt_idmap *idmap,
>  		return -EIO;
>  	}
>  
> +	{
> +		u8 id[16];
> +		u32 kind = sid_to_id_r(psid, id);
> +
> +		if (kind && sidtype == SIDOWNER &&
> +		    (kind == SID_UNIX_UUID_USER || kind == SID_UNIX_GUID_USER)) {
> +			kuid_t uid;
> +
> +			if (kind == SID_UNIX_UUID_USER)
> +				uid = KUIDT_INIT_UUID(*(uuid_t *)id);
> +			else
> +				uid = KUIDT_INIT_GUID(*(guid_t *)id);
> +			uid = from_vfsuid(idmap, &init_user_ns, VFSUIDT_INIT(uid));
> +			if (!uid_valid(uid))
> +				return -EINVAL;
> +			fattr->cf_uid = uid;
> +			return 0;
> +		}
> +		if (kind && sidtype != SIDOWNER &&
> +		    (kind == SID_UNIX_UUID_GROUP || kind == SID_UNIX_GUID_GROUP)) {
> +			kgid_t gid;
> +
> +			if (kind == SID_UNIX_UUID_GROUP)
> +				gid = KGIDT_INIT_UUID(*(uuid_t *)id);
> +			else
> +				gid = KGIDT_INIT_GUID(*(guid_t *)id);
> +			gid = from_vfsgid(idmap, &init_user_ns, VFSGIDT_INIT(gid));
> +			if (!gid_valid(gid))
> +				return -EINVAL;
> +			fattr->cf_gid = gid;
> +			return 0;
> +		}
> +		if (kind)
> +			return -EINVAL;
> +	}

[Severity: High]
Can sid_to_id() in fs/smb/server/smbacl.c allow an SMB client to persist
malformed UUIDs or nil GUIDs into inode ownership and POSIX ACLs?

When a client sends an SMB2_SET_INFO or SMB2_CREATE security descriptor
containing an S-1-22-<kind>-w0-w1-w2-w3 SID (via set_info_sec() ->
parse_sec_desc() / parse_dacl() -> sid_to_id()), sid_to_id() builds the
kuid_t or kgid_t directly with KUIDT_INIT_UUID(), KUIDT_INIT_GUID(),
KGIDT_INIT_UUID(), and KGIDT_INIT_GUID() instead of calling make_kuid_r()
and make_kgid_r().

Unlike make_kuid_r() and make_kgid_r(), which validate the 16-byte payload
via kid_r_valid() in kernel/uidgid_r.c:

kernel/uidgid_r.c:kid_r_valid() {
	switch (id->type) {
	...
	case UID_R_UUID:
		return (id->uuid[6] >> 4) == 4 && (id->uuid[8] & 0xc0) == 0x80;
	case UID_R_GUID:
		return memchr_inv(id->guid, 0, UID_R_SIZE);
	}
	return false;
}

the uid_valid() and gid_valid() checks in include/linux/uidgid.h
unconditionally return true for any KID_UUID or KID_GUID:

include/linux/uidgid.h:uid_valid() {
	return !uid_is_legacy(uid) || __kuid_val(uid) != (uid_t) -1;
}

As a result, a client can pass a non-v4/non-RFC-9562 UUID (such as
S-1-22-3-0-0-0-0) or a nil GUID (S-1-22-5-0-0-0-0) and have it stored on
 the inode or in POSIX ACL xattrs, causing subsequent userspace operations
that validate via kid_r_valid() (such as chown_r() or setxattr() on POSIX
ACLs) to fail with -EINVAL.

Should sid_to_id() construct these IDs via make_kuid_r() and make_kgid_r()
so that invalid UUID and GUID payloads are rejected?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=39