Pārlūkot izejas kodu

fix(ssh): pass through already single-quoted tokens in shq

Replace the shallow `if starts_with('\'')` special case in the docker command
builder with a correct analysis inside shq: a value that is already a complete,
well-formed single-quoted shell word (e.g. an operator pre-quoted recipe arg
such as `'{"json":1}'`) is self-delimiting and is passed through unchanged.
Single quotes already suppress every metachar/interpolation, so re-escaping
would corrupt the token; the old check only looked at the first character and
silently mishandled unbalanced quotes or multi-token input.

Everything else is still wrapped in single quotes with interior quotes escaped,
so the existing hardening for data (paths, env values, mount points, labels) is
unchanged and keeps the security tests green. Adds focused shq unit tests.
Alexander Korolev 2 mēneši atpakaļ
vecāks
revīzija
5a6ec550d7
3 mainītis faili ar 53 papildinājumiem un 7 dzēšanām
  1. 1 7
      src/docker/command.rs
  2. 22 0
      src/ssh.rs
  3. 30 0
      src/ssh_tests.rs

+ 1 - 7
src/docker/command.rs

@@ -315,13 +315,7 @@ pub(crate) fn build_docker_command(
         cmd.push(img);
         // Shell-quote every tail word (entrypoint, model, args, flags) so a
         // crafted recipe arg/env cannot break out of the remote `docker run`.
-        cmd.extend(tail.iter().map(|t| {
-            if t.starts_with('\'') {
-                t.clone()
-            } else {
-                shq(t)
-            }
-        }));
+        cmd.extend(tail.iter().map(|t| shq(t)));
     } else {
         let script = mods::build_mod_script(&ctx.recipe.mods.commands, &tail);
         // `--entrypoint sh` and `-c` stay literal; `img` and `script` are

+ 22 - 0
src/ssh.rs

@@ -298,6 +298,15 @@ async fn fetch_host_key_data(config: &Config, node: &Node) -> Result<Vec<u8>> {
 }
 
 pub(crate) fn shq(s: &str) -> String {
+    // A value that is already a complete single-quoted shell word (e.g. a recipe
+    // arg the operator pre-quoted, like `'{"json":1}'`) is self-delimiting and
+    // needs no escaping: single quotes already suppress every metachar/interp,
+    // so wrapping it again would corrupt the token (the old `if starts_with('''`
+    // special case). Everything else is wrapped in single quotes with any
+    // interior quote escaped.
+    if is_single_quoted_token(s) {
+        return s.to_string();
+    }
     let mut out = String::with_capacity(s.len() + 2);
     out.push('\'');
     for c in s.chars() {
@@ -311,6 +320,15 @@ pub(crate) fn shq(s: &str) -> String {
     out
 }
 
+/// True when `s` is a well-formed single-quoted shell word: it starts and ends
+/// with a single quote and its interior contains no (unescaped) quote. A valid
+/// single-quoted token cannot inject — the shell treats its whole body as one
+/// literal argument.
+fn is_single_quoted_token(s: &str) -> bool {
+    let b = s.as_bytes();
+    b.len() >= 2 && b[0] == b'\'' && b[b.len() - 1] == b'\'' && !s[1..s.len() - 1].contains('\'')
+}
+
 /// Upload an in-memory script body to a node and make it executable.
 ///
 /// Cheaper than [`upload_dir`]: no tar, just base64-encode the body and
@@ -445,3 +463,7 @@ impl client::Handler for VerifyingHandler {
         Ok(true)
     }
 }
+
+#[cfg(test)]
+#[path = "ssh_tests.rs"]
+mod tests;

+ 30 - 0
src/ssh_tests.rs

@@ -0,0 +1,30 @@
+use super::shq;
+
+#[test]
+fn shq_quotes_plain_word() {
+    assert_eq!(shq("--kv-cache-dtype"), "'--kv-cache-dtype'");
+}
+
+#[test]
+fn shq_escapes_interior_quote() {
+    assert_eq!(shq("it's"), r#"'it'\''s'"#);
+}
+
+#[test]
+fn shq_passes_through_already_quoted_token() {
+    // an operator pre-quoted value (e.g. JSON arg) is a single-quoted shell
+    // word already -> pass through, do not double-quote it.
+    assert_eq!(shq("'{\"cuda\":\"x\"}'"), "'{\"cuda\":\"x\"}'");
+}
+
+#[test]
+fn shq_quotes_injectable_value_whole() {
+    // metachars keep the whole value as ONE quoted argument.
+    assert_eq!(shq("x; touch /tmp/pwned"), "'x; touch /tmp/pwned'");
+}
+
+#[test]
+fn shq_quotes_unbalanced_leading_quote() {
+    // a single leading quote is not a complete token -> still escaped.
+    assert_eq!(shq("'untuple"), r#"''\''untuple'"#);
+}