From f964610af748f0baef2777b7cbad15f1f618aaed Mon Sep 17 00:00:00 2001 From: duffyduck Date: Wed, 9 Sep 2026 02:45:11 +0200 Subject: [PATCH] R193-02: Truthiness-Bypass bei den drei Haken (HIGH) Befund der Pentesterin, Prod-Blocker. Der Eskalations-Guard fragte `=== true`, die Zuweisung fragte auf Truthiness. Zwischen diesen beiden Lesarten passte der Angriff: {"hasDeveloperAccess":"ja"} sah fuer den Guard nach keinem Rechtezuwachs aus (kein 403) und fuer die Zuweisung nach "gesetzt" (Rolle drauf). Ein Konto mit users:update, das developer:access nicht haelt, konnte damit einem anderen Konto Vollzugriff geben. Damit war die Kernentscheidung aus Etappe 1 ausgehebelt: developer:access und audit:admin sollten ausdruecklich nur ueber die Kommandozeile erstmalig vergebbar sein. Der Bypass machte sie wieder ueber ein Browser-Feld erreichbar. Es ist genau das Muster, das ich der Pentesterin eine Runde vorher selbst beschrieben habe - eine Absicherung, die nur in einer von zwei Kopien ankommt, ist keine. Nur dass es diesmal nicht zwei Dateien waren, sondern zwei Auffassungen desselben Wertes in derselben Datei. normalisiereHaken() prueft die drei Felder einmal streng auf Boolean, alles andere 400. Guard und Zuweisung arbeiten danach auf demselben geprueften Objekt - es gibt nur noch eine Lesart. Zusaetzlich liest setzeVersteckteRolle strikt === true / === false, damit sich die Lesart auch dann nicht spaltet, wenn die Funktion kuenftig von woanders gerufen wird. Gleiche Bauart nebenan: isActive und isServiceAccount steuern ebenfalls ein Gate, das strikt auf boolean prueft - ein "ja" rutschte daran vorbei, ohne das Gate auszuloesen (u.a. die audit:admin-Huerde fuers Dienstkonto-Kennzeichen), und lief dann in einen Prisma-Fehler. Kein Bypass, aber ein 500 fuer eine falsche Eingabe. Jetzt alle fuenf Boolean-Felder einheitlich geprueft. Nachgeprueft: ihre vier Vektoren plus 30 weitere Kombinationen ueber alle drei Haken - alle 400, Rollen unveraendert. Derselbe Angriff ueber POST /users - 400, kein Konto angelegt. Gegenprobe: Wer gdpr:* selbst haelt, setzt den Haken mit echtem true weiterhin erfolgreich. Co-Authored-By: Claude Opus 5 (1M context) --- backend/src/controllers/user.controller.ts | 30 ++++++++++++++ backend/src/services/rechte.service.ts | 47 +++++++++++++++++++--- backend/src/services/user.service.ts | 37 ++++++++++++----- docs/todo.md | 39 ++++++++++++++++++ 4 files changed, 137 insertions(+), 16 deletions(-) diff --git a/backend/src/controllers/user.controller.ts b/backend/src/controllers/user.controller.ts index 611f809e..84aab0c8 100644 --- a/backend/src/controllers/user.controller.ts +++ b/backend/src/controllers/user.controller.ts @@ -57,6 +57,11 @@ export async function createUser(req: Request, res: Response): Promise { try { // Whitelist: nur erlaubte Felder aus req.body übernehmen (Mass-Assignment-Schutz) const data = pickUserCreate(req.body) as any; + const boolFehler = pruefeBooleanFelder(data, ['isActive', 'isServiceAccount', 'hasGdprAccess', 'hasDeveloperAccess', 'hasAuditOpsAccess']); + if (boolFehler) { + res.status(400).json({ success: false, error: boolFehler } as ApiResponse); + return; + } // Email-Format prüfen, sonst landet "x@y\nBcc:..." in der DB // (Pentest 29.4 – SMTP-Header-Injection). if (!isValidEmail(data?.email) || !data?.email) { @@ -134,6 +139,11 @@ export async function updateUser(req: AuthRequest, res: Response): Promise } // Whitelist: nur erlaubte Felder aus req.body übernehmen (Mass-Assignment-Schutz) const data = pickUserUpdate(req.body) as Record; + const boolFehler = pruefeBooleanFelder(data, ['isActive', 'isServiceAccount', 'hasGdprAccess', 'hasDeveloperAccess', 'hasAuditOpsAccess']); + if (boolFehler) { + res.status(400).json({ success: false, error: boolFehler } as ApiResponse); + return; + } // Email-Validierung gegen SMTP-Header-Injection (Pentest 29.4). // null/leer ist OK (Email darf optional sein), nur falsches Format prüfen. if (data?.email !== undefined && !isValidEmail(data.email)) { @@ -554,6 +564,26 @@ export async function deleteUser(req: Request, res: Response): Promise { * stillschweigend zu "erlaubt" werden. Deshalb wird hier abgelehnt statt * durchgewinkt. */ +/** + * Weist Boolean-Felder ab, die keine Booleans sind. + * + * `isActive` und `isServiceAccount` steuern beide ein Gate, das strikt auf + * `boolean` prueft - ein `"ja"` rutschte daran vorbei, ohne das Gate + * auszuloesen, und lief danach in einen Prisma-Fehler. Kein Bypass, weil der + * Schreibvorgang scheiterte, aber ein 500 fuer eine Eingabe, die schlicht + * falsch war. Dieselbe Bauart wie R193-02 bei den drei Haken: Ein Gate, das + * strenger liest als der Rest, uebersieht genau das, was dazwischen passt. + */ +function pruefeBooleanFelder(daten: Record, felder: string[]): string | null { + for (const feld of felder) { + const wert = daten[feld]; + if (wert !== undefined && typeof wert !== 'boolean') { + return `${feld} muss true oder false sein (empfangen: ${JSON.stringify(wert)})`; + } + } + return null; +} + function handelnderOderNull(req: AuthRequest): number | null { return typeof req.user?.userId === 'number' ? req.user.userId : null; } diff --git a/backend/src/services/rechte.service.ts b/backend/src/services/rechte.service.ts index 9fbc8d5f..da086c32 100644 --- a/backend/src/services/rechte.service.ts +++ b/backend/src/services/rechte.service.ts @@ -184,11 +184,7 @@ export async function meldeTraegerAb(roleId: number): Promise { * Nur das EINSCHALTEN wird geprueft. Wer einen Haken entfernt, nimmt Rechte * weg - das darf jeder duerfen, der das Konto verwalten darf. */ -export function rechteDerHaken(haken: { - hasDeveloperAccess?: boolean; - hasGdprAccess?: boolean; - hasAuditOpsAccess?: boolean; -}): string[] { +export function rechteDerHaken(haken: Haken): string[] { const rechte: string[] = []; if (haken.hasDeveloperAccess === true) rechte.push(...rechteDerSystemrolle(ROLLE_DEVELOPER)); if (haken.hasGdprAccess === true) rechte.push(...rechteDerSystemrolle(ROLLE_DSGVO)); @@ -247,3 +243,44 @@ export async function normalisiereRollenIds(roleIds: number[]): Promise): Haken { + const gepruef: Haken = {}; + for (const feld of HAKEN_FELDER) { + const wert = roh[feld]; + if (wert === undefined) continue; + if (typeof wert !== 'boolean') { + throw new UngueltigeEingabeError( + `${feld} muss true oder false sein (empfangen: ${JSON.stringify(wert)})`, + ); + } + gepruef[feld] = wert; + } + return gepruef; +} diff --git a/backend/src/services/user.service.ts b/backend/src/services/user.service.ts index a595d65f..3499fa83 100644 --- a/backend/src/services/user.service.ts +++ b/backend/src/services/user.service.ts @@ -6,11 +6,14 @@ import { rechteVonPermissionIds, rechteVonRollen, rechteDerHaken, + normalisiereHaken, + UngueltigeEingabeError, effektiveRechte, normalisiereRollenIds, meldeTraegerAb, RollenSperrError, } from './rechte.service.js'; +import type { Haken } from './rechte.service.js'; import { istSystemrollenName, RECHTE_KATALOG, @@ -190,9 +193,13 @@ export async function createUser(data: { // schluege erst beim Schreiben fehl. const rollenIds = await normalisiereRollenIds(data.roleIds); + // Einmal pruefen, dann ueberall dieselbe Lesart: Guard und Zuweisung + // arbeiten auf DEMSELBEN Objekt (Pentest R193-02). + const haken = normalisiereHaken(data as Record); + await pruefeTeilmenge(handelnderId, [ ...(await rechteVonRollen(rollenIds)), - ...rechteDerHaken(data), + ...rechteDerHaken(haken), ]); // Cost 12 wie in prisma/seed.ts (OWASP 2026). Hier stand 10 - dieselbe @@ -233,7 +240,7 @@ export async function createUser(data: { // Die Haken in derselben Transaktion wie das Konto (Pentest R191-01). // Sonst koennte ein halb ausgestattetes Konto zurueckbleiben: angelegt, // aber ohne die zugesagte versteckte Rolle. - await setzeHaken(tx, angelegt.id, data); + await setzeHaken(tx, angelegt.id, haken); return angelegt; }); @@ -270,6 +277,10 @@ export async function updateUser( const gepruefteRollenIds = roleIds !== undefined ? await normalisiereRollenIds(roleIds) : undefined; + // Die Haken einmal pruefen - danach gibt es nur noch diese eine Lesart, + // fuer den Guard wie fuer die Zuweisung (Pentest R193-02). + const haken = normalisiereHaken({ hasDeveloperAccess, hasGdprAccess, hasAuditOpsAccess }); + // Teilmengenregel auf dem ZUWACHS, nicht auf dem Endzustand. // // Wer nur den Nachnamen eines hoeher privilegierten Kollegen korrigiert, @@ -284,7 +295,7 @@ export async function updateUser( if (!bisher.has(r)) zuwachs.add(r); } } - for (const r of rechteDerHaken({ hasDeveloperAccess, hasGdprAccess, hasAuditOpsAccess })) { + for (const r of rechteDerHaken(haken)) { if (!bisher.has(r)) zuwachs.add(r); } await pruefeTeilmenge(handelnderId, zuwachs); @@ -432,7 +443,7 @@ export async function updateUser( // Nach dem Rollentausch: Der loescht ALLE Zuordnungen, auch die // versteckten Rollen. Die Haken setzen sie anschliessend wieder. - await setzeHaken(tx, id, { hasDeveloperAccess, hasGdprAccess, hasAuditOpsAccess }); + await setzeHaken(tx, id, haken); }); return getUserById(id); @@ -460,6 +471,9 @@ async function setzeVersteckteRolle( rollenName: string, aktiv: boolean, ): Promise { + if (typeof aktiv !== 'boolean') { + throw new UngueltigeEingabeError(`Haken für „${rollenName}" muss true oder false sein`); + } const rolle = await tx.role.findUnique({ where: { name: rollenName } }); if (!rolle) { throw new Error( @@ -472,9 +486,14 @@ async function setzeVersteckteRolle( where: { userId_roleId: { userId, roleId: rolle.id } }, }); - if (aktiv && !vorhanden) { + // Strikt auf `true`, nicht auf Truthiness. Hier war die zweite Haelfte des + // Bypasses aus R193-02: Der Guard fragte `=== true`, diese Zeile fragte + // "irgendwie wahr". `normalisiereHaken` laesst inzwischen nichts anderes + // mehr durch - aber die Lesart darf sich auch dann nicht unterscheiden, + // wenn jemand diese Funktion kuenftig von woanders aufruft. + if (aktiv === true && !vorhanden) { await tx.userRole.create({ data: { userId, roleId: rolle.id } }); - } else if (!aktiv && vorhanden) { + } else if (aktiv === false && vorhanden) { await tx.userRole.delete({ where: { userId_roleId: { userId, roleId: rolle.id } }, }); @@ -493,11 +512,7 @@ async function setzeVersteckteRolle( async function setzeHaken( tx: Prisma.TransactionClient, userId: number, - haken: { - hasDeveloperAccess?: boolean; - hasGdprAccess?: boolean; - hasAuditOpsAccess?: boolean; - }, + haken: Haken, ): Promise { if (haken.hasDeveloperAccess !== undefined) { await setzeVersteckteRolle(tx, userId, ROLLE_DEVELOPER, haken.hasDeveloperAccess); diff --git a/docs/todo.md b/docs/todo.md index 5cf5bbcf..fd831474 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -101,6 +101,45 @@ isolierte Instanz (keine Multi-Tenancy im Code), Provisioning + Abrechnung + +- [x] **🚨 R193-02: Truthiness-Bypass bei den drei Haken (HIGH)** (2026-09-09) + - Befund der Pentesterin, **Prod-Blocker**: Der Eskalations-Guard fragte + `=== true`, die Zuweisung fragte auf Truthiness. Zwischen diesen beiden + Lesarten passte der Angriff: `{"hasDeveloperAccess":"ja"}` sah für den + Guard nach keinem Rechtezuwachs aus (also kein 403) und für die Zuweisung + nach „gesetzt" (also Rolle drauf). Ein Konto mit `users:update`, das + `developer:access` **nicht** hält, konnte damit einem anderen Konto + Vollzugriff geben — und sich anmelden. + - Damit war die Kernentscheidung aus Etappe 1 ausgehebelt: `developer:access` + und `audit:admin` sollten ausdrücklich **nur über die Kommandozeile** + erstmalig vergebbar sein. Der Bypass machte sie wieder über ein + Browser-Feld erreichbar. + - Es ist exakt das Muster, das ich der Pentesterin eine Runde vorher selbst + beschrieben hatte: *„Eine Absicherung, die nur in einer von zwei Kopien + ankommt, ist keine."* Nur dass es diesmal nicht zwei Dateien waren, + sondern zwei **Auffassungen desselben Wertes** in derselben Datei. + - Fix: `normalisiereHaken()` prüft die drei Felder einmal streng auf Boolean + (alles andere 400), und **Guard und Zuweisung arbeiten danach auf + demselben geprüften Objekt** — es gibt nur noch eine Lesart. Zusätzlich + liest `setzeVersteckteRolle` strikt `=== true` / `=== false`, damit sich + die Lesart auch dann nicht spaltet, wenn die Funktion künftig von + woanders gerufen wird. + - **Gleiche Bauart nebenan gefunden:** `isActive` und `isServiceAccount` + steuern ebenfalls ein Gate, das strikt auf `boolean` prüft — ein `"ja"` + rutschte daran vorbei, ohne das Gate (u. a. die `audit:admin`-Hürde für + das Dienstkonto-Kennzeichen) auszulösen, und lief dann in einen + Prisma-Fehler. Kein Bypass, weil der Schreibvorgang scheiterte, aber ein + 500 für eine schlicht falsche Eingabe. Jetzt alle fünf Boolean-Felder + einheitlich geprüft → 400. + - Nachgeprüft: Ihre vier Vektoren plus 30 weitere Kombinationen + (`1`, `"true"`, `"false"`, `{}`, `[1]`, `null`, `"0"`, `-1`, `2.5` × drei + Haken) → **alle 400, Rollen unverändert**. Derselbe Angriff über + `POST /users` → 400, **kein Konto angelegt**. Gegenprobe: Ein Täter, der + `gdpr:*` selbst hält, setzt den Haken mit echtem `true` weiterhin + erfolgreich (200), und `hasDeveloperAccess:true` bleibt 403. + - Dateien: `src/services/rechte.service.ts`, `src/services/user.service.ts`, + `src/controllers/user.controller.ts` + - [x] **🤐 R192-01: Fehlermeldungen, die für Entwickler geschrieben sind** (2026-09-09) - Befund der Pentesterin: `PUT /users/:id {"roleIds":{}}` gab 400 mit dem rohen JS-Fehler `object is not iterable (cannot read property