From: Zhiyi Zhang Subject: Re: [PATCH] ntdll: Improve invalid paramater handling in NtAccessCheck. Message-Id: <7046b26c-d6f6-d105-1119-efcff908d97b@codeweavers.com> Date: Tue, 23 Apr 2019 22:01:09 +0800 In-Reply-To: <20190423133150.868-1-infyquest@gmail.com> References: <20190423133150.868-1-infyquest@gmail.com> 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;