Skip to content

Commit

Permalink
code moved to utils packages added more secure comparisons in order t…
Browse files Browse the repository at this point in the history
…o mitigate CWE-208
  • Loading branch information
danielmain committed Apr 17, 2024
1 parent be6c225 commit 9f2f151
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 72 deletions.
8 changes: 4 additions & 4 deletions packages/hardware-ledger/src/LedgerKeyAgent.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -130,15 +130,15 @@ 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;

// Ensure all certificates, if present, only contain SCRIPT_HASH type credentials
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;

Expand Down Expand Up @@ -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');
}

Expand Down
10 changes: 4 additions & 6 deletions 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;
Expand All @@ -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
)
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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
)
Expand Down
6 changes: 3 additions & 3 deletions packages/hardware-ledger/src/transformers/requiredSigners.ts
Expand Up @@ -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<
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions 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 = (
Expand Down Expand Up @@ -38,7 +38,7 @@ export const toWithdrawal: Transform<Cardano.Withdrawal, Ledger.Withdrawal, Ledg

let ledgerWithdrawal;

if (rewardAddress.getPaymentCredential().type === Cardano.CredentialType.KeyHash) {
if (areNumbersEqual(rewardAddress.getPaymentCredential().type, Cardano.CredentialType.KeyHash)) {
const keyPath = resolveKeyPath(rewardAddress, context?.knownAddresses);
ledgerWithdrawal = keyPath
? {
Expand Down
49 changes: 0 additions & 49 deletions packages/hardware-ledger/src/utils.ts

This file was deleted.

3 changes: 2 additions & 1 deletion packages/hardware-trezor/src/TrezorKeyAgent.ts
Expand Up @@ -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';

Expand Down Expand Up @@ -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');
}

Expand Down
8 changes: 4 additions & 4 deletions packages/hardware-trezor/src/transformers/certificates.ts
Expand Up @@ -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 = {
Expand All @@ -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 };
}
Expand Down
6 changes: 3 additions & 3 deletions 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';

Expand All @@ -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
Expand Down
47 changes: 47 additions & 0 deletions packages/util/src/equals.ts
Expand Up @@ -6,3 +6,50 @@ export const strictEquals = <T>(a: T, b: T) => a === b;

export const sameArrayItems = <T>(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`.
};

0 comments on commit 9f2f151

Please sign in to comment.