Derive AppContainer capability count from the capability list - #4
Merged
Conversation
sec_cap.Capabilities was a fixed new[2] array with CapabilityCount hardcoded to 1, while the commented-out network capability lines wrote to indices 0 and 1. Uncommenting the index-0 line would have silently overwritten remoteFileAccess, and the index-1 line would have been ignored by the hardcoded count. Capabilities now append to a std::vector member and the count is taken from its size, so entries can be added or removed without keeping indices and a count in sync. The vector also owns the array that was previously leaked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NUL is \Device\Null, a device object, so its DACL cannot be reached through the SE_FILE_OBJECT named-object provider; it has to be read and written through an open handle with GetKernelObjectSecurity/SetKernelObjectSecurity instead. The kernel recreates \Device\Null with a default security descriptor on every boot, and that default does not grant the AppContainer SIDs, so a sandboxed child that opens NUL (directly, or by redirecting its stdio there) fails with ERROR_ACCESS_DENIED. Grant it to ALL APPLICATION PACKAGES and ALL RESTRICTED APPLICATION PACKAGES rather than to the per-run cesandbox<pid> SID: the device is machine wide, so a per-run grant would race with concurrently running sandboxes and would leak an ACE for every crashed run. This is a standalone maintenance mode with no target executable, meant to run elevated once per boot, so also guard the progid assignment - it previously read argv[argc] when no executable followed the flags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
InitializeCapabilitiesallocatedsec_cap.Capabilities = new SID_AND_ATTRIBUTES[2]but hardcodedsec_cap.CapabilityCount = 1, while the commented-out network capability lines wrote to indices 0 and 1. That meant:remoteFileAccessCapabilities now append to a
std::vector<SID_AND_ATTRIBUTES>member and the count is derived from its size, so entries can be added or removed without keeping indices and a count in sync. Uncommenting any of the three network capability lines now just works.CreateCapabilitySID(sids, idx, ...)/CreateCapabilitySIDFromName(sids, idx, ...)becameAddCapabilitySID(sids, ...)/AddCapabilitySIDFromName(sids, ...). The vector is a member so it outlives the pointer handed toCreateAppContainerProfileandgetSecurityCapabilitiesPtr, anddata()/size()are read only after all appends, so there is no reallocation invalidation.Side effect: the
new SID_AND_ATTRIBUTES[2]that was never deleted is gone — the vector owns that storage now. Themalloc'dSidbuffers inside each entry still leak, unchanged from before.Not build-tested — this is Win32-only code and it was written on Linux. Worth a Windows compile before merging.
🤖 Generated with Claude Code