[PATCH 1/2] ksmbd: evaluate the DACL in order for requested access

[Ian Bridges <icb@fastmail.org>]

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