Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: re-enable HKDF crypto functionality #34767

Merged
merged 2 commits into from Jun 29, 2022
Merged

fix: re-enable HKDF crypto functionality #34767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f9f35d
fix: re-enable HKDF crypto functionality
codebytere Jun 21, 2022
e0def11
chore: update patches
patchup[bot] Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions patches/node/fix_crypto_tests_to_run_with_bssl.patch
View file
Open in desktop
Expand Up @@ -537,6 +537,19 @@ index af2146982c7a3bf7bd7527f44e4b17a3b605026e..f6b91f675cfea367c608892dee078b56

// Non-XOF hash functions should accept valid outputLength options as well.
assert.strictEqual(crypto.createHash('sha224', { outputLength: 28 })
diff --git a/test/parallel/test-crypto-hkdf.js b/test/parallel/test-crypto-hkdf.js
index 16744201a935dcd25af4e0f446701b08fe08dd64..e7ef0b78a19fb755456d038fc676eedb2f71ff07 100644
--- a/test/parallel/test-crypto-hkdf.js
+++ b/test/parallel/test-crypto-hkdf.js
@@ -117,8 +117,6 @@ const algorithms = [
['sha256', 'secret', 'salt', 'info', 10],
['sha512', 'secret', 'salt', '', 15],
];
-if (!common.hasOpenSSL3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expose a property common.openSSLisBoringSSL based on the build time constant OPENSSL_IS_BORINGSSL that is already available in upstream Node.js. It could simplify the patch in a upstream friendly way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepak1556 it's stalled but i actually started that ages ago: nodejs/node#38928

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool, btw agree with the concern in nodejs/node#38928 (review), shouldn't the change be

#ifdef OPENSSL_IS_BORINGSSL
    #define IS_BORINGSSL 1
    NODE_DEFINE_CONSTANT(target, IS_BORINGSSL);
   #undef IS_BORINGSSL
#endif

- algorithms.push(['whirlpool', 'secret', '', 'info', 20]);

algorithms.forEach(([ hash, secret, salt, info, length ]) => {
{
diff --git a/test/parallel/test-crypto-padding.js b/test/parallel/test-crypto-padding.js
index f1f14b472997e76bb4100edb1c6cf4fc24d1074d..5057e3f9bc5bb78aceffa5e79530f8ceed84e6f7 100644
--- a/test/parallel/test-crypto-padding.js
Expand Down
47 changes: 6 additions & 41 deletions patches/node/fix_handle_boringssl_and_openssl_incompatibilities.patch
View file
Open in desktop
Expand Up @@ -188,28 +188,6 @@ index c7894baf00ee9ce4684f4c752f1c7c9b98163741..655895dbff8b88daa53c7b40a5feca42
if (EVP_PKEY_paramgen(param_ctx.get(), &raw_params) <= 0)
return EVPKeyCtxPointer();

diff --git a/src/crypto/crypto_hkdf.cc b/src/crypto/crypto_hkdf.cc
index 0aa96ada47abe4b66fb616c665101278bbe0afb6..1e9a4863c5faea5f6b275483ca16f3a6e8dac25b 100644
--- a/src/crypto/crypto_hkdf.cc
+++ b/src/crypto/crypto_hkdf.cc
@@ -101,6 +101,7 @@ bool HKDFTraits::DeriveBits(
Environment* env,
const HKDFConfig& params,
ByteSource* out) {
+#ifndef OPENSSL_IS_BORINGSSL
EVPKeyCtxPointer ctx =
EVPKeyCtxPointer(EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, nullptr));
if (!ctx ||
@@ -132,6 +133,9 @@ bool HKDFTraits::DeriveBits(

*out = std::move(buf);
return true;
+#else
+ return false;
+#endif
}

void HKDFConfig::MemoryInfo(MemoryTracker* tracker) const {
diff --git a/src/crypto/crypto_random.cc b/src/crypto/crypto_random.cc
index fc88deb460314c2620d842ec30141bcd13109d60..c097ccfcffb1158317ba09e7c4beb725ccbab74f 100644
--- a/src/crypto/crypto_random.cc
Expand Down Expand Up @@ -244,10 +222,10 @@ index ae4550e9fde8120c35409e495d5b763a95546509..188a7efe76df2a1aa2eb2746f4d74836

if (target
diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc
index e1ef170a9f17634d218492a2ce888c3a4365e097..8dffad89c80e0906780d1b26ba9a65ba1e76ce0a 100644
index e1ef170a9f17634d218492a2ce888c3a4365e097..f55e292fbbc75448b15dc9be0327ad2dedef49e0 100644
--- a/src/crypto/crypto_util.cc
+++ b/src/crypto/crypto_util.cc
@@ -508,24 +508,14 @@ Maybe<bool> Decorate(Environment* env, Local<Object> obj,
@@ -508,24 +508,15 @@ Maybe<bool> Decorate(Environment* env, Local<Object> obj,
V(BIO) \
V(PKCS7) \
V(X509V3) \
Expand All @@ -269,10 +247,11 @@ index e1ef170a9f17634d218492a2ce888c3a4365e097..8dffad89c80e0906780d1b26ba9a65ba
- V(ASYNC) \
- V(KDF) \
- V(SM2) \
+ V(HKDF) \
V(USER) \

#define V(name) case ERR_LIB_##name: lib = #name "_"; break;
@@ -684,7 +674,7 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
@@ -684,7 +675,7 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsUint32());
Environment* env = Environment::GetCurrent(args);
uint32_t len = args[0].As<Uint32>()->Value();
Expand All @@ -281,7 +260,7 @@ index e1ef170a9f17634d218492a2ce888c3a4365e097..8dffad89c80e0906780d1b26ba9a65ba
if (data == nullptr) {
// There's no memory available for the allocation.
// Return nothing.
@@ -696,7 +686,7 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
@@ -696,7 +687,7 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
data,
len,
[](void* data, size_t len, void* deleter_data) {
Expand All @@ -290,7 +269,7 @@ index e1ef170a9f17634d218492a2ce888c3a4365e097..8dffad89c80e0906780d1b26ba9a65ba
},
data);
Local<ArrayBuffer> buffer = ArrayBuffer::New(env->isolate(), store);
@@ -704,10 +694,12 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
@@ -704,10 +695,12 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
}

void SecureHeapUsed(const FunctionCallbackInfo<Value>& args) {
Expand All @@ -303,20 +282,6 @@ index e1ef170a9f17634d218492a2ce888c3a4365e097..8dffad89c80e0906780d1b26ba9a65ba
}
} // namespace

diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h
index c431159e6f77f8c86844bcadb86012b056d03372..0ce3a8f219a2952f660ff72a6ce36ee109add649 100644
--- a/src/crypto/crypto_util.h
+++ b/src/crypto/crypto_util.h
@@ -16,7 +16,9 @@
#include <openssl/err.h>
#include <openssl/evp.h>
#include <openssl/ec.h>
+#ifndef OPENSSL_IS_BORINGSSL
#include <openssl/kdf.h>
+#endif
#include <openssl/rsa.h>
#include <openssl/dsa.h>
#include <openssl/ssl.h>
diff --git a/src/node_metadata.h b/src/node_metadata.h
index 4486d5af2c1622c7c8f44401dc3ebb986d8e3c2e..db1769f1b3f1617ed8dbbea57b5e324183b42be2 100644
--- a/src/node_metadata.h
Expand Down
10 changes: 5 additions & 5 deletions patches/node/support_v8_sandboxed_pointers.patch
View file
Open in desktop
Expand Up @@ -118,7 +118,7 @@ index 2abf5994405e8da2a04d1b23b75ccd3658398474..024d612a04d83583b397549589d994e3

DebuggingArrayBufferAllocator::~DebuggingArrayBufferAllocator() {
diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc
index 8dffad89c80e0906780d1b26ba9a65ba1e76ce0a..45bc99ce75248794e95b2dcb0101c28152e2bfd0 100644
index f55e292fbbc75448b15dc9be0327ad2dedef49e0..7719574859637aecc98f8a4b00ba6ebca8280631 100644
--- a/src/crypto/crypto_util.cc
+++ b/src/crypto/crypto_util.cc
@@ -318,10 +318,35 @@ ByteSource& ByteSource::operator=(ByteSource&& other) noexcept {
Expand Down Expand Up @@ -171,7 +171,7 @@ index 8dffad89c80e0906780d1b26ba9a65ba1e76ce0a..45bc99ce75248794e95b2dcb0101c281
return ArrayBuffer::New(env->isolate(), std::move(store));
}

@@ -665,6 +691,16 @@ CryptoJobMode GetCryptoJobMode(v8::Local<v8::Value> args) {
@@ -666,6 +692,16 @@ CryptoJobMode GetCryptoJobMode(v8::Local<v8::Value> args) {
}

namespace {
Expand All @@ -188,7 +188,7 @@ index 8dffad89c80e0906780d1b26ba9a65ba1e76ce0a..45bc99ce75248794e95b2dcb0101c281
// SecureBuffer uses openssl to allocate a Uint8Array using
// OPENSSL_secure_malloc. Because we do not yet actually
// make use of secure heap, this has the same semantics as
@@ -692,6 +728,7 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
@@ -693,6 +729,7 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
Local<ArrayBuffer> buffer = ArrayBuffer::New(env->isolate(), store);
args.GetReturnValue().Set(Uint8Array::New(buffer, 0, len));
}
Expand All @@ -197,10 +197,10 @@ index 8dffad89c80e0906780d1b26ba9a65ba1e76ce0a..45bc99ce75248794e95b2dcb0101c281
void SecureHeapUsed(const FunctionCallbackInfo<Value>& args) {
#ifndef OPENSSL_IS_BORINGSSL
diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h
index 0ce3a8f219a2952f660ff72a6ce36ee109add649..06e9eb72e4ea60db4c63d08b24b80a1e6c4f3eaf 100644
index c431159e6f77f8c86844bcadb86012b056d03372..9f57ac58d826cb0aae422ddca54e2136618c4bfe 100644
--- a/src/crypto/crypto_util.h
+++ b/src/crypto/crypto_util.h
@@ -257,7 +257,7 @@ class ByteSource {
@@ -255,7 +255,7 @@ class ByteSource {
// Creates a v8::BackingStore that takes over responsibility for
// any allocated data. The ByteSource will be reset with size = 0
// after being called.
Expand Down
2 changes: 0 additions & 2 deletions script/node-disabled-tests.json
View file
Open in desktop
Expand Up @@ -15,7 +15,6 @@
"parallel/test-crypto-ecb",
"parallel/test-crypto-engine",
"parallel/test-crypto-fips",
"parallel/test-crypto-hkdf.js",
"parallel/test-crypto-keygen",
"parallel/test-crypto-keygen-deprecation",
"parallel/test-crypto-key-objects",
Expand Down Expand Up @@ -104,7 +103,6 @@
"parallel/test-trace-events-vm",
"parallel/test-trace-events-worker-metadata",
"parallel/test-v8-untrusted-code-mitigations",
"parallel/test-webcrypto-derivebits-hkdf",
"parallel/test-webcrypto-derivebits-node-dh",
"parallel/test-webcrypto-ed25519-ed448",
"parallel/test-webcrypto-encrypt-decrypt",
Expand Down