-
Notifications
You must be signed in to change notification settings - Fork 6
fix: restrict /proc/self/environ and docker-compose.yml secret exposure #647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Mossaka <[email protected]>
Co-authored-by: Mossaka <[email protected]>
Co-authored-by: Mossaka <[email protected]>
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
Security Review: Token Cache File Race ConditionI've reviewed PR #647 and found one security concern that should be addressed before merging.
|
- Add forward declarations for get_token_index() and format_token_value() in one-shot-token.c. The constructor and load_token_cache_file() call these before their definitions, causing implicit declaration errors with -Wall. - Write docker-compose.yml with mode 0600 so the agent container (running as awfuser) cannot read plaintext secrets via /host mount. Eliminates the TOCTOU window. - Fix TOCTOU in redactComposeSecrets: replace existsSync + readFileSync with single readFileSync in try/catch. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements two security mitigations to prevent secret exfiltration from the AWF agent container: (1) scrubbing sensitive environment variables from /proc/self/environ using a token cache file mechanism, and (2) redacting secrets from docker-compose.yml after containers start to prevent exposure via the /host mount.
Changes:
- Added token cache file mechanism:
entrypoint.shwrites token values to a temporary file (mode 0600), unsets env vars before exec; LD_PRELOAD library constructor reads the cache file, populates in-memory cache, and immediately deletes it - Added
__attribute__((constructor))to one-shot-token.c to eagerly cache and scrub tokens from environ on library load, ensuring/proc/self/environnever exposes secrets - Added
redactComposeSecrets()function in docker-manager.ts that replaces sensitive values in docker-compose.yml with***REDACTED***using regex patterns - Integrated redaction into workflow between
startContainers()andrunAgentCommand()execution
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/docker-manager.ts | Added SENSITIVE_ENV_NAMES set and redactComposeSecrets() function to redact secrets from docker-compose.yml |
| src/docker-manager.test.ts | Added comprehensive tests for redaction function covering various YAML formats and edge cases |
| src/cli.ts | Added redactComposeSecrets to workflow dependencies passed to runMainWorkflow |
| src/cli-workflow.ts | Integrated redactComposeSecrets call after container startup to redact secrets before agent command execution |
| src/cli-workflow.test.ts | Updated workflow tests to verify redaction happens in correct sequence |
| containers/agent/one-shot-token/one-shot-token.c | Added constructor with token cache file loading and eager environ scrubbing |
| containers/agent/one-shot-token/README.md | Updated documentation to describe token cache file mechanism and constructor behavior |
| containers/agent/entrypoint.sh | Added token cache file creation and scrubbing logic for both chroot and non-chroot paths |
| package-lock.json | Added "peer": true flags to various dependencies (appears to be from npm version difference) |
Comments suppressed due to low confidence (2)
src/docker-manager.ts:925
- If redaction fails (line 924), only a warning is logged and execution continues. This means the agent container will run with an unredacted docker-compose.yml file accessible via
/host/tmp/awf-*/docker-compose.yml, creating a security exposure. Consider whether this should be treated as a critical error that stops execution. At minimum, the warning should clearly indicate this is a security concern. Alternatively, document why continuing with unredacted secrets is acceptable (e.g., if other mitigations are sufficient).
fs.writeFileSync(composePath, content, { mode: 0o600 });
logger.info(`Redacted ${redactedCount} sensitive value(s) from docker-compose.yml`);
}
containers/agent/one-shot-token/one-shot-token.c:295
- The constructor calls unsetenv() for tokens found in the environment (line 295), but there's no check for unsetenv() failure. While unsetenv() rarely fails, if it does (e.g., due to invalid token name format or system error), the token would remain in /proc/self/environ, defeating the purpose of this scrubbing mechanism. Consider checking the return value and logging a warning or treating it as a critical error in security-sensitive contexts.
char *value = real_getenv(sensitive_tokens[i]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `(${name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})(=|:\\s*)([^\\n]+)`, | ||
| 'g' |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern ([^\\n]+) will match everything up to a newline, including trailing whitespace and quotes. In YAML, values can be quoted (e.g., GITHUB_TOKEN: "ghp_abc123"), and this pattern would preserve the quotes in the redacted output, resulting in GITHUB_TOKEN: "***REDACTED*** (missing closing quote). Consider adjusting the pattern to handle quoted values properly, or trimming whitespace and quotes from captured values before redaction. For example, the pattern could be ([^\\n]*?)\\s*$ to capture the value more precisely.
This issue also appears on line 923 of the same file.
| `(${name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})(=|:\\s*)([^\\n]+)`, | |
| 'g' | |
| `(${name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})(=|:\\s*)([^\\n]*?)\\s*$`, | |
| 'gm' |
| fprintf(stderr, "[one-shot-token] WARNING: Could not open token cache file: %s\n", cache_path); | ||
| return; | ||
| } |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line buffer is fixed at 8192 bytes. If a token value exceeds this length (e.g., a very long JWT or concatenated secrets), fgets will read only the first 8191 characters, potentially splitting the line. The next fgets call will read the remainder as a new line, which may parse incorrectly. While 8KB is generous for typical tokens, consider either validating line length, handling partial reads, or documenting this limitation. Alternatively, use getline() which dynamically allocates buffer space.
|
|
||
| *eq = '\0'; | ||
| const char *name = line; | ||
| const char *value = eq + 1; |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function checks !token_accessed[idx] before caching a token from the file, but if a token appears multiple times in the cache file, only the first occurrence will be loaded. While this might be intentional (first-wins policy), it's not documented. If the entrypoint.sh script accidentally writes duplicate entries, the later value will be silently ignored. Consider either warning about duplicates or documenting this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added (first-wins for duplicates) comment to the code in e767a90. This is intentional — entrypoint.sh writes each token exactly once, so duplicates would only occur from manual cache file editing.
| } | ||
|
|
||
| fclose(f); | ||
|
|
||
| /* Delete the cache file immediately to minimize exposure */ |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If unlink() fails (line 249-252), a warning is printed but execution continues. In a security-sensitive context where the cache file contains plaintext tokens, leaving the file on disk is a potential exposure risk. Consider whether the failure to delete the cache file should be treated as a critical error. At minimum, the warning message should emphasize the security implications to help with debugging and monitoring.
This issue also appears on line 295 of the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e767a90 — the cache file is no longer deleted in the constructor. It must survive the full exec chain (capsh → gosu → user command) since each exec() resets static data. Cleanup is handled by the EXIT trap in entrypoint.sh. The file is created with mode 0600 and owned by awfuser.
| 'GITHUB_API_TOKEN', | ||
| 'GITHUB_PAT', | ||
| 'GH_ACCESS_TOKEN', | ||
| 'GITHUB_PERSONAL_ACCESS_TOKEN', |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token name 'GITHUB_PERSONAL_ACCESS_TOKEN' exists in SENSITIVE_ENV_NAMES but is not present in DEFAULT_SENSITIVE_TOKENS in one-shot-token.c (lines 35-51) or in the SENSITIVE_TOKENS default list in entrypoint.sh (lines 406 and 464). According to the PR description and code comments, these three lists must stay aligned. While the PR description notes that GITHUB_PERSONAL_ACCESS_TOKEN is "only present in compose files (not passed to the agent process in default mode)", this creates a risk of misalignment and confusion. Consider either adding it to all three lists for consistency, or adding a clear comment explaining why this token is intentionally only in the TypeScript list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e767a90 — GITHUB_PERSONAL_ACCESS_TOKEN is now in all three lists (one-shot-token.c, entrypoint.sh, docker-manager.ts).
| fi | ||
| done | ||
| # Also add cache file cleanup to the exit trap | ||
| CLEANUP_CMD="${CLEANUP_CMD}; rm -f ${TOKEN_CACHE_FILE} 2>/dev/null || true" |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token cache file is created at /host${TOKEN_CACHE_FILE} (/host/tmp/.awf-token-cache-$$) from the chroot context, but the cleanup command uses ${TOKEN_CACHE_FILE} without the /host prefix. Since the trap runs within the chroot, this path should work, but if the cleanup trap executes after the chroot context has changed or if there's an error, the file might not be deleted. Consider making the cleanup path more explicit or documenting why the /host prefix is intentionally omitted in the cleanup command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation. In e767a90 this is clarified with a comment: the cleanup trap uses ${TOKEN_CACHE_FILE} (without /host prefix) because it runs inside the chroot where the file is at that path. The chroot exec chain always runs within the chroot context, so the path is correct.
src/cli-workflow.ts
Outdated
| // Step 2.5: Redact secrets from docker-compose.yml after containers start | ||
| // This prevents exposure via /host/tmp/awf-*/docker-compose.yml | ||
| dependencies.redactComposeSecrets(config.workDir); |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small race condition window between when containers start and when redaction occurs. The agent container could theoretically read the unredacted docker-compose.yml file via /host/tmp/awf-*/docker-compose.yml before redactComposeSecrets() completes. While this window is narrow (milliseconds) and requires the agent to immediately seek out and read this file, it represents an exposure risk. Consider either: (1) performing redaction before starting containers (if containers don't need the real values), (2) not mounting /tmp into the container at all, or (3) using Docker secrets/configs instead of environment variables in the compose file. The current approach relies on the agent code taking longer to start than the redaction, which may not be reliable under all conditions.
See below for a potential fix:
// Step 2: Redact secrets from docker-compose.yml before containers start
// This prevents exposure via /host/tmp/awf-*/docker-compose.yml inside containers
dependencies.redactComposeSecrets(config.workDir);
// Step 3: Start containers
await dependencies.startContainers(config.workDir, config.allowedDomains, config.proxyLogsDir, config.skipPull);
onContainersStarted?.();
// Step 4: Wait for agent to complete
const result = await dependencies.runAgentCommand(config.workDir, config.allowedDomains, config.proxyLogsDir);
// Step 5: Cleanup (logs will be preserved automatically if they exist)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redaction cannot be moved before startContainers() — Docker reads the compose file during docker compose up to configure container environment variables. Redacting before would prevent containers from receiving the tokens. The race window is mitigated by writing the compose file with mode 0600 (root-only readable), so the agent process (running as awfuser) cannot read it regardless of timing. Added explanatory comment in e767a90.
Co-authored-by: Mossaka <[email protected]>
All four issues addressed in e767a90: 1. Cache file consumed by wrong process: Fixed by removing 2. Token list misalignment: Added 3. Shell echo corruption: Replaced 4. Duplicated scrubbing code: Extracted into a
All review comments addressed in e767a90. |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
@Mossaka looks like some of the CI checks are not passing. This was from a smoke test: yaml: line 154: did not find expected alphabetic or numeric character Error: Command failed with exit code 1: docker compose down -v Node.js v20.20.0 |
…posure-issues-again
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
The `***REDACTED***` replacement value contains `*` which is a YAML alias character. When unquoted, docker compose fails to parse the file with "did not find expected alphabetic or numeric character". Wrapping in double quotes produces valid YAML that docker compose can still read during cleanup (docker compose down -v). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
PR titles: feat: implement selective mounting to prevent credential exfiltration | fix: add apt-get install retry logic to handle stale package archives
|
/proc/self/environprotection via token cache file + LD_PRELOAD constructorredactComposeSecrets()redactComposeSecrets()and workflow orderingGITHUB_PERSONAL_ACCESS_TOKENto all three token lists for alignmentechowithprintf '%s\n'for robust token value writingscrub_sensitive_tokens()shell functionstartContainers()(Docker needs to read compose file)Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.