Re: [PATCH 2/2] ntdll: Improve invalid paramater handling in NtAccessCheck.

[Sebastian Lackner]

On 15.02.2016 21:53, Qian Hong wrote:
> Signed-off-by: Qian Hong
>
> dlls/advapi32/tests/security.c |  4 ----
>  dlls/ntdll/sec.c               | 11 ++++++++++-
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> 0002-ntdll-Improve-invalid-paramater-handling-in-NtAccessCh.txt
>
> diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
> index 69c0f77..08666dd 100644
> --- a/dlls/advapi32/tests/security.c
> +++ b/dlls/advapi32/tests/security.c
> @@ -1429,10 +1429,8 @@ static void test_AccessCheck(void)
>      ret = AccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping,
>                        0, &PrivSetLen, &Access, &AccessStatus);
>      err = GetLastError();
> -todo_wine
>      ok(!ret && err == ERROR_INSUFFICIENT_BUFFER, "AccessCheck should have "
>         "failed with ERROR_INSUFFICIENT_BUFFER, instead of %d\n", err);
> -todo_wine
>      ok(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d, expects %d\n", PrivSetLen, sizeof(PRIVILEGE_SET));
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
> @@ -1444,10 +1442,8 @@ todo_wine
>      ret = AccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping,
>                        PrivSet, &PrivSetLen, &Access, &AccessStatus);
>      err = GetLastError();
> -todo_wine
>      ok(!ret && err == ERROR_INSUFFICIENT_BUFFER, "AccessCheck should have "
>         "failed with ERROR_INSUFFICIENT_BUFFER, instead of %d\n", err);
> -todo_wine
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
>
> diff --git a/dlls/ntdll/sec.c b/dlls/ntdll/sec.c
> index 125c86e..c32ae0c 100644
> --- a/dlls/ntdll/sec.c
> +++ b/dlls/ntdll/sec.c
> @@ -1586,7 +1586,16 @@ NtAccessCheck(
>          SecurityDescriptor, ClientToken, DesiredAccess, GenericMapping,
>          PrivilegeSet, ReturnLength, GrantedAccess, AccessStatus);
> - if (!PrivilegeSet || !ReturnLength)
> - if (!ReturnLength)
> -
>    return STATUS_ACCESS_VIOLATION;
> -
> - if (*ReturnLength == 0)
> - {
> -
>    *ReturnLength = sizeof(PRIVILEGE_SET);
> -
>    return STATUS_BUFFER_TOO_SMALL;
> - }

This looks a bit hacky. The code below assumes that *ReturnLength > FIELD_OFFSET( PRIVILEGE_SET, Privilege ),
so it would be interesting to know what happens for sizes 0 ... 8.
> -
> -
>
> if (!PrivilegeSet)
>     return STATUS_ACCESS_VIOLATION;
>
> SERVER_START_REQ( access_check )

[PATCH 2/2] ntdll: Improve invalid paramater handling in NtAccessCheck.

[Nikolay Sivov]

On 16.02.2016 0:10, Sebastian Lackner wrote:
> > diff --git a/dlls/ntdll/sec.c b/dlls/ntdll/sec.c
> > index 125c86e..c32ae0c 100644
> > --- a/dlls/ntdll/sec.c
> > +++ b/dlls/ntdll/sec.c
> > @@ -1586,7 +1586,16 @@ NtAccessCheck(
> >          SecurityDescriptor, ClientToken, DesiredAccess, GenericMapping,
> >          PrivilegeSet, ReturnLength, GrantedAccess, AccessStatus);
> > - if (!PrivilegeSet || !ReturnLength)
> > - if (!ReturnLength)
> > -
> >    return STATUS_ACCESS_VIOLATION;
> > -
> > - if (*ReturnLength == 0)
> > - {
> > -
> >    *ReturnLength = sizeof(PRIVILEGE_SET);
> > -
> >    return STATUS_BUFFER_TOO_SMALL;
> > - }
>
> This looks a bit hacky. The code below assumes that *ReturnLength > FIELD_OFFSET( PRIVILEGE_SET, Privilege ),
> so it would be interesting to know what happens for sizes 0 ... 8.
> > -
> > -
> >
> > if (!PrivilegeSet)
> >     return STATUS_ACCESS_VIOLATION;
> >
> > SERVER_START_REQ( access_check )

Also it would be interesting to have same tests that call NtAccessCheck
directly.

[PATCH 2/2] ntdll: Improve invalid paramater handling in NtAccessCheck.

[Qian Hong]

On Tue, Feb 16, 2016 at 5:14 AM, Nikolay Sivov  wrote:
> Also it would be interesting to have same tests that call NtAccessCheck
> directly.

Thanks, I added NtAccessCheck in [Patch 3/3]. It would be too
complicate to rewrite the test in ntdll/tests, so I added them into
advapi32/tests.