Repository navigation
fix(credentials): random-only opaque tokens, passkey credential binding, KDF verify bounds - #84
Merged
Merged
Conversation
format: 'ulid' produced consecutive, guessable tokens: the ULID generator is monotonic within a millisecond, so successive tokens differed only in their last character (…4WK, …4WM, …4WN). uuid7 is likewise mostly timestamp. And bytes: 1 was accepted. - ulid and uuid7 are refused as token formats and as generators - byte-based generators need bytes ≥ 16, alphanumeric needs length ≥ 22 (both ≥ 128 bits); uuid4 (122 random bits) stays BREAKING CHANGE: ulid / uuid7 formats and sub-128-bit sizes are rejected.
The memory store only pruned expired entries, so tokens minted without an expiry (or faster than they expired) accumulated without limit. It now evicts the oldest entry beyond maxEntries (default 100,000).
Omitting expiresIn silently minted a token that never expires. A credential with no end of life should be a decision, not an omission: create() now throws without expiresIn, and expiresIn: null mints a deliberately non-expiring token. BREAKING CHANGE: pass expiresIn (a duration, or null) to create().
The RFC 7662 introspection handler answered anyone: holding a token was enough to read its metadata back (user ids, scopes, whatever was stored), turning the endpoint into an open oracle. RFC 7662 §2.1 requires the endpoint to authorize the calling resource server. introspectionHandler now requires authorize(req) and answers 401 invalid_client unless it returns true. revocationHandler accepts the same hook optionally (revoking a token you hold is harmless self-service). BREAKING CHANGE: pass authorize to introspectionHandler.
…nd its owner authentication.finish verified the signature against whatever stored credential the caller passed, but never checked that the assertion was made with that credential, that the credential belonged to expectedUserId, or that the (unsigned) userHandle matched — while returning userHandle as if it were trustworthy. An app that loaded "the credential" by response.id and "the user" from the challenge or the userHandle accepted an attacker's own passkey as the victim's login. - credential.id (as returned by registration) is required and must equal the assertion's rawId - with expectedUserId, credential.userId is required and must match - a stored credential.userHandle must equal the response userHandle - mismatches are the new CREDENTIAL_MISMATCH BREAKING CHANGE: pass credential.id (and credential.userId when using expectedUserId) to authentication.finish.
requireTrustAnchor rejected an unanchored certificate chain but exempted fmt: none and packed self attestation. The client chooses the attestation format, so anyone could satisfy the policy by simply not attesting — the option enforced nothing. It now accepts only trustPath 'trust-anchor'; none, self and unanchored chains fail with ATTESTATION_TRUST_ANCHOR_MISSING. BREAKING CHANGE: with requireTrustAnchor, none and self attestation are rejected.
The packed verifier required an AAGUID extension in the leaf certificate to match authData's AAGUID, but the TPM verifier never looked, so a TPM AIK certificate for one authenticator model could attest another (WebAuthn L3 §8.3). The check is now shared by both verifiers.
readAuthenticatorExtensions copied every CBOR extension key into a plain
{} under raw, so an authenticator-supplied __proto__ key replaced the
object's prototype. raw is now a null-prototype object.
importCoseKey checked the curve for EC2 / OKP keys but accepted any RSA key regardless of alg, so an RSA key labelled ES256 imported and then verified RSA signatures under the ECDSA name (node:crypto picks the scheme from the key). The kty must now be the one the algorithm is defined over (EC2 for ES*, OKP for EdDSA, RSA for RS* / PS*).
verifyChain checked that every issuer was a CA (and, through checkIssued, that it may sign certificates) but ignored basicConstraints pathLenConstraint, so a CA restricted to signing end-entity certificates (pathlen:0) could anchor a chain with an extra intermediate below it (RFC 5280 §4.2.1.9). The constraint is now enforced for every issuer in the path.
The SafetyNet leaf CN check and the packed OU check ran regexes over Node's flattened subject string. Node escapes separators inside values, so a certificate with CN="evil,CN=attest.android.com" satisfied the SafetyNet CN check, and any value containing "OU=Authenticator Attestation" (e.g. in O) satisfied the packed one. The subject is now parsed into attributes and compared exactly. (Chain verification to the trust anchors still applies on top.)
buildAaguidIndex copied each entry's metadata statement and dropped its FIDO statusReports, so an RP consulting the index could not see that an authenticator model was REVOKED or had a reported key compromise. Each entry now carries statusReports and the latest status. The index is a null-prototype object, since its keys come from the blob.
The strict decoder refused duplicate integer / text map keys, but byte-string keys decode to fresh Uint8Arrays, which a Map compares by identity — so two equal byte-string keys were both accepted and the later one silently won. They are now compared by content.
verify() reads its cost parameters back from the stored hash, which a database compromise or bad import can poison. scrypt capped ln / r / p individually but still allowed a 64 GiB working set (ln=24, r=32); argon2 passed m / t / p straight to the native code; PBKDF2 (and scrypt) derived as many output bytes as the stored hash had, and both could throw instead of returning false. - scrypt: 128·N·r ≤ 1 GiB and 128·N·r·p ≤ 4 GiB - argon2: m ≤ 1 GiB, t ≤ 10, p ≤ 16, parsed before the native call - every algorithm: stored hash length 16-64 bytes - scrypt / pbkdf2 return false when node:crypto rejects the parameters Out-of-bounds hashes are a mismatch (false), returned before any derivation.
singleUse was enforced only when the verifier passed consume: true. Without it, a challenge minted as single-use verified any number of times — the guarantee depended on every call site remembering the flag. Verifying a single-use challenge without consume now fails with the new reason 'consume_required'. BREAKING CHANGE: pass consume: true (and the store) when verifying single-use challenges.
rehashApiKey parsed the presented key and wrote a fresh hash for its secret under the key's id without checking that the secret was the one on file. Anyone who knew a key id — it appears in every masked key — could overwrite that key's hash with one for a secret of their choosing and take the key over. The presented secret must now match the stored hash under one of the configured peppers; otherwise nothing is written and the call returns false.
verifyApiKey answered 'revoked' or 'expired' before comparing the secret. The key id is not secret (it is part of every masked key), so anyone holding an id could learn whether that key was revoked or expired. The secret is now compared first; a wrong secret is always 'bad_secret'.
Crockford decoding is forgiving — lowercase, I/L for 1 and O for 0 all decode to the same bytes — so one API key had many accepted spellings. Anything keyed on the wire string (logs, deny lists, dedupe) missed the variants. A secret that does not re-encode to exactly itself no longer matches.
Emails were used exactly as given for the record index, the per-email rate-limit counter and revokeAllForEmail / listPendingForEmail. So Victim@x.com and victim@x.com were two identities: each had its own rate-limit budget (multiply it by every casing), and revoking links for one spelling left the other's links valid. Addresses are now trimmed and lower-cased before any of these, and expectedEmail is compared in the same form. BREAKING CHANGE: stored records keep the email they were created with; links issued to a mixed-case address before upgrading are no longer found by listPendingForEmail / revokeAllForEmail.
When full, the in-memory IncrStore dropped its least-recently-used key to make room. Those keys are rate-limit counts and single-use markers (challenge jti, OTP replay guard), so a flood of fresh keys reset other users' limits and reopened replay windows — an evicted challenge jti counted 1 again and verified twice. A full store now reclaims expired keys (sweeping at most once a second) and, if every slot is still live, throws instead of evicting; callers already treat a store failure as a refusal (challenge: store_unavailable). Used by challenge and magic-link memory stores.
The memory store kept every link forever — consumed links were only flagged, expired ones never removed — so link requests grew memory without limit. Expired links are now swept in the background and the store holds at most maxRecords (default 100,000), dropping the oldest.
verifyTotp stored the replay marker for (window + 1) periods from now. A code matched ahead of the window centre stays acceptable for up to (2·window + 1) periods, so the marker expired first and the same code verified again. The TTL now runs until the matched counter leaves the acceptance window.
normalizePassword ran NFKC over the whole input and only then checked the 1024-byte cap, so the normalization pass itself was exposed to an arbitrarily large password field. The input is now rejected first when its UTF-16 length already exceeds the cap (UTF-8 is never shorter), then normalized and checked again in bytes. Defence in depth: plain ASCII normalizes quickly, so the test asserts the early rejection rather than reproducing a slowdown.
pepper.wrap HMAC'd the raw input, skipping the NFKC normalization, NUL check and length cap every unpeppered hash gets — so the same visible password typed with composed or decomposed characters peppered to two different values (a user locked out after switching devices), and an unbounded input went straight into the HMAC. wrap now normalizes first. wrapAll additionally yields the legacy raw-input digest so hashes peppered before this change still verify; callers already rehash under candidates[0] when a later candidate matches.
The word-list comment claimed 12.9 bits per word (~77 bits for six words) — the figure for a 7776-word Diceware list. The bundled list has 256 words: 8 bits per word, ~48 bits for the default six. The option's JSDoc already said 48; the comment now agrees and says what that is good for.
…n authorize Drop ulid / uuid7 from the format docs and state the 128-bit minimum, mark expiresIn as required (null for a non-expiring token), document the authorize hook introspectionHandler now requires, and the memory store's maxEntries bound.
Document credential.id / userId / userHandle and CREDENTIAL_MISMATCH for authentication.finish, requireTrustAnchor admitting only anchored chains (plus pathLenConstraint and the TPM AAGUID check), and the status / statusReports on MDS index entries. Also drop ATTESTATION_CERT_REVOKED from the error catalogue: no code path ever raised it.
…hecks Document the consume_required reason and the memory store refusing to evict live keys; for apikey, rehashApiKey's secret check, secret-first verification and canonical key spelling.
…play TTL Document password verify's cost ceilings and the normalizing pepper (with legacy candidates in wrapAll), the TOTP replay marker lasting as long as the matched code is acceptable plus what 10-digit codes really carry, and magic-link's normalized addresses and bounded memory store.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / why
Hardening across the credential packages. Each fix has a regression test that reproduced the problem.
@exortek/opaqueformat: 'ulid'tokens were sequential (the ULID generator is monotonic within a millisecond — consecutive tokens differed only in their last character), andbytes: 1was accepted. Tokens are now random-only with ≥ 128 bits.create()requiresexpiresIn(null= deliberately non-expiring);introspectionHandlerrequires anauthorizehook (it was an open token-metadata oracle); the memory store is bounded.@exortek/passkeyauthentication.finishnever checked that the assertion came from the supplied credential, that the credential belonged toexpectedUserId, or that the unsigneduserHandlematched. It now does (CREDENTIAL_MISMATCH).requireTrustAnchorexemptednone/ self attestation, so the client could opt out by not attesting — now only anchored chains pass.pathLenConstraint, exact subject-attribute matching (an escaped comma smuggledCN=attest.android.com), MDS status reports kept, COSE kty bound to alg, duplicate byte-string CBOR keys rejected.@exortek/password—verifybounds the cost a (possibly tampered) stored hash can demand (scrypt 64 GiB, unbounded Argon2m/t, unbounded digest length) and never throws; pepper now normalizes like the unpeppered path (legacy candidate kept inwrapAll); length cap before NFKC.@exortek/challenge— asingleUsechallenge verified withoutconsumeused to pass indefinitely →consume_required.@exortek/apikey—rehashApiKeylet anyone who knew a key id overwrite its hash; it now verifies the secret first. Secret checked before lifecycle state; canonical spelling only.@exortek/magic-link— emails normalized (case variants had separate rate-limit budgets and escapedrevokeAllForEmail); bounded memory store.@exortek/otp— the TOTP replay marker could expire while the code was still acceptable.@exortek/shared— the memory incr store evicted live counters under a key flood (rate-limit reset, challenge replay); it now fails closed instead.Which package(s)?
@exortek/opaque@exortek/passkey@exortek/password@exortek/challenge@exortek/apikey@exortek/magic-link@exortek/otpType of change
Checklist
yarn verifypasses locally (Redis integration suites skipped locally — CI runs them)web/content/…)Additional notes
Not released on merge.