From 9f2f1517f2b75ed42a220ca0abd4013fadb1fb10 Mon Sep 17 00:00:00 2001 From: Daniel Main Date: Wed, 17 Apr 2024 17:19:44 +0200 Subject: [PATCH] code moved to utils packages added more secure comparisons in order to mitigate CWE-208 --- .../hardware-ledger/src/LedgerKeyAgent.ts | 8 +-- .../src/transformers/certificates.ts | 10 ++-- .../src/transformers/requiredSigners.ts | 6 +-- .../src/transformers/withdrawals.ts | 4 +- packages/hardware-ledger/src/utils.ts | 49 ------------------- .../hardware-trezor/src/TrezorKeyAgent.ts | 3 +- .../src/transformers/certificates.ts | 8 +-- .../src/transformers/requiredSigners.ts | 6 +-- packages/util/src/equals.ts | 47 ++++++++++++++++++ 9 files changed, 69 insertions(+), 72 deletions(-) delete mode 100644 packages/hardware-ledger/src/utils.ts diff --git a/packages/hardware-ledger/src/LedgerKeyAgent.ts b/packages/hardware-ledger/src/LedgerKeyAgent.ts index f0fd26c6095..e585819f2ab 100644 --- a/packages/hardware-ledger/src/LedgerKeyAgent.ts +++ b/packages/hardware-ledger/src/LedgerKeyAgent.ts @@ -15,7 +15,7 @@ import { } from '@cardano-sdk/key-management'; import { HID } from 'node-hid'; import { LedgerDevice, LedgerTransportType } from './types'; -import { areConstantTimeEqualEnums, areConstantTimeEqualStrings } from './utils'; +import { areNumbersEqual, areStringsEqual } from '@cardano-sdk/util'; import { str_to_path } from '@cardano-foundation/ledgerjs-hw-app-cardano/dist/utils/address'; import { toLedgerTx } from './transformers'; import TransportNodeHid from '@ledgerhq/hw-transport-node-hid-noevents'; @@ -130,7 +130,7 @@ const containsOnlyScriptHashCreds = (tx: Transaction): boolean => { // Ensure all withdrawals have the SCRIPT_HASH type const withdrawalsAllScriptHash = tx.withdrawals ? tx.withdrawals.every((withdrawal) => - areConstantTimeEqualEnums(withdrawal.stakeCredential.type, StakeCredentialParamsType.SCRIPT_HASH) + areNumbersEqual(withdrawal.stakeCredential.type, StakeCredentialParamsType.SCRIPT_HASH) ) : true; @@ -138,7 +138,7 @@ const containsOnlyScriptHashCreds = (tx: Transaction): boolean => { const certificatesAllScriptHash = tx.certificates ? tx.certificates.every((cert) => { const certParams = cert.params as unknown as StakeCredentialCertificateParams; - return areConstantTimeEqualEnums(certParams.stakeCredential.type, StakeCredentialParamsType.SCRIPT_HASH); + return areNumbersEqual(certParams.stakeCredential.type, StakeCredentialParamsType.SCRIPT_HASH); }) : true; @@ -553,7 +553,7 @@ export class LedgerKeyAgent extends KeyAgentBase { tx: ledgerTxData }); - if (!areConstantTimeEqualStrings(result.txHashHex, hash)) { + if (!areStringsEqual(result.txHashHex, hash)) { throw new errors.HwMappingError('Ledger computed a different transaction id'); } diff --git a/packages/hardware-ledger/src/transformers/certificates.ts b/packages/hardware-ledger/src/transformers/certificates.ts index 804ccd443f1..18708e2b4fb 100644 --- a/packages/hardware-ledger/src/transformers/certificates.ts +++ b/packages/hardware-ledger/src/transformers/certificates.ts @@ -1,9 +1,7 @@ -import * as Crypto from '@cardano-sdk/crypto'; import * as Ledger from '@cardano-foundation/ledgerjs-hw-app-cardano'; import { Cardano } from '@cardano-sdk/core'; -import { InvalidArgumentError, Transform } from '@cardano-sdk/util'; +import { InvalidArgumentError, Transform, areStringsEqual } from '@cardano-sdk/util'; import { LedgerTxTransformerContext } from '../types'; -import { areConstantTimeEqualStrings } from '../utils'; import { util } from '@cardano-sdk/key-management'; type StakeKeyCertificateType = Ledger.CertificateType.STAKE_REGISTRATION | Ledger.CertificateType.STAKE_DEREGISTRATION; @@ -21,7 +19,7 @@ const getStakeAddressCertificate = ( type: StakeKeyCertificateType ): StakeKeyCertificate => { const knownAddress = context?.knownAddresses.find((address) => - areConstantTimeEqualStrings( + areStringsEqual( Cardano.RewardAccount.toHash(address.rewardAccount) as unknown as string, certificate.stakeCredential.hash as unknown as string ) @@ -72,7 +70,7 @@ export const stakeDelegationCertificate: Transform< > = (certificate, context): Ledger.Certificate => { const poolIdKeyHash = Cardano.PoolId.toKeyHash(certificate.poolId); const knownAddress = context?.knownAddresses.find((address) => - areConstantTimeEqualStrings( + areStringsEqual( Cardano.RewardAccount.toHash(address.rewardAccount) as unknown as string, certificate.stakeCredential.hash as unknown as string ) @@ -233,7 +231,7 @@ const poolRetirementCertificate: Transform< const poolIdKeyHash = Cardano.PoolId.toKeyHash(certificate.poolId); const knownAddress = context?.knownAddresses.find((address) => - areConstantTimeEqualStrings( + areStringsEqual( Cardano.RewardAccount.toHash(address.rewardAccount) as unknown as string, poolIdKeyHash as unknown as string ) diff --git a/packages/hardware-ledger/src/transformers/requiredSigners.ts b/packages/hardware-ledger/src/transformers/requiredSigners.ts index b672b821f75..24cfc5b3013 100644 --- a/packages/hardware-ledger/src/transformers/requiredSigners.ts +++ b/packages/hardware-ledger/src/transformers/requiredSigners.ts @@ -2,7 +2,7 @@ import * as Crypto from '@cardano-sdk/crypto'; import * as Ledger from '@cardano-foundation/ledgerjs-hw-app-cardano'; import { Cardano } from '@cardano-sdk/core'; import { LedgerTxTransformerContext } from '../types'; -import { Transform } from '@cardano-sdk/util'; +import { Transform, areStringsEqual } from '@cardano-sdk/util'; import { util } from '@cardano-sdk/key-management'; export const toRequiredSigner: Transform< @@ -12,12 +12,12 @@ export const toRequiredSigner: Transform< > = (keyHash, context) => { const paymentCredKnownAddress = context?.knownAddresses.find((address) => { const paymentCredential = Cardano.Address.fromBech32(address.address)?.asBase()?.getPaymentCredential().hash; - return paymentCredential && paymentCredential.toString() === keyHash; + return !!paymentCredential && areStringsEqual(paymentCredential.toString(), keyHash); }); const stakeCredKnownAddress = context?.knownAddresses.find((address) => { const stakeCredential = Cardano.RewardAccount.toHash(address.rewardAccount); - return stakeCredential && stakeCredential.toString() === keyHash; + return !!stakeCredential && areStringsEqual(stakeCredential.toString(), keyHash); }); const paymentKeyPath = paymentCredKnownAddress diff --git a/packages/hardware-ledger/src/transformers/withdrawals.ts b/packages/hardware-ledger/src/transformers/withdrawals.ts index b307cfb18c2..6b681d46fcf 100644 --- a/packages/hardware-ledger/src/transformers/withdrawals.ts +++ b/packages/hardware-ledger/src/transformers/withdrawals.ts @@ -1,7 +1,7 @@ import * as Ledger from '@cardano-foundation/ledgerjs-hw-app-cardano'; import { Cardano } from '@cardano-sdk/core'; import { CardanoKeyConst, GroupedAddress, util } from '@cardano-sdk/key-management'; -import { InvalidArgumentError, Transform } from '@cardano-sdk/util'; +import { InvalidArgumentError, Transform, areNumbersEqual } from '@cardano-sdk/util'; import { LedgerTxTransformerContext } from '../types'; const resolveKeyPath = ( @@ -38,7 +38,7 @@ export const toWithdrawal: Transform - (a ^ b) === 0; // This performs a constant-time comparison for enum values - -/** - * Performs a constant-time comparison of two strings to mitigate timing attacks (CWE-208). - * - * This function is designed to prevent timing attacks by ensuring that the time it takes to compare two strings - * does not depend on the contents of the strings themselves. It achieves this by comparing all characters - * up to the length of the longer string, treating out-of-bounds characters as zeros, and using bitwise operations - * to maintain constant time execution. - * - * @param {string} a - The first string to compare. - * @param {string} b - The second string to compare. - * @returns {boolean} - Returns true if the strings are identical, false otherwise. - */ -export const areConstantTimeEqualStrings = (a: string, b: string): boolean => { - if (a.length !== b.length) return false; - - // Calculate the maximum length of the two strings. This value will be used to loop through each character. - const maxLength = Math.max(a.length, b.length); - - // It loops through the maximum length of the two strings, ensuring that the iteration count doesn't leak information about the length difference. - const results: (0 | 1)[] = Array.from({ length: maxLength }, (_, i) => { - const charCodeA = i < a.length ? a.charCodeAt(i) : 0; - const charCodeB = i < b.length ? b.charCodeAt(i) : 0; - return charCodeA === charCodeB ? 1 : 0; // Return 1 if the characters are equal, otherwise return 0. - }); - - // Reduce the results array to a single value, using bitwise AND, explicitly casting the result as binary 0 | 1. - // We do not use booleans here (true/false) instead 0 and 1 because bitwise operations in JavaScript operate on 32-bit integers. - const areAllCharactersEqual = results.reduce( - (accumulator: 0 | 1, currentValue: 0 | 1) => (accumulator & currentValue) as 0 | 1, // Cast to 0 | 1 to satisfy TypeScript's strict type checking. - 1 as 0 | 1 - ); - - return areAllCharactersEqual === 1; // Return `true` if all characters matched, otherwise `false`. -}; diff --git a/packages/hardware-trezor/src/TrezorKeyAgent.ts b/packages/hardware-trezor/src/TrezorKeyAgent.ts index 59ca3f66a44..c38610a9ed9 100644 --- a/packages/hardware-trezor/src/TrezorKeyAgent.ts +++ b/packages/hardware-trezor/src/TrezorKeyAgent.ts @@ -15,6 +15,7 @@ import { errors, util } from '@cardano-sdk/key-management'; +import { areStringsEqual } from '@cardano-sdk/util'; import { txToTrezor } from './transformers/tx'; import _TrezorConnectWeb from '@trezor/connect-web'; @@ -238,7 +239,7 @@ export class TrezorKeyAgent extends KeyAgentBase { const signedData = result.payload; - if (signedData.hash !== hash) { + if (!areStringsEqual(signedData.hash, hash)) { throw new errors.HwMappingError('Trezor computed a different transaction id'); } diff --git a/packages/hardware-trezor/src/transformers/certificates.ts b/packages/hardware-trezor/src/transformers/certificates.ts index 490b88e0205..3d0c8c884fb 100644 --- a/packages/hardware-trezor/src/transformers/certificates.ts +++ b/packages/hardware-trezor/src/transformers/certificates.ts @@ -3,7 +3,7 @@ import * as Trezor from '@trezor/connect'; import { BIP32Path } from '@cardano-sdk/crypto'; import { Cardano } from '@cardano-sdk/core'; import { GroupedAddress, util } from '@cardano-sdk/key-management'; -import { InvalidArgumentError, Transform } from '@cardano-sdk/util'; +import { InvalidArgumentError, Transform, areNumbersEqual, areStringsEqual } from '@cardano-sdk/util'; import { TrezorTxTransformerContext } from '../types'; type CertCredentialsType = { @@ -16,12 +16,12 @@ const getCertCredentials = ( stakeKeyHash: Crypto.Ed25519KeyHashHex, knownAddresses: GroupedAddress[] | undefined ): CertCredentialsType => { - const knownAddress = knownAddresses?.find( - (address) => Cardano.RewardAccount.toHash(address.rewardAccount) === stakeKeyHash + const knownAddress = knownAddresses?.find((address) => + areStringsEqual(Cardano.RewardAccount.toHash(address.rewardAccount), stakeKeyHash) ); const rewardAddress = knownAddress ? Cardano.Address.fromBech32(knownAddress.rewardAccount)?.asReward() : null; - if (rewardAddress?.getPaymentCredential().type === Cardano.CredentialType.KeyHash) { + if (!!rewardAddress && areNumbersEqual(rewardAddress?.getPaymentCredential().type, Cardano.CredentialType.KeyHash)) { const path = util.stakeKeyPathFromGroupedAddress(knownAddress); return path ? { path } : { keyHash: stakeKeyHash }; } diff --git a/packages/hardware-trezor/src/transformers/requiredSigners.ts b/packages/hardware-trezor/src/transformers/requiredSigners.ts index 65f7afbd313..42c5a53cf76 100644 --- a/packages/hardware-trezor/src/transformers/requiredSigners.ts +++ b/packages/hardware-trezor/src/transformers/requiredSigners.ts @@ -1,7 +1,7 @@ import * as Crypto from '@cardano-sdk/crypto'; import * as Trezor from '@trezor/connect'; import { Cardano } from '@cardano-sdk/core'; -import { Transform } from '@cardano-sdk/util'; +import { Transform, areStringsEqual } from '@cardano-sdk/util'; import { TrezorTxTransformerContext } from '../types'; import { util } from '@cardano-sdk/key-management'; @@ -12,12 +12,12 @@ export const toRequiredSigner: Transform< > = (keyHash, context) => { const paymentCredKnownAddress = context?.knownAddresses.find((address) => { const paymentCredential = Cardano.Address.fromBech32(address.address)?.asBase()?.getPaymentCredential().hash; - return paymentCredential && paymentCredential.toString() === keyHash; + return !!paymentCredential && areStringsEqual(paymentCredential.toString(), keyHash); }); const stakeCredKnownAddress = context?.knownAddresses.find((address) => { const stakeCredential = Cardano.RewardAccount.toHash(address.rewardAccount); - return stakeCredential && stakeCredential.toString() === keyHash; + return !!stakeCredential && areStringsEqual(stakeCredential.toString(), keyHash); }); const paymentKeyPath = paymentCredKnownAddress diff --git a/packages/util/src/equals.ts b/packages/util/src/equals.ts index 3cfa9f53a36..b6d4af75cc9 100644 --- a/packages/util/src/equals.ts +++ b/packages/util/src/equals.ts @@ -6,3 +6,50 @@ export const strictEquals = (a: T, b: T) => a === b; export const sameArrayItems = (arrayA: T[], arrayB: T[], itemEquals: (a: T, b: T) => boolean) => arrayA.length === arrayB.length && arrayA.every((a) => arrayB.some((b) => itemEquals(a, b))); + +/** + * Performs a constant-time comparison of two number values to mitigate timing attacks (CWE-208). + * + * This function prevents timing attacks by ensuring that the time it takes to compare two number values + * is consistent, regardless of the values being compared. + * + * @param {number} a - The first number value to compare. + * @param {number} b - The second number value to compare. + * @returns {boolean} - Returns true if both number values are identical, otherwise returns false. + */ +export const areNumbersEqual = (a: number, b: number): boolean => (a ^ b) === 0; // This performs a constant-time comparison for number values + +/** + * Performs a constant-time comparison of two strings to mitigate timing attacks (CWE-208). + * + * This function is designed to prevent timing attacks by ensuring that the time it takes to compare two strings + * does not depend on the contents of the strings themselves. It achieves this by comparing all characters + * up to the length of the longer string, treating out-of-bounds characters as zeros, and using bitwise operations + * to maintain constant time execution. + * + * @param {string} a - The first string to compare. + * @param {string} b - The second string to compare. + * @returns {boolean} - Returns true if the strings are identical, false otherwise. + */ +export const areStringsEqual = (a: string, b: string): boolean => { + if (a.length !== b.length) return false; + + // Calculate the maximum length of the two strings. This value will be used to loop through each character. + const maxLength = Math.max(a.length, b.length); + + // It loops through the maximum length of the two strings, ensuring that the iteration count doesn't leak information about the length difference. + const results: (0 | 1)[] = Array.from({ length: maxLength }, (_, i) => { + const charCodeA = i < a.length ? a.charCodeAt(i) : 0; + const charCodeB = i < b.length ? b.charCodeAt(i) : 0; + return charCodeA === charCodeB ? 1 : 0; // Return 1 if the characters are equal, otherwise return 0. + }); + + // Reduce the results array to a single value, using bitwise AND, explicitly casting the result as binary 0 | 1. + // We do not use booleans here (true/false) instead 0 and 1 because bitwise operations in JavaScript operate on 32-bit integers. + const areAllCharactersEqual = results.reduce( + (accumulator: 0 | 1, currentValue: 0 | 1) => (accumulator & currentValue) as 0 | 1, // Cast to 0 | 1 to satisfy TypeScript's strict type checking. + 1 as 0 | 1 + ); + + return areAllCharactersEqual === 1; // Return `true` if all characters matched, otherwise `false`. +};