File CVE-2023-38552-node-integrity-checks-according-... of Package nodejs-electron

Overview Repositories Revisions Requests Users Attributes Meta

File CVE-2023-38552-node-integrity-checks-according-to-policies.patch of Package nodejs-electron

From 1c538938ccadfd35fbc699d8e85102736cd5945c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Sun, 6 Aug 2023 12:56:02 +0000
Subject: [PATCH] policy: use tamper-proof integrity check function

Using the JavaScript Hash class is unsafe because its internals can be
tampered with. In particular, an application can cause
Hash.prototype.digest() to return arbitrary values, thus allowing to
circumvent the integrity verification that policies are supposed to
guarantee.

Add and use a new C++ binding internalVerifyIntegrity() that (hopefully)
cannot be tampered with from JavaScript.

PR-URL: https://github.com/nodejs-private/node-private/pull/462
Backport-PR-URL: https://github.com/nodejs-private/node-private/pull/493
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
CVE-ID: CVE-2023-38552
---
 lib/internal/policy/manifest.js               | 17 ++-----
 src/crypto/crypto_hash.cc                     | 50 +++++++++++++++++++
 src/crypto/crypto_hash.h                      |  2 +
 .../crypto-hash-tampering/.gitattributes      |  1 +
 .../policy/crypto-hash-tampering/main.js      |  8 +++
 .../policy/crypto-hash-tampering/policy.json  | 15 ++++++
 .../policy/crypto-hash-tampering/protected.js |  1 +
 .../test-policy-crypto-hash-tampering.js      | 21 ++++++++
 8 files changed, 102 insertions(+), 13 deletions(-)
 create mode 100644 test/fixtures/policy/crypto-hash-tampering/.gitattributes
 create mode 100644 test/fixtures/policy/crypto-hash-tampering/main.js
 create mode 100644 test/fixtures/policy/crypto-hash-tampering/policy.json
 create mode 100644 test/fixtures/policy/crypto-hash-tampering/protected.js
 create mode 100644 test/parallel/test-policy-crypto-hash-tampering.js

diff --git a/lib/internal/policy/manifest.js b/lib/internal/policy/manifest.js
index 698971ee57f2b..d2afb10ec30e4 100644
--- a/third_party/electron_node/lib/internal/policy/manifest.js
+++ b/third_party/electron_node/lib/internal/policy/manifest.js
@@ -16,7 +16,6 @@ const {
   StringPrototypeEndsWith,
   StringPrototypeStartsWith,
   Symbol,
-  uncurryThis,
 } = primordials;
 const {
   ERR_MANIFEST_ASSERT_INTEGRITY,
@@ -28,13 +27,8 @@ let debug = require('internal/util/debuglog').debuglog('policy', (fn) => {
   debug = fn;
 });
 const SRI = require('internal/policy/sri');
-const crypto = require('crypto');
-const { Buffer } = require('buffer');
 const { URL } = require('internal/url');
-const { createHash, timingSafeEqual } = crypto;
-const HashUpdate = uncurryThis(crypto.Hash.prototype.update);
-const HashDigest = uncurryThis(crypto.Hash.prototype.digest);
-const BufferToString = uncurryThis(Buffer.prototype.toString);
+const { internalVerifyIntegrity } = internalBinding('crypto');
 const kRelativeURLStringPattern = /^\.{0,2}\//;
 const { getOptionValue } = require('internal/options');
 const shouldAbortOnUncaughtException = getOptionValue(
@@ -588,16 +582,13 @@ class Manifest {
         // Avoid clobbered Symbol.iterator
         for (let i = 0; i < integrityEntries.length; i++) {
           const { algorithm, value: expected } = integrityEntries[i];
-          const hash = createHash(algorithm);
           // TODO(tniessen): the content should not be passed as a string in the
           // first place, see https://github.com/nodejs/node/issues/39707
-          HashUpdate(hash, content, 'utf8');
-          const digest = HashDigest(hash, 'buffer');
-          if (digest.length === expected.length &&
-            timingSafeEqual(digest, expected)) {
+          const mismatchedIntegrity = internalVerifyIntegrity(algorithm, content, expected);
+          if (mismatchedIntegrity === undefined) {
             return true;
           }
-          realIntegrities.set(algorithm, BufferToString(digest, 'base64'));
+          realIntegrities.set(algorithm, mismatchedIntegrity);
         }
       }
 
diff --git a/src/crypto/crypto_hash.cc b/src/crypto/crypto_hash.cc
index 200603a85ef33..3cb39d795c732 100644
--- a/third_party/electron_node/src/crypto/crypto_hash.cc
+++ b/third_party/electron_node/src/crypto/crypto_hash.cc
@@ -69,6 +69,9 @@ void Hash::Initialize(Environment* env, Local<Object> target) {
   SetMethodNoSideEffect(context, target, "getHashes", GetHashes);
 
   HashJob::Initialize(env, target);
+
+  SetMethodNoSideEffect(
+      context, target, "internalVerifyIntegrity", InternalVerifyIntegrity);
 }
 
 void Hash::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
@@ -78,6 +81,8 @@ void Hash::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(GetHashes);
 
   HashJob::RegisterExternalReferences(registry);
+
+  registry->Register(InternalVerifyIntegrity);
 }
 
 void Hash::New(const FunctionCallbackInfo<Value>& args) {
@@ -310,5 +315,50 @@ bool HashTraits::DeriveBits(
   return true;
 }
 
+void InternalVerifyIntegrity(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  CHECK_EQ(args.Length(), 3);
+
+  CHECK(args[0]->IsString());
+  Utf8Value algorithm(env->isolate(), args[0]);
+
+  CHECK(args[1]->IsString() || IsAnyByteSource(args[1]));
+  ByteSource content = ByteSource::FromStringOrBuffer(env, args[1]);
+
+  CHECK(args[2]->IsArrayBufferView());
+  ArrayBufferOrViewContents<unsigned char> expected(args[2]);
+
+  const EVP_MD* md_type = EVP_get_digestbyname(*algorithm);
+  unsigned char digest[EVP_MAX_MD_SIZE];
+  unsigned int digest_size;
+  if (md_type == nullptr || EVP_Digest(content.data(),
+                                       content.size(),
+                                       digest,
+                                       &digest_size,
+                                       md_type,
+                                       nullptr) != 1) {
+    return ThrowCryptoError(
+        env, ERR_get_error(), "Digest method not supported");
+  }
+
+  if (digest_size != expected.size() ||
+      CRYPTO_memcmp(digest, expected.data(), digest_size) != 0) {
+    Local<Value> error;
+    MaybeLocal<Value> rc =
+        StringBytes::Encode(env->isolate(),
+                            reinterpret_cast<const char*>(digest),
+                            digest_size,
+                            BASE64,
+                            &error);
+    if (rc.IsEmpty()) {
+      CHECK(!error.IsEmpty());
+      env->isolate()->ThrowException(error);
+      return;
+    }
+    args.GetReturnValue().Set(rc.FromMaybe(Local<Value>()));
+  }
+}
+
 }  // namespace crypto
 }  // namespace node
diff --git a/src/crypto/crypto_hash.h b/src/crypto/crypto_hash.h
index 96a9804420db6..2d17c3510ed21 100644
--- a/third_party/electron_node/src/crypto/crypto_hash.h
+++ b/third_party/electron_node/src/crypto/crypto_hash.h
@@ -82,6 +82,8 @@ struct HashTraits final {
 
 using HashJob = DeriveBitsJob<HashTraits>;
 
+void InternalVerifyIntegrity(const v8::FunctionCallbackInfo<v8::Value>& args);
+
 }  // namespace crypto
 }  // namespace node

Places

File CVE-2023-38552-node-integrity-checks-according-to-policies.patch of Package nodejs-electron

Places