[PATCH v2] ksmbd: grant a caller what its groups and OWNER RIGHTS ACEs allow

[PATCH v2] ksmbd: grant a caller what its groups and OWNER RIGHTS ACEs allow

[Ian Bridges]

smb_check_perm_dacl() does not match ACEs for the caller's groups. It
also refuses an owner named only by an OWNER RIGHTS ACE, although its
MAXIMUM_ALLOWED walk already counts that ACE. These callers cannot use
rights the DACL grants them under MS-DTYP 2.5.3.2.

Run the existing ordered walk for every request and match Unix group
ACEs (S-1-22-2) against the caller's credentials. Use its result when
the existing SID lookup and POSIX ACL fallback do not decide the open.

Group ACEs remain excluded from MAXIMUM_ALLOWED. An open that asks for
the maximal access reply is therefore still refused for a caller that
only group ACEs name. Adding them requires corresponding changes in
smb2_open(), which derives open flags and the maximal access reply from
that answer.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Signed-off-by: Ian Bridges <icb@fastmail.org>
---

Notes:
    Since v1, clear access_bits after the ordered walk for explicit
    requests. In v1, when the caller's first ACE had a type the lookup does
    not handle, such as an allow-callback ACE, an explicit request got the
    walk's result instead of the earlier refusal. Sashiko found this [1].
    The message now also says that an open asking for the maximal access
    reply is still refused for a caller that only group ACEs name.
    
    v1: https://lore.kernel.org/linux-cifs/20261102061537.830714-3-icb@fastmail.org/
    [1] https://sashiko.dev/#/patchset/20261102061537.830714-1-icb%40fastmail.org
    
    Reachable on a share with "vfs objects = acl_xattr", for example on a
    file whose DACL only has ACEs for Unix groups (S-1-22-2) or OWNER
    RIGHTS.
    
    MAXIMUM_ALLOWED does not count groups here because smb2_open() also
    builds the open flags and the maximal access reply from that answer.
    Counting them there makes some MAXIMUM_ALLOWED opens fail that work
    today.
    
    Tested on ksmbd-for-next ffc855fe16af with KASAN. Results change for
    members of the file's group and of other groups the DACL names, and for
    the owner. A member of a group that the DACL grants full access now
    reads the file. An owner that only an OWNER RIGHTS ACE names now gets
    what that ACE allows. Its MAXIMUM_ALLOWED opens, refused before, now
    succeed with the rights in the maximal access reply. DACLs with no ACE
    give the same results as before. So do DACLs whose first ACE for the
    caller has a type the lookup does not handle, such as an allow-callback
    or audit ACE.
    
    A reproducer is available on request.
    
    This fix was developed with LLM assistance.

 fs/smb/server/smbacl.c | 58 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index ab76c5c9e0ea..df456c635004 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1459,6 +1459,22 @@ bool smb_inherit_flags(int flags, bool is_dir)
 	return false;
 }
 
+/* Whether the caller's credentials hold the group of an S-1-22-2-<gid> SID */
+static bool smb_sid_in_group(const struct smb_sid *sid)
+{
+	if (sid->num_subauth != 2 || compare_sids(sid, &sid_unix_groups))
+		return false;
+	/*
+	 * ksmbd_override_fsids() builds the caller's credentials with
+	 * make_kgid(&init_user_ns, ...), so the group membership test must
+	 * compare in that space.  Applying the mount idmap here (as when
+	 * resolving a SID to an on-disk gid) would mismatch on idmapped
+	 * mounts.
+	 */
+	return in_group_p(make_kgid(&init_user_ns,
+				    le32_to_cpu(sid->sub_auth[1])));
+}
+
 int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 			__le32 *pdaccess, __le32 raw_daccess, int uid,
 			bool strict)
@@ -1475,13 +1491,16 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 	int granted = requested;
 	struct smb_ace *ace;
 	int i, found = 0;
-	unsigned int access_bits = 0, denied = 0;
+	unsigned int access_bits = 0, denied = 0, ordered = 0;
 	struct smb_ace *others_ace = NULL;
-	bool or_ace = false;
+	bool or_ace = false, in_group = false;
 	struct posix_acl_entry *pa_entry;
 	unsigned int sid_type = SIDOWNER;
 	unsigned short ace_size;
 	bool is_owner, owner_rights = false;
+	bool maximal = *pdaccess & FILE_MAXIMAL_ACCESS_LE;
+	/* The answer to MAXIMUM_ALLOWED stays without the caller's groups */
+	bool groups = !maximal;
 	vfsuid_t vfsuid;
 
 	ksmbd_debug(SMB, "check permission using windows acl\n");
@@ -1522,7 +1541,7 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 		 */
 		if (is_owner)
 			allowed = READ_CONTROL | WRITE_DAC;
-		if (*pdaccess & FILE_MAXIMAL_ACCESS_LE)
+		if (maximal)
 			requested |= allowed;
 		if (!allowed || requested & ~allowed)
 			rc = -EACCES;
@@ -1531,7 +1550,12 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 		goto err_out;
 	}
 
-	if (*pdaccess & FILE_MAXIMAL_ACCESS_LE) {
+	/*
+	 * Evaluate the DACL in order (MS-DTYP 2.5.3.2). MAXIMUM_ALLOWED takes
+	 * its result, and so does a caller that only group or OWNER RIGHTS
+	 * ACEs name.
+	 */
+	{
 		ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
 		aces_size = pdacl_size - sizeof(struct smb_acl);
 		for (i = 0; i < le16_to_cpu(pdacl->num_aces); i++) {
@@ -1562,7 +1586,8 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 			     compare_sids(&sid_unix_NFS_mode, &ace->sid) &&
 			     compare_sids(&sid_everyone, &ace->sid) &&
 			     compare_sids(&sid_authusers, &ace->sid) &&
-			     compare_sids(&sid_owner_rights, &ace->sid)))
+			     compare_sids(&sid_owner_rights, &ace->sid) &&
+			     !(groups && smb_sid_in_group(&ace->sid))))
 				goto next_ace;
 
 			switch (ace->type) {
@@ -1578,15 +1603,20 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 next_ace:
 			ace = (struct smb_ace *)((char *)ace + le16_to_cpu(ace->size));
 		}
-		if (is_owner && !owner_rights)
+		ordered = access_bits & ~denied;
+		if (maximal && is_owner && !owner_rights)
 			access_bits |= READ_CONTROL | WRITE_DAC |
 				FILE_READ_ATTRIBUTES | DELETE;
 		access_bits &= ~denied;
-		if ((raw_daccess & FILE_GENERIC_EXECUTE_LE) &&
+		if (maximal && (raw_daccess & FILE_GENERIC_EXECUTE_LE) &&
 		    S_ISREG(d_inode(path->dentry)->i_mode) &&
 		    (access_bits & GENERIC_READ_FLAGS) == GENERIC_READ_FLAGS)
 			access_bits |= FILE_EXECUTE;
-		granted = requested | access_bits;
+		/* An explicit request takes only ordered from this walk */
+		if (maximal)
+			granted = requested | access_bits;
+		else
+			access_bits = 0;
 	}
 
 	ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
@@ -1615,6 +1645,10 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 		if (!compare_sids(&sid_owner_rights, &ace->sid) &&
 		    !(ace->flags & INHERIT_ONLY_ACE))
 			or_ace = true;
+		if (!(ace->flags & INHERIT_ONLY_ACE) &&
+		    (smb_sid_in_group(&ace->sid) ||
+		     (is_owner && !compare_sids(&sid_owner_rights, &ace->sid))))
+			in_group = true;
 		if (!compare_sids(&sid_everyone, &ace->sid) ||
 		    !compare_sids(&sid_authusers, &ace->sid))
 			others_ace = ace;
@@ -1668,7 +1702,13 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 			 */
 			if (is_owner && !or_ace)
 				allowed = READ_CONTROL | WRITE_DAC;
-			if (*pdaccess & FILE_MAXIMAL_ACCESS_LE)
+			/*
+			 * A group the caller is in, or OWNER RIGHTS for the
+			 * owner, gives what the DACL grants it in order.
+			 */
+			if (in_group)
+				allowed |= ordered;
+			if (maximal)
 				requested |= allowed;
 			if (!allowed || requested & ~allowed) {
 				ksmbd_debug(SMB, "Can't find corresponding sid\n");

base-commit: ffc855fe16afe02b22305111aa68cd8d07360941
-- 
2.47.3

Re: [PATCH v2] ksmbd: grant a caller what its groups and OWNER RIGHTS ACEs allow

[Namjae Jeon]

> @@ -1562,7 +1586,8 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
>                              compare_sids(&sid_unix_NFS_mode, &ace->sid) &&
>                              compare_sids(&sid_everyone, &ace->sid) &&
>                              compare_sids(&sid_authusers, &ace->sid) &&
> -                            compare_sids(&sid_owner_rights, &ace->sid)))
> +                            compare_sids(&sid_owner_rights, &ace->sid) &&
> +                            !(groups && smb_sid_in_group(&ace->sid))))
groups is false for MAXIMUM_ALLOWED, so this skips group deny ACEs
too. Consider a file owner whose group has a deny ACE for
FILE_WRITE_RIGHTS, followed by an OWNER RIGHTS ACE granting full
access. The group deny ACE is skipped, leaving full access in ordered.
The new fallback then returns those rights.
An explicit FILE_WRITE_DATA request is denied, but MAXIMUM_ALLOWED
returns 0x001f01ff, including write access. Before this patch, the
latter request was rejected. Since smb2_open() uses this mask for the
handle's access rights, the client can write if the vfs permissions
allow it. Could we also process group deny ACEs for MAXIMUM_ALLOWED
requests?