fix(evaluator): git ownership exception now reaches tools that call git
The argv rewrite added in 491449f fixed `git status` and nothing else.
gitleaks, trivy and semgrep run git themselves, so they still hit:
fatal: detected dubious ownership in repository at '...'
Mission 019fc073 showed both halves at once: git reported a clean tree while
gitleaks "scanned 0 commits", and the judge correctly refused to call the
condition met rather than accepting a scan that had examined nothing. That is
the fail-closed behaviour working — and a scan reporting clean after scanning
zero commits is precisely the false signal this tranche keeps finding.
Replaces the argv rewrite with `GIT_CONFIG_COUNT`/`_KEY_0`/`_VALUE_0`, git's
documented environment form of `-c`. Being environment, it is inherited by
subprocesses, so one setting covers git and every tool that shells out to it.
Still scoped to the single checkout — never `--global` or `*`, which would
disable the protection container-wide.
`container_exec` grows `exec_with_env`; `exec` keeps its signature.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
d90a42b759
commit
dd8dad2ad4
@@ -90,7 +90,19 @@ pub async fn exec(
|
|||||||
argv: &[String],
|
argv: &[String],
|
||||||
timeout: Duration,
|
timeout: Duration,
|
||||||
) -> Result<ExecOutput, String> {
|
) -> Result<ExecOutput, String> {
|
||||||
let fut = exec_inner(docker, container, workdir, argv);
|
exec_with_env(docker, container, workdir, argv, &[], timeout).await
|
||||||
|
}
|
||||||
|
|
||||||
|
/// As [`exec`], with extra environment for the command.
|
||||||
|
pub async fn exec_with_env(
|
||||||
|
docker: &Docker,
|
||||||
|
container: &str,
|
||||||
|
workdir: Option<&str>,
|
||||||
|
argv: &[String],
|
||||||
|
env: &[String],
|
||||||
|
timeout: Duration,
|
||||||
|
) -> Result<ExecOutput, String> {
|
||||||
|
let fut = exec_inner(docker, container, workdir, argv, env);
|
||||||
match tokio::time::timeout(timeout, fut).await {
|
match tokio::time::timeout(timeout, fut).await {
|
||||||
Err(_) => Err(format!(
|
Err(_) => Err(format!(
|
||||||
"timed out after {}s (the command may still be running in {container})",
|
"timed out after {}s (the command may still be running in {container})",
|
||||||
@@ -105,6 +117,7 @@ async fn exec_inner(
|
|||||||
container: &str,
|
container: &str,
|
||||||
workdir: Option<&str>,
|
workdir: Option<&str>,
|
||||||
argv: &[String],
|
argv: &[String],
|
||||||
|
env: &[String],
|
||||||
) -> Result<ExecOutput, String> {
|
) -> Result<ExecOutput, String> {
|
||||||
let created = docker
|
let created = docker
|
||||||
.create_exec(
|
.create_exec(
|
||||||
@@ -112,6 +125,11 @@ async fn exec_inner(
|
|||||||
CreateExecOptions {
|
CreateExecOptions {
|
||||||
cmd: Some(argv.to_vec()),
|
cmd: Some(argv.to_vec()),
|
||||||
working_dir: workdir.map(str::to_string),
|
working_dir: workdir.map(str::to_string),
|
||||||
|
env: if env.is_empty() {
|
||||||
|
None
|
||||||
|
} else {
|
||||||
|
Some(env.to_vec())
|
||||||
|
},
|
||||||
attach_stdout: Some(true),
|
attach_stdout: Some(true),
|
||||||
attach_stderr: Some(true),
|
attach_stderr: Some(true),
|
||||||
..Default::default()
|
..Default::default()
|
||||||
|
|||||||
@@ -258,12 +258,12 @@ impl Sandbox {
|
|||||||
Err(e) => return CheckOutcome::could_not_run(argv, format!("COULD NOT RUN: {e}")),
|
Err(e) => return CheckOutcome::could_not_run(argv, format!("COULD NOT RUN: {e}")),
|
||||||
};
|
};
|
||||||
let workdir = self.workdir.display().to_string();
|
let workdir = self.workdir.display().to_string();
|
||||||
let effective = with_safe_directory(argv, &workdir);
|
let out = crate::container_exec::exec_with_env(
|
||||||
let out = crate::container_exec::exec(
|
|
||||||
&docker,
|
&docker,
|
||||||
&self.container,
|
&self.container,
|
||||||
Some(&workdir),
|
Some(&workdir),
|
||||||
&effective,
|
argv,
|
||||||
|
&git_ownership_env(&workdir),
|
||||||
COMMAND_TIMEOUT,
|
COMMAND_TIMEOUT,
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
@@ -299,36 +299,34 @@ impl Sandbox {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Let git read a checkout it does not own.
|
/// Let git read a checkout it does not own — including from inside another
|
||||||
|
/// tool.
|
||||||
///
|
///
|
||||||
/// The server clones the mission repo as uid 65532; the runtime container the
|
/// The server clones the mission repo as uid 65532; the runtime container the
|
||||||
/// judge execs into runs as root. Git's ownership check then refuses the
|
/// judge execs into runs as root. Git's ownership check then refuses the
|
||||||
/// repository outright:
|
/// repository:
|
||||||
///
|
///
|
||||||
/// ```text
|
/// ```text
|
||||||
/// fatal: detected dubious ownership in repository at '/var/lib/clawmates-missions/<id>/repo'
|
/// fatal: detected dubious ownership in repository at '/var/lib/clawmates-missions/<id>/repo'
|
||||||
/// ```
|
/// ```
|
||||||
///
|
///
|
||||||
/// Observed in production on mission 019fc02e: `git status` returned 128 and
|
/// The first fix rewrote `git` argv to carry `-c safe.directory=…`, which
|
||||||
/// the judge had to infer a clean tree from `ls` instead — weaker evidence for
|
/// worked for `git status` and did nothing for `gitleaks`, which runs git
|
||||||
/// the exact question `git status` answers directly. The protection is aimed
|
/// itself. Observed on mission 019fc073: git reported a clean tree while
|
||||||
/// at multi-user machines where another user could plant a hostile
|
/// gitleaks "scanned 0 commits" and the judge — correctly — refused to call
|
||||||
/// `.git/config`; here the two uids are both ours and the path is one we
|
/// the condition met.
|
||||||
/// constructed, so scoping the exception to this single directory is the
|
|
||||||
/// narrow fix. `--global` would disable the check everywhere in the container.
|
|
||||||
///
|
///
|
||||||
/// Applied per-invocation rather than by configuring the image, so it travels
|
/// `GIT_CONFIG_COUNT`/`_KEY_n`/`_VALUE_n` is git's documented environment form
|
||||||
/// with the workdir and cannot drift out of sync with it.
|
/// of `-c`, and it is inherited, so one setting covers git, gitleaks, trivy,
|
||||||
fn with_safe_directory(argv: &[String], workdir: &str) -> Vec<String> {
|
/// semgrep and anything else that shells out. Scoped to this checkout; never
|
||||||
if argv.first().map(String::as_str) != Some("git") {
|
/// `--global`, which would disable the protection container-wide for every
|
||||||
return argv.to_vec();
|
/// path.
|
||||||
}
|
fn git_ownership_env(workdir: &str) -> Vec<String> {
|
||||||
let mut out = Vec::with_capacity(argv.len() + 2);
|
vec![
|
||||||
out.push("git".to_string());
|
"GIT_CONFIG_COUNT=1".to_string(),
|
||||||
out.push("-c".to_string());
|
"GIT_CONFIG_KEY_0=safe.directory".to_string(),
|
||||||
out.push(format!("safe.directory={workdir}"));
|
format!("GIT_CONFIG_VALUE_0={workdir}"),
|
||||||
out.extend(argv[1..].iter().cloned());
|
]
|
||||||
out
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// One verification command and what became of it.
|
/// One verification command and what became of it.
|
||||||
@@ -503,40 +501,24 @@ mod tests {
|
|||||||
assert!(check_argv(&argv(&["bash", "-c", "ls"])).is_err());
|
assert!(check_argv(&argv(&["bash", "-c", "ls"])).is_err());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The exception must reach tools that invoke git internally, not just
|
||||||
|
/// `git` itself — the first version rewrote argv and left gitleaks
|
||||||
|
/// scanning 0 commits.
|
||||||
#[test]
|
#[test]
|
||||||
fn git_gets_a_scoped_safe_directory_exception() {
|
fn git_ownership_is_set_by_environment_so_subprocesses_inherit_it() {
|
||||||
let out = with_safe_directory(&argv(&["git", "status"]), "/missions/abc/repo");
|
let env = git_ownership_env("/missions/abc/repo");
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
out,
|
env,
|
||||||
argv(&["git", "-c", "safe.directory=/missions/abc/repo", "status"]),
|
vec![
|
||||||
"the exception is scoped to this checkout, never --global"
|
"GIT_CONFIG_COUNT=1".to_string(),
|
||||||
);
|
"GIT_CONFIG_KEY_0=safe.directory".to_string(),
|
||||||
// Flags and subcommand order survive.
|
"GIT_CONFIG_VALUE_0=/missions/abc/repo".to_string(),
|
||||||
let diff = with_safe_directory(&argv(&["git", "diff", "--stat"]), "/w");
|
]
|
||||||
assert_eq!(
|
|
||||||
diff,
|
|
||||||
argv(&["git", "-c", "safe.directory=/w", "diff", "--stat"])
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
#[test]
|
|
||||||
fn non_git_commands_are_untouched() {
|
|
||||||
let cmd = argv(&["cargo", "test"]);
|
|
||||||
assert_eq!(with_safe_directory(&cmd, "/w"), cmd);
|
|
||||||
assert_eq!(with_safe_directory(&[], "/w"), Vec::<String>::new());
|
|
||||||
}
|
|
||||||
|
|
||||||
/// The injected `-c` must not become a way past the allow-list: validation
|
|
||||||
/// runs on the original argv, so the subcommand check still sees `status`
|
|
||||||
/// rather than a flag.
|
|
||||||
#[test]
|
|
||||||
fn injection_happens_after_validation() {
|
|
||||||
assert!(check_argv(&argv(&["git", "push"])).is_err());
|
|
||||||
let rewritten = with_safe_directory(&argv(&["git", "push"]), "/w");
|
|
||||||
assert!(
|
|
||||||
check_argv(&rewritten).is_err(),
|
|
||||||
"a rewritten command must still fail the allow-list"
|
|
||||||
);
|
);
|
||||||
|
// Scoped to the one checkout. `--global`, or a bare `*`, would switch
|
||||||
|
// the protection off for every path in the container.
|
||||||
|
assert!(!env.iter().any(|e| e.contains('*')));
|
||||||
|
assert!(!env.iter().any(|e| e.contains("--global")));
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── What a check may claim about itself ────────────────────────────
|
// ── What a check may claim about itself ────────────────────────────
|
||||||
|
|||||||
Reference in New Issue
Block a user