From: Zhiyi Zhang Subject: Re: [PATCH] ntdll: Improve invalid paramater handling in NtAccessCheck. Message-Id: <4497211b-9908-d700-a13c-476e7c373485@codeweavers.com> Date: Wed, 24 Apr 2019 20:40:30 +0800 In-Reply-To: References: <20190423133150.868-1-infyquest@gmail.com> <7046b26c-d6f6-d105-1119-efcff908d97b@codeweavers.com> On 4/24/19 8:21 PM, Vijay Kiran Kamuju wrote: > 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. 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;