* [PATCH 1/2] ksmbd: evaluate the DACL in order for requested access
2026-10-30 2:05 [PATCH 0/2] ksmbd: fix DACL evaluation for denies and guest sessions Ian Bridges
@ 2026-10-30 2:05 ` Ian Bridges
2026-10-30 2:05 ` [PATCH 2/2] ksmbd: exclude guest and anonymous sessions from Authenticated Users ACEs Ian Bridges
1 sibling, 0 replies; 3+ messages in thread
From: Ian Bridges @ 2026-10-30 2:05 UTC (permalink / raw)
To: Namjae Jeon
Cc: Sergey Senozhatsky, Tom Talpey, ChenXiaoSong, linux-cifs,
linux-kernel, Ian Bridges, stable
On an acl_xattr share, smb_check_perm_dacl() evaluates the whole DACL in
order only for a MAXIMUM_ALLOWED open. Any other open is checked against
a single ACE, the first one whose SID matches the caller.
If that ACE is a deny, the caller gets every right the deny does not
name. For example, a deny for writing grants read. Later ACEs are never
checked, and an inherit-only ACE counts as if it applied to the file.
Evaluate the DACL in order (MS-DTYP 2.5.3.2) for every open, as the
MAXIMUM_ALLOWED path already does. The extra rights that path gives the
owner, and the execute right it adds, stay limited to MAXIMUM_ALLOWED.
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Signed-off-by: Ian Bridges <icb@fastmail.org>
---
Notes:
Reachable on a share with "vfs objects = acl_xattr", for a file whose
DACL has a deny ACE. An authenticated user reaches it through a deny
that names them. A guest reaches it through a deny for Everyone on a
share that allows guests.
Tested on ksmbd-for-next af8c72f65507. tester2's file has the DACL
[DENY tester WRITE_DATA] [ALLOW tester2 FULL]. Before this change tester
reads its contents over SMB. After it, the open returns
STATUS_ACCESS_DENIED. Every combination of one or two allow or deny ACEs
for the caller, Everyone and Authenticated Users was tested, with
READ_DATA, WRITE_DATA, WRITE_EA, APPEND_DATA or full access as the mask
(930 DACLs). Each DACL also ended with an allow ACE giving the owner
full access. The DACLs were set on a file and probed with real reads
and writes. Every result that changes for an authenticated user moves to
what ordered evaluation gives, and none moves away. smbtorture smb2.acls
and eight related suites match the base results, apart from smb2.create
mkdir-dup, which also fails intermittently on base.
The ordered walk this reuses came with cc2f133e80eb
("ksmbd: fix maximum allowed access checks"), so a stable tree without
it needs that commit first.
A reproducer is available on request.
This fix was developed with LLM assistance.
fs/smb/server/smbacl.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index 664f01263611..3542ae9a605c 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1481,6 +1481,7 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
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");
@@ -1523,7 +1524,11 @@ 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) for every request, so a
+ * deny ACE only denies and a later allow ACE still counts.
+ */
+ {
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++) {
@@ -1570,15 +1575,16 @@ 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)
+ 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;
+ if (maximal)
+ granted = requested | access_bits;
}
ace = (struct smb_ace *)((char *)pdacl + sizeof(struct smb_acl));
@@ -1651,18 +1657,6 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
}
}
- if (!(*pdaccess & FILE_MAXIMAL_ACCESS_LE)) {
- switch (ace->type) {
- case ACCESS_ALLOWED_ACE_TYPE:
- access_bits = le32_to_cpu(ace->access_req);
- break;
- case ACCESS_DENIED_ACE_TYPE:
- case ACCESS_DENIED_CALLBACK_ACE_TYPE:
- access_bits = le32_to_cpu(~ace->access_req);
- break;
- }
- }
-
check_access_bits:
if (strict) {
access_bits &= granted;
--
2.47.3
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH 2/2] ksmbd: exclude guest and anonymous sessions from Authenticated Users ACEs
2026-10-30 2:05 [PATCH 0/2] ksmbd: fix DACL evaluation for denies and guest sessions Ian Bridges
2026-10-30 2:05 ` [PATCH 1/2] ksmbd: evaluate the DACL in order for requested access Ian Bridges
@ 2026-10-30 2:05 ` Ian Bridges
1 sibling, 0 replies; 3+ messages in thread
From: Ian Bridges @ 2026-10-30 2:05 UTC (permalink / raw)
To: Namjae Jeon
Cc: Sergey Senozhatsky, Tom Talpey, ChenXiaoSong, linux-cifs,
linux-kernel, Ian Bridges, stable
smb_check_perm_dacl() matches an Authenticated Users ACE (S-1-5-11) for
every caller, guest and anonymous sessions included. MS-DTYP 2.4.2.4
says users authenticated as Guest or Anonymous are not members of that
group. As a result, on a share with guest access a client without
credentials passes the DACL check with every right the DACL gives
Authenticated Users. For example, it can read a file that only
Authenticated Users may read. Access-based enumeration also lists that
file to it.
Tell smb_check_perm_dacl() whether the session authenticated. A guest or
anonymous session then no longer matches an Authenticated Users ACE but
still matches an Everyone ACE. An authenticated user matches both.
Fixes: cc2f133e80eb ("ksmbd: fix maximum allowed access checks")
Cc: stable@vger.kernel.org
Signed-off-by: Ian Bridges <icb@fastmail.org>
---
Notes:
Reachable on a share with "vfs objects = acl_xattr" and
"guest ok = yes", for files whose DACL grants Authenticated Users. That
is enough for an anonymous session. A guest session for an unknown user
also needs "map to guest = bad user". These settings are off by default.
The match came with cc2f133e80eb, first released in v7.3.
Tested on ksmbd-for-next af8c72f65507 with patch 1 applied, before and
after this one. Every combination of one or two allow or deny ACEs for
the caller, Everyone and Authenticated Users was tested, with READ_DATA,
WRITE_DATA, WRITE_EA, APPEND_DATA or full access as the mask (930
DACLs). Each DACL also ended with an allow ACE giving the owner full
access. The DACLs were set on a file and probed with real reads and
writes as an authenticated user, a guest and an anonymous session. The
authenticated user's results are unchanged. Every guest and anonymous
change agrees with MS-DTYP 2.5.3.2 and 2.4.2.4, and none moves away. A
guest or anonymous session no longer reads or lists a file only
Authenticated Users may read, and still reads and lists one Everyone may
read. smbtorture smb2.acls and eight related suites match the results
with patch 1 alone, apart from smb2.create mkdir-dup, which also fails
intermittently there.
A reproducer is available on request.
This fix was developed with LLM assistance.
fs/smb/server/mgmt/user_config.h | 5 +++++
fs/smb/server/smb2pdu.c | 11 ++++++++---
fs/smb/server/smbacl.c | 8 +++++---
fs/smb/server/smbacl.h | 2 +-
fs/smb/server/tests/smbacl_kunit.c | 4 ++--
5 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/fs/smb/server/mgmt/user_config.h b/fs/smb/server/mgmt/user_config.h
index cc460b4ff7d3..43c55357b95b 100644
--- a/fs/smb/server/mgmt/user_config.h
+++ b/fs/smb/server/mgmt/user_config.h
@@ -66,5 +66,10 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp,
struct ksmbd_login_response_ext *resp_ext);
void ksmbd_free_user(struct ksmbd_user *user);
bool ksmbd_anonymous_user(struct ksmbd_user *user);
+
+static inline bool ksmbd_user_authenticated(struct ksmbd_user *user)
+{
+ return user && !user_guest(user) && !ksmbd_anonymous_user(user);
+}
bool ksmbd_compare_user(struct ksmbd_user *u1, struct ksmbd_user *u2);
#endif /* __USER_CONFIG_MANAGEMENT_H__ */
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 2e955d272f4b..8c7aba240e71 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -4330,7 +4330,8 @@ static int smb2_handle_app_instance_id(struct ksmbd_work *work,
/* Authorize the referenced open even if its name changes meanwhile. */
old_path = &old_fp->filp->f_path;
rc = smb_check_perm_dacl(conn, old_path, &maximal_access, 0,
- work->sess->user->uid, false);
+ work->sess->user->uid,
+ ksmbd_user_authenticated(work->sess->user), false);
if (rc) {
/* Access denial prevents takeover, not the CREATE itself. */
ksmbd_put_durable_fd(old_fp);
@@ -4851,14 +4852,17 @@ int smb2_open(struct ksmbd_work *work)
if (file_present && !(req->CreateOptions & FILE_DELETE_ON_CLOSE_LE)) {
rc = smb_check_perm_dacl(conn, &path, &daccess,
req->DesiredAccess,
- sess->user->uid, false);
+ sess->user->uid,
+ ksmbd_user_authenticated(sess->user), false);
if (rc)
goto err_out;
if (maximal_access_ctxt) {
maximal_access = FILE_MAXIMAL_ACCESS_LE;
rc = smb_check_perm_dacl(conn, &path, &maximal_access,
- 0, sess->user->uid, false);
+ 0, sess->user->uid,
+ ksmbd_user_authenticated(sess->user),
+ false);
if (rc)
goto err_out;
@@ -6326,6 +6330,7 @@ static int process_query_dir_entries(struct smb2_query_dir_private *priv)
rc = smb_check_perm_dacl(priv->work->conn, &path,
&daccess, daccess,
priv->work->sess->user->uid,
+ ksmbd_user_authenticated(priv->work->sess->user),
true);
if (rc) {
dput(dent);
diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index 3542ae9a605c..b83aa84815ce 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1461,7 +1461,7 @@ bool smb_inherit_flags(int flags, bool is_dir)
int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
__le32 *pdaccess, __le32 raw_daccess, int uid,
- bool strict)
+ bool authenticated, bool strict)
{
const struct mnt_idmap *idmap = mnt_idmap(path->mnt);
struct smb_ntsd *pntsd = NULL;
@@ -1558,7 +1558,8 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
(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) &&
+ (!authenticated ||
+ compare_sids(&sid_authusers, &ace->sid)) &&
compare_sids(&sid_owner_rights, &ace->sid)))
goto next_ace;
@@ -1611,7 +1612,8 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
break;
}
if (!compare_sids(&sid_everyone, &ace->sid) ||
- !compare_sids(&sid_authusers, &ace->sid))
+ (authenticated &&
+ !compare_sids(&sid_authusers, &ace->sid)))
others_ace = ace;
ace = (struct smb_ace *)((char *)ace + le16_to_cpu(ace->size));
diff --git a/fs/smb/server/smbacl.h b/fs/smb/server/smbacl.h
index 3342cebab333..bbd35948a379 100644
--- a/fs/smb/server/smbacl.h
+++ b/fs/smb/server/smbacl.h
@@ -97,7 +97,7 @@ int smb_inherit_dacl(struct ksmbd_conn *conn, const struct path *path,
unsigned int uid, unsigned int gid);
int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
__le32 *pdaccess, __le32 raw_daccess, int uid,
- bool strict);
+ bool authenticated, bool strict);
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/tests/smbacl_kunit.c b/fs/smb/server/tests/smbacl_kunit.c
index 33496b4d31a3..bf28439a7696 100644
--- a/fs/smb/server/tests/smbacl_kunit.c
+++ b/fs/smb/server/tests/smbacl_kunit.c
@@ -229,7 +229,7 @@ static void ksmbd_smb_check_perm_dacl_boundary_test(struct kunit *test)
goto out;
rc = smb_check_perm_dacl(NULL, &file->f_path, &daccess,
- cpu_to_le32(FILE_READ_DATA), 0, false);
+ cpu_to_le32(FILE_READ_DATA), 0, true, false);
/*
* The post-boundary ACE (ACE #2, beyond pdacl->size) grants
@@ -271,7 +271,7 @@ ksmbd_smb_check_perm_dacl_maximal_boundary_test(struct kunit *test)
goto out;
rc = smb_check_perm_dacl(NULL, &file->f_path, &daccess,
- FILE_MAXIMAL_ACCESS_LE, 0, false);
+ FILE_MAXIMAL_ACCESS_LE, 0, true, false);
KUNIT_EXPECT_EQ(test, 0, rc);
if (rc)
goto out;
--
2.47.3
^ permalink raw reply [flat|nested] 3+ messages in thread