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

[PATCH v3] 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 Unix
groups and refuses an owner named only by OWNER RIGHTS. These callers
cannot use rights the DACL grants them under MS-DTYP 2.5.3.2.

Match Unix group SIDs (S-1-22-2) against the caller's credentials. Use
the ordered DACL result when the existing SID lookup and POSIX ACL
fallback do not decide access. In that result, deny ACEs for the
caller's groups limit what it grants, also for MAXIMUM_ALLOWED. Match
OWNER RIGHTS exactly, since compare_sids() also matches longer SIDs,
and return only file access rights. Preserve the refusal for
non-inherit-only object deny ACEs, which ksmbd does not parse.

MAXIMUM_ALLOWED still excludes group allow ACEs. Including them
requires corresponding changes in smb2_open() to avoid breaking
existing opens. An open relying on group grants can therefore still
fail when it requests a maximal access reply. For the owner, that
reply leaves out the rights of its groups.

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

Notes:
    Since v2
    - Honor deny ACEs for the caller's groups in the new MAXIMUM_ALLOWED
      fallback (Namjae Jeon [2]).
    - Match OWNER RIGHTS exactly and return only file access rights.
    - Keep the existing refusal for non-inherit-only object deny ACEs.

    Since v1
    - Clear access_bits for explicit requests, preserving refusals for
      ACE types the existing lookup does not handle (Sashiko [1]).

    The hide unreadable listing and the app instance check use the same
    answer and change with it.

    Tested on ksmbd-for-next e49296d51ccb with KASAN, with no reports or
    warnings. A reproducer is available on request.

    v2: https://lore.kernel.org/linux-cifs/20261102200353.884509-1-icb@fastmail.org/
    v1: https://lore.kernel.org/linux-cifs/20261102061537.830714-3-icb@fastmail.org/
    [1] https://sashiko.dev/#/patchset/20261102061537.830714-1-icb%40fastmail.org
    [2] https://lore.kernel.org/linux-cifs/CAKYAXd-s-g2V4LNY8e5SsH=+0r3qLY+AeOa5Lp9K0T5p_tnA8A@mail.gmail.com/

    This fix was developed with LLM assistance.

 fs/smb/server/smbacl.c | 94 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 82 insertions(+), 12 deletions(-)

diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index ab76c5c9e0ea..acb80cf0f2ca 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1459,6 +1459,29 @@ 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])));
+}
+
+/* compare_sids() also matches a longer SID, such as S-1-3-4-777 */
+static bool smb_sid_is_owner_rights(const struct smb_sid *sid)
+{
+	return sid->num_subauth == sid_owner_rights.num_subauth &&
+	       !compare_sids(sid, &sid_owner_rights);
+}
+
 int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 			__le32 *pdaccess, __le32 raw_daccess, int uid,
 			bool strict)
@@ -1475,13 +1498,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;
+	unsigned int any_allowed = 0, any_denied = 0, bits;
 	struct smb_ace *others_ace = NULL;
-	bool or_ace = false;
+	bool or_ace = false, in_group = false, member, counted;
+	bool obj_deny = 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;
 	vfsuid_t vfsuid;
 
 	ksmbd_debug(SMB, "check permission using windows acl\n");
@@ -1522,7 +1548,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 +1557,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++) {
@@ -1551,42 +1582,69 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 				      sizeof(__le32) * ace->sid.num_subauth)
 				break;
 
+			if (ace->type == ACCESS_DENIED_OBJECT_ACE_TYPE ||
+			    ace->type == ACCESS_DENIED_CALLBACK_OBJECT_ACE_TYPE)
+				obj_deny |= !(ace->flags & INHERIT_ONLY_ACE);
+
 			if (!compare_sids(&sid_owner_rights, &ace->sid)) {
 				owner_rights = true;
 				if (!is_owner)
 					goto next_ace;
 			}
 
+			member = smb_sid_in_group(&ace->sid);
+			/* ordered counts groups and OWNER RIGHTS only */
+			counted = member;
+			if (is_owner && smb_sid_is_owner_rights(&ace->sid))
+				counted = true;
 			if (ace->flags & INHERIT_ONLY_ACE ||
 			    (compare_sids(&sid, &ace->sid) &&
 			     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) &&
+			     !member))
 				goto next_ace;
 
+			bits = le32_to_cpu(ace->access_req);
 			switch (ace->type) {
 			case ACCESS_ALLOWED_ACE_TYPE:
-				access_bits |= le32_to_cpu(ace->access_req);
+				if (counted)
+					any_allowed |= bits;
+				/* MAXIMUM_ALLOWED takes no group grants */
+				if (maximal && member)
+					break;
+				if (counted)
+					ordered |= bits;
+				access_bits |= bits;
 				break;
 			case ACCESS_DENIED_ACE_TYPE:
 			case ACCESS_DENIED_CALLBACK_ACE_TYPE:
-				denied |= ~access_bits &
-					le32_to_cpu(ace->access_req);
+				if (counted)
+					any_denied |= ~any_allowed & bits;
+				/* access_bits still ignores group denials */
+				if (!member)
+					denied |= ~access_bits & bits;
 				break;
 			}
 next_ace:
 			ace = (struct smb_ace *)((char *)ace + le16_to_cpu(ace->size));
 		}
-		if (is_owner && !owner_rights)
+		/* Keep only file rights, not MAXIMUM_ALLOWED or generic bits */
+		ordered &= GENERIC_ALL_FLAGS & ~any_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 +1673,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 && smb_sid_is_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 +1730,15 @@ 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. An
+			 * object deny ACE is not parsed, so it keeps the
+			 * refusal.
+			 */
+			if (in_group && !obj_deny)
+				allowed |= ordered;
+			if (maximal)
 				requested |= allowed;
 			if (!allowed || requested & ~allowed) {
 				ksmbd_debug(SMB, "Can't find corresponding sid\n");

base-commit: e49296d51ccbf2ad0ed0484db1382f1d7217bd6e
-- 
2.47.3