Windows: deny-read cleanup uses REVOKE_ACCESS, which can succeed while retaining deny ACEs

Summary

The rust-v0.153.4 deny-read reconciliation path uses revoke_ace, which constructs a
replacement ACL with REVOKE_ACCESS. A native in-memory reproduction shows that this
operation returns success while preserving denied ACEs. The reconciler can then
remove the path from its tracking state without having removed its deny entry.

This is independently reproducible at the Windows API level without changing any
file's permissions or running Codex. It is more specific than merely ignoring an
API error: no failing return code is required.

Minimal native reproduction

The accompanying cleanup-repro.ps1 builds three ACLs entirely in memory, uses an
invented SID, and calls SetEntriesInAclW with the fields used by tagged revoke_ace:

grfAccessPermissions = 0
grfAccessMode = 4  // REVOKE_ACCESS
grfInheritance = 3  // OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE
TrusteeForm = TRUSTEE_IS_SID
TrusteeType = TRUSTEE_IS_UNKNOWN

Run in Windows PowerShell/PowerShell with Add-Type and these Win32 APIs available:

& .\cleanup-repro.ps1

The attachment was executed on native Windows and its three results were checked
against the earlier in-memory command. Process exit was 0. Full observed output is
included as cleanup-repro-output.json:

All entries used FILE_GENERIC_READ (1179785) and object/container inheritance.
The script never calls SetNamedSecurityInfo, touches a real ACL, resolves an account,
or changes Codex state. It frees all unmanaged buffers.

Source and documented behavior

Microsoft ACCESS_MODE
documents REVOKE_ACCESS in terms of removing allowed/audit ACEs, not denied ACEs.
SetEntriesInAclW
returns the newly constructed ACL and reports whether that operation succeeded.

Tagged acl.rs / revoke_ace
uses mode 4 and subsequently attempts to write that replacement ACL. It also returns
no error to its caller and ignores some API failures; that is an additional concern.
deny_read_state.rs
invokes revoke_ace for stale paths, then updates/saves tracking without verifying removal.
The setup helper
uses reconciliation for the sandbox-users group, including empty desired lists.

Corroborating Codex observation

With CLI version reporting 0.153.4, a fresh normal-user-owned fixture and sequential
app-server command/exec requests gave:

The server was initialized with experimentalApi enabled. Both command-local profiles
disabled network; they differed only in the private path's read/deny rule. Default
profile was the allow control. No AI thread or turn was created.
All commands exited 0 with empty stderr; the probe reported read outcomes separately.
The server shut down cleanly. Group deny entries remained on the synthetic private
directory and child marker. A later tracker snapshot contained zero principals.

Persistence immediately after exit is intentional for surviving descendants and is
not itself the reported defect. The concern is failed reconciliation on the later
allow request. The current state snapshot was taken after other setup activity and
cannot reconstruct per-request tracking. Installed helper/source equivalence remains
unverified; these observations corroborate rather than conclusively trace the API mismatch.

Requested investigation

Please consider removing only the intended stale deny entries explicitly, preserving
unrelated ACL entries, propagating API failures, and verifying removal before dropping
tracking state. A regression test should cover allow/deny/allow and the three memory
cases above. No patch is proposed or validated by this report; inheritance behavior
and surviving-process implications also need maintainer review.

Attachment: cleanup-repro.ps1

# Native API reproduction. In-memory ACLs only; no filesystem security setters.
$ErrorActionPreference = 'Stop'
if (-not ('B01DraftMemoryAcl' -as [type])) {
Add-Type @'
using System;
using System.Runtime.InteropServices;
public static class B01DraftMemoryAcl {
 [StructLayout(LayoutKind.Sequential)] public struct Trustee { public IntPtr Multiple; public int Operation; public int Form; public int Type; public IntPtr Name; }
 [StructLayout(LayoutKind.Sequential)] public struct Explicit { public uint Permissions; public int Mode; public uint Inheritance; public Trustee Trustee; }
 [DllImport("advapi32.dll", CharSet=CharSet.Unicode)] public static extern uint SetEntriesInAclW(uint count, ref Explicit entry, IntPtr oldAcl, out IntPtr newAcl);
 [DllImport("kernel32.dll")] public static extern IntPtr LocalFree(IntPtr memory);
}
'@
}
$syntheticSid = 'S-1-5-21-111111111-222222222-333333333-1234'
$sid = [Security.Principal.SecurityIdentifier]::new($syntheticSid)
$sidBytes = [byte[]]::new($sid.BinaryLength)
$sid.GetBinaryForm($sidBytes, 0)
$sidPtr = [Runtime.InteropServices.Marshal]::AllocHGlobal($sidBytes.Length)
try {
 [Runtime.InteropServices.Marshal]::Copy($sidBytes, 0, $sidPtr, $sidBytes.Length)
 $cases = @(
  [pscustomobject]@{Name='allow-only'; SDDL="D:(A;OICI;FR;;;$syntheticSid)"},
  [pscustomobject]@{Name='deny-only'; SDDL="D:(D;OICI;FR;;;$syntheticSid)"},
  [pscustomobject]@{Name='deny-and-allow'; SDDL="D:(D;OICI;FR;;;$syntheticSid)(A;OICI;FR;;;$syntheticSid)"}
 )
 $results = foreach ($case in $cases) {
  $sd = [Security.AccessControl.RawSecurityDescriptor]::new($case.SDDL)
  $bytes = [byte[]]::new($sd.DiscretionaryAcl.BinaryLength)
  $sd.DiscretionaryAcl.GetBinaryForm($bytes, 0)
  $oldPtr = [Runtime.InteropServices.Marshal]::AllocHGlobal($bytes.Length)
  $newPtr = [IntPtr]::Zero
  try {
   [Runtime.InteropServices.Marshal]::Copy($bytes, 0, $oldPtr, $bytes.Length)
   $entry = [B01DraftMemoryAcl+Explicit]::new()
   $entry.Permissions=0; $entry.Mode=4; $entry.Inheritance=3
   $trustee = [B01DraftMemoryAcl+Trustee]::new()
   $trustee.Form=0; $trustee.Type=0; $trustee.Name=$sidPtr; $entry.Trustee=$trustee
   $rc = [B01DraftMemoryAcl]::SetEntriesInAclW(1, [ref]$entry, $oldPtr, [ref]$newPtr)
   if ($rc -ne 0) { throw "SetEntriesInAclW returned $rc" }
   $size = [uint16][Runtime.InteropServices.Marshal]::ReadInt16($newPtr, 2)
   $outBytes = [byte[]]::new($size)
   [Runtime.InteropServices.Marshal]::Copy($newPtr, $outBytes, 0, $size)
   $acl = [Security.AccessControl.RawAcl]::new($outBytes, 0)
   $aces = @(foreach ($ace in $acl) {
    [pscustomobject]@{Type=$ace.AceType.ToString(); Flags=$ace.AceFlags.ToString(); Mask=$ace.AccessMask; SID=$ace.SecurityIdentifier.Value}
   })
   [pscustomobject]@{Case=$case.Name; Input=$case.SDDL; ReturnCode=$rc; InputAceCount=$sd.DiscretionaryAcl.Count; OutputAceCount=$acl.Count; OutputAces=$aces}
  } finally {
   if ($newPtr -ne [IntPtr]::Zero) { [void][B01DraftMemoryAcl]::LocalFree($newPtr) }
   [Runtime.InteropServices.Marshal]::FreeHGlobal($oldPtr)
  }
 }
 $results | ConvertTo-Json -Depth 5
} finally {
 [Runtime.InteropServices.Marshal]::FreeHGlobal($sidPtr)
}

Attachment: cleanup-repro-output.json

[
  {
    "Case": "allow-only",
    "Input": "D:(A;OICI;FR;;;S-1-5-21-111111111-222222222-333333333-1234)",
    "ReturnCode": 0,
    "InputAceCount": 1,
    "OutputAceCount": 0,
    "OutputAces": []
  },
  {
    "Case": "deny-only",
    "Input": "D:(D;OICI;FR;;;S-1-5-21-111111111-222222222-333333333-1234)",
    "ReturnCode": 0,
    "InputAceCount": 1,
    "OutputAceCount": 1,
    "OutputAces": [
      {
        "Type": "AccessDenied",
        "Flags": "ObjectInherit, ContainerInherit",
        "Mask": 1179785,
        "SID": "S-1-5-21-111111111-222222222-333333333-1234"
      }
    ]
  },
  {
    "Case": "deny-and-allow",
    "Input": "D:(D;OICI;FR;;;S-1-5-21-111111111-222222222-333333333-1234)(A;OICI;FR;;;S-1-5-21-111111111-222222222-333333333-1234)",
    "ReturnCode": 0,
    "InputAceCount": 2,
    "OutputAceCount": 1,
    "OutputAces": [
      {
        "Type": "AccessDenied",
        "Flags": "ObjectInherit, ContainerInherit",
        "Mask": 1179785,
        "SID": "S-1-5-21-111111111-222222222-333333333-1234"
      }
    ]
  }
]

Start with codex-rs/windows-sandbox-rs/src/acl.rs and its revoke_ace path, then trace stale-path handling in deny_read_state.rs and reconciliation in setup_main/win.rs. Run cleanup-repro.ps1 on Windows and inspect the documented ACL results. Done means stale deny entries are removed explicitly, unrelated entries remain, API failures propagate, tracking is updated only after verified removal, and allow/deny/allow plus the three memory cases are covered.