commit 1c538938ccadfd35fbc699d8e85102736cd5945c Author: Tobias Nießen Date: Sun Aug 6 12:56:02 2023 +0000 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 CVE-ID: CVE-2023-38552 Index: node-v16.20.2/lib/internal/policy/manifest.js =================================================================== --- node-v16.20.2.orig/lib/internal/policy/manifest.js +++ node-v16.20.2/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/debug 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( @@ -589,16 +583,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); } } Index: node-v16.20.2/src/crypto/crypto_hash.cc =================================================================== --- node-v16.20.2.orig/src/crypto/crypto_hash.cc +++ node-v16.20.2/src/crypto/crypto_hash.cc @@ -69,6 +69,9 @@ void Hash::Initialize(Environment* env, 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(Ex registry->Register(GetHashes); HashJob::RegisterExternalReferences(registry); + + registry->Register(InternalVerifyIntegrity); } void Hash::New(const FunctionCallbackInfo& args) { @@ -322,5 +327,50 @@ bool HashTraits::DeriveBits( return true; } +void InternalVerifyIntegrity(const v8::FunctionCallbackInfo& 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 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 error; + MaybeLocal rc = + StringBytes::Encode(env->isolate(), + reinterpret_cast(digest), + digest_size, + BASE64, + &error); + if (rc.IsEmpty()) { + CHECK(!error.IsEmpty()); + env->isolate()->ThrowException(error); + return; + } + args.GetReturnValue().Set(rc.FromMaybe(Local())); + } +} + } // namespace crypto } // namespace node Index: node-v16.20.2/src/crypto/crypto_hash.h =================================================================== --- node-v16.20.2.orig/src/crypto/crypto_hash.h +++ node-v16.20.2/src/crypto/crypto_hash.h @@ -82,6 +82,8 @@ struct HashTraits final { using HashJob = DeriveBitsJob; +void InternalVerifyIntegrity(const v8::FunctionCallbackInfo& args); + } // namespace crypto } // namespace node Index: node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/.gitattributes =================================================================== --- /dev/null +++ node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/.gitattributes @@ -0,0 +1 @@ +*.js text eol=lf Index: node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/main.js =================================================================== --- /dev/null +++ node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/main.js @@ -0,0 +1,8 @@ +const h = require('crypto').createHash('sha384'); +const fakeDigest = h.digest(); + +const kHandle = Object.getOwnPropertySymbols(h) + .find((s) => s.description === 'kHandle'); +h[kHandle].constructor.prototype.digest = () => fakeDigest; + +require('./protected.js'); Index: node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/policy.json =================================================================== --- /dev/null +++ node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/policy.json @@ -0,0 +1,15 @@ +{ + "resources": { + "./main.js": { + "integrity": true, + "dependencies": { + "./protected.js": true, + "crypto": true + } + }, + "./protected.js": { + "integrity": "sha384-OLBgp1GsljhM2TJ+sbHjaiH9txEUvgdDTAzHv2P24donTt6/529l+9Ua0vFImLlb", + "dependencies": true + } + } +} Index: node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/protected.js =================================================================== --- /dev/null +++ node-v16.20.2/test/fixtures/policy/crypto-hash-tampering/protected.js @@ -0,0 +1 @@ +console.log(require('fs').readFileSync('/etc/passwd').length); Index: node-v16.20.2/test/parallel/test-policy-crypto-hash-tampering.js =================================================================== --- /dev/null +++ node-v16.20.2/test/parallel/test-policy-crypto-hash-tampering.js @@ -0,0 +1,21 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +common.requireNoPackageJSONAbove(); + +const fixtures = require('../common/fixtures'); + +const assert = require('assert'); +const { spawnSync } = require('child_process'); + +const mainPath = fixtures.path('policy', 'crypto-hash-tampering', 'main.js'); +const policyPath = fixtures.path( + 'policy', + 'crypto-hash-tampering', + 'policy.json'); +const { status, stderr } = + spawnSync(process.execPath, ['--experimental-policy', policyPath, mainPath], { encoding: 'utf8' }); +assert.strictEqual(status, 1); +assert(stderr.includes('sha384-Bnp/T8gFNzT9mHj2G/AeuMH8LcAQ4mljw15nxBNl5yaGM7VgbMzDT7O4+dXZTJJn'));