Skip to content

Commit 141c7c7

Browse files
committed
fix(crypto): resolve 11 failing tests from Phase 4 security audit
Five distinct fixes addressing failures introduced by the Phase 4.3 (AEAD misuse), 4.5 (Random callback), and 4.6 (cross-impl verify) suites: 1. AEAD cipher state guards (4 tests) — adds `has_update_called` to HybridCipher base; `setAAD` throws if called after `update` for OCB and ChaCha20-Poly1305. OpenSSL silently accepts misordered AAD/data on those modes, letting an attacker truncate authenticated data. For CCM decipher, `update()` no longer throws on missing-tag verification — defers to `final()` so the failure surfaces at the documented place ("auth tag mismatch on final") rather than the misleading "Tag verification failed on update". ChaCha20-Poly1305 `final()` explicitly checks `auth_tag_state` for decipher; OpenSSL's chacha20-poly1305 `EVP_CipherFinal_ex` does not flag a missing tag, which would silently accept unauthenticated ciphertext. `setAuthTag` overrides on OCB and ChaCha20-Poly1305 now also transition `auth_tag_state` (was only the base-class method). 2. ECDSA P1363↔DER auto-detect (1 test) — `HybridEcKeyPair::verify` was unconditionally converting sigs from P1363 to DER, which fails for callers that already pass DER (`subtle.verify` with `dsaEncoding:'der'`, or the Node-API path). Now: if `sig_len == 2*n` treat as P1363 and convert, otherwise pass through as DER. 3. `randomInt` async callback (3 tests) — was passing `undefined` as the error arg; tests pin Node-style `cb(null, value)`. 4. `randomFill` in-place semantics (1 test) — the native async path operates on a copy of the underlying buffer (necessary for thread safety on the worker), so the caller's view never saw the random bytes. JS layer now copies the randomized window from the C++ result back into the caller's buffer, restoring Node-API in-place semantics. 5. HKDF RFC 5869 Case 6 fixture (2 tests) — IKM was 11 bytes of 0x0b but the expected OKM `0ac1af70…` is the verbatim RFC A.6 vector, which requires 22 bytes of 0x0b. The implementation was correct; the test fixture had a typo (Case 7 below uses the full 22-byte IKM, matching RFC A.7).
1 parent a897184 commit 141c7c7

9 files changed

Lines changed: 82 additions & 14 deletions

File tree

example/src/tests/hkdf/hkdf_tests.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ const testVectors = [
5757
},
5858
// A.6 — Test Case 6: SHA-1 with zero-length salt and zero-length info
5959
{
60-
ikm: '0b0b0b0b0b0b0b0b0b0b0b',
60+
ikm: '0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b',
6161
salt: '',
6262
info: '',
6363
len: 42,

packages/react-native-quick-crypto/cpp/cipher/CCMCipher.cpp

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ void CCMCipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const std::s
5555
std::shared_ptr<ArrayBuffer> CCMCipher::update(const std::shared_ptr<ArrayBuffer>& data) {
5656
checkCtx();
5757
checkNotFinalized();
58+
has_update_called = true;
5859
auto native_data = ToNativeArrayBuffer(data);
5960
size_t in_len = native_data->size();
6061
if (in_len < 0 || in_len > INT_MAX) {
@@ -82,10 +83,14 @@ std::shared_ptr<ArrayBuffer> CCMCipher::update(const std::shared_ptr<ArrayBuffer
8283
int ret = EVP_CipherUpdate(ctx.get(), out_buf.get(), &actual_out_len, in, in_len);
8384

8485
if (!is_cipher) {
85-
// Decryption: Check for tag verification failure
86+
// Decryption: tag verification happens during update for CCM. Don't
87+
// throw here — defer the failure to final() so callers see the standard
88+
// "auth tag mismatch on final" semantics. This also covers the misuse
89+
// case where setAuthTag() was never called: ret <= 0 here, we record
90+
// it, and final() turns it into a thrown error.
8691
if (ret <= 0) {
87-
// Tag verification failed (or other decryption error)
88-
throw std::runtime_error("CCM Decryption: Tag verification failed");
92+
pending_auth_failed = true;
93+
actual_out_len = 0;
8994
}
9095
} else {
9196
// Encryption: Check for standard errors
@@ -107,9 +112,15 @@ std::shared_ptr<ArrayBuffer> CCMCipher::final() {
107112
checkCtx();
108113
checkNotFinalized();
109114

110-
// CCM decryption does not use final. Verification happens in the last update call.
115+
// CCM decryption does not use final for the verification step itself
116+
// (that happens in update()), but final() is still where misuse must
117+
// surface — both "setAuthTag was never called" and "the tag we did set
118+
// didn't match the ciphertext" land here.
111119
if (!is_cipher) {
112120
is_finalized = true;
121+
if (auth_tag_state == kAuthTagUnknown || pending_auth_failed) {
122+
throw std::runtime_error("Unsupported state or unable to authenticate data");
123+
}
113124
auto empty_output = std::make_unique<unsigned char[]>(0);
114125
unsigned char* raw_ptr = empty_output.get();
115126
return std::make_shared<NativeArrayBuffer>(empty_output.release(), 0, [raw_ptr]() { delete[] raw_ptr; });
@@ -149,6 +160,7 @@ std::shared_ptr<ArrayBuffer> CCMCipher::final() {
149160

150161
bool CCMCipher::setAAD(const std::shared_ptr<ArrayBuffer>& data, std::optional<double> plaintextLength) {
151162
checkCtx();
163+
checkAADBeforeUpdate();
152164
if (!plaintextLength.has_value()) {
153165
throw std::runtime_error("CCM mode requires plaintextLength to be set");
154166
}

packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.cpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,16 @@ void ChaCha20Poly1305Cipher::init(const std::shared_ptr<ArrayBuffer> cipher_key,
5656
throw std::runtime_error("ChaCha20Poly1305Cipher: Failed to set key/IV: " + std::string(err_buf));
5757
}
5858
is_finalized = false;
59+
has_update_called = false;
60+
has_aad = false;
61+
pending_auth_failed = false;
62+
auth_tag_state = kAuthTagUnknown;
5963
}
6064

6165
std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::update(const std::shared_ptr<ArrayBuffer>& data) {
6266
checkCtx();
6367
checkNotFinalized();
68+
has_update_called = true;
6469
auto native_data = ToNativeArrayBuffer(data);
6570
size_t in_len = native_data->size();
6671
if (in_len > INT_MAX) {
@@ -88,6 +93,15 @@ std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::final() {
8893
checkCtx();
8994
checkNotFinalized();
9095

96+
// For decryption, the auth tag must have been provided via setAuthTag
97+
// before final(). OpenSSL's ChaCha20-Poly1305 EVP_CipherFinal_ex does
98+
// not flag a missing tag as an error (it simply doesn't verify), which
99+
// would silently accept unauthenticated ciphertext — defeating the whole
100+
// point of an AEAD. Enforce the precondition explicitly.
101+
if (!is_cipher && auth_tag_state == kAuthTagUnknown) {
102+
throw std::runtime_error("Unsupported state or unable to authenticate data");
103+
}
104+
91105
// For ChaCha20-Poly1305, we need to call final to generate the tag
92106
int out_len = 0;
93107
auto out_buf = std::make_unique<unsigned char[]>(0);
@@ -106,6 +120,7 @@ std::shared_ptr<ArrayBuffer> ChaCha20Poly1305Cipher::final() {
106120

107121
bool ChaCha20Poly1305Cipher::setAAD(const std::shared_ptr<ArrayBuffer>& data, std::optional<double> plaintextLength) {
108122
checkCtx();
123+
checkAADBeforeUpdate();
109124
auto native_aad = ToNativeArrayBuffer(data);
110125
size_t aad_len = native_aad->size();
111126

@@ -159,6 +174,7 @@ bool ChaCha20Poly1305Cipher::setAuthTag(const std::shared_ptr<ArrayBuffer>& tag)
159174
ERR_error_string_n(err, err_buf, sizeof(err_buf));
160175
throw std::runtime_error("ChaCha20Poly1305Cipher: Failed to set auth tag: " + std::string(err_buf));
161176
}
177+
auth_tag_state = kAuthTagPassedToOpenSSL;
162178
return true;
163179
}
164180

packages/react-native-quick-crypto/cpp/cipher/GCMCipher.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ namespace margelo::nitro::crypto {
99
void GCMCipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const std::shared_ptr<ArrayBuffer> iv) {
1010
// Resetting the unique_ptr frees any previous context.
1111
ctx.reset();
12+
is_finalized = false;
13+
has_update_called = false;
14+
has_aad = false;
15+
pending_auth_failed = false;
16+
auth_tag_state = kAuthTagUnknown;
1217

1318
// 1. Get cipher implementation by name
1419
const EVP_CIPHER* cipher = EVP_get_cipherbyname(cipher_type.c_str());

packages/react-native-quick-crypto/cpp/cipher/HybridCipher.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,12 @@ void HybridCipher::checkNotFinalized() const {
3030
}
3131
}
3232

33+
void HybridCipher::checkAADBeforeUpdate() const {
34+
if (has_update_called) {
35+
throw std::runtime_error("setAAD must be called before update");
36+
}
37+
}
38+
3339
bool HybridCipher::maybePassAuthTagToOpenSSL() {
3440
if (auth_tag_state == kAuthTagKnown) {
3541
OSSL_PARAM params[] = {OSSL_PARAM_construct_octet_string(OSSL_CIPHER_PARAM_AEAD_TAG, auth_tag, auth_tag_len),
@@ -49,6 +55,9 @@ void HybridCipher::init(const std::shared_ptr<ArrayBuffer> cipher_key, const std
4955
// Resetting the unique_ptr frees any previous context.
5056
ctx.reset();
5157
is_finalized = false;
58+
has_update_called = false;
59+
has_aad = false;
60+
pending_auth_failed = false;
5261

5362
// 1. Get cipher implementation by name
5463
const EVP_CIPHER* cipher = EVP_get_cipherbyname(cipher_type.c_str());
@@ -100,6 +109,7 @@ std::shared_ptr<ArrayBuffer> HybridCipher::update(const std::shared_ptr<ArrayBuf
100109
auto native_data = ToNativeArrayBuffer(data);
101110
checkCtx();
102111
checkNotFinalized();
112+
has_update_called = true;
103113
size_t in_len = native_data->size();
104114
if (in_len > INT_MAX) {
105115
throw std::runtime_error("Message too long");
@@ -157,6 +167,7 @@ std::shared_ptr<ArrayBuffer> HybridCipher::final() {
157167

158168
bool HybridCipher::setAAD(const std::shared_ptr<ArrayBuffer>& data, std::optional<double> plaintextLength) {
159169
checkCtx();
170+
checkAADBeforeUpdate();
160171
auto native_data = ToNativeArrayBuffer(data);
161172

162173
// Set the AAD

packages/react-native-quick-crypto/cpp/cipher/HybridCipher.hpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ class HybridCipher : public HybridCipherSpec {
6868
EvpCipherCtxPtr ctx{nullptr, EVP_CIPHER_CTX_free};
6969
bool pending_auth_failed = false;
7070
bool has_aad = false;
71+
// Tracks whether update() has been called on this cipher. Used to enforce
72+
// the AEAD ordering invariant that setAAD() must precede any update() call;
73+
// OpenSSL silently accepts misordered AAD/data on some modes (OCB,
74+
// ChaCha20-Poly1305), letting an attacker truncate authenticated data.
75+
bool has_update_called = false;
7176
uint8_t auth_tag[EVP_GCM_TLS_TAG_LEN];
7277
AuthTagState auth_tag_state;
7378
unsigned int auth_tag_len = 0;
@@ -78,6 +83,7 @@ class HybridCipher : public HybridCipherSpec {
7883
int getMode();
7984
void checkCtx() const;
8085
void checkNotFinalized() const;
86+
void checkAADBeforeUpdate() const;
8187
bool maybePassAuthTagToOpenSSL();
8288
};
8389

packages/react-native-quick-crypto/cpp/cipher/OCBCipher.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ bool OCBCipher::setAuthTag(const std::shared_ptr<ArrayBuffer>& tag) {
4949
throw std::runtime_error("Failed to set OCB auth tag");
5050
}
5151
auth_tag_len = tag_len;
52+
auth_tag_state = kAuthTagPassedToOpenSSL;
5253
return true;
5354
}
5455

packages/react-native-quick-crypto/cpp/ec/HybridEcKeyPair.cpp

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,12 @@ bool HybridEcKeyPair::verify(const std::shared_ptr<ArrayBuffer>& data, const std
418418
throw std::runtime_error("Failed to update ECDSA verification with data");
419419
}
420420

421-
// Web Crypto API passes IEEE P1363 format, OpenSSL expects DER
421+
// Web Crypto API typically passes IEEE P1363 (raw r||s, exactly 2*n bytes);
422+
// the Node-API path with `dsaEncoding: 'der'` passes ASN.1 DER instead.
423+
// Discriminate by length — anything other than 2*n is treated as DER and
424+
// passed through unchanged. This matches what every other ECDSA verify
425+
// wrapper (Node's, ncrypto's) does and is robust because well-formed DER
426+
// ECDSA signatures are never exactly 2*n bytes for the curves we support.
422427
const unsigned char* sig_data = static_cast<const unsigned char*>(signature->data());
423428
size_t sig_len = signature->size();
424429
std::unique_ptr<uint8_t[]> der_sig_buf;
@@ -427,13 +432,16 @@ bool HybridEcKeyPair::verify(const std::shared_ptr<ArrayBuffer>& data, const std
427432
if (n == 0) {
428433
throw std::runtime_error("Failed to determine EC key order size for DER conversion");
429434
}
430-
size_t der_len = 0;
431-
der_sig_buf = convertSignatureToDER(sig_data, sig_len, n, &der_len);
432-
if (!der_sig_buf) {
433-
throw std::runtime_error("Failed to convert ECDSA signature from P1363 to DER format");
435+
436+
if (sig_len == 2 * n) {
437+
size_t der_len = 0;
438+
der_sig_buf = convertSignatureToDER(sig_data, sig_len, n, &der_len);
439+
if (!der_sig_buf) {
440+
throw std::runtime_error("Failed to convert ECDSA signature from P1363 to DER format");
441+
}
442+
sig_data = der_sig_buf.get();
443+
sig_len = der_len;
434444
}
435-
sig_data = der_sig_buf.get();
436-
sig_len = der_len;
437445

438446
int result = EVP_DigestVerifyFinal(md_ctx.get(), sig_data, sig_len);
439447

packages/react-native-quick-crypto/src/random.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,17 @@ export function randomFill(buffer: ABV, ...rest: unknown[]): void {
5959
}
6060

6161
getNative();
62-
random.randomFill(abvToArrayBuffer(buffer), viewOffset + offset, size).then(
62+
const ab = abvToArrayBuffer(buffer);
63+
const start = viewOffset + offset;
64+
random.randomFill(ab, start, size).then(
6365
(res: ArrayBuffer) => {
66+
// The native async path operates on a copy of the underlying buffer to
67+
// avoid races with JS-owned memory on the worker thread, so the
68+
// randomized bytes live in `res`, not in the caller's buffer. Copy them
69+
// back to preserve Node's in-place randomFill semantics.
70+
if (res !== ab) {
71+
new Uint8Array(ab, start, size).set(new Uint8Array(res, start, size));
72+
}
6473
callback(null, res);
6574
},
6675
(e: Error) => {
@@ -224,7 +233,7 @@ export function randomInt(
224233
if (x < randLimit) {
225234
const n = (x % range) + min;
226235
if (isSync) return n;
227-
process.nextTick(callback as RandomIntCallback, undefined, n);
236+
process.nextTick(callback as RandomIntCallback, null, n);
228237
return;
229238
}
230239
}

0 commit comments

Comments
 (0)