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

[Zhiyi Zhang]

On 4/23/19 9:31 PM, Vijay Kiran Kamuju wrote:
> From: Qian Hong
>
> Signed-off-by: Qian Hong 
> Signed-off-by: Vijay Kiran Kamuju
>
> dlls/advapi32/tests/security.c | 8 --------
>  dlls/ntdll/sec.c               | 6 ++++++
>  2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
> index d9cae64da8b..d886ab713f3 100644
> --- a/dlls/advapi32/tests/security.c
> +++ b/dlls/advapi32/tests/security.c
> @@ -1454,10 +1454,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\n", PrivSetLen);
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
> @@ -1508,12 +1506,9 @@ 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(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen);
> -todo_wine
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
>
> @@ -1625,12 +1620,9 @@ 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(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen);
> - 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 02fc77dc1cc..f7f1a925770 100644
> --- a/dlls/ntdll/sec.c
> +++ b/dlls/ntdll/sec.c
> @@ -1670,6 +1670,12 @@ NtAccessCheck(
>      if (!PrivilegeSet || !ReturnLength)
>          return STATUS_ACCESS_VIOLATION;
> - if (*ReturnLength == 0)

I think testing *ReturnLength < sizeof(PRIVILEGE_SET) is more appropriate. You should add some tests to verify its
behavior, e.g., When *ReturnLength = sizeof(PRIVILEGE_SET)-1
> - {
> -
>    *ReturnLength = sizeof(PRIVILEGE_SET);
> -
>    return STATUS_BUFFER_TOO_SMALL;
> - }
> - SERVER_START_REQ( access_check )
>    {
>        struct security_descriptor sd;