diff --git a/docs/UNOSIM_REFACTORING_OPL.md b/docs/UNOSIM_REFACTORING_OPL.md index fa08e78fd..31f32a7c4 100644 --- a/docs/UNOSIM_REFACTORING_OPL.md +++ b/docs/UNOSIM_REFACTORING_OPL.md @@ -14,7 +14,7 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet. | ID | Befund | Kategorie | Evidenz | Risiko | Nutzen | Aufwand | Abhängigkeiten | Ziel-PR | Status | Verifikation | Merge-SHA/Ergebnis | |---|---|---|---|---|---|---|---|---|---|---|---| -| R1 | Compile-Pfad absichern: Env-Allowlist für Compiler-Spawns, sichere Include-Grenze | Security | S1 [plausibel] | hoch | schließt möglichen Secret-Leak über Diagnosen | klein | – | fix/compile-env-include-guard | OPEN | Sentinel-Test (synthetisch), Include-Ablehnung, Toolchain-Canary | – | +| R1 | Compile-Pfad absichern: sichere Include-Grenze vor arduino-cli (Env-Allowlist entfällt, siehe S1-ENV) | Security | S1 [Code, mit Sentinel verifiziert] | hoch | schließt den Kanal, über den eine beliebige lesbare Datei vollständig in Diagnosen erscheint | klein | – | fix/compile-include-guard | DONE | RED→GREEN: `include-guard.test.ts` (25), `arduino-compiler-include-guard.test.ts` (2), Toolchain-Sentinel-Test `compile-include-boundary.test.ts` (echte arduino-cli; vorher Sentinel in der Antwort, nachher nicht); Unit 2697 grün | PR-Merge siehe Verlauf | | R2 | `lastCompiledCode`-Fallback und globales schreibbares Sketch-CRUD entfernen | Isolation | S2, S3 [Code] | mittel–hoch | Nutzerisolation | klein | – | fix/remove-global-legacy-state | OPEN | Start ohne Code → Fehler; Sketch-Schreibrouten weg | – | | R6 | Einheitlicher Compile-Hash inkl. Header; kein Binary in REST-Payload/LRU | Korrektheit/Performance | A4, A5 [Code] | mittel | keine veralteten Cache-Treffer, kleinere Antworten | klein | – | fix/compile-cache-key-and-payload | OPEN | Header-only-Änderung → frischer Compile; Response ohne `binary` | – | | R7 | Globaler API-Limiter im Gateway-Modus nach `subject` statt IP | Skalierung | P1 [Code] | mittel (topologieabhängig) | keine klassenweiten 429 hinter NAT | klein | – | fix/api-rate-limit-identity | OPEN | Route-Tests beider Trust-Modi | – | @@ -33,7 +33,10 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet. | ID | Befund | Kategorie | Evidenz | Risiko | Nutzen | Aufwand | Abhängigkeiten | Ziel-PR | Status | Verifikation | Merge-SHA/Ergebnis | |---|---|---|---|---|---|---|---|---|---|---|---| -| S1 | Ungeprüfter REST-Compile im Backend erbt Env, absolute Includes erlaubt, stderr ungefiltert | Security | [plausibel] | hoch | – | – | – | R1 | OPEN | Sentinel-Verifikation | – | +| S1 | Absolutes/traversierendes `#include` lässt arduino-cli eine beliebige lesbare Datei lesen und in stderr ausgeben | Security | [Code] verifiziert: synthetische Sentinel-Datei erschien vollständig in der REST-Compile-Antwort | hoch | – | – | – | R1 | DONE | `compile-include-boundary.test.ts` | durch R1 geschlossen | +| S1-ENV | Env-Leak über geerbte Prozessumgebung und `/proc/*/environ` | Security | g++ 12 (Sandbox-Image) und avr-g++ 7.3 (Backend-Image) lesen `/proc/self/environ` und `/proc/1/environ` per `#include` und `.incbin` als leer (Sentinel-Env-Variable nicht sichtbar) | – | – | – | – | – | FALSIFIED | Docker-Experiment mit synthetischer Variable | keine Env-Allowlist umgesetzt | +| S1-INCBIN | `.incbin` im Inline-Assembler bettet reguläre Dateien ins HEX ein; das HEX geht derzeit im REST-JSON an den Client | Security | [Code] verifiziert (Marker-Datei im Objekt) | mittel | – | – | – | R6 | OPEN | Response ohne `binary` | Kanal schließt mit R6 | +| S1-ASM | GAS `.include` im Inline-Assembler gibt die ersten ~10 Zeichen je Zeile einer beliebigen Datei als Fehlermeldung aus; per String-Konkatenation nicht robust textuell filterbar | Security | [Code] verifiziert (Marker-Präfix in der Assemblermeldung) | mittel (Teilinhalt; Default-Deployment hält Secrets nur in Env) | – | groß | – | – | BLOCKED_DECISION | – | Robuster Fix = REST-Compile ohne Zugriff auf Backend-Dateien (Sandbox/Namespace); Architekturentscheidung | | S2 | Globaler `lastCompiledCode` | Isolation | [Code] | mittel–hoch | – | – | – | R2 | OPEN | – | – | | S3 | Sketch-CRUD ohne Besitzer | Isolation | [Code] | mittel | – | – | – | R2 | OPEN | – | – | | S4 | Runner-Reuse-Race | Isolation/Lifecycle | [plausibel] | hoch | – | – | – | R3a | OPEN | Race-Test | – | @@ -63,10 +66,12 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet. ## Reihenfolge-Änderungen -Noch keine. +- R1 umfasst nur die Include-Grenze. Der `.incbin`-Kanal (S1-INCBIN) wird mit R6 + geschlossen, weil dort das Binary aus der REST-Antwort entfernt wird. ## Verlauf | PR | Inhalt | Merge-SHA | CI | |---|---|---|---| -| – | Audit-Bericht und diese OPL | – | – | +| #159 | Audit-Bericht und diese OPL | `0863ba1f` | PR-CI 5/5 grün; Post-Merge-CI siehe nächster Eintrag | +| R1 | Include-Grenze für den REST-Compiler | – | – | diff --git a/server/services/arduino-compiler.ts b/server/services/arduino-compiler.ts index 6cbf20671..08d230037 100644 --- a/server/services/arduino-compiler.ts +++ b/server/services/arduino-compiler.ts @@ -27,6 +27,7 @@ import { checkCacheHits, } from "./compiler/cache-manager"; import { processHeaderIncludes } from "./compiler/header-processor"; +import { findUnsafeIncludes } from "./compiler/include-guard"; import { compileWithArduinoCli, type CLICompileConfig } from "./compiler/cli-runner"; // Re-export for backwards compatibility @@ -198,6 +199,21 @@ export class ArduinoCompiler { const compileStartedAt = process.hrtime.bigint(); try { + // 0. arduino-cli runs inside the backend, not a sandbox: an include must + // not make the preprocessor read and echo a file outside the project. + const unsafeIncludes = findUnsafeIncludes(sourceProject); + if (unsafeIncludes.length > 0) { + return { + success: false, + output: "", + stderr: unsafeIncludes.map(({ file, line, message }) => `${file}:${line}: error: ${message}`).join("\n"), + errors: unsafeIncludes.map(({ file, line, message }) => ({ file, line, column: 1, type: "error" as const, message })), + arduinoCliStatus: "error", + parserMessages: allParserMessages, + ioRegistry, + }; + } + // 1. Validate sketch has required entry points const validation = this.validateSketchEntrypoints(code); if (!validation.valid) { diff --git a/server/services/compiler/include-guard.ts b/server/services/compiler/include-guard.ts new file mode 100644 index 000000000..201b93ca0 --- /dev/null +++ b/server/services/compiler/include-guard.ts @@ -0,0 +1,148 @@ +import { normalizeSourcePath, type SourceProject } from "@shared/source-project"; + +/** + * Include boundary for the REST compiler, which runs arduino-cli inside the + * backend instead of a sandbox. The preprocessor reads every included file and + * echoes its lines in diagnostics, so an include must not name a file outside + * the submitted project or the toolchain's own include directories. + * + * The scan is deliberately conservative: it treats every `#`/`%:` that is + * preceded on its logical line only by whitespace or comments as a directive, + * also inside block comments and raw strings. A false positive rejects a sketch + * with an explicit message; a false negative would let the compiler read a file. + */ + +export interface UnsafeInclude { + readonly file: string; + readonly line: number; + readonly message: string; +} + +const FILE_DIRECTIVES = new Set(["include", "include_next", "import", "embed"]); +const DIRECTIVE_START = /(?:#|%:|\?\?=)[ \t\f\v]*/y; +const HAS_INCLUDE = /__has_include(?:_next)?\b/g; +const HEADER_NAME_DELIMITERS: Readonly> = { + '"': { close: '"', kind: "quoted" }, + "<": { close: ">", kind: "angled" }, +}; + +interface LogicalLine { + readonly text: string; + readonly line: number; +} + +/** Translation phase 2: join backslash-continued physical lines. */ +function logicalLines(source: string): LogicalLine[] { + const physical = source.split(/\r?\n/); + const lines: LogicalLine[] = []; + let buffer = ""; + let startLine = 1; + physical.forEach((text, index) => { + if (buffer === "") startLine = index + 1; + if (text.endsWith("\\")) { + buffer += text.slice(0, -1); + return; + } + lines.push({ text: buffer + text, line: startLine }); + buffer = ""; + }); + if (buffer !== "") lines.push({ text: buffer, line: startLine }); + return lines; +} + +/** Skips whitespace and block comments; returns the index of the next token. */ +function skipSpaceAndComments(text: string, start: number): number { + let index = start; + for (;;) { + while (index < text.length && /[ \t\f\v]/.test(text[index])) index += 1; + if (text.startsWith("/*", index)) { + const end = text.indexOf("*/", index + 2); + if (end < 0) return text.length; + index = end + 2; + continue; + } + return index; + } +} + +/** Index where directive candidates may start: after a leading comment tail or comments. */ +function directiveCandidateStart(text: string): number { + // A block comment opened on an earlier line may end on this one; whatever + // follows its end can still start a directive. Considering both positions + // keeps the scan conservative. + return skipSpaceAndComments(text, 0); +} + +function parseHeaderName(text: string, start: number): { kind: "quoted" | "angled"; path: string } | null { + const index = skipSpaceAndComments(text, start); + const delimiters = HEADER_NAME_DELIMITERS[text[index]]; + if (!delimiters) return null; + const end = text.indexOf(delimiters.close, index + 1); + if (end < 0) return null; + return { kind: delimiters.kind, path: text.slice(index + 1, end) }; +} + +function directoryOf(file: string): string { + const separator = file.lastIndexOf("/"); + return separator < 0 ? "" : file.slice(0, separator); +} + +function isSafeHeaderName(fromFile: string, header: { kind: "quoted" | "angled"; path: string }): boolean { + const { path } = header; + if (!path || path.includes("\\") || path.includes("\0") || path.startsWith("/")) return false; + if (header.kind === "angled") { + return path.split("/").every((segment) => segment !== ".." && segment !== ""); + } + const base = directoryOf(fromFile); + return normalizeSourcePath(base ? `${base}/${path}` : path) !== undefined; +} + +function checkDirective(file: string, line: LogicalLine, at: number): UnsafeInclude | null { + DIRECTIVE_START.lastIndex = at; + if (!DIRECTIVE_START.exec(line.text)) return null; + const nameStart = skipSpaceAndComments(line.text, DIRECTIVE_START.lastIndex); + const name = /^[A-Za-z0-9_\\$]*/.exec(line.text.slice(nameStart))?.[0] ?? ""; + if (name.includes("\\")) { + return { file, line: line.line, message: "Escaped preprocessor directive names are not allowed" }; + } + if (!FILE_DIRECTIVES.has(name)) return null; + const header = parseHeaderName(line.text, nameStart + name.length); + if (!header) { + return { file, line: line.line, message: `#${name} must name a literal "file" or ` }; + } + if (!isSafeHeaderName(file, header)) { + return { file, line: line.line, message: `#${name} path is outside the sketch project: ${header.path}` }; + } + return null; +} + +function checkHasInclude(file: string, line: LogicalLine): UnsafeInclude | null { + for (const match of line.text.matchAll(HAS_INCLUDE)) { + const open = skipSpaceAndComments(line.text, (match.index ?? 0) + match[0].length); + if (line.text[open] !== "(") { + return { file, line: line.line, message: `${match[0]} must name a literal "file" or ` }; + } + const header = parseHeaderName(line.text, open + 1); + if (!header || !isSafeHeaderName(file, header)) { + return { file, line: line.line, message: `${match[0]} path is not allowed` }; + } + } + return null; +} + +/** Returns every include that could make the compiler read a file outside the project. */ +export function findUnsafeIncludes(project: SourceProject): UnsafeInclude[] { + return Object.entries(project.files).flatMap(([file, source]) => + logicalLines(source).flatMap((line) => scanLine(file, line)), + ); +} + +function scanLine(file: string, line: LogicalLine): UnsafeInclude[] { + const candidates = new Set([directiveCandidateStart(line.text)]); + const commentTail = line.text.indexOf("*/"); + if (commentTail >= 0) candidates.add(skipSpaceAndComments(line.text, commentTail + 2)); + return [ + ...[...candidates].map((at) => checkDirective(file, line, at)), + checkHasInclude(file, line), + ].filter((finding): finding is UnsafeInclude => finding !== null); +} diff --git a/tests/integration/compile-include-boundary.test.ts b/tests/integration/compile-include-boundary.test.ts new file mode 100644 index 000000000..6328bf8f8 --- /dev/null +++ b/tests/integration/compile-include-boundary.test.ts @@ -0,0 +1,36 @@ +import { randomUUID } from "node:crypto"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { ArduinoCompiler } from "../../server/services/arduino-compiler"; + +// Synthetic marker only; no real credential is ever read by this test. +const SENTINEL = `UNOSIM_SYNTHETIC_SENTINEL_${randomUUID().replaceAll("-", "")}`; + +describe("REST compiler include boundary (real arduino-cli)", () => { + let root: string; + let sentinelFile: string; + + beforeAll(async () => { + root = await mkdtemp(join(tmpdir(), "unosim-include-boundary-")); + sentinelFile = join(root, "sentinel.txt"); + await writeFile(sentinelFile, `${SENTINEL} = not_a_secret;\n`); + }); + + afterAll(async () => { + await rm(root, { recursive: true, force: true }); + }); + + it("does not echo a file named by an absolute include", async () => { + const result = await new ArduinoCompiler().compile( + `#include "${sentinelFile}"\nvoid setup(){}\nvoid loop(){}\n`, + [], + undefined, + { entryFile: "sketch.ino", sketchHash: randomUUID(), hexCacheDir: join(root, "hex-cache") }, + ); + + expect(result.success).toBe(false); + expect(JSON.stringify(result)).not.toContain(SENTINEL); + }, 60_000); +}); diff --git a/tests/server/services/arduino-compiler-include-guard.test.ts b/tests/server/services/arduino-compiler-include-guard.test.ts new file mode 100644 index 000000000..33f0273b2 --- /dev/null +++ b/tests/server/services/arduino-compiler-include-guard.test.ts @@ -0,0 +1,41 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ArduinoCompiler } from "../../../server/services/arduino-compiler"; +import * as cliRunner from "../../../server/services/compiler/cli-runner"; +import * as cacheManager from "../../../server/services/compiler/cache-manager"; + +const UNSAFE = '#include "/etc/hosts"\nvoid setup(){}\nvoid loop(){}\n'; + +describe("ArduinoCompiler include boundary", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("rejects an include outside the project before arduino-cli or the cache is consulted", async () => { + const cli = vi.spyOn(cliRunner, "compileWithArduinoCli"); + const cache = vi.spyOn(cacheManager, "checkCacheHits"); + const compiler = await ArduinoCompiler.create(); + + const result = await compiler.compile(UNSAFE, [], undefined, { entryFile: "sketch.ino" }); + + expect(cli).not.toHaveBeenCalled(); + expect(cache).not.toHaveBeenCalled(); + expect(result).toMatchObject({ success: false, arduinoCliStatus: "error" }); + expect(result.errors).toEqual([ + expect.objectContaining({ file: "sketch.ino", line: 1, type: "error" }), + ]); + }); + + it("rejects an unsafe include in a submitted header without an entry file", async () => { + const cli = vi.spyOn(cliRunner, "compileWithArduinoCli"); + const compiler = await ArduinoCompiler.create(); + + const result = await compiler.compile( + '#include "lib.h"\nvoid setup(){}\nvoid loop(){}\n', + [{ name: "lib.h", content: "#include \n" }], + ); + + expect(cli).not.toHaveBeenCalled(); + expect(result.success).toBe(false); + expect(result.errors[0]).toMatchObject({ file: "lib.h", line: 1 }); + }); +}); diff --git a/tests/server/services/compiler/include-guard.test.ts b/tests/server/services/compiler/include-guard.test.ts new file mode 100644 index 000000000..2f6e6e63b --- /dev/null +++ b/tests/server/services/compiler/include-guard.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; +import { findUnsafeIncludes } from "../../../../server/services/compiler/include-guard"; + +const BACKSLASH = String.fromCodePoint(92); +const SKETCH_TAIL = "\nvoid setup(){}\nvoid loop(){}\n"; + +function scan(main: string, files: Record = {}) { + return findUnsafeIncludes({ entryFile: "sketch.ino", files: { "sketch.ino": main, ...files } }); +} + +describe("include guard", () => { + it.each([ + ["absolute quoted include", '#include "/etc/passwd"'], + ["absolute angled include", "#include "], + ["quoted include leaving the project", '#include "../secret.h"'], + ["angled include with a parent segment", "#include <../../etc/passwd>"], + ["include_next", '#include_next "/etc/hosts"'], + ["import", '#import "/etc/hosts"'], + ["digraph directive", '%:include "/etc/hosts"'], + ["spaces and comments around the directive", ' # /* x */ include /* y */ "/etc/hosts"'], + ["a comment before the directive", '/* c */ #include "/etc/hosts"'], + ["a line splice inside the directive name", `#inc${BACKSLASH}\nlude "/etc/hosts"`], + ["a computed include", '#define P "/etc/hosts"\n#include P'], + ["an escaped directive name", String.raw`#incl\u0075de "x.h"`], + ["__has_include with an absolute path", '#if __has_include("/etc/hosts")\n#endif'], + ["__has_include with a computed operand", "#if __has_include(P)\n#endif"], + ["a backslash path", String.raw`#include "..\x.h"`], + ])("rejects %s", (_label, source) => { + expect(scan(source + SKETCH_TAIL)).not.toHaveLength(0); + }); + + it("rejects an unsafe include inside a submitted header file", () => { + expect(scan('#include "lib.h"' + SKETCH_TAIL, { "lib.h": '#include "/etc/hosts"\n' })).toEqual([ + expect.objectContaining({ file: "lib.h", line: 1 }), + ]); + }); + + it("reports the line of the first physical line of a directive", () => { + expect(scan('int a;\n\n#include "/etc/hosts"' + SKETCH_TAIL)).toEqual([ + expect.objectContaining({ file: "sketch.ino", line: 3 }), + ]); + }); + + it.each([ + ["library include", "#include "], + ["core include", '#include "Arduino.h"'], + ["nested library include", "#include "], + ["project subdirectory include", '#include "src/config.h"'], + ["text that only mentions an include", String.raw`void f(){ Serial.println("use #include \"/etc/x\""); }`], + ["a line comment", '// #include "/etc/hosts"'], + ["other directives", "#define LED 13\n#pragma once\n#if 1\n#endif"], + ])("accepts %s", (_label, source) => { + expect(scan(source + SKETCH_TAIL)).toEqual([]); + }); + + it("accepts a parent include that stays inside the project", () => { + expect(scan('#include "src/a.h"' + SKETCH_TAIL, { "src/a.h": '#include "../config.h"\n', "config.h": "" })).toEqual([]); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 7ca3cb987..028fb57d4 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -97,6 +97,7 @@ export default defineConfig({ "tests/integration/serial-flooding.test.ts", "tests/integration/serial-flow.test.ts", "tests/integration/compiler-canaries.test.ts", + "tests/integration/compile-include-boundary.test.ts", "tests/integration/worker-pool.*.test.ts", "tests/integration/concurrent-50-clients.test.ts", "tests/server/services/scalability-stress.test.ts", @@ -133,6 +134,7 @@ export default defineConfig({ name: "integration-toolchain", include: [ "tests/integration/compiler-canaries.test.ts", + "tests/integration/compile-include-boundary.test.ts", "tests/server/telemetry-heartbeat-integration.test.ts", "tests/core/sandbox-stress.test.ts", ],