Skip to content

Commit 9a303b5

Browse files
committed
fix: use proper return value check for EVP_CIPHER_CTX_ctrl()
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 at pc 0x558a7790bb98 bp 0x7ffdcdd5ab10 sp 0x7ffdcdd5a2c8 READ of size 12 at 0x502000032b98 thread T0 #0 0x558a7790bb97 in memcpy (/work/node/out/Debug/node+0x1b0bb97) (BuildId: adb5b2c9018fdb0733966a1a971077d03aca6d5f) #1 0x7efe5386f90b (/lib/x86_64-linux-gnu/libcrypto.so.3+0x45f90b) (BuildId: aa3dafdd9b54db25d7c9f5335b73ca5fcb293b7f) #2 0x7efe536312c2 in EVP_CipherInit_ex (/lib/x86_64-linux-gnu/libcrypto.so.3+0x2212c2) (BuildId: aa3dafdd9b54db25d7c9f5335b73ca5fcb293b7f) #3 0x558a7d3e7785 in ncrypto::CipherCtxPointer::init(ncrypto::Cipher const&, bool, unsigned char const*, unsigned char const*) /work/node/out/../deps/ncrypto/ncrypto.cc:3328:10 #4 0x558a78512a1b in node::crypto::CipherBase::CommonInit(char const*, ncrypto::Cipher const&, unsigned char const*, int, unsigned char const*, int, unsigned int) /work/node/out/../src/crypto/crypto_cipher.cc:366:13 #5 0x558a785125dd in node::crypto::CipherBase::InitIv(char const*, node::crypto::ByteSource const&, node::crypto::ArrayBufferOrViewContents<unsigned char> const&, unsigned int) /work/node/out/../src/crypto/crypto_cipher.cc:409:3 #6 0x558a7850f5f4 in node::crypto::CipherBase::New(v8::FunctionCallbackInfo<v8::Value> const&) /work/node/out/../src/crypto/crypto_cipher.cc:328:11 #7 0x558a788ee605 in v8::internal::FunctionCallbackArguments::CallOrConstruct(v8::internal::Tagged<v8::internal::FunctionTemplateInfo>, bool) /work/node/out/../deps/v8/src/api/api-arguments-inl.h:93:3 #8 0x558a788ec3ba in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::DirectHandle<v8::internal::HeapObject>, v8::internal::DirectHandle<v8::internal::FunctionTemplateInfo>, v8::internal::DirectHandle<v8::internal::Object>, unsigned long*, int) /work/node/out/../deps/v8/src/builtins/builtins-api.cc:104:16 #9 0x558a788e91fc in v8::internal::Builtin_Impl_HandleApiConstruct(v8::internal::BuiltinArguments, v8::internal::Isolate*) /work/node/out/../deps/v8/src/builtins/builtins-api.cc:135:3 #10 0x558a788e91fc in v8::internal::Builtin_HandleApiConstruct(int, unsigned long*, v8::internal::Isolate*) /work/node/out/../deps/v8/src/builtins/builtins-api.cc:126:1 #11 0x7efe31a951b5 (<unknown module>) ```
1 parent 630ee8b commit 9a303b5

1 file changed

Lines changed: 4 additions & 4 deletions

File tree

src/ncrypto.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3403,19 +3403,19 @@ bool CipherCtxPointer::setKeyLength(size_t length) {
34033403
bool CipherCtxPointer::setIvLength(size_t length) {
34043404
if (!ctx_) return false;
34053405
return EVP_CIPHER_CTX_ctrl(
3406-
ctx_.get(), EVP_CTRL_AEAD_SET_IVLEN, length, nullptr);
3406+
ctx_.get(), EVP_CTRL_AEAD_SET_IVLEN, length, nullptr) > 0;
34073407
}
34083408

34093409
bool CipherCtxPointer::setAeadTag(const Buffer<const char>& tag) {
34103410
if (!ctx_) return false;
34113411
return EVP_CIPHER_CTX_ctrl(
3412-
ctx_.get(), EVP_CTRL_AEAD_SET_TAG, tag.len, const_cast<char*>(tag.data));
3412+
ctx_.get(), EVP_CTRL_AEAD_SET_TAG, tag.len, const_cast<char*>(tag.data)) > 0;
34133413
}
34143414

34153415
bool CipherCtxPointer::setAeadTagLength(size_t length) {
34163416
if (!ctx_) return false;
34173417
return EVP_CIPHER_CTX_ctrl(
3418-
ctx_.get(), EVP_CTRL_AEAD_SET_TAG, length, nullptr);
3418+
ctx_.get(), EVP_CTRL_AEAD_SET_TAG, length, nullptr) > 0;
34193419
}
34203420

34213421
bool CipherCtxPointer::setPadding(bool padding) {
@@ -3485,7 +3485,7 @@ bool CipherCtxPointer::update(const Buffer<const unsigned char>& in,
34853485

34863486
bool CipherCtxPointer::getAeadTag(size_t len, unsigned char* out) {
34873487
if (!ctx_) return false;
3488-
return EVP_CIPHER_CTX_ctrl(ctx_.get(), EVP_CTRL_AEAD_GET_TAG, len, out);
3488+
return EVP_CIPHER_CTX_ctrl(ctx_.get(), EVP_CTRL_AEAD_GET_TAG, len, out) > 0;
34893489
}
34903490

34913491
// ============================================================================

0 commit comments

Comments
 (0)