From e673c0362979f9cb2c74fc6876c45ae9be1fe853 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= 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 Reviewed-By: Rafael Gonzaga 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 fab7d132ca6..fdba0c9bd98 100644 --- a/lib/internal/policy/manifest.js +++ b/lib/internal/policy/manifest.js @@ -15,7 +15,6 @@ const { StringPrototypeEndsWith, StringPrototypeStartsWith, Symbol, - uncurryThis, } = primordials; const { ERR_MANIFEST_ASSERT_INTEGRITY, @@ -27,13 +26,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( @@ -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); } } diff --git a/src/crypto/crypto_hash.cc b/src/crypto/crypto_hash.cc index 5627bac590f..12dd5de3bd7 100644 --- a/src/crypto/crypto_hash.cc +++ b/src/crypto/crypto_hash.cc @@ -67,6 +67,9 @@ void Hash::Initialize(Environment* env, Local target) { SetMethodNoSideEffect(context, target, "getHashes", GetHashes); HashJob::Initialize(env, target); + + SetMethodNoSideEffect( + context, target, "internalVerifyIntegrity", InternalVerifyIntegrity); } void Hash::RegisterExternalReferences(ExternalReferenceRegistry* registry) { @@ -76,6 +79,8 @@ void Hash::RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(GetHashes); HashJob::RegisterExternalReferences(registry); + + registry->Register(InternalVerifyIntegrity); } void Hash::New(const FunctionCallbackInfo& args) { @@ -308,5 +313,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() || IsAnyBufferSource(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 diff --git a/src/crypto/crypto_hash.h b/src/crypto/crypto_hash.h index 96a9804420d..2d17c3510ed 100644 --- a/src/crypto/crypto_hash.h +++ b/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 diff --git a/test/fixtures/policy/crypto-hash-tampering/.gitattributes b/test/fixtures/policy/crypto-hash-tampering/.gitattributes new file mode 100644 index 00000000000..cbdcbbc258e --- /dev/null +++ b/test/fixtures/policy/crypto-hash-tampering/.gitattributes @@ -0,0 +1 @@ +*.js text eol=lf diff --git a/test/fixtures/policy/crypto-hash-tampering/main.js b/test/fixtures/policy/crypto-hash-tampering/main.js new file mode 100644 index 00000000000..2ee233fe754 --- /dev/null +++ b/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'); diff --git a/test/fixtures/policy/crypto-hash-tampering/policy.json b/test/fixtures/policy/crypto-hash-tampering/policy.json new file mode 100644 index 00000000000..3d981911533 --- /dev/null +++ b/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 + } + } +} diff --git a/test/fixtures/policy/crypto-hash-tampering/protected.js b/test/fixtures/policy/crypto-hash-tampering/protected.js new file mode 100644 index 00000000000..2b57adba5b1 --- /dev/null +++ b/test/fixtures/policy/crypto-hash-tampering/protected.js @@ -0,0 +1 @@ +console.log(require('fs').readFileSync('/etc/passwd').length); diff --git a/test/parallel/test-policy-crypto-hash-tampering.js b/test/parallel/test-policy-crypto-hash-tampering.js new file mode 100644 index 00000000000..96066defb59 --- /dev/null +++ b/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'));