diff --git a/modules/bitgo/test/ecdh.ts b/modules/bitgo/test/ecdh.ts index bfb69dc1f7..7a5b0accfa 100644 --- a/modules/bitgo/test/ecdh.ts +++ b/modules/bitgo/test/ecdh.ts @@ -5,6 +5,7 @@ import 'should'; import { bip32 } from '@bitgo/utxo-lib'; import * as crypto from 'crypto'; import * as utxolib from '@bitgo/utxo-lib'; +import * as secp256k1 from 'secp256k1'; import { getSharedSecret, signMessageWithDerivedEcdhKey, verifyEcdhSignature } from '@bitgo/sdk-core'; import { TestBitGo } from '@bitgo/sdk-test'; @@ -82,4 +83,26 @@ describe('ECDH utils', () => { isVerify = verifyEcdhSignature(JSON.stringify(message), signedMessage.toString('hex'), derivedPubKey); isVerify.should.be.false(); }); + + it('getSharedSecret rejects invalid public keys for CVE-2024-48930 mitigation', function () { + const validPrivateKey = Buffer.alloc(32, 1); // Create a dummy private key + const validPublicKey = Buffer.from(secp256k1.publicKeyCreate(validPrivateKey)); + + // Valid key should work + const sharedSecret = getSharedSecret(validPrivateKey, validPublicKey); + assert.strictEqual(sharedSecret.length, 32); + + // Test with invalid public key + const invalidPublicKey = Buffer.from('0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798', 'hex'); + // Modify last byte to make it invalid but keep the format valid + invalidPublicKey[32] = invalidPublicKey[32] ^ 0xff; + + try { + getSharedSecret(validPrivateKey, invalidPublicKey); + assert.fail('Should have thrown error for invalid public key'); + } catch (e) { + // The test passes as long as an error is thrown + assert.ok(true, 'Error was thrown as expected'); + } + }); }); diff --git a/modules/sdk-core/src/bitgo/ecdh.ts b/modules/sdk-core/src/bitgo/ecdh.ts index 983662fa91..a71ac5329e 100644 --- a/modules/sdk-core/src/bitgo/ecdh.ts +++ b/modules/sdk-core/src/bitgo/ecdh.ts @@ -16,6 +16,21 @@ import * as assert from 'assert'; import * as secp256k1 from 'secp256k1'; import { ECPairInterface, BIP32Interface, bip32, ECPair } from '@bitgo/utxo-lib'; +/** + * Validate that a public key is on the secp256k1 curve + * This is a defense-in-depth check to prevent invalid curve attacks + * @param publicKey Buffer containing the public key to validate + * @returns boolean true if the key is valid, false otherwise + */ +function validatePublicKey(publicKey: Buffer): boolean { + try { + // Validates that the public key is a point on the secp256k1 curve + return secp256k1.publicKeyVerify(publicKey); + } catch (e) { + return false; + } +} + /** * Calculate the Elliptic Curve Diffie Hellman * @param privateKey HDNode of private key @@ -55,8 +70,15 @@ export function getSharedSecret( assert.strictEqual(privateKey.length, 32); assert.strictEqual(publicKey.length, 33); + // Extra validation to ensure the public key is on the curve + // This provides defense-in-depth against CVE-2024-48930 (GHSA-584q-6j8j-r5pm) + if (!validatePublicKey(publicKey)) { + throw new Error(`invalid public key: not on secp256k1 curve`); + } + // FIXME(BG-34386): we should use `secp256k1.ecdh()` in the future // see discussion here https://github.com/bitcoin-core/secp256k1/issues/352 + // Additional validation has been added above to protect against CVE-2024-48930 const buffer = Buffer.from(secp256k1.publicKeyTweakMul(publicKey, privateKey)) // remove leading parity bit .slice(1); diff --git a/package.json b/package.json index 34776c3097..f8bc8d2c18 100644 --- a/package.json +++ b/package.json @@ -143,5 +143,21 @@ "terser": "^5.14.2", "tmp": "^0.2.3" }, + "overrides": { + "ethereum-cryptography": { + "secp256k1": "5.0.1" + }, + "hdkey": { + "secp256k1": "5.0.1" + }, + "eccrypto": { + "secp256k1": "5.0.1" + }, + "bip322-js": { + "bitcoinjs-message": { + "secp256k1": "5.0.1" + } + } + }, "packageManager": "yarn@1.22.22" } diff --git a/scripts/check-package-dependencies.ts b/scripts/check-package-dependencies.ts index feccf22f58..f55043d330 100644 --- a/scripts/check-package-dependencies.ts +++ b/scripts/check-package-dependencies.ts @@ -34,6 +34,7 @@ const options = { 'sass-loader', 'style-loader', 'ts-loader', + 'secp256k1', ], specials: [ depcheck.special.babel,