Zebediah Figura : ntdll: Check the output buffer length in NtAccessCheck().
Module: wine Branch: master Commit: be98f67f10c26b46acbf2fe98ea764c27efd3612 URL: https://source.winehq.org/git/wine.git/?a=commit;h=be98f67f10c26b46acbf2fe98... Author: Zebediah Figura <z.figura12(a)gmail.com> Date: Sun Feb 7 19:50:11 2021 -0600 ntdll: Check the output buffer length in NtAccessCheck(). Based on a patch by Qian Hong. Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> Signed-off-by: Alexandre Julliard <julliard(a)winehq.org> --- dlls/advapi32/tests/security.c | 20 ++++++-------------- dlls/ntdll/unix/security.c | 14 ++++++++++---- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 764e0bb5ef6..e6d2f565943 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -1368,11 +1368,11 @@ static void test_AccessCheck(void) ntret = pNtAccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping, PrivSet, &ntPrivSetLen, &Access, &ntAccessStatus); err = GetLastError(); - todo_wine ok(ntret == STATUS_BUFFER_TOO_SMALL, + ok(ntret == STATUS_BUFFER_TOO_SMALL, "NtAccessCheck should have failed with STATUS_BUFFER_TOO_SMALL, got %x\n", ntret); ok(err == 0xdeadbeef, "NtAccessCheck shouldn't set last error, got %d\n", err); - todo_wine ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed, + ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed, "Access and/or AccessStatus were changed!\n"); ok(ntPrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", ntPrivSetLen); @@ -1383,11 +1383,11 @@ static void test_AccessCheck(void) ntret = pNtAccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping, PrivSet, &ntPrivSetLen, &Access, &ntAccessStatus); err = GetLastError(); - todo_wine ok(ntret == STATUS_BUFFER_TOO_SMALL, + ok(ntret == STATUS_BUFFER_TOO_SMALL, "NtAccessCheck should have failed with STATUS_BUFFER_TOO_SMALL, got %x\n", ntret); ok(err == 0xdeadbeef, "NtAccessCheck shouldn't set last error, got %d\n", err); - todo_wine ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed, + ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed, "Access and/or AccessStatus were changed!\n"); ok(ntPrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", ntPrivSetLen); } @@ -1517,11 +1517,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); ok(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen); -todo_wine ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed, "Access and/or AccessStatus were changed!\n"); @@ -1532,11 +1530,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); ok(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen); -todo_wine ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed, "Access and/or AccessStatus were changed!\n"); @@ -1544,16 +1540,16 @@ todo_wine SetLastError(0xdeadbeef); Access = AccessStatus = 0x1abe11ed; PrivSetLen = sizeof(PRIVILEGE_SET) - 1; + PrivSet->PrivilegeCount = 0xdeadbeef; 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); ok(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen); -todo_wine ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed, "Access and/or AccessStatus were changed!\n"); + ok(PrivSet->PrivilegeCount == 0xdeadbeef, "buffer contents should not be changed\n"); /* Valid PrivSet with minimal sufficient PrivSetLen */ SetLastError(0xdeadbeef); @@ -1630,11 +1626,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); ok(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen); - todo_wine ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed, "Access and/or AccessStatus were changed!\n"); @@ -1645,11 +1639,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); 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/unix/security.c b/dlls/ntdll/unix/security.c index 0134b80e148..8838ca13ee4 100644 --- a/dlls/ntdll/unix/security.c +++ b/dlls/ntdll/unix/security.c @@ -709,11 +709,13 @@ NTSTATUS WINAPI NtAccessCheck( PSECURITY_DESCRIPTOR descr, HANDLE token, ACCESS_ data_size_t len; OBJECT_ATTRIBUTES attr; NTSTATUS status; + ULONG priv_len; TRACE( "(%p, %p, %08x, %p, %p, %p, %p, %p)\n", descr, token, access, mapping, privs, retlen, access_granted, access_status ); if (!privs || !retlen) return STATUS_ACCESS_VIOLATION; + priv_len = *retlen; /* reuse the object attribute SD marshalling */ InitializeObjectAttributes( &attr, NULL, 0, 0, descr ); @@ -728,16 +730,20 @@ NTSTATUS WINAPI NtAccessCheck( PSECURITY_DESCRIPTOR descr, HANDLE token, ACCESS_ req->mapping.exec = mapping->GenericExecute; req->mapping.all = mapping->GenericAll; wine_server_add_data( req, objattr + 1, objattr->sd_len ); - wine_server_set_reply( req, privs->Privilege, *retlen - offsetof( PRIVILEGE_SET, Privilege ) ); + wine_server_set_reply( req, privs->Privilege, priv_len - offsetof( PRIVILEGE_SET, Privilege ) ); status = wine_server_call( req ); if (status == STATUS_SUCCESS) { *retlen = max( offsetof( PRIVILEGE_SET, Privilege ) + reply->privileges_len, sizeof(PRIVILEGE_SET) ); - privs->PrivilegeCount = reply->privileges_len / sizeof(LUID_AND_ATTRIBUTES); - *access_status = reply->access_status; - *access_granted = reply->access_granted; + if (priv_len >= *retlen) + { + privs->PrivilegeCount = reply->privileges_len / sizeof(LUID_AND_ATTRIBUTES); + *access_status = reply->access_status; + *access_granted = reply->access_granted; + } + else status = STATUS_BUFFER_TOO_SMALL; } } SERVER_END_REQ;
participants (1)
-
Alexandre Julliard