stop over-reporting world-writable directories (#6936)

Fix world-writable audit false positives by expanding generic
permissions with MapGenericMask and then checking only concrete write
bits. The earlier check looked for FILE_GENERIC_WRITE/generic masks
directly, which shares bits with read permissions and could flag an
Everyone read ACE as writable.

Author: iceweasel-oai

codex-rs/windows-sandbox-rs/src/audit.rs

@@ -9,15 +9,29 @@ use std::path::PathBuf;
 use std::time::Duration;
 use std::time::Instant;
 use windows_sys::Win32::Foundation::CloseHandle;
-use windows_sys::Win32::Foundation::LocalFree;
 use windows_sys::Win32::Foundation::ERROR_SUCCESS;
 use windows_sys::Win32::Foundation::HLOCAL;
 use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE;
+use windows_sys::Win32::Foundation::LocalFree;
+use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE;
+use windows_sys::Win32::Security::ACE_HEADER;
+use windows_sys::Win32::Security::ACL;
+use windows_sys::Win32::Security::ACL_SIZE_INFORMATION;
+use windows_sys::Win32::Security::AclSizeInformation;
 use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW;
 use windows_sys::Win32::Security::Authorization::GetSecurityInfo;
+use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION;
+use windows_sys::Win32::Security::EqualSid;
+use windows_sys::Win32::Security::GetAce;
+use windows_sys::Win32::Security::GetAclInformation;
+use windows_sys::Win32::Security::MapGenericMask;
+use windows_sys::Win32::Security::GENERIC_MAPPING;
 use windows_sys::Win32::Storage::FileSystem::CreateFileW;
+use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS;
 use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA;
 use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS;
+use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE;
+use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ;
 use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE;
 use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE;
 use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ;
@@ -26,17 +40,6 @@ use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA;
 use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING;
-const GENERIC_ALL_MASK: u32 = 0x1000_0000;
-const GENERIC_WRITE_MASK: u32 = 0x4000_0000;
-use windows_sys::Win32::Security::AclSizeInformation;
-use windows_sys::Win32::Security::EqualSid;
-use windows_sys::Win32::Security::GetAce;
-use windows_sys::Win32::Security::GetAclInformation;
-use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE;
-use windows_sys::Win32::Security::ACE_HEADER;
-use windows_sys::Win32::Security::ACL;
-use windows_sys::Win32::Security::ACL_SIZE_INFORMATION;
-use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION;
 
 // Preflight scan limits
 const MAX_ITEMS_PER_DIR: i32 = 1000;
@@ -304,7 +307,7 @@ pub fn world_writable_warning_details(
     }
 }
 // Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for
-// Everyone that includes generic or specific write bits? Skips inherit-only
+// Everyone that grants write after generic bits are expanded? Skips inherit-only
 // ACEs (do not apply to the current object).
 unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut c_void) -> bool {
     if p_dacl.is_null() {
@@ -321,6 +324,12 @@ unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut
     if ok == 0 {
         return false;
     }
+    let mapping = GENERIC_MAPPING {
+        GenericRead: FILE_GENERIC_READ,
+        GenericWrite: FILE_GENERIC_WRITE,
+        GenericExecute: FILE_GENERIC_EXECUTE,
+        GenericAll: FILE_ALL_ACCESS,
+    };
     for i in 0..(info.AceCount as usize) {
         let mut p_ace: *mut c_void = std::ptr::null_mut();
         if GetAce(p_dacl as *const ACL, i as u32, &mut p_ace) == 0 {
@@ -337,19 +346,16 @@ unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut
         let base = p_ace as usize;
         let sid_ptr =
             (base + std::mem::size_of::<ACE_HEADER>() + std::mem::size_of::<u32>()) as *mut c_void; // skip header + mask
-        if EqualSid(sid_ptr, psid_world) != 0 {
-            let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE);
-            let mask = ace.Mask;
-            let writey = FILE_GENERIC_WRITE
-                | FILE_WRITE_DATA
-                | FILE_APPEND_DATA
-                | FILE_WRITE_EA
-                | FILE_WRITE_ATTRIBUTES
-                | GENERIC_WRITE_MASK
-                | GENERIC_ALL_MASK;
-            if (mask & writey) != 0 {
-                return true;
-            }
+        if EqualSid(sid_ptr, psid_world) == 0 {
+            continue;
+        }
+        let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE);
+        let mut mask = ace.Mask;
+        // Expand generic bits to concrete file rights before checking for write.
+        MapGenericMask(&mut mask, &mapping);
+        let write_mask = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES;
+        if (mask & write_mask) != 0 {
+            return true;
         }
     }
     false