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

[Vijay Kiran Kamuju]

On Wed, Apr 24, 2019 at 2:40 PM Zhiyi Zhang <zzhang at codeweavers.com> wrote:
>
>
>
> 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.
Sent in a new patch for it. It seems this check needs to be
implemented in advapi32.AccessCheck
> >>>> +    {
> >>>> +        *ReturnLength = sizeof(PRIVILEGE_SET);
> >>>> +        return STATUS_BUFFER_TOO_SMALL;
> >>>> +    }
> >>>> +
> >>>>      SERVER_START_REQ( access_check )
> >>>>      {
> >>>>          struct security_descriptor sd;
>