feat: replace env file with CLI args for --elevate env restore#222
feat: replace env file with CLI args for --elevate env restore#222Xeonacid wants to merge 1 commit into
Conversation
Pass each environment variable as a separate --restore-env KEY=VALUE argument instead of writing to a temp file and passing --restore-env-file. This eliminates the file-based TOCTOU surface and the need for temp file security checks.
|
@Xeonacid is attempting to deploy a commit to the kxxt's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR replaces environment variable persistence during privilege escalation from file-based (temporary file with serialized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tracexec-core/src/elevate.rs (1)
181-215:⚠️ Potential issue | 🟠 MajorAddress E2BIG risk when passing large environments to elevated sudo.
The
elevate_and_reexec()function captures all environment variables and serializes each one into two consecutive argv elements (--restore-envflag +KEY=VALUE). Sincesudois invoked without-E, the parent's originalenvpis also inherited by the child process. For users with large environments (CI runners with hundreds of variables, development shells with expansiveLS_COLORSorBASH_FUNC_*entries, locales with long paths), the combinedargv + envpcan exceedARG_MAX(~128 KiB to 2 MiB depending on platform/stack rlimit), causingexecve(2)to fail withE2BIG. This error surfaces as an opaqueerr.into()on line 214 with no diagnostic hint.Consider:
- Pre-exec validation: Measure the aggregate argv size before calling
exec()and either produce a targeted error message or fall back to an alternative mechanism (e.g.,memfd_create+--restore-env-fd <fd>).- Filter non-restorable entries: Skip shell-specific entries like
BASH_FUNC_*%%, which are often large and unnecessary for the elevated process.- Document the limit: Add a note to the
--elevatehelp text explaining the environment size constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tracexec-core/src/elevate.rs` around lines 181 - 215, elevate_and_reexec currently serializes every env var into argv entries (via capture_env_entries / build_elevated_args) and then calls Command::new("sudo").arg(&exe).args(&elevated_args).exec(), which risks execve E2BIG when argv+envp exceed ARG_MAX; add a pre-exec validation that computes the total byte size of argv (exe + elevated_args) plus current envp and if it exceeds a conservative threshold emit a clear error suggesting workarounds (or fall back to an alternative like memfd + a new flag), and also filter out large/unrestorable shell-specific vars (e.g., keys matching BASH_FUNC_* or other noisy patterns) in capture_env_entries/build_elevated_args so they are not included in --restore-env; update the error path to include the size measured and mention --restore-env limits in the --elevate help text.
🧹 Nitpick comments (3)
tests/smoke.rs (1)
70-80: Optional: considervar_os+OsStringcomposition for thePATHargument.
std::env::var("PATH").unwrap_or_default()silently drops bytes if the caller'sPATHis not UTF-8. Since the whole point of the surrounding code is to preserve arbitraryOsStringenv values, you could build the--restore-envargument withOsString+push("PATH=")+push(os_path)for parity with production behavior. Not a correctness issue under typical CI, so feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/smoke.rs` around lines 70 - 80, The test currently uses std::env::var("PATH").unwrap_or_default() which can drop non-UTF8 bytes; change the construction of the PATH restore argument to use std::env::var_os("PATH") and compose an OsString by starting with "PATH=" (OsString) then push the returned OsString (or empty OsString if None) so the .arg(...) call receives an OsString preserving non-UTF8 content; update the spot that builds the PATH restore arg (the expression passed to .arg(format!("PATH={}", ...)) in the test) to use this OsString composition instead.crates/tracexec-core/src/cli.rs (1)
79-85: Help text is worded as singular, but the flag can be repeated.Since the field is
Vec<OsString>and users will pass--restore-env KEY=VALUEmultiple times, consider wording that conveys repeatability, e.g. "Internal: restore an environment variable from --elevate (KEY=VALUE); may be specified multiple times". The flag is hidden from--help, but it still shows up in generated shell completions.✏️ Proposed wording tweak
#[arg( long = "restore-env", hide = true, conflicts_with = "elevate", - help = "Internal: restore an environment variable from --elevate (KEY=VALUE)" + help = "Internal: restore an environment variable from --elevate (KEY=VALUE); may be repeated" )] pub restore_env: Vec<OsString>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tracexec-core/src/cli.rs` around lines 79 - 85, Help text for the restore_env argument is singular but the field is Vec<OsString> and the flag can be repeated; update the help string on the #[arg(...)] for restore_env to indicate repeatability (e.g., "Internal: restore environment variables from --elevate (KEY=VALUE); may be specified multiple times") so shell completions and users understand it accepts multiple instances. Locate the restore_env field and its #[arg(...)] attributes and replace the current help text accordingly, preserving hide = true and conflicts_with = "elevate".crates/tracexec-core/src/elevate.rs (1)
245-303: Tests reimplement parsing locally rather than exercisingrestore_env_from_entries.
test_restore_env_from_entries_roundtrip,test_restore_env_handles_non_utf8, andtest_restore_env_from_entries_rejects_missing_equalsall duplicate the split-on-=logic inline instead of callingrestore_env_from_entries. I understand the real function clobbers the process env and is unsafe to call from the test harness — but then the current tests only cover a parallel copy of the parser, so a divergence (e.g. switching tosplitn, rejecting empty keys, normalizing Unicode) could regress silently.Options:
- Extract the
&OsStr -> Result<(OsString, OsString)>parsing into a purefn parse_entry(...)helper and test that directly.restore_env_from_entriesthen becomes a thinclear + for entry { parse + set_var }wrapper.- Or run the real function inside a forked child / subprocess test where clobbering env is harmless.
The first option is low-risk and gives you real coverage of the production parser, including the error path for missing
=.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tracexec-core/src/elevate.rs` around lines 245 - 303, Tests duplicate the split-on-'=' logic instead of exercising restore_env_from_entries; extract the parsing logic into a pure helper fn parse_entry(entry: &OsStr) -> Result<(OsString, OsString), ParseError> (or similar) that implements the exact current behavior (handles non-UTF8, preserves bytes after the first '=', returns Err when '=' missing or key empty), update restore_env_from_entries to call parse_entry for each entry and then set/clear the env, and change the three tests (test_restore_env_from_entries_roundtrip, test_restore_env_handles_non_utf8, test_restore_env_from_entries_rejects_missing_equals) to call parse_entry directly and assert on Ok/Err results so parsing is covered without clobbering process env.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tracexec-core/src/elevate.rs`:
- Around line 9-12: The docstring in elevate.rs currently states that
environment variables passed as CLI args "are only visible to root (via
/proc/<pid>/cmdline)" which is incorrect; update the Security section to say
that /proc/<pid>/cmdline is typically world-readable on standard Linux (so
CLI-passed secrets can be read by unprivileged local users) unless procfs is
mounted with hidepid=1|2, and explicitly call out that this makes --restore-env
entries potentially exposed for the lifetime of the elevated tracexec process;
in the same docstring (around the existing Security header and any mention of
--restore-env) add a brief note about mitigations/alternatives (e.g., use
hidepid, avoid passing secrets on cmdline, or use a more secure secret-passing
mechanism) and remove the incorrect claim that only root can view
/proc/<pid>/cmdline.
---
Outside diff comments:
In `@crates/tracexec-core/src/elevate.rs`:
- Around line 181-215: elevate_and_reexec currently serializes every env var
into argv entries (via capture_env_entries / build_elevated_args) and then calls
Command::new("sudo").arg(&exe).args(&elevated_args).exec(), which risks execve
E2BIG when argv+envp exceed ARG_MAX; add a pre-exec validation that computes the
total byte size of argv (exe + elevated_args) plus current envp and if it
exceeds a conservative threshold emit a clear error suggesting workarounds (or
fall back to an alternative like memfd + a new flag), and also filter out
large/unrestorable shell-specific vars (e.g., keys matching BASH_FUNC_* or other
noisy patterns) in capture_env_entries/build_elevated_args so they are not
included in --restore-env; update the error path to include the size measured
and mention --restore-env limits in the --elevate help text.
---
Nitpick comments:
In `@crates/tracexec-core/src/cli.rs`:
- Around line 79-85: Help text for the restore_env argument is singular but the
field is Vec<OsString> and the flag can be repeated; update the help string on
the #[arg(...)] for restore_env to indicate repeatability (e.g., "Internal:
restore environment variables from --elevate (KEY=VALUE); may be specified
multiple times") so shell completions and users understand it accepts multiple
instances. Locate the restore_env field and its #[arg(...)] attributes and
replace the current help text accordingly, preserving hide = true and
conflicts_with = "elevate".
In `@crates/tracexec-core/src/elevate.rs`:
- Around line 245-303: Tests duplicate the split-on-'=' logic instead of
exercising restore_env_from_entries; extract the parsing logic into a pure
helper fn parse_entry(entry: &OsStr) -> Result<(OsString, OsString), ParseError>
(or similar) that implements the exact current behavior (handles non-UTF8,
preserves bytes after the first '=', returns Err when '=' missing or key empty),
update restore_env_from_entries to call parse_entry for each entry and then
set/clear the env, and change the three tests
(test_restore_env_from_entries_roundtrip, test_restore_env_handles_non_utf8,
test_restore_env_from_entries_rejects_missing_equals) to call parse_entry
directly and assert on Ok/Err results so parsing is covered without clobbering
process env.
In `@tests/smoke.rs`:
- Around line 70-80: The test currently uses
std::env::var("PATH").unwrap_or_default() which can drop non-UTF8 bytes; change
the construction of the PATH restore argument to use std::env::var_os("PATH")
and compose an OsString by starting with "PATH=" (OsString) then push the
returned OsString (or empty OsString if None) so the .arg(...) call receives an
OsString preserving non-UTF8 content; update the spot that builds the PATH
restore arg (the expression passed to .arg(format!("PATH={}", ...)) in the test)
to use this OsString composition instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a7f083cb-6e6f-46be-8c38-7e062a77168d
📒 Files selected for processing (4)
crates/tracexec-core/src/cli.rscrates/tracexec-core/src/elevate.rssrc/main.rstests/smoke.rs
| //! ## Security | ||
| //! | ||
| //! The environment file is created with mode 0600 and owned by the current user. | ||
| //! On restore, the file is opened with `O_NOFOLLOW` (rejecting symlinks), its | ||
| //! metadata is checked via `fstat` (avoiding TOCTOU races), and it is unlinked | ||
| //! immediately after reading. Only the original user and root can access it. | ||
| //! Environment variables are passed as CLI arguments. The data is only visible | ||
| //! to root (via /proc/<pid>/cmdline) after the sudo exec. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is /proc/<pid>/cmdline world-readable by default on Linux, and how does the hidepid procfs mount option change this?
💡 Result:
Yes, /proc/<pid>/cmdline is world-readable by default on Linux. The hidepid procfs mount option restricts access to it for non-root users or non-owners.
Citations:
- 1: https://unix.stackexchange.com/questions/673641/is-there-any-reason-why-proc-cmdline-is-world-readable
- 2: https://man7.org/linux/man-pages/man5/proc.5.html
- 3: https://linux.die.net/man/5/proc
- 4: https://www.man7.org/linux/man-pages/man5/proc.5.html
🏁 Script executed:
cat -n crates/tracexec-core/src/elevate.rs | head -20Repository: kxxt/tracexec
Length of output: 863
Security documentation inaccuracy — /proc/<pid>/cmdline is not root-only by default.
The docstring at lines 9–12 claims environment variable data "is only visible to root (via /proc//cmdline) after the sudo exec." This is incorrect on standard Linux systems where /proc/<pid>/cmdline is world-readable by default (that is how ps and top work for unprivileged users). Access is only restricted under non-default hidepid=1|2 procfs mount options.
This means any unprivileged local user can read --restore-env entries containing secrets (e.g., AWS_SECRET_ACCESS_KEY, GITHUB_TOKEN, Kerberos credentials) for the lifetime of the elevated tracexec process. This is a documented trade-off against the previous tempfile approach, which was less visible in cmdline but had TOCTOU surface, yet users relying on the false guarantee here have no visibility into that trade-off.
Update the docstring to accurately describe the security posture and thread model:
Suggested docstring fix
//! ## Security
//!
-//! Environment variables are passed as CLI arguments. The data is only visible
-//! to root (via /proc/<pid>/cmdline) after the sudo exec.
+//! Environment variables are passed as CLI arguments to the elevated process.
+//! Note that `/proc/<pid>/cmdline` is world-readable on default Linux procfs
+//! mounts, so any local user can observe these entries (including any secrets
+//! they contain) for the lifetime of the elevated tracexec process. This is a
+//! deliberate trade-off against the previous tempfile-based approach, which
+//! avoided cmdline exposure but added file-handling TOCTOU surface. If your
+//! threat model includes unprivileged local users, avoid running tracexec
+//! under `--elevate` with sensitive variables in your environment, or mount
+//! /proc with `hidepid=2`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tracexec-core/src/elevate.rs` around lines 9 - 12, The docstring in
elevate.rs currently states that environment variables passed as CLI args "are
only visible to root (via /proc/<pid>/cmdline)" which is incorrect; update the
Security section to say that /proc/<pid>/cmdline is typically world-readable on
standard Linux (so CLI-passed secrets can be read by unprivileged local users)
unless procfs is mounted with hidepid=1|2, and explicitly call out that this
makes --restore-env entries potentially exposed for the lifetime of the elevated
tracexec process; in the same docstring (around the existing Security header and
any mention of --restore-env) add a brief note about mitigations/alternatives
(e.g., use hidepid, avoid passing secrets on cmdline, or use a more secure
secret-passing mechanism) and remove the incorrect claim that only root can view
/proc/<pid>/cmdline.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
==========================================
- Coverage 73.04% 72.95% -0.09%
==========================================
Files 80 80
Lines 18194 18113 -81
==========================================
- Hits 13289 13215 -74
+ Misses 4905 4898 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Pass each environment variable as a separate --restore-env KEY=VALUE argument instead of writing to a temp file and passing --restore-env-file. This eliminates the file-based TOCTOU surface and the need for temp file security checks.
Summary by CodeRabbit