diff --git a/ext/test b/ext/test index a101523a..f27a624d 160000 --- a/ext/test +++ b/ext/test @@ -1 +1 @@ -Subproject commit a101523a6e07070a0028406c9ac7ce221224a486 +Subproject commit f27a624d923f4eed68b4f2c2faa8968ba7ba2ea9 diff --git a/src/dll/fsop.c b/src/dll/fsop.c index 7b7f82e4..de5903ec 100644 --- a/src/dll/fsop.c +++ b/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) | diff --git a/src/dll/security.c b/src/dll/security.c index 413d2fad..f74768bc 100644 --- a/src/dll/security.c +++ b/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) ? diff --git a/tst/winfsp-tests/info-test.c b/tst/winfsp-tests/info-test.c index 6113002b..90c69f16 100644 --- a/tst/winfsp-tests/info-test.c +++ b/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! */ -#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)"; + static PWSTR Sddl = L"D:P(D;;GA;;;SY)(D;;GA;;;BA)(D;;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); }