Skip to content

Reject parent components in Linux filesystem policy paths - #1675

Open
Safet Hrbinic (safmswork) wants to merge 3 commits into
microsoft:mainfrom
safmswork:fix/reject-linux-policy-parent-components
Open

Safet Hrbinic (safmswork) wants to merge 3 commits into
microsoft:mainfrom
safmswork:fix/reject-linux-policy-parent-components

Conversation

@safmswork

@safmswork Safet Hrbinic (safmswork) commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Validate Bubblewrap and LXC paths before sandbox setup and extend the existing functional filesystem suites with nested-policy regressions.

📖 Description

Reject .. components in Bubblewrap and LXC filesystem policy paths before sandbox creation. Written path depth can otherwise differ from the actual mount destination, allowing a later parent grant to hide an earlier child restriction.

Use one shared validation helper from both backends, with an escaped error that identifies the affected policy field. Reject rather than remove parent components because symbolic links can change their meaning. Ordinary dotted filenames, literal Linux backslashes, and canonical parent/child policies remain supported.

Extend the existing Linux filesystem validation suites with parent-component rejection scenarios in all three policy lists and controls for both directions of nested policy precedence. Update the schema and backend documentation.

This intentionally rejects policy paths containing .. that were previously accepted. Callers must supply resolved policy paths without parent components. Workload paths and SDK signatures are unchanged.

Update: I also added new code to deal with "." as well; ex. /foo/././bar . In that case we don't need to reject the path. We simply ignore the "." in the MXC code when counting the components. So in the case of the above example, MXC would count 2 components (foo and bar) and place it in the correct ordering relative to other paths.

🔗 References

A path containing .. can override a Linux filesystem deny #1630

🔍 Validation

Rust commands ran from src/ using the repository-pinned toolchain:

  • cargo test -p mxc-sdk --lib filesystem_parent_components_are_rejected_by_both_validation_paths -- --nocapture — failed before the production fix: the validator incorrectly accepted a parent component.
  • cargo test -p mxc-sdk --lib filesystem_resolve — passed, 12 tests.
  • cargo test -p mxc-sdk --lib filesystem_parent_components — passed, 1 LXC regression covering both validation entry points.
  • cargo test -p mxc-sdk --lib lxc_runner::tests — passed, 73 tests.
  • cargo fmt --all -- --check — passed after formatting.
  • cargo clippy -p mxc-sdk --lib --tests -- -D warnings — passed.

From the repository root:

  • & 'C:\Program Files\Git\bin\bash.exe' -n tests/scripts/run_bwrap_filesystem_test.sh — passed.
  • & 'C:\Program Files\Git\bin\bash.exe' -n tests/scripts/run_lxc_filesystem_test.sh — passed.
  • git --no-pager diff --check — passed.

Native Linux validation passed in the LXC E2E workflow:

  • cargo build --release -p lxc --bin lxc-exec — passed.
  • sudo --preserve-env=MXC_LXC_TESTS_REQUIRE_EXECUTION bash tests/scripts/run_lxc_all_tests.sh — passed, 28 suites with no failures, skips, or disabled suites. The modified filesystem suite explicitly reported that parent segments were rejected and canonical nested policies were preserved.
  • sudo --preserve-env=MXC_LXC_TESTS_REQUIRE_EXECUTION,PATH,HOME "$(command -v cargo)" test -p mxc-sdk --test wxc_e2e_tests_e2e_lxc_network_capability — passed, 6 tests.
  • sudo --preserve-env=MXC_LXC_TESTS_REQUIRE_EXECUTION,PATH,HOME "$(command -v cargo)" test -p mxc-sdk --test streaming_lxc --test sdk_helpers — passed, 24 tests.

Actual Bubblewrap sandbox execution and its Linux-only runner regression have not been verified. This Windows host has no installed Linux environment. Both extended functional scripts are already called by the respective all-tests suites and scheduled backend validation.

✅ Checklist

  • Signed the Contributor License Agreement
  • Linked to an issue
  • For features or changes requiring a spec: linked the issue with maintainer-applied feature-accepted
  • For implementation: linked the required approved and merged design/API spec, or explained why no spec is required
  • Feature issue remains open unless this PR completes all agreed implementation, validation, and documentation
  • Updated documentation (if applicable)
  • Updated Copilot instructions (if build, architecture, or conventions changed)
  • If this PR changes Cargo.lock, the dependency-feed-check check passes (see pull request builds)

📋 Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

Validate Bubblewrap and LXC paths before sandbox setup and extend
the existing functional filesystem suites with nested-policy regressions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 34e5fe04-467a-49cd-8f04-ee6a2c84d255
@safmswork
Safet Hrbinic (safmswork) requested a review from a team as a code owner November 4, 2026 21:50
Copilot AI balanced review requested due to automatic review settings November 4, 2026 21:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Active findings identify a filesystem-policy bypass where a /work/././ grant can override a child deny, plus an unversioned validation change that breaks supported V1 SDK behavior.

2 open issues
What changed in this PR

This PR addresses #1630 by rejecting .. components in Bubblewrap and LXC filesystem policy paths before sandbox creation, preventing those paths from distorting mount order.

Changes:

  • Adds shared validation with escaped, field-specific errors and backend regression tests.
  • Extends both Linux filesystem suites with rejection scenarios and nested-policy controls.
  • Documents the new restriction and caller migration requirements; workload paths and SDK signatures remain unchanged.
File Description
tests/​scripts/​run_lxc_filesystem_test.sh Adds LXC path-rejection and nested-policy scenarios.
tests/​scripts/​run_bwrap_filesystem_test.sh Adds Bubblewrap path-rejection and nested-policy scenarios.
src/​mxc-sdk/​src/​core/​mxc_common/​filesystem_resolve.rs Adds shared path validation and diagnostic tests.
src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_runner.rs Validates paths early and tests both validation entry points.
src/​mxc-sdk/​src/​backends/​bubblewrap/​common/​bwrap_runner.rs Validates paths before environment probing and tests rejection.
docs/​schema.md Documents the Linux filesystem policy restriction.
docs/​backends/​lxc/​lxc-backend.md Explains LXC path requirements and remediation.
docs/​backends/​bwrap/​bubblewrap-backend.md Explains Bubblewrap rejection and mount-order rationale.

Review effort: Balanced

@safmswork

Copy link
Copy Markdown
Contributor Author

Copilot resolve the merge conflicts in this pull request

Copilot AI balanced review requested due to automatic review settings November 5, 2026 02:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unconditional rejection of parent-component paths breaks previously accepted Rust, .NET, and Node V1 filesystem policies without versioned behavior or a documented security-servicing exception and migration guidance.

2 open issues

Review effort: Balanced

Ignore standalone current-directory components when ordering filesystem
policy mounts, and treat backslashes as literal Unix filename characters.
Preserve the original paths passed to each backend and the existing
parent-component rejection.

Extend both backend filesystem suites with dotted parent and child grants,
and cover aliases and literal filenames in resolver regressions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 34e5fe04-467a-49cd-8f04-ee6a2c84d255
Copilot AI balanced review requested due to automatic review settings November 5, 2026 03:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unconditional rejection of previously accepted paths containing .. breaks Rust, .NET, and Node V1 execution compatibility without a new versioned surface or a documented, maintainer-approved security-servicing exception with migration guidance.

2 open issues

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The unconditional path rejection breaks previously accepted Rust, .NET, and Node V1 behavior without a versioned surface or documented security-servicing exception with migration guidance.

1 open issue
1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The containment-sensitive mount-ordering change lacks native Linux Bubblewrap validation of parent-component rejection and nested-policy precedence in both directions, including the new '.' cases.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@huzaifa-d

Copy link
Copy Markdown
Contributor

Worth looking into

where
F: FnOnce() -> Result<bwrap_version::ProbedBwrap, bwrap_version::BwrapUnavailable>,
{
crate::mxc_common::filesystem_resolve::validate_mount_paths(&request.policy)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider updating the existing broken-symlink test expectations alongside this check. Two cases in run_bwrap_denied_masking_test.sh contain /../ but still require 'could not be resolved', so this earlier rejection makes the scheduled Bubblewrap suite fail.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

EOF
}

for path in "$POLICY_DIR/work" "$POLICY_DIR/work/././"; do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes even without the .

fix: resolver_file.rs:173-178 re-sorts mounts by Path::components(), which ignores ..
Also deny $(readlink -f /etc/resolv.conf) (skips the re-sort) and a file directly in work/, and assert it stays unreadable; the merge-base leaks it.

where
F: FnOnce() -> Result<bwrap_version::ProbedBwrap, bwrap_version::BwrapUnavailable>,
{
crate::mxc_common::filesystem_resolve::validate_mount_paths(&request.policy)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@microsoft-github-policy-service microsoft-github-policy-service Bot added the Needs-Author-Feedback Waiting for additional information or action from the issue or pull-request author. label Nov 9, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs-Author-Feedback Waiting for additional information or action from the issue or pull-request author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants