fix(security): Phase 0 audit fixes — actively exploitable findings#982
Merged
fix(security): Phase 0 audit fixes — actively exploitable findings#982
Conversation
…l, X.509 Closes Phase 0.1 of the security audit. Three call sites passed the full backing ArrayBuffer to native code, ignoring view byteOffset/byteLength: - timingSafeEqual: switched from abvToArrayBuffer to binaryLikeToArrayBuffer so TypedArray / Buffer views are sliced to their window. Previously compared the entire backing, which both walked unrelated bytes and spuriously failed the byte-length check for any view smaller than its backing. - Cipher.setAAD: stopped passing buffer.buffer; now slices via binaryLikeToArrayBuffer. Sliced AAD silently authenticated the wrong bytes — a direct AEAD integrity violation across GCM/CCM/OCB/Poly1305. - X509Certificate(string): Buffer.from(str).buffer can return a pool-backed ArrayBuffer with byteOffset > 0, exposing unrelated pool bytes to native. Now routes through binaryLikeToArrayBuffer. abvToArrayBuffer's doc-comment is hardened to flag the zero-copy backing semantic and steer future callers toward binaryLikeToArrayBuffer for crypto data paths. randomFill's intentional use of the zero-copy path is preserved. Adds 5 regression tests: 3 timingSafeEqual view cases (offset views with matching content, offset views with differing content, view-vs-view across differently-sized backings) and 2 GCM sliced-AAD cases (verifies the slice is what gets authenticated, and that wrong-AAD on decrypt still throws). Also adds the implementation plan to plans/todo/security-audit.md, which sequences the remaining audit findings into 6 phases.
The previous implementation called crypto_stream_xor() on every update(), which restarts the XSalsa20 keystream at block 0. Streaming a message in N chunks therefore produced N copies of the same keystream XORed against different plaintext — a textbook two-time pad. Any caller using the streaming Cipheriv API for XSalsa20 was catastrophically vulnerable. Switch to crypto_stream_xsalsa20_xor_ic() with an explicit block counter, plus a 64-byte leftover-keystream buffer so partial-block chunks can resume cleanly on the next update(). init() resets all streaming state so re-init is safe. Output now uses unique_ptr / release() to avoid leaking on the failure path. Adds 6 streaming regression tests: block-aligned splits, mid-block splits, many-small-chunk splits, drain-to-boundary continuation, the two-time-pad regression (same plaintext → distinct ciphertexts), and a streaming round-trip across separate encrypt/decrypt instances. Closes Phase 0.2 in plans/todo/security-audit.md.
DH and ECDH both relied on EVP_PKEY_derive_set_peer() to "validate"
the peer key, but that EVP entrypoint does not call DH_check_pub_key
or EVP_PKEY_public_check, so:
- DH silently accepted peer pubkeys of 0, 1, or p-1 and produced a
"shared secret" of 0, 1, or +/-1 — the small-subgroup attack.
- ECDH silently accepted peer points on a related/weaker curve,
enabling the invalid-curve attack and leaking bits of our private
key in the resulting shared secret.
Add explicit validation up front in both computeSecret paths:
- DH: BN_bin2bn the peer key, call DH_check_pub_key against our DH
parameters, and surface TOO_SMALL / TOO_LARGE / INVALID flags as
distinct error messages (matching ncrypto's DHPointer::checkPublicKey).
- ECDH: EC_POINT_oct2point against the configured group, then
EC_POINT_is_at_infinity (reject identity), then EC_POINT_is_on_curve
(defensive — oct2point validates internally on OpenSSL 3.x but a
second check protects against regressions and partial implementations).
Adds 4 DH and 5 ECDH regression tests covering each rejection path,
including a cross-curve test (P-384 pubkey sent to a P-256 instance)
and a bit-flipped-coordinate test that exercises is_on_curve.
Closes Phase 0.3 in plans/todo/security-audit.md.
RSA decryption surfaced OpenSSL error strings verbatim through both the
C++ and TS layers, so a remote attacker could distinguish "padding
invalid" from "data too large", "bad version", "wrong key", etc. — a
classic Million Message Attack padding oracle.
Two-layer fix:
1. Implicit rejection. For PKCS#1 v1.5 decryption, enable OpenSSL 3.2+
implicit rejection via
EVP_PKEY_CTX_ctrl_str(ctx, "rsa_pkcs1_implicit_rejection", "1").
Corrupted ciphertexts now deterministically decrypt to random-looking
bytes instead of throwing. If the underlying OpenSSL build does not
support the knob (BoringSSL, pre-3.2) we hard-fail PKCS#1 v1.5
decryption with a build-config error rather than silently leaving
the timing-side oracle open — matches Node.js's crypto_cipher.cc
policy.
2. Opaque errors. Every decrypt-failure path in decrypt(),
privateDecrypt(), and publicDecrypt() (verify-recover) routes through
a single throwOpaqueDecryptFailure() helper that emits the same
"RSA decryption failed" message and clears the OpenSSL error stack so
the underlying reason never reaches the caller. The TS wrapper drops
the `: ${error.message}` interpolation and just throws
"privateDecrypt failed" / "publicDecrypt failed".
Adds 5 regression tests: corrupted PKCS#1 v1.5 doesn't throw; the
implicit-rejection output is deterministic per (key, ciphertext) and
distinct across different ciphertexts; OAEP/wrong-label errors are
opaque (no "openssl/padding/oaep/label" terms); OAEP and PKCS#1
wrong-padding errors are equivalent; publicDecrypt errors are opaque.
Closes Phase 0.4 in plans/todo/security-audit.md.
Phase 0.4 collapsed every decrypt-failure path through the new opaque
error helper, which removed the special case in publicDecrypt's second
verify_recover call that returned an empty buffer when the recovered
payload was zero bytes. That broke
`publicDecrypt(privateEncrypt(""))` round-trips — the existing test
"privateEncrypt/publicDecrypt empty plaintext" now reports
"publicDecrypt failed".
Restore the narrow OpenSSL-error-code match so empty-plaintext recovery
works again. publicDecrypt is signature verification with the PUBLIC
key (anyone can perform it), so this special case is not a
Bleichenbacher target. The fall-through still routes through the
opaque-error helper when no match is hit.
Loosen the new "Bleichenbacher: publicDecrypt errors are opaque" test
to accept either the silent-empty path or the opaque-throw path —
both are info-leak-free. The throw branch still asserts the message
contains no OpenSSL details.
Note: the broader "masks real failures" concern flagged in the audit
as MEDIUM (HybridRsaCipher.cpp:264) is preserved as a separate finding
to be tightened in a later pass with a precise OpenSSL 3.x reason-code
match.
Contributor
🤖 End-to-End Test Results - AndroidStatus: ✅ Passed 📸 Final Test ScreenshotScreenshot automatically captured from End-to-End tests and will expire in 30 days This comment is automatically updated on each test run. |
Contributor
🤖 End-to-End Test Results - iOSStatus: ✅ Passed 📸 Final Test ScreenshotScreenshot automatically captured from End-to-End tests and will expire in 30 days This comment is automatically updated on each test run. |
The previous fixup used ERR_peek_last_error which returns the NEWEST error in OpenSSL's FIFO queue. For verify_recover failures the queue typically holds an outer wrapper error on top of the inner padding-check error, so the narrow constants from the original code (0x1C880004, low-byte 0x04) never matched and every recovery went through throwOpaqueDecryptFailure. Switch to ERR_get_error to read the OLDEST queued error — same behavior as the original code that worked, restoring the empty plaintext round-trip.
Tests in this project run only in the example React Native app and the assistant cannot execute them. The default /commit and /pr skills both push immediately after committing, which has produced a "fix the fix" loop in this very session: a runtime regression shipped to the remote PR, was found by the user, the fix was committed and pushed without verification, and the fix-of-the-fix had to be pushed too. Codify the implicit norm as a HIGH/STRICT rule: after any commit that needs runtime validation in the example app, wait for the user to run the relevant test suite and explicitly confirm it passes before pushing. Iterate locally on failure; batch the validated commits into a single push when the user gives the go-ahead.
Switch the example app from the published "1.1.0" version of react-native-quick-crypto to "workspace:*" so local C++ / TS edits land on the next bun ios / bun android without a publish round-trip. Refresh Podfile.lock with the new QuickCrypto pod hash that pops out of the Phase 0 C++ changes.
4 tasks
boorad
added a commit
that referenced
this pull request
Apr 27, 2026
The e2e-android-test.yml and e2e-ios-test.yml workflows referenced 'cpp/**', 'nitrogen/**', and 'src/**' at repo root — directories that no longer exist after the workspace migration to 'packages/react-native-quick-crypto/'. Result: every C++-only PR silently skipped both E2E suites (PR #982 Phase 0, PR #983 Phase 1, and PR #984 Phase 2 all hit this). Updates both pull_request and push path filters to point at the workspace locations. Each workflow file is itself in its own paths filter, so this commit triggers the workflows on PR #984 to run for the first time on this branch's C++ changes.
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.
Summary
Phase 0 of the ongoing security audit (
plans/todo/security-audit.md) — fixes the four actively-exploitable findings before continuing with the deeper Phase 1+ sweeps. Each fix landed as its own commit with an independent crypto-specialist review and regression tests.f99a865abvToArrayBufferbyte-offset bug →timingSafeEqualHIGH, all AEADsetAADHIGH, X.509 string-ctor pool-leak (newly found)61d00aaupdate()→ catastrophic two-time pad whenever the streaming Cipheriv API was used827f0a1computeSecretaccepted invalid peer keys → DH small-subgroup attack (peer 0/1/p-1), ECDH invalid-curve attack82c6ac1Changes
0.1 — Byte-offset bug (
abvToArrayBuffer)timingSafeEqualand AEADsetAADwere passing the entire backingArrayBufferto native instead of the requested view. For sliced/offset buffers this compared/authenticated the wrong bytes.binaryLikeToArrayBuffer(offset-aware), and hardenedabvToArrayBufferdoc to flag the zero-copy semantic.X509Certificate(string)had the same shape viaBuffer.from(str).bufferreturning a pool-backed AB with non-zerobyteOffset— fixed in the same pass.0.2 — XSalsa20 keystream restart
crypto_stream_xoralways starts at block 0; calling it per-update()produced N copies of the same keystream XORed against different plaintext.crypto_stream_xsalsa20_xor_icplus per-instanceblock_counterand a 64-byteleftover_keystreamso chunked updates resume cleanly.unique_ptrfor exception safety.0.3 — DH/ECDH peer-key validation
EVP_PKEY_derive_set_peerdoes not callDH_check_pub_keyorEVP_PKEY_public_check, so off-curve / small-subgroup peers were silently accepted.DH_check_pub_keywith TOO_SMALL / TOO_LARGE / INVALID error distinction (matches ncrypto'sDHPointer::checkPublicKey).EC_POINT_oct2point→EC_POINT_is_at_infinity→EC_POINT_is_on_curveagainst the configured group.0.4 — RSA Bleichenbacher oracle
EVP_PKEY_CTX_ctrl_str(ctx, "rsa_pkcs1_implicit_rejection", "1")) for PKCS#1 v1.5 + collapse every decrypt-failure path to a single opaque"RSA decryption failed"message with the OpenSSL error stack cleared.crypto_cipher.ccpolicy.: ${error.message}interpolation inprivateDecrypt/publicDecrypt.publicDecryptopacity.Plan
plans/todo/security-audit.mdis added in this branch and tracks Phase 0–5. Phase 0 rows are now[x]; Phase 1 (sharedvalidateDouble/secureZero/EVP_CIPHER_CTXRAII helpers) is the natural next PR.Test plan
Tests run in the example RN app (per project convention — not standard Node test runners).
bun run exampleand run thecipher,dh,ecdh, andkeys.*test suites; verify all new and existing tests pass on iOS and Android./tmp/rnqc-metro.log) shows no failures from the 20 new regression tests across the 4 phases.xsalsa20callers viacreateCipherivnow produce different output than before for chunked update() patterns — review whether any consumers depend on the (broken) prior behavior.Notes
crypto-specialistreview captured in the commit message and progress log; reviewers should sanity-check the C++ paths against~/dev/ncryptopatterns referenced in the audit doc.plans/todo/security-audit.md(+1257 lines) is the audit tracker itself; actual fix code is ~250 lines of C++/TS.