fix(compiler): reject includes that leave the sketch project before arduino-cli runs

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 | – | – |

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) {

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<Record<string, { close: string; kind: "quoted" | "angled" }>> = {
+  '"': { 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 <file>` };
+  }
+  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 <file>` };
+    }
+    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);
+}

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);
+});

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 </etc/hosts>\n" }],
+    );
+
+    expect(cli).not.toHaveBeenCalled();
+    expect(result.success).toBe(false);
+    expect(result.errors[0]).toMatchObject({ file: "lib.h", line: 1 });
+  });
+});

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<string, string> = {}) {
+  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 </etc/passwd>"],
+    ["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 <Servo.h>"],
+    ["core include", '#include "Arduino.h"'],
+    ["nested library include", "#include <avr/pgmspace.h>"],
+    ["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([]);
+  });
+});

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",
           ],