From e9e4758e50af73f90b607857c36ca8be7119f579 Mon Sep 17 00:00:00 2001 From: Alec Schaefer Date: Fri, 4 Feb 2022 22:26:02 -0500 Subject: [PATCH] fix elliptic signing (#9271) ### Other changes Removes elliptic as a dependency from ODIS signer and combiner Updates get-contact-matches to rely on the common package's implementation of verifyDEKSignature [adds flake tracker to @celo/keystore and increases jest.timeout](https://github.com/celo-org/celo-monorepo/pull/9271/commits/2112edc37c0daf7171103e872cd7d39199d35d2f) ### Tested Test cases added to phone-number-privacy/common package ensuring that authenticateUser accepts both the new and the old signing schemes (backwards compatibility) Test cases added to @celo/identity ensuring that signWithRawKey signs the full message digest rather than the plain text message. ### Related issues - Fixes #9237 ### Backwards compatibility The verifyDEKSignature function used by authenticateUser() will now accept requests with both the old (incorrect) and the new (correct) authorization signatures. Once clients have upgraded to use this change in the @celo/identity package, the verifyDEKSignature function will be updated again to only accept the new authorization signatures. To monitor frequency of requests with incorrect signatures, we have added a new WarningMessage so that we can easily create a log based metric. We will not remove support for the old signing method until ODIS is no longer receiving requests that use it. ### Documentation None --- .../combiner/package.json | 1 - .../src/match-making/get-contact-matches.ts | 33 +-- .../combiner/test/end-to-end/resources.ts | 8 +- .../combiner/test/index.test.ts | 39 ++-- .../common/src/interfaces/error-utils.ts | 1 + .../common/src/utils/authentication.ts | 38 +++- .../common/test/utils/authentication.test.ts | 210 +++++++++++++++++- .../phone-number-privacy/signer/package.json | 2 - packages/sdk/identity/package.json | 2 +- .../src/odis/phone-number-identifier.test.ts | 10 +- packages/sdk/identity/src/odis/query.test.ts | 69 ++++++ packages/sdk/identity/src/odis/query.ts | 31 ++- packages/sdk/identity/tslint.json | 3 +- packages/sdk/keystores/jest.config.js | 3 + packages/sdk/keystores/package.json | 2 +- .../sdk/keystores/src/keystore-base.test.ts | 2 +- .../src/keystore-wallet-wrapper.test.ts | 2 + 17 files changed, 365 insertions(+), 91 deletions(-) create mode 100644 packages/sdk/identity/src/odis/query.test.ts diff --git a/packages/phone-number-privacy/combiner/package.json b/packages/phone-number-privacy/combiner/package.json index 1e1fa7266d4..f132824414f 100644 --- a/packages/phone-number-privacy/combiner/package.json +++ b/packages/phone-number-privacy/combiner/package.json @@ -35,7 +35,6 @@ "@celo/phone-number-privacy-common": "1.0.36-dev", "@celo/utils": "1.5.2-dev", "blind-threshold-bls": "https://github.com/celo-org/blind-threshold-bls-wasm#e1e2f8a", - "elliptic": "^6.5.4", "firebase-admin": "^9.12.0", "firebase-functions": "^3.15.7", "knex": "^0.21.1", diff --git a/packages/phone-number-privacy/combiner/src/match-making/get-contact-matches.ts b/packages/phone-number-privacy/combiner/src/match-making/get-contact-matches.ts index dde74669892..b3eb7776c84 100644 --- a/packages/phone-number-privacy/combiner/src/match-making/get-contact-matches.ts +++ b/packages/phone-number-privacy/combiner/src/match-making/get-contact-matches.ts @@ -8,11 +8,10 @@ import { hasValidIdentifier, hasValidUserPhoneNumberParam, isVerified, + verifyDEKSignature, WarningMessage, } from '@celo/phone-number-privacy-common' -import { trimLeading0x } from '@celo/utils/lib/address' import Logger from 'bunyan' -import { ec as EC } from 'elliptic' import { Request, Response } from 'firebase-functions' import { respondWithError } from '../common/error-utils' import config, { @@ -30,8 +29,6 @@ import { import { getNumberPairContacts, setNumberPairContacts } from '../database/wrappers/number-pairs' import { getContractKit } from '../web3/contracts' -const ec = new EC('secp256k1') - interface ContactMatch { phoneNumber: string } @@ -109,7 +106,11 @@ export async function handleGetContactMatches(request: Request, response: Respon logger.error('Forno error caught in handleGetContactMatches line 109') // Temporary for debugging } if (dekSigner) { - if (!verifyDEKSignature(userPhoneNumber, signedUserPhoneNumber, dekSigner, logger)) { + if ( + !verifyDEKSignature(userPhoneNumber, signedUserPhoneNumber, dekSigner, logger, { + insecureAllowIncorrectlyGeneratedSignature: true, + }) + ) { respondWithError( response, 403, @@ -235,24 +236,6 @@ async function isInvalidReplay( return false } -export function verifyDEKSignature( - message: string, - messageSignature: string, - registeredEncryptionKey: string, - logger?: Logger -) { - try { - const key = ec.keyFromPublic(trimLeading0x(registeredEncryptionKey), 'hex') - return key.verify(message, JSON.parse(messageSignature)) - } catch (err) { - if (logger) { - logger.error('Failed to verify signature with DEK') - logger.error({ err, dek: registeredEncryptionKey }) - } - return false - } -} - async function userHasNewDek( account: string, userPhoneNumber: string, @@ -262,7 +245,9 @@ async function userHasNewDek( const dekSignerRecord = await getDekSignerRecord(account, logger) const isKeyRotation = !!dekSignerRecord && - verifyDEKSignature(userPhoneNumber, signedUserPhoneNumberRecord, dekSignerRecord, logger) + verifyDEKSignature(userPhoneNumber, signedUserPhoneNumberRecord, dekSignerRecord, logger, { + insecureAllowIncorrectlyGeneratedSignature: true, + }) if (isKeyRotation) { logger.info( { diff --git a/packages/phone-number-privacy/combiner/test/end-to-end/resources.ts b/packages/phone-number-privacy/combiner/test/end-to-end/resources.ts index c4c059489e9..c372c6797e0 100644 --- a/packages/phone-number-privacy/combiner/test/end-to-end/resources.ts +++ b/packages/phone-number-privacy/combiner/test/end-to-end/resources.ts @@ -55,7 +55,13 @@ contractKit.addAccount(PRIVATE_KEY_NO_QUOTA) contractKit.addAccount(PRIVATE_KEY) contractKit.defaultAccount = ACCOUNT_ADDRESS -export const deks = [ +interface DEK { + privateKey: string + publicKey: string + address: string +} + +export const deks: DEK[] = [ { privateKey: 'bf8a2b73baf8402f8fe906ad3f42b560bf14b39f7df7797ece9e293d6f162188', publicKey: '034846bc781cacdafc66f3a77aa9fc3c56a9dadcd683c72be3c446fee8da041070', diff --git a/packages/phone-number-privacy/combiner/test/index.test.ts b/packages/phone-number-privacy/combiner/test/index.test.ts index f8ce13eff31..d8465209e85 100644 --- a/packages/phone-number-privacy/combiner/test/index.test.ts +++ b/packages/phone-number-privacy/combiner/test/index.test.ts @@ -1,6 +1,5 @@ +import { signWithDEK } from '@celo/identity/lib/odis/query' import { getDataEncryptionKey, isVerified } from '@celo/phone-number-privacy-common' -import { hexToBuffer } from '@celo/utils/lib/address' -import { ec as EC } from 'elliptic' import { Request, Response } from 'firebase-functions' import { BLSCryptographyClient } from '../src/bls/bls-cryptography-client' import config, { E2E_TEST_ACCOUNTS, E2E_TEST_PHONE_NUMBERS, VERSION } from '../src/config' @@ -13,21 +12,8 @@ import { } from '../src/database/wrappers/account' import { getNumberPairContacts, setNumberPairContacts } from '../src/database/wrappers/number-pairs' import { getBlindedMessageSig, getContactMatches } from '../src/index' -import { BLINDED_PHONE_NUMBER, deks } from './end-to-end/resources' - -const ec = new EC('secp256k1') - +import { BLINDED_PHONE_NUMBER, dekAuthSigner, deks } from './end-to-end/resources' const BLS_SIGNATURE = '0Uj+qoAu7ASMVvm6hvcUGx2eO/cmNdyEgGn0mSoZH8/dujrC1++SZ1N6IP6v2I8A' -interface DEK { - privateKey: string - publicKey: string - address: string -} - -const signWithDEK = (message: string, dek: DEK) => { - const key = ec.keyFromPrivate(hexToBuffer(dek.privateKey)) - return JSON.stringify(key.sign(message).toDER()) -} jest.mock('@celo/phone-number-privacy-common', () => ({ ...jest.requireActual('@celo/phone-number-privacy-common'), @@ -323,7 +309,7 @@ describe(`POST /getContactMatches endpoint`, () => { }) describe('w/ signedUserPhoneNumber', () => { - const signedUserPhoneNumber = signWithDEK(validInput.userPhoneNumber, deks[0]) + const signedUserPhoneNumber = signWithDEK(validInput.userPhoneNumber, dekAuthSigner(0)) const req = { body: { ...validInput, @@ -367,7 +353,10 @@ describe(`POST /getContactMatches endpoint`, () => { mockIsVerified.mockResolvedValue(false) req.body.account = E2E_TEST_ACCOUNTS[0] req.body.userPhoneNumber = E2E_TEST_PHONE_NUMBERS[0] - req.body.signedUserPhoneNumber = signWithDEK(req.body.userPhoneNumber, deks[0]) + req.body.signedUserPhoneNumber = signWithDEK( + req.body.userPhoneNumber, + dekAuthSigner(0) + ) mockGetAccountSignedUserPhoneNumberRecord.mockResolvedValue( req.body.signedUserPhoneNumber ) @@ -376,7 +365,10 @@ describe(`POST /getContactMatches endpoint`, () => { mockIsVerified.mockResolvedValue(true) req.body.account = validInput.account req.body.userPhoneNumber = validInput.userPhoneNumber - req.body.signedUserPhoneNumber = signWithDEK(req.body.userPhoneNumber, deks[0]) + req.body.signedUserPhoneNumber = signWithDEK( + req.body.userPhoneNumber, + dekAuthSigner(0) + ) mockGetAccountSignedUserPhoneNumberRecord.mockResolvedValue( req.body.signedUserPhoneNumber ) @@ -396,9 +388,12 @@ describe(`POST /getContactMatches endpoint`, () => { describe('When user has rotated their dek', () => { beforeAll(() => { mockGetDataEncryptionKey.mockResolvedValue(deks[1].publicKey) - req.body.signedUserPhoneNumber = signWithDEK(validInput.userPhoneNumber, deks[1]) + req.body.signedUserPhoneNumber = signWithDEK( + validInput.userPhoneNumber, + dekAuthSigner(1) + ) mockGetAccountSignedUserPhoneNumberRecord.mockResolvedValue( - signWithDEK(validInput.userPhoneNumber, deks[0]) + signWithDEK(validInput.userPhoneNumber, dekAuthSigner(0)) ) mockGetDekSignerRecord.mockResolvedValue(deks[0].publicKey) }) @@ -463,7 +458,7 @@ describe(`POST /getContactMatches endpoint`, () => { describe('When user has rotated their dek', () => { beforeAll(() => { mockGetAccountSignedUserPhoneNumberRecord.mockResolvedValue( - signWithDEK(validInput.userPhoneNumber, deks[1]) + signWithDEK(validInput.userPhoneNumber, dekAuthSigner(1)) ) mockGetDekSignerRecord.mockResolvedValue(deks[1].publicKey) }) diff --git a/packages/phone-number-privacy/common/src/interfaces/error-utils.ts b/packages/phone-number-privacy/common/src/interfaces/error-utils.ts index b03d5a4b940..22e75c2af5b 100644 --- a/packages/phone-number-privacy/common/src/interfaces/error-utils.ts +++ b/packages/phone-number-privacy/common/src/interfaces/error-utils.ts @@ -30,4 +30,5 @@ export enum WarningMessage { CANCELLED_REQUEST_TO_SIGNER = 'CELO_ODIS_WARN_09 SIGNER Cancelled request to signer', INVALID_USER_PHONE_NUMBER_SIGNATURE = 'CELO_ODIS_WARN_10 BAD_INPUT User phone number signature is invalid', UNKNOWN_DOMAIN = 'CELO_ODIS_WARN_11 BAD_INPUT Provided domain name and version is not recognized', + INVALID_AUTH_SIGNATURE = 'CELO_ODIS_WARN_12 BAD_INPUT Authorization signature was incorrectly generated. Request will be rejected in a future version.', } diff --git a/packages/phone-number-privacy/common/src/utils/authentication.ts b/packages/phone-number-privacy/common/src/utils/authentication.ts index 069ec65e3ae..5628a372a88 100644 --- a/packages/phone-number-privacy/common/src/utils/authentication.ts +++ b/packages/phone-number-privacy/common/src/utils/authentication.ts @@ -5,9 +5,11 @@ import { AttestationsWrapper } from '@celo/contractkit/lib/wrappers/Attestations import { trimLeading0x } from '@celo/utils/lib/address' import { verifySignature } from '@celo/utils/lib/signatureUtils' import Logger from 'bunyan' +import crypto from 'crypto' import { ec as EC } from 'elliptic' import { Request } from 'express' -import { AuthenticationMethod, ErrorMessage } from '../interfaces' +import { rootLogger } from '..' +import { AuthenticationMethod, ErrorMessage, WarningMessage } from '../interfaces' import { FULL_NODE_TIMEOUT_IN_MS, RETRY_COUNT, RETRY_DELAY_IN_MS } from './constants' const ec = new EC('secp256k1') @@ -46,7 +48,11 @@ export async function authenticateUser( return false } else { logger.info({ dek: registeredEncryptionKey, account: signer }, 'Found DEK for account') - if (verifyDEKSignature(message, messageSignature, registeredEncryptionKey, logger)) { + if ( + verifyDEKSignature(message, messageSignature, registeredEncryptionKey, logger, { + insecureAllowIncorrectlyGeneratedSignature: true, + }) + ) { return true } } @@ -64,17 +70,33 @@ export function verifyDEKSignature( message: string, messageSignature: string, registeredEncryptionKey: string, - logger?: Logger + logger?: Logger, + { insecureAllowIncorrectlyGeneratedSignature } = { + insecureAllowIncorrectlyGeneratedSignature: false, + } ) { + logger = logger ?? rootLogger() try { + const msgDigest = crypto.createHash('sha256').update(JSON.stringify(message)).digest('hex') + const key = ec.keyFromPublic(trimLeading0x(registeredEncryptionKey), 'hex') const parsedSig = JSON.parse(messageSignature) - return key.verify(message, parsedSig) - } catch (err) { - if (logger) { - logger.error('Failed to verify signature with DEK') - logger.error({ err, dek: registeredEncryptionKey }) + if (key.verify(msgDigest, parsedSig)) { + return true } + // TODO: Remove this once clients upgrade to @celo/identity v1.5.3 + // Due to an error in the original implementation of the sign and verify functions + // used here, older clients may generate signatures over the truncated message, + // instead of its hash. These signatures represent a risk to the signer as they do + // not protect against modifications of the message past the first 64 characters of the message. + if (insecureAllowIncorrectlyGeneratedSignature && key.verify(message, parsedSig)) { + logger.warn(WarningMessage.INVALID_AUTH_SIGNATURE) + return true + } + return false + } catch (err) { + logger.error('Failed to verify signature with DEK') + logger.error({ err, dek: registeredEncryptionKey }) return false } } diff --git a/packages/phone-number-privacy/common/test/utils/authentication.test.ts b/packages/phone-number-privacy/common/test/utils/authentication.test.ts index 2821e78b362..431d244c7f5 100644 --- a/packages/phone-number-privacy/common/test/utils/authentication.test.ts +++ b/packages/phone-number-privacy/common/test/utils/authentication.test.ts @@ -1,9 +1,10 @@ -import { Request } from 'express' +import { hexToBuffer } from '@celo/base' +import { ContractKit } from '@celo/contractkit' import Logger from 'bunyan' - +import { Request } from 'express' +import { signWithRawKey } from '../../../../sdk/identity/src/odis/query' import { AuthenticationMethod } from '../../src/interfaces/requests' import * as auth from '../../src/utils/authentication' -import { ContractKit } from '@celo/contractkit' describe('Authentication test suite', () => { const logger = Logger.createLogger({ @@ -23,7 +24,7 @@ describe('Authentication test suite', () => { const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) - expect(result).toBeFalsy() + expect(result).toBe(false) }) it('Should fail authentication with missing signer', async () => { @@ -35,7 +36,7 @@ describe('Authentication test suite', () => { const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) - expect(result).toBeFalsy() + expect(result).toBe(false) }) it('Should succeed authentication with error in getDataEncryptionKey', async () => { @@ -50,7 +51,7 @@ describe('Authentication test suite', () => { const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) - expect(result).toBeTruthy() + expect(result).toBe(true) }) it('Should fail authentication when key is not registered', async () => { @@ -75,7 +76,7 @@ describe('Authentication test suite', () => { const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) - expect(result).toBeFalsy() + expect(result).toBe(false) }) it('Should fail authentication when key is registered but not valid', async () => { @@ -100,7 +101,195 @@ describe('Authentication test suite', () => { const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) - expect(result).toBeFalsy() + expect(result).toBe(false) + }) + + it('Should succeed authentication when key is registered and valid', async () => { + const rawKey = '41e8e8593108eeedcbded883b8af34d2f028710355c57f4c10a056b72486aa04' + const body = { + account: '0xc1912fee45d61c87cc5ea59dae31190fffff232d', + authenticationMethod: AuthenticationMethod.ENCRYPTION_KEY, + } + const sig = signWithRawKey(JSON.stringify(body), rawKey) + const sampleRequest: Request = { + get: (name: string) => (name === 'Authorization' ? sig : ''), + body, + } as Request + const mockContractKit = { + contracts: { + getAccounts: async () => { + return Promise.resolve({ + getDataEncryptionKey: async (_: string) => { + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + const key = ec.keyFromPrivate(hexToBuffer(rawKey)) + return key.getPublic(true, 'hex') + }, + }) + }, + }, + } as ContractKit + + const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) + + expect(result).toBe(true) + }) + + it('Should fail authentication when the message is manipulated', async () => { + const rawKey = '41e8e8593108eeedcbded883b8af34d2f028710355c57f4c10a056b72486aa04' + const body = { + account: '0xc1912fee45d61c87cc5ea59dae31190fffff232d', + authenticationMethod: AuthenticationMethod.ENCRYPTION_KEY, + } + const message = JSON.stringify(body) + + // Modify every fourth character and check that the signature becomes invalid. + for (let i = 0; i < message.length; i += 4) { + const modified = + message.slice(0, i) + + String.fromCharCode(message.charCodeAt(i) + 1) + + message.slice(i + 1) + const sig = signWithRawKey(modified, rawKey) + const sampleRequest: Request = { + get: (name: string) => (name === 'Authorization' ? sig : ''), + body, + } as Request + const mockContractKit = { + contracts: { + getAccounts: async () => { + return Promise.resolve({ + getDataEncryptionKey: async (_: string) => { + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + const key = ec.keyFromPrivate(hexToBuffer(rawKey)) + return key.getPublic(true, 'hex') + }, + }) + }, + }, + } as ContractKit + + const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) + + expect(result).toBe(false) + } + }) + + it('Should fail authentication when the key is incorrect', async () => { + const rawKey = '41e8e8593108eeedcbded883b8af34d2f028710355c57f4c10a056b72486aa04' + const body = { + account: '0xc1912fee45d61c87cc5ea59dae31190fffff232d', + authenticationMethod: AuthenticationMethod.ENCRYPTION_KEY, + } + const sig = signWithRawKey(JSON.stringify(body), rawKey) + const sampleRequest: Request = { + get: (name: string) => (name === 'Authorization' ? sig : ''), + body, + } as Request + + const mockContractKit = { + contracts: { + getAccounts: async () => { + return Promise.resolve({ + getDataEncryptionKey: async (_: string) => { + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + // Send back a manipulated key. + const key = ec.keyFromPrivate(hexToBuffer('a' + rawKey.slice(1))) + return key.getPublic(true, 'hex') + }, + }) + }, + }, + } as ContractKit + + const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) + + expect(result).toBe(false) + }) + + it('Should fail authentication when the sigature is modified', async () => { + const rawKey = '41e8e8593108eeedcbded883b8af34d2f028710355c57f4c10a056b72486aa04' + const body = { + account: '0xc1912fee45d61c87cc5ea59dae31190fffff232d', + authenticationMethod: AuthenticationMethod.ENCRYPTION_KEY, + } + // Manipulate the signature. + const sig = signWithRawKey(JSON.stringify(body), rawKey) + const modified = JSON.stringify([0] + JSON.parse(sig)) + const sampleRequest: Request = { + get: (name: string) => (name === 'Authorization' ? modified : ''), + body, + } as Request + + const mockContractKit = { + contracts: { + getAccounts: async () => { + return Promise.resolve({ + getDataEncryptionKey: async (_: string) => { + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + // Send back a manipulated key. + const key = ec.keyFromPrivate(hexToBuffer(rawKey)) + return key.getPublic(true, 'hex') + }, + }) + }, + }, + } as ContractKit + + const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) + + expect(result).toBe(false) + }) + + // Backwards compatibility check + // TODO: Remove this once clients upgrade to @celo/identity v1.5.3 + it('Should succeed authentication when key is registered and valid and signature is incorrectly generated', async () => { + const rawKey = '41e8e8593108eeedcbded883b8af34d2f028710355c57f4c10a056b72486aa04' + const body = { + account: '0xc1912fee45d61c87cc5ea59dae31190fffff232d', + authenticationMethod: AuthenticationMethod.ENCRYPTION_KEY, + } + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + const key = ec.keyFromPrivate(hexToBuffer(rawKey)) + const sig = JSON.stringify(key.sign(JSON.stringify(body)).toDER()) + + const sampleRequest: Request = { + get: (name: string) => (name === 'Authorization' ? sig : ''), + body, + } as Request + const mockContractKit = { + contracts: { + getAccounts: async () => { + return Promise.resolve({ + getDataEncryptionKey: async (_: string) => { + return key.getPublic(true, 'hex') + }, + }) + }, + }, + } as ContractKit + + const result = await auth.authenticateUser(sampleRequest, mockContractKit, logger) + + expect(result).toBe(true) }) }) @@ -122,8 +311,9 @@ describe('Authentication test suite', () => { const result = await auth.isVerified('', '', mockContractKit, logger) - expect(result).toBeTruthy() + expect(result).toBe(true) }) + it('Should fail when verification is not ok', async () => { const mockContractKit = { contracts: { @@ -141,7 +331,7 @@ describe('Authentication test suite', () => { const result = await auth.isVerified('', '', mockContractKit, logger) - expect(result).toBeFalsy() + expect(result).toBe(false) }) }) }) diff --git a/packages/phone-number-privacy/signer/package.json b/packages/phone-number-privacy/signer/package.json index fbcf8f4fa51..7bf3a4c57ad 100644 --- a/packages/phone-number-privacy/signer/package.json +++ b/packages/phone-number-privacy/signer/package.json @@ -27,12 +27,10 @@ "@celo/wallet-hsm-azure": "1.5.2-dev", "@google-cloud/secret-manager": "3.0.0", "@types/bunyan": "^1.8.6", - "@types/elliptic": "^6.4.12", "@types/promise.allsettled": "^1.0.3", "aws-sdk": "^2.705.0", "blind-threshold-bls": "https://github.com/celo-org/blind-threshold-bls-wasm#e1e2f8a", "dotenv": "^8.2.0", - "elliptic": "^6.5.4", "express": "^4.17.1", "knex": "^0.21.1", "mssql": "^6.3.1", diff --git a/packages/sdk/identity/package.json b/packages/sdk/identity/package.json index e1f341b2b19..325c0f6354c 100644 --- a/packages/sdk/identity/package.json +++ b/packages/sdk/identity/package.json @@ -49,4 +49,4 @@ "engines": { "node": ">=12.9.0" } -} \ No newline at end of file +} diff --git a/packages/sdk/identity/src/odis/phone-number-identifier.test.ts b/packages/sdk/identity/src/odis/phone-number-identifier.test.ts index ac3f3ed0e9f..d7ee74adb2c 100644 --- a/packages/sdk/identity/src/odis/phone-number-identifier.test.ts +++ b/packages/sdk/identity/src/odis/phone-number-identifier.test.ts @@ -1,5 +1,3 @@ -import { hexToBuffer } from '@celo/base' -import { ec as EC } from 'elliptic' import { WasmBlsBlindingClient } from './bls-blinding-client' import { getBlindedPhoneNumber, @@ -15,6 +13,7 @@ import { EncryptionKeySigner, ErrorMessages, ServiceContext, + signWithRawKey, } from './query' jest.mock('./bls-blinding-client', () => { @@ -28,8 +27,6 @@ jest.mock('./bls-blinding-client', () => { } }) -const ec = new EC('secp256k1') - const mockE164Number = '+14155550000' const mockAccount = '0x0000000000000000000000000000000000007E57' const expectedPhoneHash = '0xf3ddadd1f488cdd42b9fa10354fdcae67c303ce182e71b30855733b50dce8301' @@ -84,8 +81,7 @@ describe(getPhoneNumberIdentifier, () => { }) const customSignerMethod = jest.fn((bodyString) => { - const key = ec.keyFromPrivate(hexToBuffer(rawKey)) - return Promise.resolve(JSON.stringify(key.sign(bodyString).toDER())) + return Promise.resolve(signWithRawKey(bodyString, rawKey)) }) const customSigner: CustomSigner = { authenticationMethod: AuthenticationMethod.CUSTOM_SIGNER, @@ -149,7 +145,7 @@ describe(getPhoneNumberIdentifier, () => { describe(getPepperFromThresholdSignature, () => { it('Hashes sigs correctly', () => { const base64Sig = 'vJeFZJ3MY5KlpI9+kIIozKkZSR4cMymLPh2GHZUatWIiiLILyOcTiw2uqK/LBReA' - const signature = new Buffer(base64Sig, 'base64') + const signature = Buffer.from(base64Sig, 'base64') expect(getPepperFromThresholdSignature(signature)).toBe('piWqRHHYWtfg9') }) }) diff --git a/packages/sdk/identity/src/odis/query.test.ts b/packages/sdk/identity/src/odis/query.test.ts new file mode 100644 index 00000000000..77ba10f75a2 --- /dev/null +++ b/packages/sdk/identity/src/odis/query.test.ts @@ -0,0 +1,69 @@ +import crypto from 'crypto' +import { hexToBuffer, trimLeading0x } from '../../../base/lib' +import { signWithRawKey } from './query' + +const rawKey = '41e8e8593108eeedcbded883b8af34d2f028710355c57f4c10a056b72486aa04' + +describe(signWithRawKey, () => { + it('Signs message digest', async () => { + const msg = `${'a'.repeat(64)} + ${'b'.repeat(16)}` + + // NOTE: Elliptic will truncate the raw msg to 64 bytes before signing, + // so make sure to always pass the hex encoded msgDigest instead. + const msgDigest = crypto.createHash('sha256').update(JSON.stringify(msg)).digest('hex') + + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + + // Sign + const key = ec.keyFromPrivate(hexToBuffer(rawKey)) + const expectedSig = JSON.stringify(key.sign(msgDigest).toDER()) + const receivedSig = signWithRawKey(msg, rawKey) + const badSig = JSON.stringify(key.sign(msg).toDER()) + + // Verify + const pub = key.getPublic(true, 'hex') + const pubKey = ec.keyFromPublic(trimLeading0x(pub), 'hex') + const isValid = (input: string, sig: string) => pubKey.verify(input, JSON.parse(sig)) + expect(isValid(msgDigest, expectedSig)).toBeTruthy() + expect(isValid(msg, badSig)).toBeTruthy() + expect(isValid(msg, expectedSig)).toBeFalsy() + expect(isValid(msg, receivedSig)).toBeFalsy() + expect(isValid(msgDigest, receivedSig)).toBeTruthy() + expect(receivedSig).toEqual(expectedSig) + }) + + it('Signs full message', async () => { + const msg1 = `${'a'.repeat(64)} + ${'b'.repeat(16)}` + const msg2 = `${'a'.repeat(64)} + ${'c'.repeat(16)}` + + const sig1 = signWithRawKey(msg1, rawKey) + const sig2 = signWithRawKey(msg2, rawKey) + + // NOTE: Elliptic will truncate the raw msg to 64 bytes before signing, + // so make sure to always pass the hex encoded msgDigest instead. + const msgDigest1 = crypto.createHash('sha256').update(JSON.stringify(msg1)).digest('hex') + const msgDigest2 = crypto.createHash('sha256').update(JSON.stringify(msg2)).digest('hex') + + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + + // Sign + const key = ec.keyFromPrivate(hexToBuffer(rawKey)) + + // Verify + const pub = key.getPublic(true, 'hex') + const pubKey = ec.keyFromPublic(trimLeading0x(pub), 'hex') + const isValid = (input: string, sig: string) => pubKey.verify(input, JSON.parse(sig)) + expect(isValid(msgDigest1, sig1)).toBeTruthy() + expect(isValid(msgDigest2, sig2)).toBeTruthy() + expect(isValid(msgDigest1, sig2)).toBeFalsy() + expect(isValid(msgDigest2, sig1)).toBeFalsy() + }) +}) diff --git a/packages/sdk/identity/src/odis/query.ts b/packages/sdk/identity/src/odis/query.ts index 9ae633f1137..ab85d0c6be8 100644 --- a/packages/sdk/identity/src/odis/query.ts +++ b/packages/sdk/identity/src/odis/query.ts @@ -1,6 +1,6 @@ // Utilities for interacting with the Oblivious Decentralized Identifier Service (ODIS) -import { hexToBuffer, trimLeading0x } from '@celo/base/lib/address' +import { hexToBuffer } from '@celo/base/lib/address' import { selectiveRetryAsyncWithBackOff } from '@celo/base/lib/async' import { ContractKit } from '@celo/contractkit' import { @@ -13,11 +13,10 @@ import { PhoneNumberPrivacyRequest, } from '@celo/phone-number-privacy-common' import fetch from 'cross-fetch' +import crypto from 'crypto' import debugFactory from 'debug' -import { ec as EC } from 'elliptic' const debug = debugFactory('kit:odis:query') -const ec = new EC('secp256k1') export interface NoSigner { authenticationMethod: AuthenticationMethod.NONE @@ -99,16 +98,24 @@ export function getServiceContext(contextName = 'mainnet') { } } -export function signWithDEK(message: string, signer: EncryptionKeySigner) { +export function signWithDEK(msg: string, signer: EncryptionKeySigner) { + return signWithRawKey(msg, signer.rawKey) +} + +export function signWithRawKey(msg: string, rawKey: string) { + // NOTE: Elliptic will truncate the raw msg to 64 bytes before signing, + // so make sure to always pass the hex encoded msgDigest instead. + const msgDigest = crypto.createHash('sha256').update(JSON.stringify(msg)).digest('hex') + + // NOTE: elliptic is disabled elsewhere in this library to prevent + // accidental signing of truncated messages. + // tslint:disable-next-line:import-blacklist + const EC = require('elliptic').ec + const ec = new EC('secp256k1') + // Sign - const key = ec.keyFromPrivate(hexToBuffer(signer.rawKey)) - const sig = JSON.stringify(key.sign(message).toDER()) - // Verify - const dek = key.getPublic(true, 'hex') - const pubkey = ec.keyFromPublic(trimLeading0x(dek), 'hex') - const validSignature: boolean = pubkey.verify(message, JSON.parse(sig)) - debug(`Signature is valid: ${validSignature} signed by ${dek}`) - return sig + const key = ec.keyFromPrivate(hexToBuffer(rawKey)) + return JSON.stringify(key.sign(msgDigest).toDER()) } /** diff --git a/packages/sdk/identity/tslint.json b/packages/sdk/identity/tslint.json index 036f000683b..e3916a756e8 100644 --- a/packages/sdk/identity/tslint.json +++ b/packages/sdk/identity/tslint.json @@ -4,6 +4,7 @@ "no-global-arrow-functions": false, "no-console": false, "member-ordering": false, - "max-classes-per-file": false + "max-classes-per-file": false, + "import-blacklist": [true, { "elliptic": ["ec"]}] } } diff --git a/packages/sdk/keystores/jest.config.js b/packages/sdk/keystores/jest.config.js index 2681a75d5a6..1b955b56292 100644 --- a/packages/sdk/keystores/jest.config.js +++ b/packages/sdk/keystores/jest.config.js @@ -1,5 +1,8 @@ +const { nodeFlakeTracking } = require('@celo/flake-tracker/src/jest/config.js') + module.exports = { preset: 'ts-jest', testEnvironment: 'node', testMatch: ['/src/**/?(*.)+(spec|test).ts?(x)'], + ...nodeFlakeTracking, } diff --git a/packages/sdk/keystores/package.json b/packages/sdk/keystores/package.json index 3187670481f..8afe00b80c4 100644 --- a/packages/sdk/keystores/package.json +++ b/packages/sdk/keystores/package.json @@ -15,7 +15,7 @@ "build": "tsc -b .", "clean": "tsc -b . --clean", "docs": "typedoc && ts-node ../utils/scripts/linkdocs.ts keystores", - "test": "jest --runInBand", + "test": "SKIP_KNOWN_FLAKES=false jest --runInBand", "lint": "tslint -c tslint.json --project .", "prepublishOnly": "yarn build" }, diff --git a/packages/sdk/keystores/src/keystore-base.test.ts b/packages/sdk/keystores/src/keystore-base.test.ts index 938e3da331c..926a8027b85 100644 --- a/packages/sdk/keystores/src/keystore-base.test.ts +++ b/packages/sdk/keystores/src/keystore-base.test.ts @@ -13,7 +13,7 @@ import { PK1, } from './test-constants' -jest.setTimeout(20000) +jest.setTimeout(30000) describe('KeystoreBase functionality via InMemoryKeystore (mock)', () => { let keystore: InMemoryKeystore diff --git a/packages/sdk/keystores/src/keystore-wallet-wrapper.test.ts b/packages/sdk/keystores/src/keystore-wallet-wrapper.test.ts index d4f22573508..1ad717e24f1 100644 --- a/packages/sdk/keystores/src/keystore-wallet-wrapper.test.ts +++ b/packages/sdk/keystores/src/keystore-wallet-wrapper.test.ts @@ -8,6 +8,8 @@ const GETH_GEN_KEYSTORE1 = `{"address":"8233d802bdc645d0d1b9b2e6face6e5825905081 const KEYSTORE_NAME1 = 'PK1 keystore name' const ADDRESS1 = normalizeAddressWith0x(privateKeyToAddress(PK1)) +jest.setTimeout(30000) + describe('KeystoreWalletWrapper using InMemoryKeystore', () => { let keystoreWalletWrapper: KeystoreWalletWrapper