25 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| quick-261009-ikt | 2026-10-09T16:45:00Z | standard | 62 |
|
|
issues_found |
Quick 261009-ikt: Code Review Report (Zertifikat-Manager Umbau)
Reviewed: 2026-10-09 Depth: standard (plus targeted reproductions in the scratchpad; no source files modified) Status: issues_found
Summary
Test suites are green (API cert-manager and common: 331 tests; web cert-manager: 142 tests; tsc clean; message gate "messages ok 237"; Biome only reports two pre-existing, untouched files). That does not cover the findings below, which I reproduced against the real code and libraries.
Verified as sound: the SSRF design in cert-aia.ts (the connect-time lookup really blocks a hostname that resolves to loopback, reproduced with a local server; the address is read from the certificate on the server; redirects are re-checked; the isPublicHttpUrl signature is unchanged, so favorites icon discovery and nextcloud-status are not broken, the hardening only blocks more); cert-json-body.ts is named so that Nest keeps its global JSON parser (jsonParser name check in registerParserMiddleware) and mounted without a global prefix; passwords and keys are never logged (the request log is path/status only; error texts are fixed strings); chain building requires checkIssued and verify, with cycle protection; template snippets never contain a real password (IIS uses Read-Host, Tomcat uses the placeholder IHR-PASSWORT); the forge.pki patch is synchronous and restored in finally.
Three problems are blockers: a decompression bomb that bypasses every ZIP limit, a quadratic PEM scanner that freezes the API process, and a PKCS#12 password-encoding bug that produces unusable "Modern" PFX files for passwords with umlauts and rejects correct umlaut passwords on read.
Critical Issues
CR-01: ZIP bomb bypasses all limits when the central directory declares size 0
File: apps/api/src/cert-manager/zip-expand.ts:102-122 (root cause in adm-zip methods/inflater.js)
Issue: All limit checks run on the declared entry.header.size from the central directory. adm-zip 0.6.0 only passes maxOutputLength to zlib when the declared size is > 0 (expectedLength > 0 ? { maxOutputLength: expectedLength } : {}). An entry that declares size = 0 but carries a deflate stream of, say, 300 KB therefore:
- passes the ratio check (
0 / compressedSize = 0), - adds 0 to
total, - is inflated without any bound by
entry.getData()(line 122). Only afterwardsdata.length > maxEntryBytesis checked, and the CRC check fails after the allocation has happened.
Reproduced: a 305,870-byte ZIP with the size field patched to 0 inflated to 314,572,800 bytes in 1.3 s. The upload limit is 5 MiB per file, and deflate reaches about 1000:1, so one request can allocate around 5 GB and crash or freeze the whole API container (an authenticated module user is enough). The header comment in the file ("adm-zip entpackt hoechstens die deklarierte Groesse") is wrong for this case. The comment's claim that "all checks run before the first unpacking" holds only for honest headers. Fix: Do not rely on the header size for the bound. Reject the inconsistent case and inflate with your own cap:
if (size === 0 && entry.header.compressedSize > 0) {
ignored.push({ file, path, reason: 'suspicious' });
continue;
}
// and for the actual extraction use a hard cap instead of entry.getData():
// zlib.inflateRawSync(rawCompressedBytes, { maxOutputLength: limits.maxEntryBytes + 1 })
// (STORED entries: compare compressed length with maxEntryBytes first)
Add a spec that patches the size field to 0 (the repro is a five-line script with buf.writeUInt32LE(0, centralDirOffset + 24)).
CR-02: Quadratic PEM block regex lets one small upload freeze the API for minutes
File: apps/api/src/cert-manager/cert-model.ts:61 and :271 (same regex in cert-aia.ts:162,185)
Issue: /-----BEGIN ([A-Z0-9 ]+)-----([\s\S]*?)-----END \1-----/g is applied to the whole file text. For every -----BEGIN X----- that has no matching END, the lazy [\s\S]*? scans to the end of the input, so a file made of repeated BEGIN lines costs O(n^2). Measured with detectBlob on '-----BEGIN A-----\n' repeated: 100 KiB takes 0.38 s, 200 KiB takes 1.3 s, 400 KiB takes 5.0 s. The upload limit is 5 MiB per file (the client limit is 10 MiB total), which extrapolates to roughly 15 minutes of blocked event loop on a single request. Node is single threaded, so the whole Tessera API (login, dashboard, all modules) stops. The AIA copy is capped at 256 KiB (about 2 s) but is fed by an attacker-chosen public server, three URLs per click.
Fix: Replace the regex with a linear scanner (indexOf('-----BEGIN '), read the label, indexOf('-----END label-----', start), and abort or skip when no END exists so that a missing END is not rescanned for every later BEGIN), and add a block-count cap (for example 200 blocks per blob). Add a timing-style spec with 400 KiB of BEGIN lines.
CR-03: "Modern (AES-256)" PFX is unreadable for non-ASCII passwords; correct umlaut passwords are rejected on read
File: apps/api/src/cert-manager/cert-pkcs12.ts:113 (read) and :185 (write)
Issue: forge derives the MAC and the PKCS#12 3DES keys from the UTF-16 password (correct), but the PBES2 key derivation (algorithm: 'aes256', and every OpenSSL-3-default PFX on read) uses forge's PBKDF2 on the raw JS string, i.e. one byte per UTF-16 unit, instead of the UTF-8 bytes OpenSSL, Windows and Java use. Reproduced with openssl 3:
- Write:
writePkcs12({profile:'modern', password:'pässwörd'})and'pw€'round-trip inside Tessera, butopenssl pkcs12 -in file -passin pass:pässwördfails withbad decrypt. The "compat" files open fine. The user downloads a PFX that no other tool can open, with the password they typed, and gets no error. - Read: an OpenSSL-3 PFX created with
-passout 'pass:pässwörd€'returns{ ok:false, reason:'passwordWrong' }for the correct password ("Das Passwort passt nicht"), because the MAC passes and the shrouded key bag then throws a decrypt error that the broad regex at:120classifies as a password problem.
Passwords with umlauts are realistic for this (German) user base. The same module's key export/import (cert-keys.ts) goes through node:crypto and is not affected.
Fix: Do not let forge decrypt or encrypt the PBES2 bag. For writing: build the shrouded key bag from key.export({type:'pkcs8',format:'der',cipher:'aes-256-cbc',passphrase}) (node uses UTF-8) and keep forge only for the MAC and the cert bags. For reading: lift each pkcs8ShroudedKeyBag and decrypt it with createPrivateKey({key: der, format:'der', type:'pkcs8', passphrase}). As a minimum, reject non-ASCII passwords for modern with a clear error and, on read, try forge.util.encodeUtf8(password) as a second candidate. Add a round-trip spec against openssl pkcs12 with a non-ASCII password for both profiles.
Warnings
WR-01: 413/400 answers of the build parser carry no CORS headers
File: apps/api/src/main.ts:24-26 (cors is enabled later at :36)
Issue: app.use(CERT_BUILD_ROUTE, certBuildJsonBody, certBuildBodyErrors) is registered before app.enableCors(...). In the Express adapter enableCors is just another use(), so middleware order is: request log, cookies, build parser (and its error handler), cors, Nest parsers. For an oversized or malformed build body, certBuildBodyErrors answers 413/400 before the cors middleware ran, so in the cross-origin setup (NEXT_PUBLIC_API_URL=http://localhost:3001) the browser blocks the response and fetch throws a TypeError. The web code then shows errors.generic instead of the intended tooLarge/invalidInput messages. Same-origin production is unaffected.
Fix: Register the build parser after enableCors (still before listen):
app.enableCors({ origin: corsOrigin, credentials: true });
app.use(CERT_BUILD_ROUTE, certBuildJsonBody, certBuildBodyErrors);
Parsers are only installed at init() inside listen, so the order relative to Nest's global parser stays correct.
WR-02: Output tabs keep working on a stale analysis (removed files, failed re-analysis)
File: apps/web/src/app/(portal)/modules/cert-manager/use-cert-workspace.ts:67-81, consumers MergeTab.tsx:42-56, TemplatesTab.tsx:71-85, ConvertTab.tsx:59-70, SplitTab.tsx:71-80
Issue: remove(), clear of single entries and password changes update entries immediately but leave the previous analysis in place until the new response arrives. If the re-analysis fails (network, 413, 500) the old analysis stays indefinitely with status:'error'. The tabs only look at status === 'analyzing' && !analysis, so they continue to offer downloads of certificates and private keys from a file the user just removed (for example a key file removed for safety). The "N Dateien" line then no longer matches what is offered.
Fix: Treat the analysis as valid only when it matches the current entries: expose fresh = status === 'idle' && analysisIds.length === entries.length && analysisIds.every((id, i) => id === entries[i].id) from the hook, show the "analysing" or error state otherwise, and disable the download buttons while !fresh.
WR-03: "Aufteilen" silently writes password-protected keys in clear text
File: apps/web/src/app/(portal)/modules/cert-manager/components/SplitTab.tsx:36-47,95-105, apps/api/src/cert-manager/cert-keys.ts:111-124
Issue: keyItemFromObject always exports an unencrypted PKCS#8 PEM, even when the source key was encrypted (wasEncrypted: true). The Split tab downloads that PEM as schluessel.key and puts every key unencrypted into the "alle als ZIP" archive. A user who splits a protected bundle gets plaintext key files with no hint (the intro text split.intro says nothing), and the unlocked key also travels in every analyze response. This is a security-relevant surprise for a tool whose purpose is handling secrets.
Fix: In Split, show a visible note on keys with wasEncrypted, or offer the same password option as the Convert tab (exportKey(key,'pkcs8',password)), or exclude keys from the ZIP unless the user ticks them. At minimum extend split.intro (de/en).
WR-04: Attacker-chosen KDF iteration counts run synchronously, up to 11 times per PFX
File: apps/api/src/cert-manager/cert-pkcs12.ts:105-122, apps/api/src/cert-manager/cert-keys.ts:155-163
Issue: readPkcs12 tries own password, empty password and up to 10 more, each with a full pure-JS forge PBKDF/MAC computation; the iteration count comes from the uploaded file and is never bounded. openWithPasswords does the same with node:crypto for encrypted PKCS#8 (synchronous OpenSSL PBKDF2). A crafted PFX or encrypted key with a huge iteration count blocks the event loop for as long as the author wants, per attempt, per file (30 files).
Fix: Parse the MAC iteration count (macData.iterations) and the PBES2/PBE iteration counts before trying passwords and treat anything above a sane cap (for example 1,000,000) as unsupported (ignored: unsupportedKey or a new reason). Keep the attempts per container low.
WR-05: No cap on the number of parsed items; chain building is O(n^2) on the event loop
File: apps/api/src/cert-manager/cert-chain.ts:27-52, apps/api/src/cert-manager/cert-analyze.ts:51-77
Issue: The ZIP path limits entries (100), but a single PEM bundle may contain thousands of distinct certificates (5 MiB is about 10k certificates). toNodes runs checkIssued over every pair; matchKeys multiplies certificates by keys; isSelfSigned verifies a signature per certificate. Measured: 600 distinct CA certificates in one file take 480 ms; the cost grows quadratically, so a 10 MiB request blocks the API for minutes.
Fix: Cap items per request (for example 200 certificates and 50 keys; return tooLarge or an ignored reason beyond that), and precompute subject/issuer buckets so only certificates with a matching name are compared.
WR-06: Multer buffers up to 150 MiB before the 20 MiB total check
File: apps/api/src/cert-manager/cert-manager.controller.ts:62-76
Issue: FilesInterceptor('files', 30, { limits: { fileSize: 5 MiB } }) uses memory storage; the total limit (CERT_MAX_TOTAL_BYTES) is only checked after all 30 files are fully buffered, so a request can hold 150 MiB (plus multer parts overhead) before it is rejected. There are also no fields/parts limits.
Fix: Add limits: { fileSize, files: 30, fields: 5, parts: 40 } and check the running total early with a small custom multer storage or a Content-Length pre-check (reject above about 21 MiB) in a guard/middleware.
WR-07: Certificates without basicConstraints are classified as server certificates, even self-signed CAs
File: apps/api/src/cert-manager/cert-model.ts:115-131
Issue: role is derived from x.ca (basicConstraints cA flag only). A self-signed certificate that lacks basicConstraints, including legacy v1 roots and some private CAs, gets isCa=false and therefore role end-entity, although selfSigned is true. Reproduced: a self-signed certificate without the extension gives ca false, selfverify true, checkIssued true. Consequences: such a root shows as "Serverzertifikat", appears as an extra chain head in defaultHeads, is never reported as rootId, and cannot be excluded by "Root-Zertifikat mitnehmen" (cert-output.ts filters by role === 'root').
Fix: Treat selfSigned && (x.ca || no basicConstraints but keyCertSign usage or v1) as root, or at least selfSigned && x.subject === x.issuer && !hasServerAuthSan heuristics. Simplest consistent rule: if the certificate is self-signed and has no basicConstraints extension, show it as role root only when keyUsage contains keyCertSign or the version is 1; otherwise keep end-entity but do not offer it as a chain member.
Info
IN-01: IPv6 literal URLs are always rejected, so the literal branch of the IPv6 hardening is dead
File: apps/api/src/common/public-url-guard.ts:162-166
Issue: URL.hostname keeps the brackets ([2606:4700::1111]), so isIP(url.hostname) is 0, the code falls into lookup('[...]'), which fails and returns false. Safe (fail closed), but the changelog and header comment suggest literal IPv6 addresses are now parsed and judged; in practice only DNS answers reach isPrivateIpv6. Public IPv6 AIA/favicon hosts given as literals can never work.
Fix: Strip the brackets before isIP (url.hostname.replace(/^\[|\]$/g, '')) and add a spec for new URL('http://[::ffff:7f00:1]/') and a public literal.
IN-02: aiaInternal text is used for port, credential and length refusals
File: apps/api/src/cert-manager/cert-aia.ts:297-299,324-326; message errors.aiaInternal in de.json
Issue: A public host with a non-default port (http://pki.example.com:8080/ca.crt) is refused with "verweist nicht auf einen öffentlichen Server", which is wrong. Also, the UI button is shown for such URLs because cert-model.ts:aiaUrls (line 90-104) does not apply the same filter as issuerUrls (credentials, length, port).
Fix: Add a separate code (for example aiaPortNotAllowed) or reword the message ("Adresse nicht zulässig"), and share one URL filter between cert-model.ts and cert-aia.ts so the button only appears when a fetch can actually happen.
IN-03: errors.generic says "Dateien konnten nicht geprüft werden" for download failures
File: apps/web/src/messages/de.json (certManager.errors.generic), used by MergeTab.tsx, ConvertTab.tsx, TemplatesTab.tsx via certErrorKey
Issue: A 500 from build shows the analysis error text, which is misleading.
Fix: Add errors.buildFailed and map generic per call site, or phrase the generic message neutrally ("Die Anfrage konnte nicht ausgeführt werden.").
IN-04: Object URL is revoked synchronously after click()
File: apps/web/src/app/(portal)/modules/cert-manager/actions.ts:282-285, components/SplitTab.tsx:56-60
Issue: URL.revokeObjectURL(url) directly after anchor.click() can cancel the download in some browsers (older Safari/Firefox).
Fix: setTimeout(() => URL.revokeObjectURL(url), 1000).
IN-05: Smaller UI accuracy and key issues in the Dateien tab and Split tab
File: apps/web/src/messages/de.json (files.locked.note), FilesTab.tsx:268-273, SplitTab.tsx:95-105
Issue: (a) The note says the password is "nur zum Öffnen der Datei übertragen", but every add/remove/unlock re-uploads all files with all passwords, and the server also tries each file's password on every other file (candidatePasswords). (b) The rejected list uses key={${r.name}-${r.reason}}, which duplicates when two files with the same name are rejected for the same reason. (c) Several keys in the Split tab get the identical file name schluessel.key for single downloads (only the ZIP is de-duplicated).
Fix: Reword the note ("wird mit jeder Prüfung mitgesendet, aber nie gespeichert"), use the index in the key, and name keys after their certificate (certIds[0]) or number them.
Reviewed: 2026-10-09 Reviewer: Claude (gsd-code-reviewer) Depth: standard
Fix status
Behoben in den lokalen Commits bfcf6d4 (API, Fixtures) und c5bffe9 (Web, Anleitungen). Jeder Blocker hat einen Regressionstest, der gegen den alten Code rot war: CR-01 vier Tests rot gegen die alte zip-expand.ts; CR-02 der Test mit 20 000 unvollständigen BEGIN-Zeilen brauchte mit der alten Regex 4,6 s; CR-03 vier Tests rot gegen das alte cert-pkcs12.ts (modern lesen und schreiben, mit pässwörd und pw€).
| ID | Status | Wie |
|---|---|---|
| CR-01 | fixed | zip-expand.ts entpackt selbst: inflateRawSync mit maxOutputLength (Einzelgrenze, höchstens Restgrenze der Summe), STORED vorab gegen den Deckel; Größe, Verhältnis und CRC am echten Ergebnis, abweichende Kopfgröße = suspicious, Summe der echten Größen > 20 MiB = ganzes ZIP zipTooLarge. Spec: ZIP mit Kopfgröße 0 (200 MiB Nullbytes) wird mit tooLarge abgewiesen, Arbeitsspeicher < 50 MiB, < 2 s; e2e live (200 MiB, Antwort in unter 5 s, API gesund). |
| CR-02 | fixed | Neu pem-scan.ts (END-Stellen je Etikett in einem Durchlauf, Zeiger je Etikett, 1000 Blöcke Deckel), benutzt von cert-model.ts und cert-aia.ts. Specs: 5 MiB BEGIN-Zeilen (gleiche und verschiedene Etiketten) weit unter 100 ms (bestes von drei Läufen), Wiederholungen ohne Etikett; detectBlob mit 5 MiB BEGIN-Zeilen; e2e live (< 2 s). |
| CR-03 | fixed | withForgeKdf in cert-pkcs12.ts ersetzt für die Dauer des synchronen Lesens und Schreibens forge.pkcs5.pbkdf2 durch node:crypto mit UTF-8-Bytes (3DES/RC2/MAC-Ableitung bleibt BMPString wie bei OpenSSL; der Reviewer-Vorschlag mit Schlüsselbeutel über node:crypto war nicht nötig). Neue Fixtures rsa-umlaut-*.pfx, rsa-euro-*.pfx (OpenSSL 3.5, modern und compat). Specs: lesen aller vier mit richtigem Passwort, falsches bleibt passwordWrong, Schreiben: der Schlüsselbeutel öffnet sich unabhängig von forge mit createPrivateKey und UTF-8-Passwort. e2e live in beide Richtungen mit openssl pkcs12 -passin pass:pässwörd und pw€, compat und modern. Kein Passwort wird abgelehnt. |
| WR-01 | fixed | Neu http-setup.ts (configureHttp), main.ts ruft es auf; der build-Leser wird nach enableCors eingehängt. Spec prüft die Reihenfolge; e2e live: 413 mit access-control-allow-origin bei gesetztem Origin. |
| WR-02 | fixed | useCertWorkspace liefert fresh (keine Analyse läuft, letzte erfolgreich, gleiche Eintragskennungen in gleicher Reihenfolge). Die fünf Ausgabe-Reiter lesen nur noch freshAnalysis(workspace); sonst AnalysisPending (Prüfhinweis oder Fehler mit „Erneut versuchen“). Dateien-Reiter zeigt weiter die letzte bekannte Analyse je Eintrag. Tests: Hook (use-cert-workspace.test.tsx) und alle fünf Reiter (stale-analysis.test.tsx); Browser dunkel geprüft (ikt-fix-*.png). |
| WR-03 | fixed | Aufteilen: war ein Schlüssel geschützt, ist „Schlüssel mit einem Passwort schützen“ vorgewählt, die Schlüsselknöpfe und die ZIP warten auf ein Passwort, die Ausgabe kommt von build (content: key, pkcs8) verschlüsselt, auch in der ZIP. Ohne Schutz sichtbarer Hinweis. Browser: heruntergeladene Datei ist ENCRYPTED PRIVATE KEY, mit dem neuen Passwort lesbar, mit dem alten nicht. Die Analyse-Antwort enthält den Schlüssel weiterhin entschlüsselt (wie von Task 4/5 für die Ausgabe vorgesehen, nirgends protokolliert). |
| WR-04 | fixed | cert-budget.ts: eine Ableitung höchstens 1 000 000 Runden, alle zusammen 6 000 000 je Anfrage, gemeldet vor dem Rechnen (PKCS#12 über die drei forge-Ableitungsfunktionen, PKCS#8 über pkcs8Iterations aus den Parametern); Folge: protectionTooExpensive für die Datei. Fixtures mit 2 000 000 Runden (PFX und PKCS#8); Specs und e2e. |
| WR-05 | fixed | RequestBudget: 200 Zertifikate, 50 Schlüssel, 50 Anfragen je Anfrage, über alle Dateien und ZIP-Einträge, geprüft beim Erzeugen (Fehler verschluckende Stellen reichen sie mit rethrowRequestError weiter); 413 tooManyItems, Meldung de/en. Specs (200 ok, 201 413, über Dateien und ZIP, 50/51 Schlüssel, Müll zählt nicht), e2e live mit 200/201 Zertifikaten. Die Zuordnung matchKeys und der Kettenbau bleiben damit begrenzt; eine eigene Vorsortierung nach Namen war dafür nicht mehr nötig. |
| WR-06 | fixed | Neu cert-upload.ts: eigener multer-Speicher zählt die Summe beim Empfang und bricht bei 20 MiB mit 413 tooLarge ab; dazu limits files 30, fields 5, parts 35, fieldSize 256 KiB. Specs mit Datenströmen; e2e live (5 Dateien zu 4,9 MiB). |
| WR-07 | fixed | cert-model.ts (isCaCertificate, wie OpenSSLs X509_check_ca): ohne basicConstraints zählt ein selbstsigniertes v1-Zertifikat oder eines mit keyCertSign als CA/Wurzel; mit basicConstraints entscheidet nur cA; ein selbstsigniertes Serverzertifikat ohne diese Erweiterung (nur digitalSignature/keyEncipherment) bleibt Serverzertifikat. Neue Fixtures (echte v1-Wurzel mit Blatt, v3 nur keyCertSign, v3 nur Server). Specs für Rolle, Kette (vollständig, Wurzel erkannt, kein zweiter Kopf), e2e. |
| IN-01 | fixed | public-url-guard.ts entfernt die Klammern vor isIP; Spec mit internen Schreibweisen in Klammern (gesperrt) und öffentlichen Literalen (erlaubt). |
| IN-02 | fixed | Neu cert-aia-url.ts (ein Filter für Anzeige in cert-model.ts und Abruf in cert-aia.ts: Protokoll, Zugangsdaten, Länge, Standardport); neuer Code aiaNotAllowed (Port, Zugangsdaten, Länge, Protokoll nach Weiterleitung, nur nicht abrufbare Adressen); aiaInternal bleibt für nicht öffentliche Server. Live-AIA (Let's Encrypt, YE2) unverändert grün. |
| IN-03 | fixed | certErrorKey(error, 'buildFailed') für Zusammenführen, Konvertieren, Vorlagen und Aufteilen; neue Meldung errors.buildFailed. |
| IN-04 | fixed | downloadBase64 und der ZIP-Download widerrufen die Objektadresse nach 1 s. |
| IN-05 | fixed | (a) Passwort-Hinweis neu formuliert (wird bei jeder Prüfung mitgesendet, nie gespeichert), (b) Schlüssel der Liste abgewiesener Dateien mit laufender Nummer, (c) Schlüssel heißen nach ihrem Zertifikat, sonst schluessel, schluessel-2 … |
Nicht geändert (Vorgabe): „Cert Manager“ in der Seitenleiste gegenüber „Zertifikat-Manager“ auf der Seite.
Messwerte der Gates: api vitest gesamt 169 Dateien / 3573 Tests grün; web vitest gesamt 160 Dateien / 1945 Tests grün; tsc api und web ohne Fehler; Biome auf allen geänderten Dateien ohne Befund; check-cert-messages.cjs 237 meldet messages ok 250; e2e-cert.sh all (jetzt mit Abschnitt review) grün inkl. Live-Abruf bei ye2.i.lencr.org.