feat(security): Phase 2 audit memory-safety sweep#984
Merged
Conversation
… EVP_PKEY*, EVP_PKEY_CTX, EVP_MD_CTX; shared_from_this in PQ async
Closes Phase 2 of plans/todo/security-audit.md across 24 C++ files
(+327/-480 net). All five Phase 2 items shipped together:
2.1 Raw `new uint8_t[]` → `std::unique_ptr<uint8_t[]>` + `release()` into
`NativeArrayBuffer` in Hash, HMAC, KMAC, BLAKE3, PBKDF2, Scrypt, HKDF,
the cipher base `update()`, ChaCha20/ChaCha20-Poly1305/XChaCha20-Poly1305/
XSalsa20-Poly1305, CCM `final()`, RSA-cipher decrypt sentinels, Ed25519
(6 sites), ML-DSA (3 sites), and ML-KEM (4 sites).
2.2 Raw `EVP_PKEY*` → `std::unique_ptr<EVP_PKEY, decltype(&EVP_PKEY_free)>`
in RSA, EC, and Ed25519 keypairs (DSA pattern as template).
`Ed25519::importPublicKey`/`importPrivateKey` now return owning
`EVP_PKEY_ptr` and use `EVP_PKEY_up_ref` for the borrow-the-instance-key
path, closing the audit-flagged leak at HybridEdKeyPair.cpp:155,221.
2.3 `Promise<…>::async([this, …])` → `auto self = this->shared_cast<…>();
[self, …]` in ML-DSA (3 sites) and ML-KEM (3 sites). Closes the
use-after-free risk if the JS object is GC'd while async is in flight.
2.4 Eliminate the unnecessary `EVP_PKEY_CTX_new_from_name` pre-creation in
Sign/Verify handles, ML-DSA, and Ed25519 — pass `nullptr` for the
`EVP_PKEY_CTX**` arg and let `EVP_DigestSignInit`/`EVP_DigestVerifyInit`
allocate from the key's keymgmt (matches `ncrypto::EVPMDCtxPointer::
signInit`). Crypto-specialist review confirmed the old code was actually
*leaking* the pre-allocated PKEY_CTX (OpenSSL silently overwrote the
pointer on success), so this fix closes both the audited double-free
*and* an unreported leak. Wraps EVP_MD_CTX/EVP_PKEY_CTX in local
`unique_ptr` aliases so all manual error-path frees collapse.
2.5 Replace Ed25519's two `ERR_error_string(ERR_get_error(), NULL)` calls
(thread-unsafe static buffer) with the shared `getOpenSSLError()` helper.
Defense-in-depth: `secureZero` added on Scrypt/HKDF error paths and on
Ed25519/ML-DSA/ML-KEM `getPrivateKey` BIO buffers.
Crypto-specialist approved all four substantive concerns: algorithm
selection unchanged (verified against OpenSSL 3.x m_sigver.c::do_sigver_init),
refcount semantics correct, BIO secure-zero is safe redundancy with
`BUF_MEM_free`'s `OPENSSL_clear_free`, `release()` + `make_shared` window
matches Nitro's own `ArrayBuffer::wrap`.
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.
…ion, gate push on local test run Adds three Phase Execution Rules to the Implementation Plan: 1. Commit after each sub-section (single numbered task or wave), not at the end of the phase. Preserves bisectability and review-ability. 2. Do not push or open a PR until the user has run the example app's tests locally and reported pass/fail. Pre-commit hooks cover lint / format / tsc / bob build only — they cannot exercise the native bridge. Tests live in example/src/tests/ and require the RN example app environment. 3. Fix path-filtered CI workflows on the same branch when they fail to trigger. Workflow files are in their own paths filters, so the fix re-triggers the 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. |
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. |
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
Closes Phase 2 of
plans/todo/security-audit.mdacross 24 C++ files (+327/-480 net — RAII actually shrinks the codebase by collapsing manual error-path frees). All five Phase 2 items shipped together.new uint8_t[]→std::unique_ptr<uint8_t[]>update()/final(), RSA-cipher decrypt sentinels, Ed25519 (6 sites), ML-DSA (3 sites), ML-KEM (4 sites)EVP_PKEY*→ smart pointers (DSA template)Promise<…>::async([this, …])→shared_cast+[self, …]EVP_PKEY_CTX_new_from_namepre-creationERR_error_string(.., NULL)→getOpenSSLError()Notable findings
importPublicKey/importPrivateKeypreviously returned rawEVP_PKEY*that callers never freed at HybridEdKeyPair.cpp:155,221 (audit HIGH). Now return owningEVP_PKEY_ptr; the borrow-the-instance-key path usesEVP_PKEY_up_refso refcounts stay balanced.EVP_PKEY_CTX_new_from_namewasn't just at risk of double-free — OpenSSL was silently leaking it on the success path, sinceEVP_DigestSignInitallocates a fresh PKEY_CTX from the key's keymgmt and overwrites the input pointer. The new code (passingnullptr) closes both the audited double-free and this previously-unreported leak. Matchesncrypto::EVPMDCtxPointer::signInit.HybridObjectalready inherits fromstd::enable_shared_from_this<HybridObject>, sothis->shared_cast<…>()works directly insidePromise::asynclambdas. Capturingselfby value closes the use-after-free risk if the JS object is GC'd while async is in flight.Defense-in-depth
secureZeroadded on Scrypt/HKDF error paths and on Ed25519/ML-DSA/ML-KEMgetPrivateKeyBIO buffers.Crypto-specialist review
Approved all four substantive concerns:
EVP_PKEY_CTX_new_from_name(verified against OpenSSL 3.xcrypto/evp/m_sigver.c::do_sigver_init).EVP_PKEY_up_refrefcount semantics correct and thread-safe.secureZerois safe redundancy withBUF_MEM_free'sOPENSSL_clear_free.release()+make_sharedwindow matches Nitro's ownArrayBuffer::wrapconvention.Phase 1 doc cleanup
Phase 1 (PR #983) shipped without updating its checklist in
security-audit.md. This PR backfills the[x]marks and adds a Progress Log entry for it.Test plan
lint-staged,clang-format --dry-run --Werror,tsc --noEmit(both packages),bob build.EVP_DigestSignInitinvocation shape.