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](2112edc)

### 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

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",

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(
       {

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',

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)
               })

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.',
 }

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
   }
 }