[PATCH v2] ksmbd: enforce the DACL permission check on delete-on-close opens

[PATCH v2] ksmbd: enforce the DACL permission check on delete-on-close opens

[Ian Bridges]

smb2_open() skips the DACL check for delete-on-close opens of existing
files. On an acl_xattr share, a caller who passes the POSIX checks can
therefore delete a file even when its stored DACL grants no access. The
handle can also get read or write access that the DACL denies.

Add a dedicated DACL check for these opens. Evaluate the applicable ACEs
in order (MS-DTYP 2.5.3.2). Allow the parent directory to grant DELETE
and FILE_READ_ATTRIBUTES under MS-FSA 2.1.5.1.2.1. Use the DACL grants
when resolving MAXIMUM_ALLOWED.

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

Notes:
    Changes in v2
    - Replace v1 and the proposed parent-delete follow-up with one patch.
    - Use a dedicated check to preserve valid delete-on-close requests,
      including smbtorture smb2.acls.INHERITANCE and opens requesting
      FILE_READ_ATTRIBUTES or MAXIMUM_ALLOWED.
    
    v1: https://lore.kernel.org/linux-cifs/20261028153441.184701-1-icb@fastmail.org/
    
    Tested on ksmbd-for-next af8c72f65507. The original deletion bypass is
    blocked. Targeted tests cover group and forced-group grants, parent
    permissions, owner rights, guest and anonymous sessions, streams and
    MAXIMUM_ALLOWED. smbtorture smb2.acls and eight related suites match the
    base results.
    
    I did not Cc stable because of the patch's size. The size comes from
    keeping this check separate from smb_check_perm_dacl(), so this fix
    changes no other open. A backport needs 67cc25f7307c ("ksmbd: require
    DELETE access for delete-on-close opens").
    
    This leaves smb_check_perm_dacl() with its own DACL walk, which could
    later use the one added here so that ksmbd has only one.
    
    A reproducer is available on request.
    
    This fix was developed with LLM assistance.

 fs/smb/server/smb2pdu.c |   8 ++-
 fs/smb/server/smbacl.c  | 153 ++++++++++++++++++++++++++++++++++++++++
 fs/smb/server/smbacl.h  |   3 +
 3 files changed, 163 insertions(+), 1 deletion(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 2e955d272f4b..b56fcbfc6039 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -4848,7 +4848,13 @@ int smb2_open(struct ksmbd_work *work)
 
 	daccess = smb_map_generic_desired_access(req->DesiredAccess);
 
-	if (file_present && !(req->CreateOptions & FILE_DELETE_ON_CLOSE_LE)) {
+	if (file_present && req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
+		rc = smb_check_perm_delete_on_close(conn, &path,
+						    path.dentry == share->vfs_path.dentry,
+						    sess->user, &daccess);
+		if (rc)
+			goto err_out;
+	} else if (file_present) {
 		rc = smb_check_perm_dacl(conn, &path, &daccess,
 					 req->DesiredAccess,
 					 sess->user->uid, false);
diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index 664f01263611..b6f4ec17c6fc 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -17,6 +17,7 @@
 #include "server.h"
 #include "misc.h"
 #include "mgmt/share_config.h"
+#include "mgmt/user_config.h"
 
 static const struct smb_sid domain = {1, 4, {0, 0, 0, 0, 0, 5},
 	{cpu_to_le32(21), cpu_to_le32(1), cpu_to_le32(2), cpu_to_le32(3),
@@ -1685,6 +1686,158 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 }
 EXPORT_SYMBOL_IF_KUNIT(smb_check_perm_dacl);
 
+static bool smb_sid_equal(const struct smb_sid *a, const struct smb_sid *b)
+{
+	return a->num_subauth == b->num_subauth && !compare_sids(a, b);
+}
+
+/* Whether the caller's credentials hold the group of an S-1-22-2-<gid> SID */
+static bool smb_sid_in_group(const struct mnt_idmap *idmap,
+			     const struct smb_sid *sid)
+{
+	kgid_t gid;
+
+	if (sid->num_subauth != 2 || compare_sids(sid, &sid_unix_groups))
+		return false;
+	gid = KGIDT_INIT(le32_to_cpu(sid->sub_auth[1]));
+	return in_group_p(from_vfsgid(idmap, &init_user_ns, VFSGIDT_INIT(gid)));
+}
+
+/*
+ * Rights the stored security descriptor of @dentry grants the caller, with
+ * the DACL evaluated in ACE order (MS-DTYP 2.5.3.2). Returns -ENODATA when
+ * there is no usable stored descriptor.
+ */
+static int smb_dacl_granted(struct ksmbd_conn *conn,
+			    const struct mnt_idmap *idmap,
+			    struct dentry *dentry, struct ksmbd_user *user,
+			    u32 *granted)
+{
+	bool authenticated = !user_guest(user) && !ksmbd_anonymous_user(user);
+	u32 allowed = 0, denied = 0, mask;
+	bool is_owner, owner_rights = false, applies;
+	unsigned int uid = user->uid;
+	int pntsd_size, aces_size, ace_size, i, rc = -EINVAL;
+	struct smb_ntsd *pntsd = NULL;
+	unsigned int dacl_offset;
+	struct smb_acl *pdacl;
+	struct smb_ace *ace;
+	struct smb_sid sid;
+	vfsuid_t vfsuid;
+
+	pntsd_size = ksmbd_vfs_get_sd_xattr(conn, idmap, dentry, &pntsd);
+	if (!pntsd_size || pntsd_size == -ENODATA ||
+	    pntsd_size == -EOPNOTSUPP || pntsd_size == -EINVAL)
+		return -ENODATA;
+	if (pntsd_size < 0)
+		return pntsd_size;
+
+	dacl_offset = le32_to_cpu(pntsd->dacloffset);
+	if (!dacl_offset) {
+		allowed = GENERIC_ALL_FLAGS;
+		goto out;
+	}
+	if (dacl_offset > pntsd_size ||
+	    pntsd_size - dacl_offset < sizeof(struct smb_acl))
+		goto err;
+	pdacl = (struct smb_acl *)((char *)pntsd + dacl_offset);
+	aces_size = le16_to_cpu(pdacl->size);
+	if (aces_size > pntsd_size - dacl_offset ||
+	    aces_size < sizeof(struct smb_acl))
+		goto err;
+	aces_size -= sizeof(struct smb_acl);
+
+	id_to_sid(uid, uid ? SIDOWNER : SIDUNIX_USER, &sid);
+	vfsuid = i_uid_into_vfsuid(idmap, d_inode(dentry));
+	is_owner = uid == from_kuid(&init_user_ns, vfsuid_into_kuid(vfsuid));
+	ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
+	for (i = 0; i < le16_to_cpu(pdacl->num_aces); i++) {
+		if (aces_size < offsetof(struct smb_ace, sid) +
+				CIFS_SID_BASE_SIZE)
+			goto err;
+		ace_size = le16_to_cpu(ace->size);
+		if (ace_size > aces_size ||
+		    ace->sid.num_subauth > SID_MAX_SUB_AUTHORITIES ||
+		    ace_size < offsetof(struct smb_ace, sid) +
+			       CIFS_SID_BASE_SIZE +
+			       sizeof(__le32) * ace->sid.num_subauth)
+			goto err;
+		aces_size -= ace_size;
+
+		if (smb_sid_equal(&ace->sid, &sid_owner_rights)) {
+			if (!(ace->flags & INHERIT_ONLY_ACE))
+				owner_rights = true;
+			applies = is_owner;
+		} else {
+			applies = smb_sid_equal(&ace->sid, &sid) ||
+				  smb_sid_equal(&ace->sid, &sid_everyone) ||
+				  (authenticated &&
+				   smb_sid_equal(&ace->sid, &sid_authusers)) ||
+				  smb_sid_in_group(idmap, &ace->sid);
+		}
+		if (applies && !(ace->flags & INHERIT_ONLY_ACE)) {
+			mask = le32_to_cpu(smb_map_generic_desired_access(ace->access_req));
+			if (ace->type == ACCESS_ALLOWED_ACE_TYPE)
+				allowed |= mask & ~denied;
+			else if (ace->type == ACCESS_DENIED_ACE_TYPE ||
+				 ace->type == ACCESS_DENIED_CALLBACK_ACE_TYPE)
+				denied |= mask & ~allowed;
+		}
+		ace = (struct smb_ace *)((char *)ace + ace_size);
+	}
+
+	/* Unless OWNER RIGHTS applies, the owner may read and change the DACL */
+	if (is_owner && !owner_rights)
+		allowed |= READ_CONTROL | WRITE_DAC;
+out:
+	*granted = allowed;
+	rc = 0;
+err:
+	kfree(pntsd);
+	return rc;
+}
+
+/*
+ * Access check for a delete-on-close open of an existing file with a stored
+ * security descriptor. The file's DACL decides every right, and the parent's
+ * DACL can also grant DELETE through FILE_DELETE_CHILD and
+ * FILE_READ_ATTRIBUTES through FILE_LIST_DIRECTORY (MS-FSA 2.1.5.1.2.1).
+ * MAXIMUM_ALLOWED becomes the rights granted that POSIX also allows.
+ */
+int smb_check_perm_delete_on_close(struct ksmbd_conn *conn,
+				   const struct path *path, bool share_root,
+				   struct ksmbd_user *user, __le32 *pdaccess)
+{
+	const struct mnt_idmap *idmap = mnt_idmap(path->mnt);
+	__le32 requested = *pdaccess & ~FILE_MAXIMAL_ACCESS_LE, posix;
+	u32 granted, pgranted;
+	struct dentry *parent;
+	int rc;
+
+	rc = smb_dacl_granted(conn, idmap, path->dentry, user, &granted);
+	if (rc)
+		return rc == -ENODATA ? 0 : rc;
+
+	if (!share_root) {
+		parent = dget_parent(path->dentry);
+		if (!smb_dacl_granted(conn, idmap, parent, user, &pgranted)) {
+			if (pgranted & FILE_DELETE_CHILD)
+				granted |= DELETE;
+			if (pgranted & le32_to_cpu(FILE_LIST_DIRECTORY_LE))
+				granted |= FILE_READ_ATTRIBUTES;
+		}
+		dput(parent);
+	}
+
+	if (le32_to_cpu(requested) & ~granted)
+		return -EACCES;
+	if (*pdaccess & FILE_MAXIMAL_ACCESS_LE) {
+		ksmbd_vfs_query_maximal_access(idmap, path->dentry, &posix);
+		*pdaccess = (cpu_to_le32(granted) & posix) | requested;
+	}
+	return 0;
+}
+
 int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
 		 const struct path *path, struct smb_ntsd *pntsd, int ntsd_len,
 		 bool type_check, bool get_write, bool posix_open,
diff --git a/fs/smb/server/smbacl.h b/fs/smb/server/smbacl.h
index 3342cebab333..d9b11bb06331 100644
--- a/fs/smb/server/smbacl.h
+++ b/fs/smb/server/smbacl.h
@@ -98,6 +98,9 @@ int smb_inherit_dacl(struct ksmbd_conn *conn, const struct path *path,
 int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
 			__le32 *pdaccess, __le32 raw_daccess, int uid,
 			bool strict);
+int smb_check_perm_delete_on_close(struct ksmbd_conn *conn,
+				   const struct path *path, bool share_root,
+				   struct ksmbd_user *user, __le32 *pdaccess);
 int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
 		 const struct path *path, struct smb_ntsd *pntsd, int ntsd_len,
 		 bool type_check, bool get_write, bool posix_open,

base-commit: af8c72f6550744fe948ef2dbc7ab8739c149aab2
-- 
2.47.3