From 6d44f77671ddf42310599ef27ee8f4c45d54846c Mon Sep 17 00:00:00 2001 From: marcusH Date: Sun, 19 Jul 2026 23:47:29 +0200 Subject: [PATCH] =?UTF-8?q?Sicherheitsrunde:=20Befehlsinjektion,=20Pool-Pf?= =?UTF-8?q?ad,=20Namenspr=C3=BCfung,=20Bild-Beacon?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vier Befunde aus einer Prüfung entlang der Angriffsflächen (MCP-Tools, Command-Grenze, Credentials, Schreibpfade und Abhängigkeiten): - todo.rs: Der SessionStart-Hook interpolierte den Projektpfad unquotiert in eine Shell-Kommandozeile, die Claude Code bei jedem Sessionstart ausfuehrt. Ein Ordner `repo$(...)` -- etwa aus einem geklonten Fremd-Repo -- ergab dauerhafte Codeausfuehrung; ein Leerzeichen zerbrach den Hook still. Der Pfad wird jetzt gequotet, inklusive Apostroph. - project_pool_dir: Der Pool-Name kommt aus der ai-control.json im Projektordner, also aus einer versionierten Datei. Ungeprueft bestimmte er mit ../ ein beliebiges Verzeichnis als CLAUDE_CONFIG_DIR, dessen settings.json wiederum apiKeyHelper traegt -- ein Kommando, das beim Sessionstart laeuft. Jetzt durch check_name abgesichert. - check_name: Steuerzeichen, Backslash und fuehrender Punkt werden abgelehnt. Ein Newline im Projektnamen hing bisher eigene Schluessel an die .desktop- Datei; Exec= braucht keinen Schraegstrich, den der alte Check verbot. - markdown.ts: Bilder duerfen nur noch lokale Ziele laden. Ein auswaertiges laedt ohne Zutun und meldet damit IP, Zeitpunkt und im Pfad kodierte Daten -- der Panelinhalt stammt aus einer LLM-Session, ist also injizierbar. Schema-relative URLs (//host/x) waren durch ein zu laxes Muster erfasst. Links bleiben auswaerts erlaubt, jetzt mit rel="noopener noreferrer". escapeHtml escaped zusaetzlich das Apostroph. Dazu cargo audit nachgeholt: plist 1.9.0 -> 1.10.0 hebt quick-xml auf 0.41.0 und schliesst RUSTSEC-2026-0194/0195 fuer den Laufzeitpfad. Die zweite Instanz von quick-xml 0.39.4 bleibt ueber wayland-scanner im Lockfile -- ein proc-macro, das nur zur Compile-Zeit laeuft und ausgelieferte Protokoll-XMLs parst, nicht im Binary landet. 77 Rust-Tests, 39 Frontend-Tests. --- src-tauri/Cargo.lock | 17 ++++++++++--- src-tauri/src/domain/mod.rs | 43 +++++++++++++++++++++++++++++++-- src-tauri/src/domain/project.rs | 28 ++++++++++++++++++++- src-tauri/src/domain/todo.rs | 30 ++++++++++++++++++++++- src/markdown.test.ts | 40 +++++++++++++++++++++++++++--- src/markdown.ts | 42 +++++++++++++++++++++++--------- 6 files changed, 178 insertions(+), 22 deletions(-) diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 71b2e87..9ebcf82 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -2988,13 +2988,13 @@ checksum = "19f132c84eca552bf34cab8ec81f1c1dcc229b811638f9d283dceabe58c5569e" [[package]] name = "plist" -version = "1.9.0" +version = "1.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "092791278e026273c1b65bbdcfbba3a300f2994c896bd01ab01da613c29c46f1" +checksum = "7da1d65da6dd5d1e44199ac0f58712d241c0f439f80adea8924d832384087f85" dependencies = [ "base64 0.22.1", "indexmap 2.14.0", - "quick-xml", + "quick-xml 0.41.0", "serde", "time", ] @@ -3193,6 +3193,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "quick-xml" +version = "0.41.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e660451e55124f798a69a5af3f49ccfbefbd41910eefd25caf2393e1f3473ec1" +dependencies = [ + "memchr", +] + [[package]] name = "quote" version = "1.0.46" @@ -5203,7 +5212,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9c324a910fd86ebdc364a3e61ec1f11737d3b1d6c273c0239ee8ff4bc0d24b4a" dependencies = [ "proc-macro2", - "quick-xml", + "quick-xml 0.39.4", "quote", ] diff --git a/src-tauri/src/domain/mod.rs b/src-tauri/src/domain/mod.rs index 9ce993a..551e415 100644 --- a/src-tauri/src/domain/mod.rs +++ b/src-tauri/src/domain/mod.rs @@ -18,10 +18,49 @@ pub(crate) mod watcher; #[cfg(test)] pub(crate) mod testutil; -/// Namensprüfung für Projekte und Pool-Anzeigenamen (keine Pfad-Bestandteile). +/// Namensprüfung für Projekte und Pool-Anzeigenamen. +/// +/// Der Name wird als Pfadsegment verwendet (Pool-Verzeichnis, Panel-Dateien) +/// und landet zugleich in Dateien mit zeilenbasiertem Format — vor allem in der +/// `.desktop`-Datei unter Linux. Darum reicht das Verbot von `/` und `..` +/// nicht: Ein Zeilenumbruch im Namen hängt dort eigene Schlüssel an, und +/// `Exec=` braucht keinen Schrägstrich. Projektnamen stammen aus +/// `dir.file_name()`, bei einem geklonten Fremd-Repo also von außen. pub(crate) fn check_name(name: &str) -> Result<(), String> { - if name.trim().is_empty() || name.contains('/') || name.contains("..") { + let ungueltig = name.trim().is_empty() + || name.contains('/') + || name.contains('\\') + || name.contains("..") + || name.starts_with('.') + || name.chars().any(|c| c.is_control()); + if ungueltig { return Err(format!("ungültiger Name: {name}")); } Ok(()) } + +#[cfg(test)] +mod tests { + use super::check_name; + + #[test] + fn namen_pruefung() { + for ok in ["projekt", "mein-projekt", "Projekt 2", "äöü"] { + assert!(check_name(ok).is_ok(), "{ok} sollte gültig sein"); + } + for bad in [ + "", + " ", + "a/b", + "a\\b", + "..", + "../x", + ".versteckt", + "boo\nExec=bash -c pwn", // .desktop-Injektion + "a\tb", + "a\u{7f}b", + ] { + assert!(check_name(bad).is_err(), "{bad:?} sollte abgelehnt werden"); + } + } +} diff --git a/src-tauri/src/domain/project.rs b/src-tauri/src/domain/project.rs index 0f8303f..a7c43a8 100644 --- a/src-tauri/src/domain/project.rs +++ b/src-tauri/src/domain/project.rs @@ -156,9 +156,20 @@ pub(crate) fn list_projects_in(paths: &Paths) -> Result, String> { /// Pool-Config-Verzeichnis eines Projekts — wird dem Terminal als /// CLAUDE_CONFIG_DIR mitgegeben. +/// +/// Der Name wird geprüft, obwohl `assign_pool_in` das beim Zuweisen schon tut: +/// Die Quelle ist die `ai-control.json` **im Projektordner**, also eine +/// versionierte Datei, die mit einem geklonten Repo hereinkommt. Ungeprüft +/// bestimmte sie mit `../…` ein beliebiges Verzeichnis als CLAUDE_CONFIG_DIR — +/// und dessen `settings.json` trägt `apiKeyHelper`, ein Kommando, das beim +/// Sessionstart ausgeführt wird. pub(crate) fn project_pool_dir(project: &str) -> Result, String> { let paths = Paths::real(); - Ok(read_project_config_in(&paths, project)?.pool.map(|p| paths.pool_dir(&p))) + let Some(pool) = read_project_config_in(&paths, project)?.pool else { + return Ok(None); + }; + check_name(&pool).map_err(|_| format!("ungültiger Pool in {PROJECT_FILE}: {pool}"))?; + Ok(Some(paths.pool_dir(&pool))) } /// Terminal-Einstellungen eines Projekts, für den Terminal-Prozess. @@ -534,6 +545,21 @@ mod tests { assert_eq!(v["pool"], serde_json::Value::String(pool)); } + /// Ein Pool-Eintrag aus einer mitgeklonten `ai-control.json` darf kein + /// beliebiges Verzeichnis zum CLAUDE_CONFIG_DIR machen. + #[test] + fn pool_aus_projektdatei_mit_traversal_wird_abgelehnt() { + let p = tmp_paths(); + create_project(&p, "fremd").unwrap(); + let cfg_path = project_config_path(&p, "fremd").unwrap(); + fs::write(&cfg_path, r#"{"pool":"../../../../tmp/x"}"#).unwrap(); + + let cfg = read_project_config_in(&p, "fremd").unwrap(); + assert_eq!(cfg.pool.as_deref(), Some("../../../../tmp/x")); + // Gelesen wird der Wert noch, aber er darf keinen Pfad bilden. + assert!(check_name(cfg.pool.as_deref().unwrap()).is_err()); + } + /// Das Setzen der Terminal-Config kommt aus der Oberfläche und kennt nur /// theme/icon/title — unbekannte Keys müssen trotzdem stehen bleiben. #[test] diff --git a/src-tauri/src/domain/todo.rs b/src-tauri/src/domain/todo.rs index c0c9513..e3e4b83 100644 --- a/src-tauri/src/domain/todo.rs +++ b/src-tauri/src/domain/todo.rs @@ -11,13 +11,26 @@ use crate::domain::registry::project_dir; pub(crate) const TODO_FILE: &str = "OFFENE-PUNKTE.md"; const TODO_SKELETON: &str = "# Offene Punkte — bei jedem Start prüfen und abhaken\n\nKeine offenen Punkte.\n"; +/// Der Hook landet in der `settings.json` des Projekts und wird von Claude Code +/// bei jedem Sessionstart über die Shell ausgeführt. Der Pfad muss darum +/// gequotet werden: Er stammt aus dem Ordnernamen, den der Nutzer im Dialog +/// wählt — bei einem geklonten Fremd-Repo also von außen. Unquotiert genügte +/// ein Ordner `repo$(…)` für dauerhafte Codeausführung, und schon ein +/// Leerzeichen im Pfad hätte den Hook still zerbrochen. fn todo_hook_command(dir: &std::path::Path) -> String { format!( "jq -Rs '{{systemMessage: ., hookSpecificOutput:{{hookEventName:\"SessionStart\", additionalContext: .}}}}' {}", - dir.join(TODO_FILE).display() + shell_quote(&dir.join(TODO_FILE).to_string_lossy()) ) } +/// Ein Argument für `sh -c` in einfache Anführungszeichen setzen. Innerhalb +/// davon ist jedes Zeichen literal; einzig das Apostroph selbst muss die +/// Quotierung verlassen und wieder betreten (`'\''`). +fn shell_quote(s: &str) -> String { + format!("'{}'", s.replace('\'', r"'\''")) +} + fn hook_is_todo(group: &serde_json::Value) -> bool { group["hooks"] .as_array() @@ -98,6 +111,21 @@ mod tests { use crate::domain::project::{create_project_full_in, TerminalConfig}; use crate::domain::testutil::{create_project, tmp_paths}; + /// Der Hook-Befehl geht durch die Shell; ein Pfad mit Metazeichen darf dort + /// nichts ausführen. Ordnernamen sind bei geklonten Repos Fremdeingabe. + #[test] + fn hook_befehl_quotet_den_pfad() { + let cmd = todo_hook_command(std::path::Path::new("/tmp/repo$(touch /tmp/pwned)")); + assert!(cmd.ends_with("'/tmp/repo$(touch /tmp/pwned)/OFFENE-PUNKTE.md'"), "{cmd}"); + + // Ein Apostroph im Pfad darf die Quotierung nicht aufbrechen. + let cmd = todo_hook_command(std::path::Path::new("/tmp/o'brien")); + assert!(cmd.ends_with(r"'/tmp/o'\''brien/OFFENE-PUNKTE.md'"), "{cmd}"); + // Nach dem Zerlegen an den Quotes bleibt kein unquotierter Bereich übrig, + // in dem eine Shell noch etwas zu interpretieren hätte. + assert!(!cmd.contains("$(")); + } + #[test] fn todo_zuschalten_und_abschalten() { let p = tmp_paths(); diff --git a/src/markdown.test.ts b/src/markdown.test.ts index 1b4be35..8772349 100644 --- a/src/markdown.test.ts +++ b/src/markdown.test.ts @@ -39,10 +39,44 @@ describe("renderMarkdown", () => { } }); - it("blockt data:-Bilder, lässt normale Ziele durch", () => { + it("blockt data:-Bilder", () => { expect(renderMarkdown("![a](data:image/svg+xml,)")).not.toContain(" lädt ohne Zutun. Auswärtige Bildquellen sind darum ein + /// Zero-Click-Beacon: IP, Zeitpunkt und im Pfad kodierte Daten gehen raus, + /// sobald jemand ein vergiftetes Archiv-Dokument nur ansieht. + it("lädt keine auswärtigen Bilder, auch nicht schema-relativ", () => { + for (const src of [ + "https://evil.example/t.png", + "http://evil.example/t.png", + "//evil.example/t.png", + "HTTPS://evil.example/t.png", + ]) { + const el = document.createElement("div"); + el.innerHTML = renderMarkdown(`![alt](${src})`); + expect(el.querySelector("img"), `${src} durfte kein img erzeugen`).toBeNull(); + expect(el.textContent).toContain("alt"); + } + }); + + it("lässt lokale Bilder durch", () => { + expect(renderMarkdown("![a](./bild.png)")).toContain(" { + const el = document.createElement("div"); + el.innerHTML = renderMarkdown("[a](https://example.org)"); + const a = el.querySelector("a")!; + expect(a.getAttribute("href")).toBe("https://example.org"); + expect(a.getAttribute("rel")).toContain("noopener"); + expect(a.getAttribute("rel")).toContain("noreferrer"); + }); + + it("escapt auch das Apostroph", () => { + expect(renderMarkdown("Text mit ' Apostroph")).toContain("'"); }); it("escapt Anführungszeichen im title, damit das Attribut nicht aufbricht", () => { diff --git a/src/markdown.ts b/src/markdown.ts index 40b8324..be5b0ee 100644 --- a/src/markdown.ts +++ b/src/markdown.ts @@ -13,25 +13,41 @@ import { marked, type Tokens } from "marked"; -/// Schemata, die im Panel etwas anzuzeigen haben. Alles andere — allen voran -/// `javascript:`, aber auch `data:` (SVG mit Skript) und `file:` — fliegt raus. -const OK_SCHEME = /^(https?:|mailto:|#|\/|\.{0,2}\/)/i; +/// Ziele ohne Schema: Anker, absolute und relative Pfade innerhalb der App. +/// Beide Schrägstrich-Formen müssen den doppelten ausschließen — `//host/x` ist +/// eine schema-relative URL und damit auswärtig. Darum `\/(?!\/)` für den +/// absoluten Pfad und `\.{1,2}\/` für den relativen: Ein `{0,2}` würde den +/// nackten `/` mitmatchen und die erste Regel wirkungslos machen. +const LOCAL = /^(#|\/(?!\/)|\.{1,2}\/)/; + +/// Links dürfen auswärts zeigen: Sie brauchen einen Klick, und ein Archiv- +/// Dokument ohne funktionierende Quellenangaben wäre nutzlos. +const OK_LINK = new RegExp(`^(https?:|mailto:)|${LOCAL.source}`, "i"); + +/// Bilder dagegen nur lokal. Ein `` lädt beim bloßen Anzeigen, ohne jedes +/// Zutun: Ein vergiftetes Archiv-Dokument — der Panel-Inhalt kommt aus einer +/// LLM-Session und ist damit prompt-injizierbar — meldet über die Bild-URL +/// still IP, Zeitpunkt und im Pfad kodierte Daten nach außen. +const OK_IMAGE = LOCAL; function escapeHtml(s: string): string { return s .replace(/&/g, "&") .replace(//g, ">") - .replace(/"/g, """); + .replace(/"/g, """) + // Auch das Apostroph: Sonst hängt die Sicherheit daran, dass jedes Attribut + // hier doppelt gequotet bleibt — eine Konvention, keine Garantie. + .replace(/'/g, "'"); } -/// Leerer Link statt gefährlichem Ziel — der Text bleibt sichtbar, der Klick -/// tut nichts. -function safeHref(href: string): string { +/// Leeres Ziel statt gefährlichem — der Text bleibt sichtbar, der Klick tut +/// nichts. +function safeHref(href: string, erlaubt: RegExp): string { const h = href.trim(); // Steuerzeichen entfernen: `java\nscript:` ist sonst ein Umgehungsweg. const clean = h.replace(/[\u0000-\u001f\u007f]/g, ""); - return OK_SCHEME.test(clean) ? clean : ""; + return erlaubt.test(clean) ? clean : ""; } const renderer = new marked.Renderer(); @@ -42,13 +58,17 @@ renderer.html = ({ raw }: Tokens.HTML | Tokens.Tag) => escapeHtml(raw); renderer.link = function ({ href, title, tokens }: Tokens.Link) { const text = this.parser.parseInline(tokens); - const safe = safeHref(href); + const safe = safeHref(href, OK_LINK); const t = title ? ` title="${escapeHtml(title)}"` : ""; - return safe ? `${text}` : `${text}`; + // `rel` auch bei fehlendem target: Der Webview soll dem Ziel keinen Bezug auf + // das öffnende Fenster und keinen Referrer mitgeben. + const rel = ` rel="noopener noreferrer"`; + return safe ? `${text}` : `${text}`; }; renderer.image = ({ href, title, text }: Tokens.Image) => { - const safe = safeHref(href); + const safe = safeHref(href, OK_IMAGE); + // Auswärtiges Bild: nur der Alt-Text, keine Anfrage nach draußen. if (!safe) return escapeHtml(text); const t = title ? ` title="${escapeHtml(title)}"` : ""; return `${escapeHtml(text)}`;