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

[Vijay Kiran Kamuju]

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.
> >
> > > +    {
> > > +        *ReturnLength = sizeof(PRIVILEGE_SET);
> > > +        return STATUS_BUFFER_TOO_SMALL;
> > > +    }
> > > +
> > >      SERVER_START_REQ( access_check )
> > >      {
> > >          struct security_descriptor sd;
> >