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

[Zhiyi Zhang]

On 4/24/19 8:21 PM, Vijay Kiran Kamuju wrote:
> On Tue, Apr 23, 2019 at 4:10 PM Vijay Kiran Kamuju <infyquest at gmail.com> wrote:
>> On Tue, Apr 23, 2019 at 4:01 PM Zhiyi Zhang <zzhang at codeweavers.com> wrote:
>>>
>>>
>>> On 4/23/19 9:31 PM, Vijay Kiran Kamuju wrote:
>>>> From: Qian Hong <qhong at codeweavers.com>
>>>>
>>>> Signed-off-by: Qian Hong <qhong at codeweavers.com>
>>>> Signed-off-by: Vijay Kiran Kamuju <infyquest at gmail.com>
>>>> ---
>>>>  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
>> I will check this behavior tomorrow with tests, I was trying to
>> understand the tests today.
>> Sent a wrong test patch earlier :)
> I have checked, there are already tests for that. If we add the check
> you requested, it fails the tests for advapi32AccessCheck functions.
> It seems the advapi32.AccessCheck and ntdll.NtAccessCheck work differently.
As you said it seems advapi32.AccessCheck and ntdll.NtAccessCheck work differently. The tests you mention that checked size
are for advapi32.AccessCheck, not for NtAccessCheck. You should at least test sizeof(PRIVILEGE_SET)-1 for NtAccessCheck,
just in case.

>>>> +    {
>>>> +        *ReturnLength = sizeof(PRIVILEGE_SET);
>>>> +        return STATUS_BUFFER_TOO_SMALL;
>>>> +    }
>>>> +
>>>>      SERVER_START_REQ( access_check )
>>>>      {
>>>>          struct security_descriptor sd;