From: Vijay Kiran Kamuju Subject: Re: [PATCH] ntdll: Improve invalid paramater handling in NtAccessCheck. Message-Id: Date: Wed, 24 Apr 2019 14:21:07 +0200 In-Reply-To: References: <20190423133150.868-1-infyquest@gmail.com> <7046b26c-d6f6-d105-1119-efcff908d97b@codeweavers.com> On Tue, Apr 23, 2019 at 4:10 PM Vijay Kiran Kamuju wrote: > > On Tue, Apr 23, 2019 at 4:01 PM Zhiyi Zhang wrote: > > > > > > > > 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 > 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; > >