[PATCH 2/6] server: Fix lacuna in server side implementation of SetFileSecurity()

[Paul Bryan Roberts]

---
 dlls/advapi32/tests/security.c |  132 ++++++++++++++++++++++++++++++++++++++++
 server/file.c                  |    2 +
 server/handle.c                |    1 +
 3 files changed, 135 insertions(+), 0 deletions(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 6d7825e..5088ee0 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -697,6 +697,100 @@ static void test_luid(void)
     test_lookupPrivilegeValue();
 }
 
+/* Used in test_FileSecurity(void) to get the Security Descriptor for a file or directory */
+static void test_FileSecurity_Get(LPCSTR name, SECURITY_INFORMATION request, LPSTR *sdString)
+{
+    DWORD sdSize;
+    DWORD retSize;
+    BYTE *sd;
+    BOOL rc;
+
+    /* First attempt - pass in size 0 to get needed size out */
+    retSize = 0;
+    SetLastError (NO_ERROR);
+    rc = pGetFileSecurityA (name, request, NULL, 0, &retSize);
+    ok (!rc, "GetFileSecurityA "
+        "was expected to fail for file '%s'\n", name);
+    ok (GetLastError() == ERROR_INSUFFICIENT_BUFFER, "GetFileSecurityA "
+        "returned %d; expected ERROR_INSUFFICIENT_BUFFER\n", GetLastError());
+    ok (retSize > sizeof (SECURITY_DESCRIPTOR), "GetFileSecurityA "
+        "returned size %d; expected > %d\n", retSize, sizeof (SECURITY_DESCRIPTOR));
+
+    sdSize = retSize;
+    sd = HeapAlloc (GetProcessHeap (), 0, sdSize);
+
+    /* Second attempt - get the security descriptor for real */
+    retSize = 0;
+    SetLastError (NO_ERROR);
+    rc = pGetFileSecurityA (name, request, sd, sdSize, &retSize);
+    ok (rc, "GetFileSecurityA "
+        "was not expected to fail for '%s'\n", name);
+    ok (GetLastError () == NO_ERROR, "GetFileSecurityA "
+        "returned %d; expected NO_ERROR\n", GetLastError ());
+    ok (retSize == sdSize, "GetFileSecurityA "
+        "returned size %d; expected %d\n", retSize, sdSize);
+
+    if (sd)
+        ok (ConvertSecurityDescriptorToStringSecurityDescriptorA
+            (sd, SDDL_REVISION_1, request, sdString, NULL),
+            "ConvertSecurityDescriptorToStringSecurityDescriptorA failed\n");
+
+    HeapFree (GetProcessHeap (), 0, sd);
+}
+
+/* Used in test_FileSecurity(void) to change the Security Descriptor for a file or directory */
+static void test_FileSecurity_Set(LPCSTR name, SECURITY_INFORMATION request, LPCSTR sdString, DWORD err)
+{
+    BOOL rc;
+    PSECURITY_DESCRIPTOR sd = NULL;
+
+    if (sdString)
+    {
+        ok (ConvertStringSecurityDescriptorToSecurityDescriptorA
+            (sdString, SDDL_REVISION_1, &sd, NULL),
+            "ConvertStringSecurityDescriptorToSecurityDescriptorA failed\n");
+
+        if (sd)
+        {
+            SetLastError (NO_ERROR);
+            rc = pSetFileSecurityA (name, request, sd);
+            if (err == NO_ERROR)
+            {
+                ok (rc, "SetFileSecurityA was not expected to fail for '%s' with '%s'\n", name, sdString);
+                ok (GetLastError () == NO_ERROR, "SetFileSecurityA returned %d; expected NO_ERROR\n", GetLastError ());
+            }
+            else
+            {
+                ok (!rc, "SetFileSecurityA was not expected to pass for '%s' with '%s'\n", name, sdString);
+                ok (GetLastError () == err, "SetFileSecurityA returned %d; expected %d\n", GetLastError (), err);
+            }
+        }
+    }
+}
+
+/* Used in test_FileSecurity(void) to put the Security Descriptor back to what is was before */
+static void test_FileSecurity_Put(LPCSTR name, SECURITY_INFORMATION request, LPSTR sdString)
+{
+    BOOL rc;
+    PSECURITY_DESCRIPTOR sd = NULL;
+
+    ok (ConvertStringSecurityDescriptorToSecurityDescriptorA
+        (sdString, SDDL_REVISION_1, &sd, NULL),
+        "ConvertStringSecurityDescriptorToSecurityDescriptorA failed\n");
+
+    if (sd)
+    {
+        SetLastError (NO_ERROR);
+        rc = pSetFileSecurityA (name, request, sd);
+        ok (rc, "SetFileSecurityA "
+            "was not expected to fail file '%s'\n", name);
+        ok (GetLastError () == NO_ERROR, "SetFileSecurityA "
+            "returned %d; expected NO_ERROR\n", GetLastError ());
+    }
+
+    LocalFree (sdString);
+}
+
 static void test_FileSecurity(void)
 {
     char wintmpdir [MAX_PATH];
@@ -706,6 +800,7 @@ static void test_FileSecurity(void)
     HANDLE fh;
     SECURITY_DESCRIPTOR toosmall;
     DWORD sdSize;
+    LPSTR refString, tstString;
     SECURITY_INFORMATION const request = OWNER_SECURITY_INFORMATION
                                        | GROUP_SECURITY_INFORMATION 
                                        | DACL_SECURITY_INFORMATION;
@@ -764,6 +859,43 @@ static void test_FileSecurity(void)
     ok (sdSize > sizeof (SECURITY_DESCRIPTOR), "GetFileSecurityA "
         "returned size %d; expected > %d\n", sdSize, sizeof (SECURITY_DESCRIPTOR));
 
+
+    /* Get security descriptor and write it straight back again */
+    refString = NULL;
+    test_FileSecurity_Get (file, request, &refString);
+    test_FileSecurity_Put (file, request, refString);
+
+    /* Try changing the owner - always fails ? */
+    refString = NULL;
+    test_FileSecurity_Get (file, request, &refString);
+    test_FileSecurity_Set (file, OWNER_SECURITY_INFORMATION, "O:CO", ERROR_INVALID_OWNER);
+    test_FileSecurity_Put (file, request, refString);
+
+    /* Try changing the group - always succeeds ? */
+    refString = NULL;
+    test_FileSecurity_Get (file, request, &refString);
+    test_FileSecurity_Set (file, GROUP_SECURITY_INFORMATION, "G:CG", NO_ERROR);
+    test_FileSecurity_Put (file, request, refString);
+
+    /* Try changing the read/write permissions for everyone */
+    refString = NULL;
+    test_FileSecurity_Get (file, request, &refString);
+
+    test_FileSecurity_Get (file, DACL_SECURITY_INFORMATION, &tstString);
+    if (tstString)
+    {
+        LPSTR sp = tstString;
+
+        while (*sp && *sp++ != 'F');
+
+        for ( ; *sp; sp++)
+            if (sp[0] == 'F' && sp[2] == ';')
+                sp[1] = (sp[1] == 'R') ? 'W' : 'R';
+    }
+    test_FileSecurity_Set (file, DACL_SECURITY_INFORMATION, tstString, NO_ERROR);
+
+    test_FileSecurity_Put (file, request, refString);
+
     /* Remove temporary file and directory */
     DeleteFileA (file);
     RemoveDirectoryA (path);
diff --git a/server/file.c b/server/file.c
index 5224e85..fb1035e 100644
--- a/server/file.c
+++ b/server/file.c
@@ -550,6 +550,8 @@ static int file_set_sd( struct object *obj, const struct security_descriptor *sd
         if (!obj->sd || !security_equal_sid( owner, sd_get_owner( obj->sd ) ))
         {
             /* FIXME: get Unix uid and call fchown */
+            set_error( STATUS_INVALID_OWNER );
+            return 0;
         }
     }
     else if (obj->sd)
diff --git a/server/handle.c b/server/handle.c
index 29214d6..878ed95 100644
--- a/server/handle.c
+++ b/server/handle.c
@@ -636,6 +636,7 @@ DECL_HANDLER(set_security_object)
 
     if (!(obj = get_handle_obj( current->process, req->handle, access, NULL ))) return;
 
+    obj->ops->get_sd( obj );
     obj->ops->set_sd( obj, sd, req->security_info );
     release_object( obj );
 }
-- 
1.5.4.3


--------------040806060201030006030005--