From 6ede13b854e8ff5dde4f4d97da235f21ad03fa78 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sat, 11 Jul 2026 10:20:30 -0700 Subject: [PATCH] credential: reject seekable stdin, validate header names, honor fs-deny, match decoded query params Signed-off-by: Cong Wang --- crates/sandlock-core/src/credential.rs | 148 ++++++++++++++++++-- crates/sandlock-core/src/sandbox/builder.rs | 82 ++++++++--- 2 files changed, 205 insertions(+), 25 deletions(-) diff --git a/crates/sandlock-core/src/credential.rs b/crates/sandlock-core/src/credential.rs index 325b72c..da4bfbd 100644 --- a/crates/sandlock-core/src/credential.rs +++ b/crates/sandlock-core/src/credential.rs @@ -123,10 +123,12 @@ fn read_capped(r: &mut R, what: &str) -> Result, Sandb /// Read a credential from an already-open fd (e.g. a shell `<(...)` process /// substitution passed as `fd:N`, or the secret piped on stdin as `fd:0`). Reads -/// through a *dup*, so the caller's fd is left open. `fd:0` (stdin) is allowed — -/// it's the most portable secret-passing pattern (`printf %s "$SECRET" | sandlock -/// … --credential k=fd:0`, docker `-i`, systemd credential fds) — but `fd:1`/`fd:2` -/// are refused so a typo can't close/consume stdout/stderr. +/// through a *dup*, so the caller's fd is left open. `fd:1`/`fd:2` are refused so +/// a typo can't close/consume stdout/stderr. `fd:0` (stdin) is allowed for the +/// portable pipe pattern (`printf %s "$SECRET" | sandlock … --credential k=fd:0`, +/// docker `-i`, systemd credential fds), but only when it cannot be rewound: +/// unlike higher fds, stdin survives into the child, and a seekable stdin (a +/// `< secret.txt` redirect) would let the child lseek back and read the secret. fn read_fd(n: i32) -> Result, SandboxError> { use std::os::fd::FromRawFd; if n == 1 || n == 2 { @@ -143,9 +145,25 @@ fn read_fd(n: i32) -> Result, SandboxError> { } // Owns `dup` (not `n`), so only the dup is closed on drop. let mut f = unsafe { std::fs::File::from_raw_fd(dup) }; + if n == 0 && fd_is_rewindable(&mut f) { + return Err(SandboxError::Invalid( + "credential fd 0 (stdin) is seekable, so the sandboxed child could rewind \ + it and re-read the secret; pipe it instead (printf %s \"$SECRET\" | sandlock …)" + .into(), + )); + } read_capped(&mut f, &format!("fd {n}")) } +/// Whether the fd behind `f` can be rewound. Probing the dup is sound because a +/// dup shares the file offset with the original; for the same reason, draining a +/// seekable stdin supervisor-side would not protect the secret. A pipe/socket/tty +/// cannot rewind (lseek fails with ESPIPE). +fn fd_is_rewindable(f: &mut std::fs::File) -> bool { + use std::io::Seek; + f.stream_position().is_ok() +} + /// How the secret is attached to a matching request. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AuthShape { @@ -283,8 +301,12 @@ impl InjectRule { // — no `=`, which `starts_with("key=")` would miss — is still recognised // as the target: otherwise Replace would append `key&key=secret`, and a // first-occurrence-reading upstream would authenticate with the child's - // empty value instead of the injected one. - let is_target = |kv: &str| kv.split('=').next().unwrap_or(kv) == enc; + // empty value instead of the injected one. Compare names DECODED, so a + // child spelling the name with stray percent-encoding (`%6Bey=` for + // `key=`) still counts as the target instead of evading the filter. + let is_target = |kv: &str| { + percent_decode_lossy(kv.split('=').next().unwrap_or(kv)) == param.as_bytes() + }; // Honor AddOnly: don't append a param the request already carries. if self.on_existing == OnExistingHeader::AddOnly { if let Some(q) = existing { @@ -341,8 +363,17 @@ pub fn parse_auth(spec: &str, credential: &str) -> Result AuthShape::Basic { username: user.to_string() }, - // `apikey:
` and `header:` are the same rendering. - ("header" | "apikey", Some(name)) if !name.is_empty() => AuthShape::Header { name: name.to_string() }, + // `apikey:
` and `header:` are the same rendering. Validate + // the name here so a typo fails at build time; unchecked, it would only + // surface as a per-request 502 when `apply` first renders the header. + ("header" | "apikey", Some(name)) if !name.is_empty() => { + if hyper::header::HeaderName::from_bytes(name.as_bytes()).is_err() { + return Err(SandboxError::Invalid(format!( + "invalid header name {name:?} in auth shape {spec:?} for credential {credential:?}" + ))); + } + AuthShape::Header { name: name.to_string() } + } ("query", Some(param)) if !param.is_empty() => AuthShape::Query { param: param.to_string() }, _ => { return Err(SandboxError::Invalid(format!( @@ -480,6 +511,28 @@ fn base64_encode(input: &[u8]) -> String { out } +/// Decode `%XX` escapes in a query token to raw bytes (an invalid escape is kept +/// literally), so two spellings of the same param name compare equal. +fn percent_decode_lossy(s: &str) -> Vec { + let b = s.as_bytes(); + let mut out = Vec::with_capacity(b.len()); + let mut i = 0; + while i < b.len() { + if b[i] == b'%' && i + 2 < b.len() { + let hi = (b[i + 1] as char).to_digit(16); + let lo = (b[i + 2] as char).to_digit(16); + if let (Some(hi), Some(lo)) = (hi, lo) { + out.push((hi as u8) << 4 | lo as u8); + i += 3; + continue; + } + } + out.push(b[i]); + i += 1; + } + out +} + /// Percent-encode raw bytes as a query component (encode everything not /// unreserved, so `&`/`=`/`#`/`%` and any binary byte can't break out). fn urlencode_bytes(bytes: &[u8]) -> String { @@ -695,6 +748,85 @@ mod tests { assert!(load_secret("fd:2").is_err()); // stderr (fd:0/stdin is now allowed) } + #[test] + fn fd0_rejects_seekable_stdin_but_allows_pipe() { + use std::os::fd::IntoRawFd; + // A seekable stdin (file redirect) must be refused: stdin survives into + // the child, which could lseek back and re-read the secret. A pipe on + // stdin (the documented pattern) can't rewind and stays allowed. Lib + // tests run single-threaded in CI, and stdin is restored either way. + let path = std::env::temp_dir() + .join(format!("sandlock-cred-fd0-{}", std::process::id())); + std::fs::write(&path, "sk-file\n").unwrap(); + let file_fd = std::fs::File::open(&path).unwrap().into_raw_fd(); + let saved = unsafe { libc::dup(0) }; + assert!(saved >= 0); + + assert_eq!(unsafe { libc::dup2(file_fd, 0) }, 0); + let res_file = load_secret("fd:0"); + + let mut fds = [0i32; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0); + unsafe { + assert_eq!(libc::write(fds[1], b"sk-pipe\n".as_ptr().cast(), 8), 8); + libc::close(fds[1]); + libc::dup2(fds[0], 0); + libc::close(fds[0]); + } + let res_pipe = load_secret("fd:0"); + + unsafe { + libc::dup2(saved, 0); // restore the real stdin before asserting + libc::close(saved); + libc::close(file_fd); + } + let _ = std::fs::remove_file(&path); + + let err = res_file.expect_err("seekable stdin must be rejected"); + assert!(err.to_string().contains("pipe it instead"), "got: {err}"); + assert_eq!(res_pipe.unwrap().expose(), b"sk-pipe"); + } + + #[test] + fn fd_above_two_may_be_seekable() { + use std::os::fd::IntoRawFd; + // Fds above 2 are closed in the child post-fork, so a seekable file fd + // is safe there; only fd 0 carries the rewind restriction. + let path = std::env::temp_dir() + .join(format!("sandlock-cred-fdn-{}", std::process::id())); + std::fs::write(&path, "sk-n").unwrap(); + let fd = std::fs::File::open(&path).unwrap().into_raw_fd(); + let s = load_secret(&format!("fd:{fd}")).unwrap(); + assert_eq!(s.expose(), b"sk-n"); + unsafe { libc::close(fd) }; + let _ = std::fs::remove_file(&path); + } + + #[test] + fn parse_auth_rejects_invalid_header_name_at_build_time() { + // A malformed header name must fail when the rule is parsed, not as a + // per-request 502 the first time the rule fires. + assert!(parse_auth("header:bad name", "c").is_err()); + assert!(parse_auth("apikey:x:y", "c").is_err()); + assert!(parse_auth("header:x-ok", "c").is_ok()); + } + + #[test] + fn query_encoded_spelling_of_param_is_still_target() { + // `%6Bey` decodes to `key`: Replace must treat it as the target and drop + // it, not leave the child's pair alongside the injected one. + let mut p = parts_of("https://api.example.com/x?%6Bey=child&a=1", &[]); + rule(AuthShape::Query { param: "key".into() }, "sk-real", OnExistingHeader::Replace) + .apply(&mut p).unwrap(); + assert_eq!(p.uri.query().unwrap(), "a=1&key=sk-real"); + + // AddOnly sees the encoded spelling as present and keeps the child's value. + let mut p2 = parts_of("https://api.example.com/x?%6Bey=child", &[]); + let r = rule(AuthShape::Query { param: "key".into() }, "sk-real", OnExistingHeader::AddOnly); + assert!(matches!(r.apply(&mut p2), Ok(Applied::Skipped))); + assert_eq!(p2.uri.query().unwrap(), "%6Bey=child"); + } + #[test] fn resolve_default_is_replace_add_only_is_opt_in() { std::env::set_var("SANDLOCK_TEST_ONEX", "sk-x"); diff --git a/crates/sandlock-core/src/sandbox/builder.rs b/crates/sandlock-core/src/sandbox/builder.rs index efe1f5f..3a25d6e 100644 --- a/crates/sandlock-core/src/sandbox/builder.rs +++ b/crates/sandlock-core/src/sandbox/builder.rs @@ -753,33 +753,28 @@ impl SandboxBuilder { // is often a broad dir the workload needs), so warn on the overlap. Every // exposing grant is covered: read grants, write grants (write access // includes read), bind-mounted host dirs, and the chroot root (visible - // regardless of Landlock). Best-effort — canonicalize where possible. + // regardless of Landlock). An fs-deny covering the file suppresses the + // warning, so following its own advice actually silences it. for c in &self.credentials { let Some(path) = c.split_once('=').and_then(|(_, s)| s.strip_prefix("file:")) else { continue; }; - let secret = std::path::Path::new(path); - let secret_abs = secret.canonicalize(); - let secret_ref = secret_abs.as_deref().unwrap_or(secret); let grants = self .fs_readable .iter() .chain(self.fs_writable.iter()) .chain(self.fs_mount.iter().map(|(_, host)| host)) .chain(self.chroot.as_ref()); - for grant in grants { - let grant_abs = grant.canonicalize(); - let grant_ref = grant_abs.as_deref().unwrap_or(grant.as_path()); - if secret_ref.starts_with(grant_ref) { - eprintln!( - "sandlock: warning: credential file {} is inside the sandbox grant {} — \ - the sandboxed child can read the secret directly; keep it outside every \ - fs grant (or add an fs-deny for it)", - path, - grant.display() - ); - break; - } + if let Some(grant) = + exposing_grant(std::path::Path::new(path), grants, &self.fs_denied) + { + eprintln!( + "sandlock: warning: credential file {} is inside the sandbox grant {}; \ + the sandboxed child can read the secret directly; keep it outside every \ + fs grant (or add an fs-deny for it)", + path, + grant.display() + ); } } // Resolve credentials + injection rules, loading each secret into the @@ -907,3 +902,56 @@ impl SandboxBuilder { Ok(p) } } + +/// The first fs grant that exposes `secret` to the sandboxed child, or `None` +/// when no grant reaches it or an fs-deny covers it (the deny closes the hole, +/// so no warning is due). Best-effort: canonicalize where possible. +fn exposing_grant<'a>( + secret: &std::path::Path, + grants: impl Iterator, + denies: &[std::path::PathBuf], +) -> Option { + let secret_abs = secret.canonicalize(); + let secret_ref = secret_abs.as_deref().unwrap_or(secret); + let covered_by = |p: &std::path::PathBuf| { + let abs = p.canonicalize(); + secret_ref.starts_with(abs.as_deref().unwrap_or(p.as_path())) + }; + if denies.iter().any(&covered_by) { + return None; + } + grants.into_iter().find(|g| covered_by(g)).cloned() +} + +#[cfg(test)] +mod tests { + use super::exposing_grant; + use std::path::PathBuf; + + #[test] + fn exposing_grant_reports_overlap_and_fs_deny_suppresses() { + let dir = std::env::temp_dir().join(format!("sandlock-grant-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let secret = dir.join("key.txt"); + std::fs::write(&secret, "s").unwrap(); + let grants = vec![dir.clone()]; + + // Inside a read grant: the exposing grant is reported. + assert_eq!(exposing_grant(&secret, grants.iter(), &[]), Some(dir.clone())); + // An fs-deny on the file itself, or on a covering directory, closes the + // hole: following the warning's own advice must silence it. + assert_eq!(exposing_grant(&secret, grants.iter(), std::slice::from_ref(&secret)), None); + assert_eq!(exposing_grant(&secret, grants.iter(), std::slice::from_ref(&dir)), None); + // A deny elsewhere does not suppress the warning. + assert_eq!( + exposing_grant(&secret, grants.iter(), &[PathBuf::from("/nonexistent-deny")]), + Some(dir.clone()) + ); + // Outside every grant: nothing to report. + let other = vec![PathBuf::from("/nonexistent-grant")]; + assert_eq!(exposing_grant(&secret, other.iter(), &[]), None); + + let _ = std::fs::remove_file(&secret); + let _ = std::fs::remove_dir(&dir); + } +}