Fix: eth_sign does not validate input (#12679)

* Fix #5039

* Converted function into async

* Added more explicit explanation of why the number of bits for EcSign

* eth_sign and eth_personalSign now report errors correctly back to the user

* Added leeway to unsigned message byte check

* Fix lint

app/scripts/lib/message-manager.js

@@ -79,9 +79,9 @@ export default class MessageManager extends EventEmitter {
    * @returns {promise} after signature has been
    *
    */
-  addUnapprovedMessageAsync(msgParams, req) {
+  async addUnapprovedMessageAsync(msgParams, req) {
-    return new Promise((resolve, reject) => {
+    const msgId = this.addUnapprovedMessage(msgParams, req);
-      const msgId = this.addUnapprovedMessage(msgParams, req);
+    return await new Promise((resolve, reject) => {
       // await finished
       this.once(`${msgId}:finished`, (data) => {
         switch (data.status) {
@@ -93,6 +93,10 @@ export default class MessageManager extends EventEmitter {
                 'MetaMask Message Signature: User denied message signature.',
               ),
             );
+          case 'errored':
+            return reject(
+              new Error(`MetaMask Message Signature: ${data.error}`),
+            );
           default:
             return reject(
               new Error(
@@ -233,6 +237,19 @@ export default class MessageManager extends EventEmitter {
     this._setMsgStatus(msgId, 'rejected');
   }
 
+  /**
+   * Sets a Message status to 'errored' via a call to this._setMsgStatus.
+   *
+   * @param {number} msgId - The id of the Message to error
+   *
+   */
+  errorMessage(msgId, error) {
+    const msg = this.getMsg(msgId);
+    msg.error = error;
+    this._updateMsg(msg);
+    this._setMsgStatus(msgId, 'errored');
+  }
+
   /**
    * Clears all unapproved messages from memory.
    */
@@ -304,7 +321,7 @@ export default class MessageManager extends EventEmitter {
  * @returns {string} A hex string conversion of the buffer data
  *
  */
-function normalizeMsgData(data) {
+export function normalizeMsgData(data) {
   if (data.slice(0, 2) === '0x') {
     // data is already hex
     return data;

app/scripts/lib/personal-message-manager.js

@@ -107,6 +107,9 @@ export default class PersonalMessageManager extends EventEmitter {
               ),
             );
             return;
+          case 'errored':
+            reject(new Error(`MetaMask Message Signature: ${data.error}`));
+            return;
           default:
             reject(
               new Error(
@@ -254,6 +257,19 @@ export default class PersonalMessageManager extends EventEmitter {
     this._setMsgStatus(msgId, 'rejected');
   }
 
+  /**
+   * Sets a Message status to 'errored' via a call to this._setMsgStatus.
+   *
+   * @param {number} msgId - The id of the Message to error
+   *
+   */
+  errorMessage(msgId, error) {
+    const msg = this.getMsg(msgId);
+    msg.error = error;
+    this._updateMsg(msg);
+    this._setMsgStatus(msgId, 'errored');
+  }
+
   /**
    * Clears all unapproved messages from memory.
    */

app/scripts/metamask-controller.js

@@ -17,6 +17,7 @@ import LedgerBridgeKeyring from '@metamask/eth-ledger-bridge-keyring';
 import LatticeKeyring from 'eth-lattice-keyring';
 import EthQuery from 'eth-query';
 import nanoid from 'nanoid';
+import { ethErrors } from 'eth-rpc-errors';
 import { captureException } from '@sentry/browser';
 import {
   AddressBookController,
@@ -61,7 +62,7 @@ import AlertController from './controllers/alert';
 import OnboardingController from './controllers/onboarding';
 import ThreeBoxController from './controllers/threebox';
 import IncomingTransactionsController from './controllers/incoming-transactions';
-import MessageManager from './lib/message-manager';
+import MessageManager, { normalizeMsgData } from './lib/message-manager';
 import DecryptMessageManager from './lib/decrypt-message-manager';
 import EncryptionPublicKeyManager from './lib/encryption-public-key-manager';
 import PersonalMessageManager from './lib/personal-message-manager';
@@ -1843,14 +1844,22 @@ export default class MetamaskController extends EventEmitter {
    * @param {Object} msgParams - The params passed to eth_sign.
    * @param {Function} cb - The callback function called with the signature.
    */
-  newUnsignedMessage(msgParams, req) {
+  async newUnsignedMessage(msgParams, req) {
-    const promise = this.messageManager.addUnapprovedMessageAsync(
+    const data = normalizeMsgData(msgParams.data);
-      msgParams,
+    let promise;
-      req,
+    // 64 hex + "0x" at the beginning
-    );
+    // This is needed because Ethereum's EcSign works only on 32 byte numbers
-    this.sendUpdate();
+    // For 67 length see: https://github.com/MetaMask/metamask-extension/pull/12679/files#r749479607
-    this.opts.showUserConfirmation();
+    if (data.length === 66 || data.length === 67) {
-    return promise;
+      promise = this.messageManager.addUnapprovedMessageAsync(msgParams, req);
+      this.sendUpdate();
+      this.opts.showUserConfirmation();
+    } else {
+      throw ethErrors.rpc.invalidParams(
+        'eth_sign requires 32 byte message hash',
+      );
+    }
+    return await promise;
   }
 
   /**
@@ -1859,24 +1868,23 @@ export default class MetamaskController extends EventEmitter {
    * @param {Object} msgParams - The params passed to eth_call.
    * @returns {Promise<Object>} Full state update.
    */
-  signMessage(msgParams) {
+  async signMessage(msgParams) {
     log.info('MetaMaskController - signMessage');
     const msgId = msgParams.metamaskId;
-
+    try {
-    // sets the status op the message to 'approved'
+      // sets the status op the message to 'approved'
-    // and removes the metamaskId for signing
+      // and removes the metamaskId for signing
-    return this.messageManager
+      const cleanMsgParams = await this.messageManager.approveMessage(
-      .approveMessage(msgParams)
+        msgParams,
-      .then((cleanMsgParams) => {
+      );
-        // signs the message
+      const rawSig = await this.keyringController.signMessage(cleanMsgParams);
-        return this.keyringController.signMessage(cleanMsgParams);
+      this.messageManager.setMsgStatusSigned(msgId, rawSig);
-      })
+      return this.getState();
-      .then((rawSig) => {
+    } catch (error) {
-        // tells the listener that the message has been signed
+      log.info('MetaMaskController - eth_sign failed', error);
-        // and can be returned to the dapp
+      this.messageManager.errorMessage(msgId, error);
-        this.messageManager.setMsgStatusSigned(msgId, rawSig);
+      throw error;
-        return this.getState();
+    }
-      });
   }
 
   /**
@@ -1923,23 +1931,27 @@ export default class MetamaskController extends EventEmitter {
    * @param {Object} msgParams - The params of the message to sign & return to the Dapp.
    * @returns {Promise<Object>} A full state update.
    */
-  signPersonalMessage(msgParams) {
+  async signPersonalMessage(msgParams) {
     log.info('MetaMaskController - signPersonalMessage');
     const msgId = msgParams.metamaskId;
     // sets the status op the message to 'approved'
     // and removes the metamaskId for signing
-    return this.personalMessageManager
+    try {
-      .approveMessage(msgParams)
+      const cleanMsgParams = await this.personalMessageManager.approveMessage(
-      .then((cleanMsgParams) => {
+        msgParams,
-        // signs the message
+      );
-        return this.keyringController.signPersonalMessage(cleanMsgParams);
+      const rawSig = await this.keyringController.signPersonalMessage(
-      })
+        cleanMsgParams,
-      .then((rawSig) => {
+      );
-        // tells the listener that the message has been signed
+      // tells the listener that the message has been signed
-        // and can be returned to the dapp
+      // and can be returned to the dapp
-        this.personalMessageManager.setMsgStatusSigned(msgId, rawSig);
+      this.personalMessageManager.setMsgStatusSigned(msgId, rawSig);
-        return this.getState();
+      return this.getState();
-      });
+    } catch (error) {
+      log.info('MetaMaskController - eth_personalSign failed', error);
+      this.personalMessageManager.errorMessage(msgId, error);
+      throw error;
+    }
   }
 
   /**

app/scripts/metamask-controller.test.js

@@ -835,7 +835,8 @@ describe('MetaMaskController', function () {
     let msgParams, metamaskMsgs, messages, msgId;
 
     const address = '0xc42edfcc21ed14dda456aa0756c153f7985d8813';
-    const data = '0x43727970746f6b697474696573';
+    const data =
+      '0x0000000000000000000000000000000000000043727970746f6b697474696573';
 
     beforeEach(async function () {
       sandbox.stub(metamaskController, 'getBalance');
@@ -885,6 +886,19 @@ describe('MetaMaskController', function () {
       assert.equal(messages[0].status, TRANSACTION_STATUSES.REJECTED);
     });
 
+    it('checks message length', async function () {
+      msgParams = {
+        from: address,
+        data: '0xDEADBEEF',
+      };
+
+      try {
+        await metamaskController.newUnsignedMessage(msgParams);
+      } catch (error) {
+        assert.equal(error.message, 'eth_sign requires 32 byte message hash');
+      }
+    });
+
     it('errors when signing a message', async function () {
       try {
         await metamaskController.signMessage(messages[0].msgParams);