[PATCH 1/3] ntdll: Always return at least sizeof(PRIVILEGE_SET) in NtAccessCheck().

[Zebediah Figura]

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/advapi32/tests/security.c | 10 ++--------
 dlls/ntdll/unix/security.c     |  2 +-
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index bd57609447e..e6078996e19 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -1367,7 +1367,7 @@ static void test_AccessCheck(void)
          "NtAccessCheck shouldn't set last error, got %d\n", err);
       todo_wine ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed,
          "Access and/or AccessStatus were changed!\n");
-      todo_wine ok(ntPrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", ntPrivSetLen);
+      ok(ntPrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", ntPrivSetLen);
 
       /* Key access mask - insufficient returnlength */
       SetLastError(0xdeadbeef);
@@ -1382,7 +1382,7 @@ static void test_AccessCheck(void)
          "NtAccessCheck shouldn't set last error, got %d\n", err);
       todo_wine ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed,
          "Access and/or AccessStatus were changed!\n");
-      todo_wine ok(ntPrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", ntPrivSetLen);
+      ok(ntPrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", ntPrivSetLen);
     }
     else
        win_skip("NtAccessCheck unavailable. Skipping.\n");
@@ -1513,7 +1513,6 @@ todo_wine
 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,
@@ -1529,7 +1528,6 @@ todo_wine
 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,
@@ -1545,7 +1543,6 @@ todo_wine
 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,
@@ -1560,7 +1557,6 @@ todo_wine
                       PrivSet, &PrivSetLen, &Access, &AccessStatus);
     err = GetLastError();
     ok(ret, "AccessCheck failed with error %d\n", GetLastError());
-todo_wine
     ok(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen);
     ok(AccessStatus && (Access == KEY_READ),
         "AccessCheck failed to grant access with error %d\n", GetLastError());
@@ -1630,7 +1626,6 @@ todo_wine
     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,
@@ -1646,7 +1641,6 @@ todo_wine
     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,
diff --git a/dlls/ntdll/unix/security.c b/dlls/ntdll/unix/security.c
index 6f945b24e68..8a5b1515ba9 100644
--- a/dlls/ntdll/unix/security.c
+++ b/dlls/ntdll/unix/security.c
@@ -732,7 +732,7 @@ NTSTATUS WINAPI NtAccessCheck( PSECURITY_DESCRIPTOR descr, HANDLE token, ACCESS_
 
         status = wine_server_call( req );
 
-        *retlen = offsetof( PRIVILEGE_SET, Privilege ) + reply->privileges_len;
+        *retlen = max( offsetof( PRIVILEGE_SET, Privilege ) + reply->privileges_len, sizeof(PRIVILEGE_SET) );
         privs->PrivilegeCount = reply->privileges_len / sizeof(LUID_AND_ATTRIBUTES);
         if (status == STATUS_SUCCESS)
         {
-- 
2.20.1

[PATCH 2/3] ntdll: Do not touch the privilege output buffers if access_check fails.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 dlls/advapi32/tests/security.c | 4 ++--
 dlls/ntdll/unix/security.c     | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index e6078996e19..f6c37bdd738 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -1337,7 +1337,7 @@ static void test_AccessCheck(void)
          "NtAccessCheck shouldn't set last error, got %d\n", err);
       ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed,
          "Access and/or AccessStatus were changed!\n");
-      todo_wine ok(ntPrivSetLen == 0, "PrivSetLen returns %d\n", ntPrivSetLen);
+      ok(ntPrivSetLen == 0, "PrivSetLen returns %d\n", ntPrivSetLen);
 
       /* Generic access mask - insufficient returnlength */
       SetLastError(0xdeadbeef);
@@ -1352,7 +1352,7 @@ static void test_AccessCheck(void)
          "NtAccessCheck shouldn't set last error, got %d\n", err);
       ok(Access == 0x1abe11ed && ntAccessStatus == 0x1abe11ed,
          "Access and/or AccessStatus were changed!\n");
-      todo_wine ok(ntPrivSetLen == sizeof(PRIVILEGE_SET)-1, "PrivSetLen returns %d\n", ntPrivSetLen);
+      ok(ntPrivSetLen == sizeof(PRIVILEGE_SET)-1, "PrivSetLen returns %d\n", ntPrivSetLen);
 
       /* Key access mask - zero returnlength */
       SetLastError(0xdeadbeef);
diff --git a/dlls/ntdll/unix/security.c b/dlls/ntdll/unix/security.c
index 8a5b1515ba9..0134b80e148 100644
--- a/dlls/ntdll/unix/security.c
+++ b/dlls/ntdll/unix/security.c
@@ -732,10 +732,10 @@ NTSTATUS WINAPI NtAccessCheck( PSECURITY_DESCRIPTOR descr, HANDLE token, ACCESS_
 
         status = wine_server_call( req );
 
-        *retlen = max( offsetof( PRIVILEGE_SET, Privilege ) + reply->privileges_len, sizeof(PRIVILEGE_SET) );
-        privs->PrivilegeCount = reply->privileges_len / sizeof(LUID_AND_ATTRIBUTES);
         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;
         }
-- 
2.20.1

[PATCH 3/3] ntdll: Check the output buffer length in NtAccessCheck().

[Zebediah Figura]

Based on a patch by Qian Hong.

Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com>
---
 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 f6c37bdd738..991f57f1fc7 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -1361,11 +1361,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);
 
@@ -1376,11 +1376,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);
     }
@@ -1510,11 +1510,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");
 
@@ -1525,11 +1523,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");
 
@@ -1537,16 +1533,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);
@@ -1623,11 +1619,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");
 
@@ -1638,11 +1632,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;
-- 
2.20.1