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