credential: address the four remaining findings from the PR #127 review

crates/sandlock-core/src/credential.rs

@@ -123,10 +123,12 @@ fn read_capped<R: std::io::Read>(r: &mut R, what: &str) -> Result<Vec<u8>, 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<Vec<u8>, SandboxError> {
     use std::os::fd::FromRawFd;
     if n == 1 || n == 2 {
@@ -143,9 +145,25 @@ fn read_fd(n: i32) -> Result<Vec<u8>, 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, SandboxErro
             )))
         }
         ("basic", Some(user)) if !user.is_empty() => AuthShape::Basic { username: user.to_string() },
-        // `apikey:<header>` and `header:<name>` are the same rendering.
-        ("header" | "apikey", Some(name)) if !name.is_empty() => AuthShape::Header { name: name.to_string() },
+        // `apikey:<header>` and `header:<name>` 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<u8> {
+    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");

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<Item = &'a std::path::PathBuf>,
+    denies: &[std::path::PathBuf],
+) -> Option<std::path::PathBuf> {
+    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);
+    }
+}