[PATCH 2/2] ksmbd: exclude guest and anonymous sessions from Authenticated Users ACEs

[Ian Bridges <icb@fastmail.org>]

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