fix(mxc): reject a non-absolute wxc_exec_path at gateway startup - #3497
Conversation
wxc_exec_path is the binary that builds every sandbox, but nothing validated it before spawning: a relative value (including the shipped default, a bare "wxc-exec.exe") let PATH-lookup or working-directory- relative resolution execute a decoy binary with the gateway's identity instead of the approved wxc-exec, turning the containment mechanism itself into an arbitrary-code-execution primitive. Add MxcComputeConfig::validate_configuration, wired into the existing (previously no-op) compute-driver config preflight, rejecting an empty or non-absolute wxc_exec_path with a clear diagnostic. Change the default from the relative "wxc-exec.exe" to an empty string so the field must be explicitly configured -- no usable-but-insecure fallback survives. Update the architecture doc's stale "else PATH" discovery claim to match. Signed-off-by: Prashant Khodade <pkhodade@nvidia.com> (cherry picked from commit d4192a0072f8f0ca0a4035f10cae07889c2b578f)
shailendra-nv
left a comment
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes for two gaps in this security fix: the gateway enforcement boundary is not covered by a regression test, and the new mandatory absolute-path configuration requirement is not documented. The core validation logic itself looks correct.
| let _: openshell_driver_mxc::MxcComputeConfig = context.driver_config()?; | ||
| Ok(()) | ||
| let config: openshell_driver_mxc::MxcComputeConfig = context.driver_config()?; | ||
| config.validate_configuration() |
There was a problem hiding this comment.
This is the actual enforcement boundary for the security fix, but the new tests only call MxcComputeConfig::validate_configuration directly. If this factory call regressed to the previous no-op, all added tests would still pass and relative paths would again reach startup. Please add a Windows gateway/config-preflight test that selects mxc and verifies omitted and relative paths fail while an absolute path passes.
| // comment above). Shipping a bare relative filename here would | ||
| // silently reintroduce the exact PATH/CWD-hijack risk the | ||
| // validation exists to reject. | ||
| wxc_exec_path: String::new(), |
There was a problem hiding this comment.
This changes an existing omitted field from a usable default to a startup error, but docs/reference/gateway-config.mdx and the MXC README only show examples; neither states that wxc_exec_path is now required and absolute. Please document the migration and requirement in this PR, as required for driver configuration default changes.
MxcComputeConfig::validate_configuration already had unit coverage, but that only proves the validation function itself is correct -- it says nothing about whether MxcFactory::validate_config (src/lib.rs) still calls it. Before this fix, that factory method discarded the parsed config entirely, so a regression back to that no-op shape would leave every unit test passing while a relative wxc_exec_path again reached gateway startup. Add an integration test that spawns the actual compiled openshell-gateway binary through its config preflight subcommand, exercising the real chain: CLI parsing, TOML loading, driver selection, MxcFactory::validate_config, and MxcComputeConfig::validate_configuration. Assertions check only pass/fail, not message content: run_effective_config_preflight replaces any validation failure with a generic message whenever a config file is used, to keep file-sourced values out of preflight diagnostics -- pre-existing, deliberate, and covered by its own tests. Also document the new required-and-absolute wxc_exec_path constraint in the gateway config reference and the driver README, which previously only showed example values without stating the requirement. Signed-off-by: Prashant Khodade <pkhodade@nvidia.com>
…solute-wxc-exec-path # Conflicts: # docs/reference/gateway-config.mdx
|
🌿 Preview your docs: https://nvidia-preview-pr-3497.docs.buildwithfern.com/openshell |
shailendra-nv
left a comment
shailendra-nv
left a comment
There was a problem hiding this comment.
The two requested changes are addressed on head 55b753e: the gateway/config-preflight path now has integration coverage, and the required absolute wxc_exec_path plus migration guidance are documented. No remaining review findings.
|
/ok to test |
@shailendra-nv, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 55b753e |
2 participants
Summary
wxc_exec_pathis the binary that builds every sandbox, but nothing validated it before spawning. A relative value -- including the shipped default, a bare"wxc-exec.exe"-- let PATH-lookup or working-directory-relative resolution execute a decoy binary with the gateway's identity instead of the approvedwxc-exec, turning the containment mechanism itself into an arbitrary-code-execution primitive.wxc_exec_path = "wxc-exec.exe"in the gateway config and starting the gateway succeeded cleanly with zero warning or diagnostic about the relative path.Related Issue
No linked issue -- this is a security-sensitive, localized fix to a compute-driver config preflight hook that already existed for exactly this purpose (
MxcFactory::validate_config, gated bysupports_config_preflight() -> true) but performed no semantic validation at all, only shape deserialization.Changes
openshell-driver-mxc: addedMxcComputeConfig::validate_configuration, rejecting an empty or non-absolutewxc_exec_pathwith a diagnostic naming the field -- matching thePath::is_absolute()validation convention already used elsewhere in this codebase for exec/trust-relevant paths.openshell-gateway: wiredMxcFactory::validate_configto call it. This runs on every real gateway startup, matching the existing pattern other compute-driver factories (Docker, Kubernetes) already use for their own config validation.MxcComputeConfig::default()'swxc_exec_pathfrom the relative"wxc-exec.exe"to an empty string. The old default was itself the exact vulnerability this fix closes -- rejecting explicit relative values while leaving an equally-relative default in place would let an operator (or attacker steering config) route around the check by simply not setting the field. No usable-but-insecure fallback survives;wxc_exec_pathmust now be explicitly configured as an absolute path.build()intentionally does not re-validate -- it trusts the single preflight gate, matching how Docker's factory is structured.Testing
openshell-driver-mxc:validate_configuration_rejects_unset_wxc_exec_path(covers the now-empty default),validate_configuration_rejects_relative_wxc_exec_path,validate_configuration_accepts_absolute_wxc_exec_path.cargo test -p openshell-driver-mxc --target x86_64-pc-windows-msvc --lib.cargo test -p openshell-gateway --target x86_64-pc-windows-msvc --libpasses, unaffected by this change.Checklist
Originally opened as GitLab MR !115 against our internal mirror; re-opened here against
windowsfor upstream review. The GitLab MR also updatedarchitecture/mxc-compute-driver.md, which does not exist onwindows(the architecture doc structure has diverged) -- that doc change is dropped here as inapplicable; only the code fix (driver validation + gateway wiring) is carried over.