ntdll: Check pointers in NtAccessCheck to prevent access violation
Nikolay Sivov
bunglehead at gmail.com
Tue Jan 13 14:02:57 CST 2009
http://bugs.winehq.org/show_bug.cgi?id=14748
Added tests show that no crash occurs on native when NULL pointers passed for
PrivilegeSet, ReturnLength or both. Check them in NtAccessCheck is enough,
last error is set correctly AccessCheck with existing code after this changes.
Changelog:
- added some tests for advapi32.AccessCheck and NtAccessCheck it relies on
- make NtAccessCheck check pointers
>From b5d6f3837f62f652ba662776218536ba1c772476 Mon Sep 17 00:00:00 2001
From: Nikolay Sivov <bunglehead at gmail.com>
Date: Tue, 13 Jan 2009 22:54:32 +0300
Subject: Check pointers in NtAccessCheck to prevent access violation
---
dlls/advapi32/tests/security.c | 97 ++++++++++++++++++++++++++++++++++++++++
dlls/ntdll/sec.c | 3 +
2 files changed, 100 insertions(+), 0 deletions(-)
diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 8682cc9..8e8728a 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -107,6 +107,8 @@ static BOOL (WINAPI *pSetSecurityDescriptorControl)(PSECURITY_DESCRIPTOR, SECURI
SECURITY_DESCRIPTOR_CONTROL);
static DWORD (WINAPI *pGetSecurityInfo)(HANDLE, SE_OBJECT_TYPE, SECURITY_INFORMATION,
PSID*, PSID*, PACL*, PACL*, PSECURITY_DESCRIPTOR*);
+static NTSTATUS (WINAPI *pNtAccessCheck)(PSECURITY_DESCRIPTOR, HANDLE, ACCESS_MASK, PGENERIC_MAPPING,
+ PPRIVILEGE_SET, PULONG, PULONG, NTSTATUS*);
static HMODULE hmod;
static int myARGC;
@@ -142,6 +144,7 @@ static void init(void)
hntdll = GetModuleHandleA("ntdll.dll");
pNtQueryObject = (void *)GetProcAddress( hntdll, "NtQueryObject" );
+ pNtAccessCheck = (void *)GetProcAddress( hntdll, "NtAccessCheck" );
hmod = GetModuleHandle("advapi32.dll");
pAddAccessAllowedAceEx = (void *)GetProcAddress(hmod, "AddAccessAllowedAceEx");
@@ -842,6 +845,7 @@ static void test_AccessCheck(void)
HMODULE NtDllModule;
BOOLEAN Enabled;
DWORD err;
+ NTSTATUS ntret, ntAccessStatus;
NtDllModule = GetModuleHandle("ntdll.dll");
if (!NtDllModule)
@@ -917,6 +921,7 @@ static void test_AccessCheck(void)
/* Generic access mask */
SetLastError(0xdeadbeef);
+ Access = AccessStatus = 0xdeadbeef;
ret = AccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
PrivSet, &PrivSetLen, &Access, &AccessStatus);
err = GetLastError();
@@ -925,7 +930,41 @@ static void test_AccessCheck(void)
ok(Access == 0xdeadbeef && AccessStatus == 0xdeadbeef,
"Access and/or AccessStatus were changed!\n");
+ /* Generic access mask - no privilegeset buffer */
+ SetLastError(0xdeadbeef);
+ Access = AccessStatus = 0xdeadbeef;
+ ret = AccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
+ NULL, &PrivSetLen, &Access, &AccessStatus);
+ err = GetLastError();
+ ok(!ret && err == ERROR_NOACCESS, "AccessCheck should have failed "
+ "with ERROR_NOACCESS, instead of %d\n", err);
+ ok(Access == 0xdeadbeef && AccessStatus == 0xdeadbeef,
+ "Access and/or AccessStatus were changed!\n");
+
+ /* Generic access mask - no returnlength */
+ SetLastError(0xdeadbeef);
+ Access = AccessStatus = 0xdeadbeef;
+ ret = AccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
+ PrivSet, NULL, &Access, &AccessStatus);
+ err = GetLastError();
+ ok(!ret && err == ERROR_NOACCESS, "AccessCheck should have failed "
+ "with ERROR_NOACCESS, instead of %d\n", err);
+ ok(Access == 0xdeadbeef && AccessStatus == 0xdeadbeef,
+ "Access and/or AccessStatus were changed!\n");
+
+ /* Generic access mask - no privilegeset buffer, no returnlength */
+ SetLastError(0xdeadbeef);
+ Access = AccessStatus = 0xdeadbeef;
+ ret = AccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
+ NULL, NULL, &Access, &AccessStatus);
+ err = GetLastError();
+ ok(!ret && err == ERROR_NOACCESS, "AccessCheck should have failed "
+ "with ERROR_NOACCESS, instead of %d\n", err);
+ ok(Access == 0xdeadbeef && AccessStatus == 0xdeadbeef,
+ "Access and/or AccessStatus were changed!\n");
+
/* sd with no dacl present */
+ Access = AccessStatus = 0xdeadbeef;
ret = SetSecurityDescriptorDacl(SecurityDescriptor, FALSE, NULL, FALSE);
ok(ret, "SetSecurityDescriptorDacl failed with error %d\n", GetLastError());
ret = AccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping,
@@ -935,7 +974,63 @@ static void test_AccessCheck(void)
"AccessCheck failed to grant access with error %d\n",
GetLastError());
+ /* sd with no dacl present - no privilegeset buffer */
+ SetLastError(0xdeadbeef);
+ Access = AccessStatus = 0xdeadbeef;
+ ret = AccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
+ NULL, &PrivSetLen, &Access, &AccessStatus);
+ err = GetLastError();
+ ok(!ret && err == ERROR_NOACCESS, "AccessCheck should have failed "
+ "with ERROR_NOACCESS, instead of %d\n", err);
+ ok(Access == 0xdeadbeef && AccessStatus == 0xdeadbeef,
+ "Access and/or AccessStatus were changed!\n");
+
+ if(pNtAccessCheck)
+ {
+ /* Generic access mask - no privilegeset buffer */
+ SetLastError(0xdeadbeef);
+ Access = ntAccessStatus = 0xdeadbeef;
+ ntret = pNtAccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
+ NULL, &PrivSetLen, &Access, &ntAccessStatus);
+ err = GetLastError();
+ ok(ntret == STATUS_ACCESS_VIOLATION,
+ "NtAccessCheck should have failed with STATUS_ACCESS_VIOLATION, got %x\n", ntret);
+ ok(err == 0xdeadbeef,
+ "NtAccessCheck shouldn't set last error, got %d\n", err);
+ ok(Access == 0xdeadbeef && ntAccessStatus == 0xdeadbeef,
+ "Access and/or AccessStatus were changed!\n");
+
+ /* Generic access mask - no returnlength */
+ SetLastError(0xdeadbeef);
+ Access = ntAccessStatus = 0xdeadbeef;
+ ntret = pNtAccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
+ PrivSet, NULL, &Access, &ntAccessStatus);
+ err = GetLastError();
+ ok(ntret == STATUS_ACCESS_VIOLATION,
+ "NtAccessCheck should have failed with STATUS_ACCESS_VIOLATION, got %x\n", ntret);
+ ok(err == 0xdeadbeef,
+ "NtAccessCheck shouldn't set last error, got %d\n", err);
+ ok(Access == 0xdeadbeef && ntAccessStatus == 0xdeadbeef,
+ "Access and/or AccessStatus were changed!\n");
+
+ /* Generic access mask - no privilegeset buffer, no returnlength */
+ SetLastError(0xdeadbeef);
+ Access = ntAccessStatus = 0xdeadbeef;
+ ntret = pNtAccessCheck(SecurityDescriptor, Token, GENERIC_READ, &Mapping,
+ NULL, NULL, &Access, &ntAccessStatus);
+ err = GetLastError();
+ ok(ntret == STATUS_ACCESS_VIOLATION,
+ "NtAccessCheck should have failed with STATUS_ACCESS_VIOLATION, got %x\n", ntret);
+ ok(err == 0xdeadbeef,
+ "NtAccessCheck shouldn't set last error, got %d\n", err);
+ ok(Access == 0xdeadbeef && ntAccessStatus == 0xdeadbeef,
+ "Access and/or AccessStatus were changed!\n");
+ }
+ else
+ win_skip("NtAccessCheck unavailable. Skipping.\n");
+
/* sd with NULL dacl */
+ Access = AccessStatus = 0xdeadbeef;
ret = SetSecurityDescriptorDacl(SecurityDescriptor, TRUE, NULL, FALSE);
ok(ret, "SetSecurityDescriptorDacl failed with error %d\n", GetLastError());
ret = AccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping,
@@ -963,6 +1058,7 @@ static void test_AccessCheck(void)
ok(res, "AddAccessDeniedAce failed with error %d\n", GetLastError());
/* sd with dacl */
+ Access = AccessStatus = 0xdeadbeef;
ret = AccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping,
PrivSet, &PrivSetLen, &Access, &AccessStatus);
ok(ret, "AccessCheck failed with error %d\n", GetLastError());
@@ -980,6 +1076,7 @@ static void test_AccessCheck(void)
/* Access denied by SD */
SetLastError(0xdeadbeef);
+ Access = AccessStatus = 0xdeadbeef;
ret = AccessCheck(SecurityDescriptor, Token, KEY_WRITE, &Mapping,
PrivSet, &PrivSetLen, &Access, &AccessStatus);
ok(ret, "AccessCheck failed with error %d\n", GetLastError());
diff --git a/dlls/ntdll/sec.c b/dlls/ntdll/sec.c
index a764e21..6a8a0ce 100644
--- a/dlls/ntdll/sec.c
+++ b/dlls/ntdll/sec.c
@@ -1562,6 +1562,9 @@ NtAccessCheck(
SecurityDescriptor, ClientToken, DesiredAccess, GenericMapping,
PrivilegeSet, ReturnLength, GrantedAccess, AccessStatus);
+ if (!PrivilegeSet || !ReturnLength)
+ return STATUS_ACCESS_VIOLATION;
+
SERVER_START_REQ( access_check )
{
struct security_descriptor sd;
--
1.5.6.5
More information about the wine-patches
mailing list