BaseContainer: PowerShell cannot use the granted working directory - #876
BaseContainer: PowerShell cannot use the granted working directory#876Carlos Alexandro Becker (caarlos0) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds temporary traverse-only ancestor ACL grants for BaseContainer filesystem paths.
Changes:
- Derives cwd and allow-path ancestors and queries the child AppContainer SID.
- Adds non-inheritable
FILE_TRAVERSEgrants with restoration. - Adds unit coverage for ancestor selection and ACL behavior.
Show a summary per file
| File | Description |
|---|---|
base_container_runner.rs |
Applies and retains ancestor traversal grants. |
filesystem_dacl.rs |
Implements traversal grants, validation, and tests. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
src/backends/appcontainer/common/src/base_container_runner.rs:2060
- Making this runtime DACL write mandatory prevents the motivating
C:\Users\...case from launching under a normal user.wxc-exec.exeruns asInvoker (docs/host-prep.md:8-13), whileDaclManagerneedsWRITE_DAC; a standard user generally cannot modify the ACL onC:\Users, so this reaches the error branch instead of fixing the cwd. Ancestor grants that require elevation need to be provisioned through the elevated host-prep path, or the launch must avoid depending on host ACL mutation.
let mut manager = DaclManager::new().map_err(|error| error.to_string())?;
manager
.grant_traverse_access(&sid, &traversal_paths)
.map_err(|error| error.to_string())?;
src/backends/appcontainer/common/src/base_container_runner.rs:2060
- This converts accepted filesystem policy into a new launch-time hard failure. The parser intentionally treats non-existent allow paths as advisory because targets may be created dynamically (
config_parser.rs:379-383), but an input such asC:\future\childnow yields a missing ancestor andgrant_traverse_accessreturnsPathNotFound, aborting T1/T2. Only existing directory ancestors should be considered for runtime grants, without changing the existing acceptance semantics for future targets.
manager
.grant_traverse_access(&sid, &traversal_paths)
.map_err(|error| error.to_string())?;
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/backends/appcontainer/common/src/base_container_runner.rs:136
- The implementation no longer matches the PR description:
traversal_pathsreceives only the resolved cwd, so ancestors of the remaining read-write/read-only policy paths are never considered, despite the description and validation section claiming they are. Either include those policy paths here or update the PR scope and validation claims to state that this fix is cwd-only.
fn traversal_paths(working_directory: &str) -> Vec<PathBuf> {
traversal_ancestors([working_directory])
}
src/backends/appcontainer/common/src/base_container_runner.rs:2055
- This cannot fix the motivating
C:\Userscase for a normalasInvokercaller: that caller generally has noWRITE_DAConC:\Users, sogrant_traverse_accessreturnsWriteDacDeniedand this path always aborts the launch. The grant needs a mechanism that does not depend on the unelevated caller owning every shared ancestor (for example, native broker policy or explicit elevated host preparation); otherwise the reported scenario changes from wrong cwd to no sandbox.
let mut manager = DaclManager::new().map_err(|error| error.to_string())?;
manager
.grant_traverse_access(&sid, &traversal_paths)
.map_err(|error| error.to_string())?;
src/core/wxc_common/src/filesystem_dacl.rs:526
- The new file-target rejection is the security boundary preventing
0x20from becomingFILE_EXECUTE, but the added test exercises only a directory success. Add a temp-file test that assertsTraverseTargetNotDirectoryand verifies no ACE/state entry was retained.
if !inherit_on_directories && !is_directory {
// Only the traverse grant is non-inheriting, and bit 0x20 means
// FILE_EXECUTE on a file. Refuse rather than hand out execute.
return Err(DaclError::TraverseTargetNotDirectory(canonical));
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/core/mxc-sdk/tests/streaming_processcontainer.rs:19
- This broad read-only grant makes the regression test unable to prove the traverse-only fix:
C:\includesC:\Usersand the test cwd, reproducing the normal recursive policy workaround that already fixes startup. The test can therefore pass even if the new traversal grant is removed. Keep the read-only list empty (as the existingcmdstreaming test below does) or narrow it to dependencies outside the cwd ancestry.
readonly_paths: vec!["C:\\".to_string()],
src/backends/appcontainer/common/src/base_container_runner.rs:2042
- The PR description says ancestors of the cwd and both filesystem allow lists are granted, but this production path derives ancestors only from the resolved working directory. Any allowed path that is not selected as the cwd receives no traverse ACE; the advertised allow-path test is also absent and has been replaced by a cwd-only test. Either include
request.policy.readwrite_pathsandreadonly_pathshere or narrow the stated scope and validation.
let traversal_paths = traversal_paths(&working_directory.path)
src/backends/appcontainer/common/src/base_container_runner.rs:2528
- Taking the manager invokes
Drop, which discards bothrestore()errors and the per-entry failures stored only inDaclManager::warnings(). The persisted state is still owned by this live SDK process, sorecover_orphaned_stateskips it (filesystem_dacl.rs:644-651); a failed restore can therefore leave these shared-ancestor ACEs silently active for the lifetime of a long-running host. Explicitly restore here and surface its warnings/errors through teardown before discarding the manager.
self.dacl_manager.take();
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
A process launched in a BaseContainer sandbox that grants `readwritePaths` on directory D, and is started with D as its working directory, cannot resolve D. Reading and writing D work; only resolution fails, because resolving a path walks it from the drive root and stats every ancestor, and the ancestors are denied. The user-visible result is a sandboxed shell reporting `C:\` instead of the directory it was started in. `cmd /c cd` cannot catch this: it prints the process's stored current-directory string without resolving it, so it reports the correct directory and looks healthy. These tests assert through PowerShell's `pwd` and `Set-Location`, which do resolve. `pwd_reports_the_granted_working_directory` and `set_location_into_the_granted_working_directory_succeeds` currently fail, and document the bug. `granting_the_working_directory_does_not_expose_its_siblings` passes and pins the boundary the fix must not cross: `readonlyPaths` on the ancestors would make the first two pass, but those rules are recursive, so it would also expose everything beside the working directory. All three are `#[ignore]`d — they need an elevated, host-prepped Windows host — so CI is unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
310d8bc to
839a46a
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/core/mxc-sdk/tests/streaming_processcontainer_cwd.rs:256
- This negative test passes whenever PowerShell fails to start, times out, or the probe command otherwise produces no output, because all of those cases also omit the secret. That would incorrectly report the confidentiality boundary as intact. Assert both a successful exit and the explicit
DENIEDmarker so the test proves that the access attempt actually ran and was rejected.
assert!(
!result.stdout.contains("TOP-SECRET-CONTENT"),
"a sibling of the working directory must not be readable\nstdout: {:?}",
result.stdout.trim()
);
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The first version of this reproduction attributed the failure to the container being unable to traverse the ancestors of a granted directory. That was wrong. `cmd.exe` and .NET find and enter the granted directory correctly with no permission on any ancestor; only PowerShell fails. Each condition now has two tests, one per shell, sharing a policy and a working directory so the shell is the only variable. The cmd.exe tests pass, which is what demonstrates the container is behaving correctly and narrows the fault to PowerShell's FileSystem provider walking the path from the drive root. `cmd_chdir_into_the_granted_working_directory_succeeds` chdirs to a subdirectory first and then back by full path, so it exercises a real SetCurrentDirectory rather than a no-op. It deliberately does not route through `C:\`, which the policy does not grant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/core/mxc-sdk/tests/streaming_processcontainer_cwd.rs:311
- This negative check can pass without exercising the containment boundary. A timeout, a PowerShell error before
Get-Content, or an output-read failure all leave the secret absent and make the test green. Since the script emits a sentinel on the expected denial path, require a successful process exit and exactly that sentinel so the regression test cannot pass on an unrelated failure.
This issue also appears on line 442 of the same file.
assert!(
!result.stdout.contains("TOP-SECRET-CONTENT"),
"a sibling of the working directory must not be readable\nstdout: {:?}",
result.stdout.trim()
);
src/core/mxc-sdk/tests/streaming_processcontainer_cwd.rs:446
- This assertion also reports success when the probe times out,
cmd.exefails beforetype, or stdout capture fails, because all of those outputs omit the secret. Verify the expectedDENIEDsentinel and successful exit so the test proves that the access attempt ran and was rejected.
assert!(
!result.stdout.contains("TOP-SECRET-CONTENT"),
"a sibling of the working directory must not be readable\nstdout: {:?}",
result.stdout.trim()
);
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
.github/copilot-instructions.md.Note
This PR is a bug report with an executable reproduction. It contains no fix.
It adds six
#[ignore]d integration tests: four pass, two fail and document the bug.Important
Correction to the first version of this report. The original diagnosis was wrong. It said the sandbox could not traverse the ancestors of a granted directory. It can.
cmd.exeand .NET find and enter the granted directory correctly with no permission on any ancestor — only PowerShell fails. The ask at the end is unchanged; only the explanation is.Summary
A BaseContainer sandbox is given a filesystem policy granting
fs_read_writeon directoryD, and starts a process whose working directory isD.PowerShell cannot use
Das a location. It silently falls back to the drive root, with no error.The container is behaving correctly. A process in the sandbox can open, read, write and list
D, and can enterDby full path. Win32 and .NET all work.PowerShell's
FileSystemprovider checks every directory along the path, from the drive root down toD. The policy grants no permission on those ancestors, so that check fails.The user sees a sandboxed PowerShell reporting
C:\instead of the directory it started in.Environment
26623.1007.amd64fre.ge_current_directwinpd_oem.260527-1700amd64fre)Experimental_CreateProcessInSandbox(legacySBOXFlatBuffer contract)pwsh.exe) — does not occur incmd.exe0.7.0-alpha(policyversionused by the tests)326f47d3Reproduction
The minimum policy — permission on the working directory and nothing else:
{ "filesystem": { "readwritePaths": ["C:\\mxc-cwd-tests\\work"] }, "process": { "cwd": "C:\\mxc-cwd-tests\\work" } }C:\mxc-cwd-testsis created and owned by the unelevated user running the test, so this is not about a system-owned directory such asC:\Users.Start PowerShell in the sandbox and run
Get-Location:Set-Locationon the same path shows the underlying error:C:\mxc-cwd-testsis a parent of the granted directory, not the granted directory itself.The container is correct
Same policy, no permission on any ancestor. All of these succeed:
cmd /c cd(prints the stored path)C:\mxc-cwd-tests\workcmd /c cd /d "C:\mxc-cwd-tests\work"from another directoryERRORLEVEL 0cmd /c dir /b "C:\mxc-cwd-tests\work"[IO.Directory]::GetCurrentDirectory()C:\mxc-cwd-tests\work[IO.Directory]::SetCurrentDirectory(...)[IO.Path]::GetFullPath(...)C:\mxc-cwd-tests\work[IO.Directory]::Exists(...)TrueOnly these two fail:
Set-Location -LiteralPath ...Access to the path 'C:\mxc-cwd-tests' is denied.(Get-Location).PathC:\The four .NET operations and the two PowerShell operations run in the same
pwsh.exeprocess. Within that one process, .NET reports the correct directory whileGet-LocationreportsC:\.So
NtCreateFileon the full path succeeds at every ancestor — the container token fails no path-component check. Only the check inside the PowerShell provider fails.PowerShell requires permission on every ancestor
Using a deeper working directory,
C:\mxc-cwd-tests\<id>\lvl1\lvl2\work:C:\mxc-cwd-tests, the first ancestor below the drive root....\lvl1\lvl2) only → same error, namingC:\mxc-cwd-testsagain.Set-Locationsucceeds andGet-Locationis correct.Note also: the policy grants no permission to enter
C:\, andcd /d C:\fails. PowerShell falls back to a drive root it cannot itself enter.The tests
src/core/mxc-sdk/tests/streaming_processcontainer_cwd.rs. Every condition is tested twice — once through PowerShell, once throughcmd.exe— with the same policy and working directory, so the shell is the only variable. All are#[ignore]d because they need an elevated, host-prepped Windows host, so CI is unaffected.The passing
cmd_*tests are what establish that the container is correct; the two failures are the bug. The sibling tests pin the boundary a fix must not cross, once per shell.cmd_chdir_into_the_granted_working_directory_succeedsfirst chdirs into a subdirectory of the granted directory and then back by full path, so it exercises a realSetCurrentDirectoryrather than a no-op. It deliberately does not route throughC:\, which the policy does not grant.Why this matters
Our users run PowerShell; telling them to use another shell is not an answer.
A sandbox starts a shell in a project directory, PowerShell lands on
C:\and says nothing. Every relative path the shell then constructs is wrong, with no indication of the fault.It is also easy to miss in manual testing:
cmd /c cdprints the process's stored path without resolving it, so it reports the correct directory and the sandbox looks healthy.Expected behavior
When the filesystem policy grants a directory, an ordinary shell must be able to use it as a working directory — which requires the sandbox to read the attributes of each ancestor. The minimum mask per ancestor:
0x001200A8. Verified sufficient: stamping exactly this on each ancestor forALL APPLICATION PACKAGES, non-inheritable, makesGet-LocationandSet-Locationbehave correctly.FILE_LIST_DIRECTORY(0x1) is not required — the sandbox need not enumerate the ancestors.Why the alternatives are unsatisfactory
Add the ancestors to
fs_read_only. Works, but these rules are recursive, so grantingC:\Users(or any ancestor) also grants everything beneath it. Verified: a file placed beside the granted working directory becomes readable from inside the sandbox. For a repo under a user profile, this exposes the entire profile.granting_the_working_directory_does_not_expose_its_siblingspins this boundary, per shell.Stamp host DACLs per run. Needs
WRITE_DACon each ancestor, which an unelevated caller does not hold for system-owned ancestors such asC:\Users. It also mutates host ACLs outside the sandbox for the duration of the run.A one-time elevated host-prep step. Puts an administrator requirement on every machine for what is a sandbox policy decision.
Tell users not to use PowerShell. Not viable — it is our users' default shell, and the failure is silent.
The container already knows every granted path, so it is the right place to make those paths' ancestors attribute-readable. This grants no additional ability to read files or list directory contents.
Ask
Change the BaseContainer filesystem policy so that, for each granted path, the sandbox can read the attributes of every ancestor: mask
0x001200A8, noFILE_LIST_DIRECTORY, non-inheritable. That makes a granted directory usable as a working directory in PowerShell.If this cannot be done in the container, then the change belongs in PowerShell:
Set-Locationand theFileSystemprovider should not require access to every ancestor when the target directory itself opens successfully — and should not silently fall back to the drive root when the check fails.On the withdrawn fix
Earlier revisions of this branch implemented alternative 2, stamping traverse ACEs on the ancestors per run. That work has been dropped and the branch reset. Review surfaced that it mutated host DACLs without honoring
fallback.allowDaclMutation, that concurrent sandboxes sharing an identity raced each other's ACE restore, and that it could not work unelevated for system-owned ancestors at all. It also grantedFILE_TRAVERSEalone, which the mask above shows is insufficient. Given the corrected diagnosis, the mechanism was addressing the wrong layer regardless.