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

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

[Ian Bridges]

smb2_open() runs the NT ACL permission check, smb_check_perm_dacl(), for
an existing file only when FILE_DELETE_ON_CLOSE is not requested.

On an acl_xattr share the stored security descriptor is the authoritative
permission model. set_info_sec() maps a descriptor into POSIX ACLs and
mode, so its read and write restrictions are also checked by
inode_permission() when smb2_open() opens the file. A file's DELETE right
cannot be expressed in a POSIX ACL, because Unix delete is governed by the
parent directory's write permission and not by the file's own ACL. So
only smb_check_perm_dacl() can check it.

Because delete-on-close opens skip that call, a client can open an
existing file with DesiredAccess FILE_READ_ATTRIBUTES | DELETE and
CreateOptions FILE_DELETE_ON_CLOSE and have the file unlinked on close,
even when the descriptor grants the caller no access at all. Only the
POSIX checks remain, read on the file and write on its parent directory.
The normal open-for-DELETE path is denied by smb_check_perm_dacl(), so
this lets a user delete files that a normal delete request could not.

Run the DACL check for every existing-file open, so a delete-on-close
open gets the same DACL check as any other open of an existing file. A
caller the descriptor does not grant access to is denied. An authorized
caller is unaffected, and shares without a stored descriptor are
unchanged.

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" by an authenticated
    user who passes the POSIX checks for read on the file and write on the
    parent directory, for example a shared drop-box directory where many
    users can write the parent and individual files carry restrictive ACLs.
    Impact is unauthorized file deletion. It is not a privilege escalation
    and does not expose file contents.
    
    Reproduced on ksmbd-for-next 206fc30f71b0. tester2 creates a file and
    sets a DACL granting only tester2. tester's open(DELETE) fails with
    STATUS_ACCESS_DENIED, but open(FILE_READ_ATTRIBUTES | DELETE) with
    FILE_DELETE_ON_CLOSE succeeds and the file is unlinked on close. With
    this change that open returns STATUS_ACCESS_DENIED and the file
    survives. A file's owner can still delete it with delete-on-close when
    the DACL grants them access, and shares without a stored descriptor are
    unaffected. A reproducer is available on request.
    
    This fix was developed with LLM assistance.

 fs/smb/server/smb2pdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index be226664b8af..b664a02057eb 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -4836,7 +4836,7 @@ 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) {
 		rc = smb_check_perm_dacl(conn, &path, &daccess,
 					 req->DesiredAccess,
 					 sess->user->uid, false);

base-commit: 206fc30f71b005156085fd8523ccc84a2c67791b
-- 
2.47.3