[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

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

[Namjae Jeon]

On Thu, Oct 29, 2026 at 12:35 AM Ian Bridges <icb@fastmail.org> wrote:
>
> 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>
This patch causes smb2.acls.INHERITANCE failure in smbtorture test.
Can you check it ?

failure: INHERITANCE [
(../../source4/torture/smb2/acls.c:1054) Incorrect status
NT_STATUS_OBJECT_NAME_COLLISION - should be NT_STATUS_OK
]

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

[Ian Bridges]

On Thu, Oct 29, 2026 at 10:38 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
> This patch causes smb2.acls.INHERITANCE failure in smbtorture test.
> Can you check it ?
>
> failure: INHERITANCE [
> (../../source4/torture/smb2/acls.c:1054) Incorrect status
> NT_STATUS_OBJECT_NAME_COLLISION - should be NT_STATUS_OK
> ]

Thanks for running this. I can reproduce it with smbtorture 4.22, and it
passes without this patch.

The inheritance checks themselves aren't what fails. It's the rmdir at
the end of the i == 1 iteration, which the test doesn't check, so it
shows up as a collision when the next iteration creates the same
directory.

In that iteration the parent directory's DACL is

  ALLOW CREATOR OWNER  WRITE_DATA   (object inherit)
  ALLOW Everyone       full access  (no inheritance)

so the new subdirectory ends up with no ACE that grants DELETE. Windows
and Samba still let you delete it, because the parent grants
FILE_DELETE_CHILD. ksmbd doesn't have that rule, so even a plain DELETE
open of that subdirectory fails today. Before this patch, the
delete-on-close open that smb2_util_rmdir() uses skipped the DACL check
and got through anyway. This patch sends that open through the DACL
check, and the check denies it.

So the test is right. What's missing is the FILE_DELETE_CHILD rule.
Reverting this patch would just hide that again, and reopen the
delete-on-close bypass.

I have a fix that treats FILE_DELETE_CHILD on the parent as a second way
to get DELETE, like Samba's parent_override_delete(). It only uses a
parent's stored DACL, and never looks above the share root. With it,
INHERITANCE passes, the rest of smb2.acls and smb2.delete-on-close-perms
are unchanged, and the cases this patch blocks stay blocked.

Would you like the fix as a separate patch on top of this one? Or, if
you'd rather not keep this one in ksmbd-for-next for now, I can resend
both as a two-patch series, fix first, so they reach stable together.

Thanks,
Ian

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

[Namjae Jeon]

On Thu, Oct 29, 2026 at 11:20 AM Ian Bridges <icb@fastmail.org> wrote:
>
> On Thu, Oct 29, 2026 at 10:38 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
> > This patch causes smb2.acls.INHERITANCE failure in smbtorture test.
> > Can you check it ?
> >
> > failure: INHERITANCE [
> > (../../source4/torture/smb2/acls.c:1054) Incorrect status
> > NT_STATUS_OBJECT_NAME_COLLISION - should be NT_STATUS_OK
> > ]
>
> Thanks for running this. I can reproduce it with smbtorture 4.22, and it
> passes without this patch.
>
> The inheritance checks themselves aren't what fails. It's the rmdir at
> the end of the i == 1 iteration, which the test doesn't check, so it
> shows up as a collision when the next iteration creates the same
> directory.
>
> In that iteration the parent directory's DACL is
>
>   ALLOW CREATOR OWNER  WRITE_DATA   (object inherit)
>   ALLOW Everyone       full access  (no inheritance)
>
> so the new subdirectory ends up with no ACE that grants DELETE. Windows
> and Samba still let you delete it, because the parent grants
> FILE_DELETE_CHILD. ksmbd doesn't have that rule, so even a plain DELETE
> open of that subdirectory fails today. Before this patch, the
> delete-on-close open that smb2_util_rmdir() uses skipped the DACL check
> and got through anyway. This patch sends that open through the DACL
> check, and the check denies it.
>
> So the test is right. What's missing is the FILE_DELETE_CHILD rule.
> Reverting this patch would just hide that again, and reopen the
> delete-on-close bypass.
>
> I have a fix that treats FILE_DELETE_CHILD on the parent as a second way
> to get DELETE, like Samba's parent_override_delete(). It only uses a
> parent's stored DACL, and never looks above the share root. With it,
> INHERITANCE passes, the rest of smb2.acls and smb2.delete-on-close-perms
> are unchanged, and the cases this patch blocks stay blocked.
>
> Would you like the fix as a separate patch on top of this one? Or, if
> you'd rather not keep this one in ksmbd-for-next for now, I can resend
> both as a two-patch series, fix first, so they reach stable together.
I will revert it in ksmbd-for-next. You can send v2 patch to the list and me.
Thanks!
>
> Thanks,
> Ian