Security/harden session deserialization - #24
Merged
Conversation
SessionRecord is rebuilt from persisted JSON via JSON.parse, which surfaces a literal "__proto__" member as an own enumerable key. Writing it back with obj[key] = ... on a fresh object reparents that object's prototype, so a tampered or untrusted session store could mutate prototype chains during deserialization (SEC-001, CWE-1321). Skip __proto__/constructor/prototype at both untrusted-key assignment sites (the per-session loop in deserialize and the per-chain loop in deserializeSession). Legitimate ratchet/session keys are 33-byte binary strings and never collide with these names, so dropping them is safe. Add regression tests covering a tampered session key and a tampered chain key; both fail without the guard and pass with it.
The config shipped the unmodified GitHub template with an empty
package-ecosystem (""), which Dependabot ignores — so no dependency or
security-update PRs were ever opened for this security-critical library
(SEC-002).
Configure the npm ecosystem (runtime + dev dependencies) and the
github-actions ecosystem (CI / CodeQL workflow actions). Routine dev-tooling
minor/patch bumps are grouped into one PR to reduce noise; the runtime
dependency and security fixes still get dedicated PRs.
The CodeQL workflow used deprecated action versions (actions/checkout@v3, github/codeql-action/*@v2), left the security-extended query pack commented out, and only analyzed JavaScript — so the vendored native Curve25519/Ed25519 C sources under native/ (the crypto core) were never scanned (SEC-009). - Bump to actions/checkout@v4 and github/codeql-action/*@V3. - Add c-cpp to the language matrix so native/** is analyzed. - Use build-mode: none for both languages: the C is compiled with emscripten, which Autobuild could not drive, so the build-less scan covers the native code without adding an emsdk toolchain to CI. Drop the now-unused Autobuild step. - Enable the security-extended query suite (quality/style queries omitted to keep alerts high-signal). - Switch to the current language IDs (javascript-typescript, c-cpp) and tag analyze results per language via category.
A nonzero (err) result means the result is undefined, so the library could emit a bogus public key, shared secret, or signature instead of failing. Throw on a nonzero status. The _free calls are kept on the error path (moved above the check), so failing closed leaks no WASM heap.
X25519 performs no point validation, so a peer supplying a low-order / small-subgroup public key drives the agreement to the all-zero value (RFC 7748 section 6.1). #sharedSecret now rejects an all-zero result as a contributory- behaviour check, covering both the X3DH and Double Ratchet DH paths. The OR-accumulate runs over all 32 bytes unconditionally, so the check is constant-time and leaks nothing about the secret. A legitimate agreement is all-zero only with negligible (~2^-256) probability, so there are no false positives.
`#validatePubKeyFormat` accepted a raw 32-byte (un-prefixed) public key with only a console.error and then proceeded, silently coercing a mis-encoded wire key into a curve point on the ECDH and signature-verification paths. The warning was invisible in production. Make the canonical 33-byte 0x05-prefixed form the default and reject raw 32-byte keys. Acceptance of the raw form is now an explicit opt-in (allowRawKey), used only by curvePubKeyToEd25519PubKey, whose public contract documents accepting both forms.
`#keyPair` built its working array with new Uint8Array(privKey), a view over the caller's ArrayBuffer, so the X25519 clamping ran in place: it rewrote the caller's input buffer and returned a privKey aliasing that same buffer. Clamping is idempotent and the library only ever passes freshly generated keys internally, so this never produced a wrong result, but it is a footgun for consumers of the exported createKeyPair that hold or reuse the private key. For example, a later zeroize of the returned key would also wipe their copy.
isEqual compared via substring(0, Math.min(maxLength, str.length)), which always reduces to the full string (min is the string's own length) — dead, confusing code that boils down to aStr === bStr, a short-circuiting, non-constant-time compare (SEC-006). Replace with an explicit length check plus an OR-accumulate over all characters so the run time no longer depends on where two values first diverge.
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
1 participant
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.
No description provided.