dll: FspAccessCheckEx: if the desired access includes DELETE or FILE_READ_ATTRIBUTES and access was denied, check with the parent

ext/test

@@ -1 +1 @@
-Subproject commit a101523a6e07070a0028406c9ac7ce221224a486
+Subproject commit f27a624d923f4eed68b4f2c2faa8968ba7ba2ea9

src/dll/fsop.c

@@ -25,6 +25,15 @@ NTSTATUS FspFileSystemCreateCheck(FSP_FILE_SYSTEM *FileSystem,
 {
     NTSTATUS Result;
 
+    /*
+     * CreateCheck consists of checking the parent directory for the
+     * FILE_ADD_SUBDIRECTORY or FILE_ADD_FILE rights (depending on whether
+     * we are creating a file or directory).
+     *
+     * If the access check succeeds and MAXIMUM_ALLOWED has been requested
+     * then we go ahead and grant all access to the creator.
+     */
+
     Result = FspAccessCheckEx(FileSystem, Request, TRUE, AllowTraverseCheck,
         (Request->Req.Create.CreateOptions & FILE_DIRECTORY_FILE) ?
             FILE_ADD_SUBDIRECTORY : FILE_ADD_FILE,
@@ -45,6 +54,16 @@ NTSTATUS FspFileSystemOpenCheck(FSP_FILE_SYSTEM *FileSystem,
 {
     NTSTATUS Result;
 
+    /*
+     * OpenCheck consists of checking the file for the desired access,
+     * unless FILE_DELETE_ON_CLOSE is requested in which case we also
+     * check for DELETE access.
+     *
+     * If the access check succeeds and MAXIMUM_ALLOWED was not requested
+     * then we reset the DELETE access based on whether it was actually
+     * requested in DesiredAccess.
+     */
+
     Result = FspAccessCheck(FileSystem, Request, FALSE, AllowTraverseCheck,
         Request->Req.Create.DesiredAccess |
             ((Request->Req.Create.CreateOptions & FILE_DELETE_ON_CLOSE) ? DELETE : 0),
@@ -66,6 +85,17 @@ NTSTATUS FspFileSystemOverwriteCheck(FSP_FILE_SYSTEM *FileSystem,
     NTSTATUS Result;
     BOOLEAN Supersede = FILE_SUPERSEDE == ((Request->Req.Create.CreateOptions >> 24) & 0xff);
 
+    /*
+     * OverwriteCheck consists of checking the file for the desired access,
+     * unless FILE_DELETE_ON_CLOSE is requested in which case we also
+     * check for DELETE access. Furthermore we grant DELETE or FILE_WRITE_DATA
+     * access based on whether this is a Supersede or Overwrite operation.
+     *
+     * If the access check succeeds and MAXIMUM_ALLOWED was not requested
+     * then we reset the DELETE and FILE_WRITE_DATA accesses based on whether
+     * they were actually requested in DesiredAccess.
+     */
+
     Result = FspAccessCheck(FileSystem, Request, FALSE, AllowTraverseCheck,
         Request->Req.Create.DesiredAccess |
             (Supersede ? DELETE : FILE_WRITE_DATA) |

src/dll/security.c

@@ -61,6 +61,10 @@ FSP_API NTSTATUS FspAccessCheckEx(FSP_FILE_SYSTEM *FileSystem,
     if (FspFsctlTransactCreateKind != Request->Kind)
         return STATUS_INVALID_PARAMETER;
 
+    if (CheckParentDirectory &&
+        L'\\' == ((PWSTR)Request->Buffer)[0] && L'\0' == ((PWSTR)Request->Buffer)[1])
+        return STATUS_INVALID_PARAMETER;
+
     if (0 == FileSystem->Interface->GetSecurityByName ||
         (!Request->Req.Create.UserMode && 0 == PSecurityDescriptor))
     {
@@ -78,7 +82,7 @@ FSP_API NTSTATUS FspAccessCheckEx(FSP_FILE_SYSTEM *FileSystem,
     UINT8 PrivilegeSetBuf[sizeof(PRIVILEGE_SET) + 15 * sizeof(LUID_AND_ATTRIBUTES)];
     PPRIVILEGE_SET PrivilegeSet = (PVOID)PrivilegeSetBuf;
     DWORD PrivilegeSetLength = sizeof PrivilegeSetBuf;
-    UINT32 TraverseAccess;
+    UINT32 TraverseAccess, ParentAccess, DesiredAccess2;
     BOOL AccessStatus;
 
     if (CheckParentDirectory)
@@ -147,7 +151,54 @@ FSP_API NTSTATUS FspAccessCheckEx(FSP_FILE_SYSTEM *FileSystem,
             else
                 Result = FspNtStatusFromWin32(GetLastError());
             if (!NT_SUCCESS(Result))
-                goto exit;
+            {
+                /*
+                 * If the desired access includes the DELETE or FILE_READ_ATTRIBUTES
+                 * (or MAXIMUM_ALLOWED) rights we must still check with our parent to
+                 * see if it gives us access (through the FILE_DELETE_CHILD and
+                 * FILE_LIST_DIRECTORY rights).
+                 *
+                 * Does the Windows security model suck? Ermmmm...
+                 */
+                if (STATUS_ACCESS_DENIED != Result ||
+                    0 == ((MAXIMUM_ALLOWED | DELETE | FILE_READ_ATTRIBUTES) & DesiredAccess))
+                    goto exit;
+
+                Result = FspAccessCheck(FileSystem, Request, TRUE, FALSE,
+                    (MAXIMUM_ALLOWED & DesiredAccess) ? (FILE_DELETE_CHILD | FILE_LIST_DIRECTORY) :
+                    (
+                        ((DELETE & DesiredAccess) ? FILE_DELETE_CHILD : 0) |
+                        ((FILE_READ_ATTRIBUTES & DesiredAccess) ? FILE_LIST_DIRECTORY : 0)
+                    ),
+                    &ParentAccess);
+                if (!NT_SUCCESS(Result))
+                {
+                    /* any failure just becomes ACCESS DENIED at this point */
+                    Result = STATUS_ACCESS_DENIED;
+                    goto exit;
+                }
+
+                /* redo the access check but remove the DELETE and/or FILE_READ_ATTRIBUTES rights */
+                DesiredAccess2 = DesiredAccess & ~(
+                    ((FILE_DELETE_CHILD & ParentAccess) ? DELETE : 0) |
+                    ((FILE_LIST_DIRECTORY & ParentAccess) ? FILE_READ_ATTRIBUTES : 0));
+                if (0 != DesiredAccess2)
+                {
+                    if (AccessCheck(SecurityDescriptor, (HANDLE)Request->Req.Create.AccessToken, DesiredAccess2,
+                        &FspFileGenericMapping, PrivilegeSet, &PrivilegeSetLength, PGrantedAccess, &AccessStatus))
+                        Result = AccessStatus ? STATUS_SUCCESS : STATUS_ACCESS_DENIED;
+                    else
+                        /* any failure just becomes ACCESS DENIED at this point */
+                        Result = STATUS_ACCESS_DENIED;
+                    if (!NT_SUCCESS(Result))
+                        goto exit;
+                }
+
+                if (FILE_DELETE_CHILD & ParentAccess)
+                    *PGrantedAccess |= DELETE;
+                if (FILE_LIST_DIRECTORY & ParentAccess)
+                    *PGrantedAccess |= FILE_READ_ATTRIBUTES;
+            }
         }
 
         if (CheckParentDirectory)
@@ -192,6 +243,11 @@ FSP_API NTSTATUS FspAccessCheckEx(FSP_FILE_SYSTEM *FileSystem,
         if (0 == SecurityDescriptorSize)
             *PGrantedAccess = (MAXIMUM_ALLOWED & DesiredAccess) ?
                 FspFileGenericMapping.GenericAll : DesiredAccess;
+
+        if (0 != (FileAttributes & FILE_ATTRIBUTE_READONLY) &&
+            0 != (MAXIMUM_ALLOWED & DesiredAccess))
+            *PGrantedAccess &= ~(FILE_WRITE_DATA | FILE_APPEND_DATA |
+                FILE_ADD_SUBDIRECTORY | FILE_DELETE_CHILD);
     }
     else
         *PGrantedAccess = (MAXIMUM_ALLOWED & DesiredAccess) ?

tst/winfsp-tests/info-test.c

@@ -313,14 +313,11 @@ static void delete_access_dotest(ULONG Flags, PWSTR Prefix, ULONG FileInfoTimeou
     Success = DeleteFileW(FilePath);
     ASSERT(Success);
 
-    /* enable this test when we have proper FILE_DELETE_CHILD support on the parent directory! */
+    static PWSTR Sddl = L"D:P(D;;GA;;;SY)(D;;GA;;;BA)(D;;GA;;;WD)";
-#if 0
-    static PWSTR Sddl0 = L"D:P(D;;GA;;;SY)(D;;GA;;;BA)(D;;GA;;;WD)";
-    static PWSTR Sddl1 = L"D:P(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;WD)";
     PSECURITY_DESCRIPTOR SecurityDescriptor;
     SECURITY_ATTRIBUTES SecurityAttributes = { 0 };
 
-    Success = ConvertStringSecurityDescriptorToSecurityDescriptorW(Sddl0, SDDL_REVISION_1, &SecurityDescriptor, 0);
+    Success = ConvertStringSecurityDescriptorToSecurityDescriptorW(Sddl, SDDL_REVISION_1, &SecurityDescriptor, 0);
     ASSERT(Success);
 
     SecurityAttributes.nLength = sizeof SecurityAttributes;
@@ -332,28 +329,10 @@ static void delete_access_dotest(ULONG Flags, PWSTR Prefix, ULONG FileInfoTimeou
     ASSERT(INVALID_HANDLE_VALUE != Handle);
     CloseHandle(Handle);
 
-    LocalFree(SecurityDescriptor);
-
-    Success = DeleteFileW(FilePath);
-    ASSERT(!Success);
-    ASSERT(ERROR_ACCESS_DENIED == GetLastError());
-
-    Success = ConvertStringSecurityDescriptorToSecurityDescriptorW(Sddl1, SDDL_REVISION_1, &SecurityDescriptor, 0);
-    ASSERT(Success);
-
-    Handle = CreateFileW(FilePath,
-        GENERIC_READ | GENERIC_WRITE | WRITE_DAC, FILE_SHARE_READ | FILE_SHARE_WRITE, 0,
-        OPEN_EXISTING, 0, 0);
-    ASSERT(INVALID_HANDLE_VALUE != Handle);
-    Success = SetKernelObjectSecurity(Handle, DACL_SECURITY_INFORMATION, SecurityDescriptor);
-    ASSERT(Success);
-    CloseHandle(Handle);
-
-    LocalFree(SecurityDescriptor);
-
     Success = DeleteFileW(FilePath);
     ASSERT(Success);
-#endif
+
+    LocalFree(SecurityDescriptor);
 
     memfs_stop(memfs);
 }