fix: use proper return value check for EVP_CIPHER_CTX_ctrl()#36
Conversation
jasnell
left a comment
There was a problem hiding this comment.
LGTM but this needs lint fixups before it can land.
|
Right, the problem is the asan output that's longer than the max line length. Do you have a preference how to solve this? |
Um.. the lint issues are pretty straight forward. Just fix them? |
This function can theoretically return -1, which would then be converted
to a truthy value. If this happens, then this can cause issues at the
use sites. E.g. for the test-crypto-cipheriv-decipheriv test in Node
this can cause a buffer overflow when we test with an injected error:
```
==714700==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x502000032b98
READ of size 12 at 0x502000032b98 thread T0
#0 0x558a7790bb97 in memcpy (/work/node/out/Debug/node+0x1b0bb97)
nodejs#1 0x7efe5386f90b (/lib/x86_64-linux-gnu/libcrypto.so.3+0x45f90b)
nodejs#2 0x7efe536312c2 in EVP_CipherInit_ex (/lib/x86_64-linux-gnu/libcrypto.so.3+0x2212c2)
nodejs#3 0x558a7d3e7785 in ncrypto::CipherCtxPointer::init
/work/node/out/../deps/ncrypto/ncrypto.cc:3328:10
nodejs#4 0x558a78512a1b in node::crypto::CipherBase::CommonInit
/work/node/out/../src/crypto/crypto_cipher.cc:366:13
nodejs#5 0x558a785125dd in node::crypto::CipherBase::InitIv
/work/node/out/../src/crypto/crypto_cipher.cc:409:3
nodejs#6 0x558a7850f5f4 in node::crypto::CipherBase::New
/work/node/out/../src/crypto/crypto_cipher.cc:328:11
nodejs#7 0x558a788ee605 in v8::internal::FunctionCallbackArguments::CallOrConstruct
/work/node/out/../deps/v8/src/api/api-arguments-inl.h:93:3
nodejs#8 0x558a788ec3ba in v8::internal::MaybeHandle<v8::internal::Object>
v8::internal::(anonymous namespace)::HandleApiCallHelper<true>
/work/node/out/../deps/v8/src/builtins/builtins-api.cc:104:16
nodejs#9 0x558a788e91fc in v8::internal::Builtin_Impl_HandleApiConstruct
/work/node/out/../deps/v8/src/builtins/builtins-api.cc:135:3
nodejs#10 0x558a788e91fc in v8::internal::Builtin_HandleApiConstruct
/work/node/out/../deps/v8/src/builtins/builtins-api.cc:126:1
nodejs#11 0x7efe31a951b5 (<unknown module>)
```
No, the commit message one isn't really. I just shortened the ASAN output a bit so that succeeds now. |
Bun's `src/jsc/bindings/ncrypto.{cpp,h}` is a port of
[nodejs/ncrypto@134ac40](nodejs/ncrypto@134ac40)
(February 2025), which maps to Node's `deps/ncrypto` from late January
2025. This ports the upstream correctness fixes made since then that
apply to code Bun already has, plus one Bun-local typo found while
auditing the drift.
### Upstream fixes
| Change | Upstream |
| --- | --- |
| `DataPointer::resize()`: handle `OPENSSL_realloc` failure instead of
returning a `DataPointer` with a null pointer and a nonzero length. Also
frees the old block on failure. Upstream's version calls a `free()`
helper after `release()` has already nulled `data_`, which is a no-op,
so the old block leaks; this keeps the intent without that. |
[nodejs/ncrypto#37](nodejs/ncrypto#37) |
| `CipherCtxPointer::{setIvLength, setAeadTag, setAeadTagLength,
getAeadTag}`: compare the `EVP_CIPHER_CTX_ctrl` return against `> 0`.
The raw `int` can be negative, which converts to `true`. BoringSSL
normalizes `-1` to `0`, so this is masked in Bun's build today, but it
is still the wrong check. |
[nodejs/ncrypto#36](nodejs/ncrypto#36) |
| `BignumPointer::{GetWord, getWord}` return `std::optional` so a
`BN_get_word` overflow (reported as the all-ones word) is
distinguishable from the real all-ones value. The optional wraps
`BN_ULONG`, not upstream's `unsigned long`: BoringSSL's `BN_ULONG` is
`uint64_t` on every 64-bit target, so `unsigned long` would still
silently truncate 33-to-64-bit values on LLP64 (Windows x64). |
[nodejs/node#63895](nodejs/node#63895) |
| `BIOPointer::New(method)`: return an empty pointer for a null method.
| [nodejs/node#61788](nodejs/node#61788) |
| `EVPKeyCtxPointer::publicCheck()`: remove an unconditional return that
made the `EVP_PKEY_public_check_quick` branch unreachable. The whole
block is non-BoringSSL, so it is dead in Bun's build. |
[nodejs/node#59471](nodejs/node#59471) |
| `X509Name::Iterator::operator*()`: free the buffer allocated by
`ASN1_STRING_to_UTF8`, and return early on a negative size (the
out-pointer is never written on failure). Dead code today:
`JSX509Certificate.cpp` has its own correct loop. |
[nodejs/node#60609](nodejs/node#60609) |
### `GetWord` callers updated
- `JSDiffieHellmanConstructor.cpp`: the generator-below-2 check becomes
`has_value() && *word < 2`. This is the one spot that needed care:
`std::optional<T> < 2` compiles and evaluates `nullopt < 2` as `true`,
so a naive translation would have started rejecting any generator wider
than a word.
- `JSX509Certificate.cpp` (both legacy-object sites): `exponent` is
reported as `null` when the RSA public exponent does not fit in a word,
matching Node. This branch is unreachable under BoringSSL, which rejects
RSA public exponents wider than 33 bits at SPKI parse time.
### Bun-local typo
`SSLCtxPointer::setCipherSuites` passed `ciphers.length()` where
`SSL_CTX_set_ciphersuites` expects the string. It is in the
non-BoringSSL `#ifndef` branch so it never compiles in Bun's build;
introduced by the original `WTF::StringView` port of the file.
### Testing
No test here can fail against the unmodified source on a Linux machine,
because nothing in this diff changes observable behavior on Linux. I
checked each one: `DataPointer::resize` only diverges under
`OPENSSL_realloc` failure, the `EVP_CIPHER_CTX_ctrl` returns are already
normalized to `0` or `1` by BoringSSL, `publicCheck` and
`setCipherSuites` are inside non-BoringSSL `#ifndef` branches,
`BIOPointer::New(method)` and `X509Name::Iterator` have no callers, and
`BN_ULONG` and `unsigned long` are the same 64-bit type on LP64. The one
real before-and-after is the `BN_ULONG` truncation, and it only exists
on Windows x64 (LLP64).
So the new tests pin invariants rather than prove a failure.
`node-crypto.test.js` gains three `DiffieHellman` cases:
- A 72-bit buffer generator is accepted. This is what a naive `optional
< 2` translation would have broken on every platform.
- A 33-bit (5-byte) buffer generator is accepted. Equivalent on
platforms with a 64-bit `unsigned long`; on Windows x64 the `unsigned
long` version of this change truncates it to `0` and wrongly rejects it
as below 2, so this is the real regression test for the `BN_ULONG`
return type on that platform.
- Generators below 2 are still rejected and exactly 2 is still accepted.
Node v26 agrees on all three.
<details>
<summary>Local verification against the debug build</summary>
-
`test/js/node/crypto/{crypto,node-crypto,crypto.key-objects,crypto-rsa,x509-subclass}.test.*`:
677 pass, 0 fail
-
`test/js/node/test/parallel/test-crypto-{x509,dh,dh-errors,dh-constructor,dh-odd-key,dh-generate-keys,dh-shared,dh-padding,cipheriv-decipheriv,gcm-explicit-short-tag,gcm-implicit-short-tag,aes-wrap,padding-aes256,getcipherinfo,rsa-dsa}.js`,
`test-webcrypto-encrypt-decrypt-aes.js`,
`test-crypto-webcrypto-aes-decrypt-tag-too-small.js`: all pass
</details>
### Deliberately not included
- `Cipher::MAX_AUTH_TAG_LENGTH`
([nodejs/node#57803](nodejs/node#57803)): of the
three macros upstream's `static_assert` uses, BoringSSL only defines
`EVP_GCM_TLS_TAG_LEN`, so the assert degenerates to `16 <= 16`.
- `ECKeyPointer::setPublicKeyRaw`
([nodejs/node#62396](nodejs/node#62396)): the
expensive check that rewrite avoids (the `order * Q == infinity` step of
`EC_KEY_check_key`) does not exist in BoringSSL's `EC_KEY_check_key`, so
there is no perf win for Bun.
- The BoringSSL build guards the standalone repo added
(`NCRYPTO_NO_DSA_KEYGEN`, `NCRYPTO_NO_EVP_DH`, rejecting DSA in
`NewFromID`, ...): those exist for vanilla BoringSSL CI, and Bun's DSA
and DH paths work today against its BoringSSL.
---------
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
This function can theoretically return -1, which would then be converted to a truthy value. If this happens, then this can cause issues at the use sites. E.g. for the test-crypto-cipheriv-decipheriv test in Node this can cause a buffer overflow when we test with an injected error:
Note: this was found by a static-dynamic analyser I'm developing.