Re: [PATCH v2 1/2] ksmbd: bound smb_check_perm_dacl() ACE walks by DACL size

[ChenXiaoSong]

Reviewed-by: ChenXiaoSong <chenxiaosong@xxxxxxxxxx>

On 8/12/26 18:39, Hang Nan wrote:
> smb_check_perm_dacl() validates that the DACL fits inside the NT
> security descriptor, but then bounds its two ACE walks by the
> remaining NTSD length (acl_size) rather than the DACL's declared
> size (pdacl_size).
>
> When pdacl->size is smaller than the trailing NTSD buffer, bytes
> after the declared DACL boundary - still inside the stored security
> descriptor - are parsed as ACEs during access checks.  A crafted
> DACL can place an access-granting ACE beyond pdacl->size, and the
> current code accepts it during SMB2_CREATE access validation, while
> parse_dacl() and smb_inherit_dacl() stop at pdacl_size.
>
> Bound both ACE walks by pdacl_size to match the DACL boundary
> semantics used elsewhere in the server.
>
> Validation (KUnit, UML, x86_64, KASAN; first KUnit tests for
> fs/smb/server, see follow-up patch):
> - ksmbd_dacl_walk_must_stop_at_declared_size (semantic harness):
>    with the current code the ACE placed after pdacl->size is
>    selected and access is granted; with the fix the same sample
>    is denied (EACCES).
> - ksmbd_smb_check_perm_dacl_boundary (drives the real function):
>    passes with the fix; fails with the fix reverted (rc == 0).
> - ksmbd-for-next-next (rebased, base e9d76059ff03), x86_64
>
> Fixes: 8f0541186e9a ("ksmbd: fix heap-based overflow in set_ntacl_dacl()")
> Signed-off-by: Hang Nan<2122295973@xxxxxx>

--
ChenXiaoSong <chenxiaosong@xxxxxxxxxxxxxxxx>
Chinese Homepage: https://chenxiaosong.com
English Homepage: https://chenxiaosong.com/en