fix elliptic signing

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