From 26959ec909f571358d417d60ce586c2494e942e5 Mon Sep 17 00:00:00 2001 From: duffyduck Date: Sun, 21 Jun 2026 12:50:45 +0200 Subject: [PATCH] Pentest R87: Identifier-Whitelist vor stripHtml ziehen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R87.1 LOW: stripHtml lief im R86-Fix VOR der Whitelist. `bold` ging als `"bold"` mit 200 OK durch, `` reduzierte auf leeren String → null in DB → vorheriger Wert ohne Fehlermeldung überschrieben. Fix: validateContractIdentifier läuft jetzt direkt gegen den Raw-Input für die fünf Identifier-Felder. Die strikte Whitelist lehnt eh alles ab, was stripHtml normalerweise auffangen würde (Tags, Schemes, Zero-Width, Homoglyphe, Percent-Encoding) – Defense-in-Depth bleibt, nur ehrlich (400 statt silent-200). Co-Authored-By: Claude Opus 4.7 --- .../src/controllers/contract.controller.ts | 15 ++++++-- docs/SECURITY-HARDENING.md | 38 +++++++++++++++++++ docs/todo.md | 14 +++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/backend/src/controllers/contract.controller.ts b/backend/src/controllers/contract.controller.ts index 5c569e98..b280a1f3 100644 --- a/backend/src/controllers/contract.controller.ts +++ b/backend/src/controllers/contract.controller.ts @@ -36,14 +36,23 @@ function sanitizeContractBody(body: unknown, parentKey?: string): unknown { if (body === null || body === undefined) return body; if (typeof body === 'string') { if (parentKey && PASSTHROUGH_KEYS.has(parentKey)) return body; - const stripped = stripHtml(body); // Pentest 86.1/86.2 (LOW, 2026-06-19): Längen- + Whitelist-Check auf // Kunden-/Vertrags-/Auftragsnummer-Feldern. validateContractIdentifier // wirft ApiError(400) bei Verstoß → saubere 400-Antwort statt 500. + // + // Pentest 87.1 (LOW, 2026-06-19): Identifier-Felder MÜSSEN gegen den + // Raw-Input geprüft werden, NICHT gegen den stripHtml-Output. Sonst + // verschluckt der Sanitizer Tag-Verstöße still: `bold` würde + // als `"bold"` mit 200 OK durchgehen, `` + // sogar zu `null` und damit den vorherigen Wert überschreiben. + // Die strikte Whitelist (`^[A-Za-z0-9_\-/. ]{0,100}$`) deckt alle + // Bypässe ab, die stripHtml normalerweise auffangen würde + // (Tags, Schemes, Zero-Width-Chars, Homoglyphe, Percent-Encoding) – + // sie sind alle nicht in der Allowlist und fliegen mit 400 raus. if (parentKey && isContractIdentifierField(parentKey)) { - return validateContractIdentifier(stripped, parentKey); + return validateContractIdentifier(body, parentKey); } - return stripped; + return stripHtml(body); } if (Array.isArray(body)) return body.map((v) => sanitizeContractBody(v, parentKey)); if (typeof body === 'object') { diff --git a/docs/SECURITY-HARDENING.md b/docs/SECURITY-HARDENING.md index 6c7ca14a..851f60b2 100644 --- a/docs/SECURITY-HARDENING.md +++ b/docs/SECURITY-HARDENING.md @@ -537,6 +537,44 @@ erneut als „offenes Finding" auftaucht. --- +## 🔒 Runde 87 – Whitelist vor Sanitizer (silent-mutation-Schutz) + +**Finding (LOW): Sanitizer-Order maskiert Tag-Verstöße** + +Im ursprünglichen R86-Fix lief `stripHtml(body)` **vor** +`validateContractIdentifier`. Das hatte einen subtilen Bypass: + +| Payload | Status | Tatsächlich gespeichert | +|--------------------------------------|------------|-------------------------| +| `bold` | 200 OK | `"bold"` (silent strip) | +| `EVN2024` | 200 OK | `"EVN2024"` | +| `` | **200 OK** | `null` – **vorherigen Wert überschrieben** | +| `foobaz` | 200 OK | `"foobarbaz"` | + +Kein direkter XSS-Vektor (React + DB-Whitelist greifen weiterhin), +aber zwei reale UX-/Datenintegritäts-Risiken: + +1. Admin tippt `VG<2024>001`, bekommt 200 zurück, gespeichert ist + `VG2024001` ohne Hinweis auf die Mutation. +2. Werte die komplett aus Tags bestehen (``) + werden vom Sanitizer auf den leeren String reduziert → + `null` in der DB → **vorheriger Wert wird stillschweigend + gelöscht**. + +**Fix:** Validierungs-Reihenfolge für die fünf Identifier-Felder +umgedreht – `validateContractIdentifier` läuft jetzt **direkt +gegen den Raw-Input**, ohne dass `stripHtml` ihn vorher +glättet. Die strikte Whitelist +`^[A-Za-z0-9_\-/. ]{0,100}$` lehnt sowieso alles ab, was +`stripHtml` normalerweise abgefangen hätte (Tags, Schemes, +Zero-Width-Chars, Homoglyphe, Percent-Encoding) – Defense-in- +Depth bleibt unverändert, nur jetzt ehrlich (400 statt silent-200). + +Single-Line-Patch in [`backend/src/controllers/contract.controller.ts`](../backend/src/controllers/contract.controller.ts) +`sanitizeContractBody`. + +--- + ## 🧭 Wann ist „dicht" dicht? 100 % gibt es nicht. Erreicht ist: diff --git a/docs/todo.md b/docs/todo.md index a352f047..35a226d5 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -97,6 +97,20 @@ isolierte Instanz (keine Multi-Tenancy im Code), Provisioning + Abrechnung ## ✅ Erledigt +- [x] **🔒 Pentest R87 – Whitelist vor Sanitizer (silent-mutation-Schutz)** + - R87.1 (LOW): `stripHtml` lief im R86-Fix VOR der Whitelist. + Tags wurden still weggestrippt → 200 OK mit mutierten Werten, + `` reduzierte auf leeren String → `null` in + der DB → vorheriger Wert ohne Fehlermeldung überschrieben. + - Fix: Validierungs-Reihenfolge für die fünf Identifier-Felder + umgedreht – `validateContractIdentifier` läuft jetzt direkt + gegen den Raw-Input. Die strikte Whitelist lehnt eh alles + ab, was stripHtml normalerweise auffangen würde (Tags, + Schemes, Zero-Width, Homoglyphe, Percent-Encoding) – Defense- + in-Depth bleibt, nur ehrlich (400 statt silent-200). + - Single-Line-Patch in `contract.controller.ts`, Doku in + `SECURITY-HARDENING.md § Runde 87`. + - [x] **🔒 Pentest R86 – Vertrags-Identifier härten** - R86.1 (LOW): >999-Zeichen-Strings auf Kunden-/Vertrags-/ Auftragsnummer warfen 500 (DB-Overflow `VARCHAR(191)`) statt 400.