Security: prevent env var leakage into AppContainer child process#460
Merged
Conversation
Previously, when no explicit env was specified in config.json, NULL was passed to CreateProcessW -- inheriting the parent process's full environment into the sandboxed child. This is a security bug: secrets, tokens, and internal vars from wxc-exec leak into the AppContainer. Fix: always build an explicit environment block via CreateEnvironmentBlock(token, bInherit=FALSE) which returns only the clean system/user profile variables. New behavior for all four cases: - Explicit env, no proxy: use caller's env vars verbatim - Explicit env, with proxy: use caller's env vars, strip any existing proxy vars, inject HTTP_PROXY/HTTPS_PROXY from config - No explicit env, no proxy: call CreateEnvironmentBlock(FALSE) for a clean default user env block (no inheritance from parent process) - No explicit env, with proxy: call CreateEnvironmentBlock(FALSE), strip any profile-level proxy vars, inject HTTP_PROXY/HTTPS_PROXY from config Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a security issue in the Windows AppContainer runner where the sandboxed child process could inherit the full wxc-exec process environment (potentially leaking secrets) when no explicit env was provided. It changes the runner to always pass an explicit environment block to CreateProcessW, using CreateEnvironmentBlock(..., bInherit=FALSE) as the “clean default” baseline when the caller doesn’t specify env vars.
Changes:
- Always builds and passes a Unicode environment block to
CreateProcessW(neverNULLinheritance). - Adds logic to construct a clean default env block via
CreateEnvironmentBlockand optionally strip/re-inject proxy vars. - Introduces env-block parsing from OS-provided UTF-16 blocks into
(key, value)pairs to support re-encoding.
Show a summary per file
| File | Description |
|---|---|
src/wxc_common/src/appcontainer_runner.rs |
Builds an explicit child environment block in all cases; adds default-env creation via CreateEnvironmentBlock plus proxy override handling. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
Comment on lines
+108
to
+116
| // Split on the first '=' (env vars can have '=' in the value). | ||
| // Skip entries that start with '=' (hidden drive-letter vars like "=C:=C:\"). | ||
| if let Some(eq_pos) = entry.find('=') { | ||
| if eq_pos > 0 { | ||
| let key = entry[..eq_pos].to_string(); | ||
| let value = entry[eq_pos + 1..].to_string(); | ||
| entries.push((key, value)); | ||
| } | ||
| } |
Comment on lines
+613
to
+618
| let env_block: Vec<u16> = if !request.env.is_empty() { | ||
| let entries = build_explicit_entries(&request.env, self.proxy_address.as_ref()); | ||
| Some(encode_env_block(&entries)) | ||
| } else if let Some(addr) = self.proxy_address.as_ref() { | ||
| // No explicit env but proxy is active -- inject only proxy vars. | ||
| let proxy_url = addr.to_url(); | ||
| let entries = vec![ | ||
| ("HTTP_PROXY".to_string(), proxy_url.clone()), | ||
| ("HTTPS_PROXY".to_string(), proxy_url), | ||
| ]; | ||
| Some(encode_env_block(&entries)) | ||
| encode_env_block(&entries) | ||
| } else { | ||
| None | ||
| // Get clean default user env without inheriting process env vars. | ||
| let mut entries = create_default_env_entries()?; |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MGudgin
approved these changes
May 29, 2026
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
bbonaby
approved these changes
May 29, 2026
Comment on lines
+106
to
+109
| while unsafe { *block.add(offset) } != 0 { | ||
| offset += 1; | ||
| } | ||
| let slice = unsafe { std::slice::from_raw_parts(block.add(start), offset - start) }; |
Collaborator
There was a problem hiding this comment.
note: missing safety statements although I don't expect this to actually be malformed since we get them directly from the WIndows API
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.
Summary
Fixes a security bug where the AppContainer child process inherited the parent's full environment block (secrets, tokens, internal vars from wxc-exec) when no explicit env was specified in config.json.
Fix: Always build an explicit environment block. When no env is provided by the caller, use
CreateEnvironmentBlock(token, bInherit=FALSE)to get only the clean system/user profile variables.New behavior for all four cases:
CreateEnvironmentBlock(FALSE)for a clean default user env block (no inheritance from parent process)CreateEnvironmentBlock(FALSE), strip any profile-level proxy vars, inject HTTP_PROXY/HTTPS_PROXY from configMicrosoft Reviewers: Open in CodeFlow