Repository navigation
fix(crypto,session): full-length AEAD tags, canonical tokens, secret minimums, impersonation authorization - #82
Merged
Merged
Conversation
decryptSymmetric and decryptHybrid passed the caller's tag straight to setAuthTag without pinning its length, so a 4-byte GCM tag verified (Node only prints a deprecation warning). A forger then needs 2^32 attempts instead of 2^128. The tag must now be exactly the algorithm's tag length (16 bytes for GCM / ChaCha20-Poly1305) and authTagLength is pinned on the decipher; IV length is checked too. A short tag is DECRYPT_FAILED.
unseal and decryptFromString decoded tokens with Buffer.from(…, 'base64url'), which skips characters outside the alphabet and ignores padding. t + '=', t + '\n', ' ' + t and a stray '!' mid-token all unsealed to the same value, so anything keyed on the token string (deny lists, replay caches, dedupe sets) saw one token as many. Both now accept only the canonical unpadded base64url form (TOKEN_MALFORMED / INVALID_CIPHERTEXT otherwise).
seal / unseal only type-checked the secret, so seal(x, '') or a short password-like secret worked — the HKDF input then provides no real protection. Both now require ≥ 32 bytes per secret (every entry of a rotation list included) and throw INVALID_ARGUMENT otherwise. BREAKING CHANGE: secrets shorter than 32 bytes are rejected.
createSessionManager and createTrustedDeviceCookie only type-checked their secret, so an empty string (or Buffer.alloc(0)) was accepted even though the error text said non-empty, and cookies sealed under a short secret are forgeable. Every secret entry must now be at least 32 bytes, checked at configuration time. BREAKING CHANGE: secrets shorter than 32 bytes are rejected.
…rink
impersonate() checked that the caller had a valid, non-impersonated
session — and nothing else. Any logged-in user of an app that turned on
impersonation: true could become any other user; the authorization was
left implicit. A per-call options.ttl could also exceed the configured
impersonationTtl.
- createSessionManager requires impersonationAuthorize({ admin,
targetUserId, reason, req }) with impersonation: true; anything but
true from it is the new FORBIDDEN (403)
- options.ttl must be positive and at most impersonationTtl
BREAKING CHANGE: impersonation: true without impersonationAuthorize
throws at configuration time.
A trusted-device cookie stayed valid until it expired (30 days, say) wherever it had been copied — logout or a password change could not withdraw it. createTrustedDeviceCookie takes an optional revokedBefore(userId) → epoch ms; cookies issued before the returned cutoff no longer verify, so "forget all devices" is one timestamp.
Once full, every insert into the memory session store ran a full sweep and then scanned all sessions for the oldest anonymous one — O(n) per insert, exactly when an anonymous-session flood fills it. Anonymous sessions now live in their own insertion-ordered LRU, so picking a victim is O(1), and the garbage sweep runs at most once a second from the insert path (the background timer still runs). Eviction order is unchanged: expired / revoked first, then the oldest anonymous, then the oldest authenticated session. 10k-session store, insert at capacity: authenticated 72.1 → 2.6 µs, anonymous 39.8 → 2.2 µs.
The rolling touch in verify() read the record with get() and then update() read it again before writing — MGET, MGET, SET per sampled request. update() now takes the record the caller already holds (optional third argument; custom stores may ignore it) and the Redis store skips the re-read. Revocations are separate tombstone keys, so writing over that copy can never un-revoke a session. put() also sends the record write and the user-index write together instead of one after the other.
fingerprint() kept every visited object in its cycle set for the whole
walk, so an object reached twice through siblings ({ x: o, y: o }) was
reported as a cyclic reference and threw. The set now holds only the
current ancestors; real cycles are still rejected.
encryptSymmetric / decryptSymmetric accepted options.aad with aes-256-cbc and silently dropped it — CBC has no tag to bind it to — so callers believed a ciphertext was tied to a context it was not. AAD with a non-AEAD algorithm is now INVALID_ARGUMENT.
unseal compared now > expiresAt + clockSkew * 1000. With clockSkew: NaN (for example a mis-parsed config value) the comparison is always false, so an expired token unsealed forever; Infinity did the same. clockSkew must now be a non-negative finite number.
encryptWithPassphrase defaulted to a sound PBKDF2-SHA512 at 210,000 iterations but accepted any caller value, down to iterations: 1. New tokens now require the OWASP floor for the chosen digest (SHA-512 210,000; SHA-256 / SHA-384 600,000). Decryption still honours whatever the caller pins, so older low-iteration tokens stay readable. BREAKING CHANGE: encryptWithPassphrase rejects iterations below the floor.
base58.decode multiplies a BigInt per character, so its cost grows quadratically (32k characters took 73 ms, and it keeps growing) with no limit on attacker-supplied input. Inputs over 1024 characters are now INVALID_ENCODING unless the caller raises options.maxLength.
…8 cap Document the 32-byte seal secret minimum, canonical-only sealed / encrypted strings, the refusal of truncated AEAD tags and of AAD on CBC, the clockSkew contract, the PBKDF2 iteration floors for encryptWithPassphrase and base58.decode's length cap.
…evocation Document impersonationAuthorize (required) and FORBIDDEN, the per-call impersonation ttl that can only shrink, the 32-byte secret minimum, trusted-device revokedBefore and the optional current-record argument to a store's update().
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
Cipher, seal and session hardening. Each fix has a regression test that reproduced the problem.
@exortek/cryptodecryptSymmetric/decryptHybridnever pinned the tag length, so a 4-byte tag decrypted (2^32 forgery work instead of 2^128). The full 16-byte tag is now required andauthTagLengthpinned; IV length checked.unseal/decryptFromStringused lenient base64url decoding —t + '=',t + '\n',' ' + tand a stray!all unsealed. Only the canonical form is accepted now.seal(x, '')worked; secrets must be ≥ 32 bytes.clockSkew: NaNdisabled expiry → rejected.aadwith CBC was silently ignored → rejected.encryptWithPassphraseacceptediterations: 1→ OWASP floor per digest.base58.decodewas quadratic with no cap → 1024-character default limit.fingerprinttreated shared references as cycles → fixed.@exortek/sessionimpersonate()once the feature was on.impersonationAuthorizeis now required and decides every impersonation (FORBIDDENon refusal); a per-callttlcan only shortenimpersonationTtl.revokedBefore(userId).Which package(s)?
@exortek/crypto@exortek/sessionType of change
Checklist
yarn verifypasses locally (Redis integration suites skipped locally — CI runs them)web/content/crypto,web/content/session)Additional notes
Independent of #78–#80. Not released on merge.