search

MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
12.02k stars 4.91k forks source link

'eth_decrypt' Firefox Internal Error too much recursion #9042

Closed xmaysonnave closed 2 years ago

xmaysonnave commented 4 years ago

Metamask v8.0.5 in debug mode Firefox 78.0.2 Debian Buster

No issue with Chrome Version 84.0.4147.89 (Official Build) (64-bit)

'eth_decrypt' is not performed successfully with Firefox as an Internal Error exception is raised (too much recursion)

line 1231-> app/scripts/metamask-controller.js -> method call -> async decryptMessage (msgParams)

line 1244 -> method call -> const rawMess = await this.keyringController.decryptMessage(cleanMsgParams)

line 100 -> nodes_modules/eth-simple-keyring/index.js -> const sig = sigUtil.decrypt(encryptedData, privKey)

Exception generated

line 373 -> nodes_modules/eth-sig-util/index.js -> var ciphertext = nacl.util.decodeBase64(encryptedData.ciphertext);

Thanks

Gudahtt commented 4 years ago

Thanks for the report! What was the context for this error - does it happen anytime you try to use eth_decrypt? Or is it intermittent, or only in certain circumstances?

xmaysonnave commented 4 years ago

@Gudahtt Yes this problem occurs all the time on Firefox with a specific 3MB encrypted content. Could you provide a public encryption key ? I can prepare build an IPFS content. It will help :

Gudahtt commented 4 years ago

Thanks for the offer @xmaysonnave , but I was able to reproduce this myself with a 3.5 MB file made up of lorem ipsum text.

I reproduced this with v8.0.5 of the extension (the prod build), with Firefox 68.10.0esr

xmaysonnave commented 4 years ago

@Gudahtt Glad you reproduced this issue.

1 - I made a comment about performance issue I faced while decrypting: https://github.com/MetaMask/metamask-extension/issues/8991#issuecomment-659309265

The eth-sig-util is quite fast when used from nodejs (encrypt and decrypt). With the same sample who crashes with Firefox it usually takes less than 400ms roughly.

With Metamask v8.0.5 under Chrome, decryption could take more than 25s, up to 50s in some situations, however when Firefox is able to decrypt the same process is definitely faster as I was able to decrypt with Metamask in less than 5s. I'm a little bit disturbed to see such a big difference between Chrome and Firefox.

I will do more tests when your performance patch will be released. https://github.com/MetaMask/metamask-extension/commit/c7fad8f4004023ad6a45dd61e42b3cfdbf7d17db

2 - In the current situation my DApp needs to encrypt. The public encryption key is requested from Metamask however the eth-sig-util is not exposed by the in-page provider. Let me know if you have any plan to expose eth-sig-util from your in-page provider.

Thanks

xmaysonnave commented 4 years ago

I rebuild a version of eth-sig-util and upgraded tweetnacl and tweetnacl-util bump tweetnacl from 1.0.0 to 1.0.3 bump tweetnacl from 0.15.0 to 0.15.1 I rebuild a metamask-extension v8.0.9 and the problem is gone

The problem came from the tweetnacl-util/nacl-util.js -> validateBase64

function validateBase64(s) {
    if (!(/^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/.test(s))) {
      throw new TypeError('invalid encoding');
    }
 }

This method has been upgraded in the 0.15.1 version

function validateBase64(s) {
    if (!(/^(?:[A-Za-z0-9+\/]{2}[A-Za-z0-9+\/]{2})*(?:[A-Za-z0-9+\/]{2}==|[A-Za-z0-9+\/]{3}=)?$/.test(s))) {
      throw new TypeError('invalid encoding');
    }
 }

I built a v2.5.4 version of eth-sig-util in my repo : https://github.com/xmaysonnave/eth-sig-util/tree/v2.5.4

Gudahtt commented 4 years ago

Thanks @xmaysonnave , great find! I'm guessing it was fixed by this change: https://github.com/dchest/tweetnacl-util-js/pull/13

xmaysonnave commented 4 years ago

@Gudahtt Yes you're right this is the commit who fixed this issue.

xmaysonnave commented 3 years ago

@Gudahtt Any chance to see an upgrade with Metamask ? With the latest even Chrome is unable to decrypt my sample. Plex and Multiplex issues, 100% CPU, 95°, looping as crazy... Thanks

Gudahtt commented 3 years ago

Thanks for the reminder @xmaysonnave! I'll revisit this tomorrow, and try to get it in one of the next couple of releases.

Gudahtt commented 3 years ago

@xmaysonnave I have updated the tweetnacl dependencies here: #10028

Unfortunately I can no longer reproduce this error. My attempts to decrypt a large message fail for an entirely different reason now:

This message cannot be decrypted due to error: JSON.parse: unexpected non-whitespace character after JSON data at line 1 column 2 of the JSON data

The failure is on this line: https://github.com/MetaMask/metamask-extension/blob/b7d9ed566ac6bd7b4a19546e69555c2aa9840ccb/app/scripts/metamask-controller.js#L1525

Are you seeing the same thing? Can you still repro this with the original error?

xmaysonnave commented 3 years ago

I'm unable to reproduce your issue. However I built a while back a v3.0.1 of eth-sig-util with the fix as I need this library to encrypt. So far so good.. https://github.com/xmaysonnave/eth-sig-util

I tried to use it but I'm facing a lot of issues. If I try to yarn metamask-extension it crashed as orbit-db (probably from 3box) requires node v12 not node v10. The only options is to patch the yarn.lock then it's buildable but the fixed library is never called.

I'm investigating right now but as far as I can see multiple dependency of eth-sig-util with various versions are installed. I rebuilt succesfully node-scrypt but I struggle to build scrypt.js. https://github.com/xmaysonnave/node-scrypt (v6.0.4) The idea here is to have an eth-simple-keyring with the proper eth-sig-util dependency.

I'll let you know.

Gudahtt commented 3 years ago

If I try to yarn metamask-extension it crashed as orbit-db (probably from 3box) requires node v12 not node v10.

Huh - I hadn't heard of that happening before. Maybe you accidentally updated the version being used?

I'm investigating right now but as far as I can see multiple dependency of eth-sig-util with various versions are installed.

On the develop branch at the moment, there are still multiple versions of eth-sig-util being used but they all use the updated tweetnacl dependencies. I thought that would be sufficient to fix this issue. Maybe the problem goes beyond just the tweetnacl version used.

xmaysonnave commented 3 years ago

About orbit-db-store. I checkout the 8.1.8 Deleted the yarn.lock then yarn to installed fresh packages. It crashes : error orbit-db-store@3.5.0: The engine "node" is incompatible with this module. Expected version ">=12.0.0". Got "10.18.1"

Gudahtt commented 3 years ago

Deleted the yarn.lock then yarn to installed fresh packages.

Ah, so this step will get you different versions of dependencies than we use. You will have to leave the lockfile as-is to replicate the same environment we use.

Sometimes we make adjustments to the lockfile to update packages, but re-generating the entire lockfile isn't done often because it has wide-ranging effects.

xmaysonnave commented 3 years ago

You're right. I definitely noticed that. I try to find a way to call the proper library (tweetnacl-util). 1 - However during my first run I used your technic and was unable to reproduce you result. 2 - I'm not sure if I reported that the 8.1.8 with Chrome is unresponsive while unencrypting (it was very slow but usable before). Thanks

“Matter is Energy. Energy is Light. We are all Light Beings.” - Albert EinsteinPudhuveedu / Xavier

Le ven. 11 déc. 2020 à 11:33, Mark Stacey notifications@github.com a écrit :

Deleted the yarn.lock then yarn to installed fresh packages.

Ah, so this step will get you different versions of dependencies than we use. You will have to leave the lockfile as-is to replicate the same environment we use.

Sometimes we make adjustments to the lockfile to update packages, but re-generating the entire lockfile isn't done often because it has wide-ranging effects.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MetaMask/metamask-extension/issues/9042#issuecomment-742991057, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE47PBE7NAIS7RFL4RHSXOLSUGYZLANCNFSM4PDGXDZQ .

xmaysonnave commented 3 years ago

I'm still facing issues but made some steps forward. Tagged version v8.1.8, Firefox 83, Debian Buster Patched yarn.lock with the following;

tweetnacl-util@^0.15.0, tweetnacl-util@^0.15.1:
  version "0.15.1"
  resolved "https://registry.yarnpkg.com/tweetnacl-util/-/tweetnacl-util-0.15.1.tgz#b80fcdb5c97bcc508be18c44a4be50f022eea00b"
  integrity sha512-RKJBIj8lySrShN4w6i/BonWp2Z/uxwC3h4y7xsRrpP59ZboCd0GpEVsOnMDYLMmKBpYhb5TgHzZXy7wTfYFBRw==

tweetnacl@^1.0.0, tweetnacl@^1.0.1, tweetnacl@^1.0.3:
  version "1.0.3"
  resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-1.0.3.tgz#ac0af71680458d8a6378d0d0d050ab1407d35596"
  integrity sha512-6rt+RN7aOi1nGMyC4Xa5DdYiukl2UWCbcJft7YhxReBGQD7OAM8Pbxw6YMo4r2diNEA8FEmu32YOn9rhaiE5yw==

I also added the eth-sig-util I built with the required dependencies : https://github.com/xmaysonnave/eth-sig-util tag: v3.0.1

eth-sig-util@^3.0.0, eth-sig-util@xmaysonnave/eth-sig-util#v3.0.1:
  version "3.0.1"
  resolved "https://codeload.github.com/xmaysonnave/eth-sig-util/tar.gz/5dfbf631d2349891d64f2cdca166886a38c74d5f"
  dependencies:
    buffer "^5.2.1"
    elliptic "^6.4.0"
    ethereumjs-abi "0.6.5"
    ethereumjs-util "^5.1.1"
    tweetnacl "^1.0.3"
    tweetnacl-util "^0.15.1"

A little bit different than your setup.

Two tests are crashing :

  1680 passing (14s)
  2 failing

  1) Gas Duck
       fetchBasicGasEstimates
         should fallback to network if retrieving estimates from storage fails:

      AssertionError [ERR_ASSERTION]: should fetch metaswap /gasPrices
      + expected - actual

      -false
      +true

      at Context.<anonymous> (ui/app/ducks/gas/gas-duck.test.js:241:23)

  2) useTransactionDisplayData
       should return an appropriate object:

      AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual ... Lines skipped

  {
    category: 'send',
-   date: 'May 13',
+   date: 'May 12',
    displayedStatusKey: 'confirmed',
...
    title: 'Send ETH'
  }
      + expected - actual

       {
         "category": "send"
      -  "date": "May 13"
      +  "date": "May 12"
         "displayedStatusKey": "confirmed"
         "isPending": false
         "isSubmitted": false
         "primaryCurrency": "-1 ETH"

      at Context.<anonymous> (ui/app/hooks/tests/useTransactionDisplayData.test.js:243:21)

No more recursion errors, the naclUtil.decodeBase64 call is successfull however it crashes a little bit further : eth-sig-util/index.js line 382

var decryptedMessage = nacl.box.open(ciphertext, nonce, ephemPublicKey, recieverEncryptionPrivateKey);

decryptedMessage is null

Thanks

xmaysonnave commented 3 years ago

By the way. In my project I use the eth-sig-util I built and have two tests (encrypt and decrypt) both are passing.

xmaysonnave commented 3 years ago

I made a mistake yesterday as I was using the wrong ethereum account so a null was expected... Now, it works in Firefox. Chrome is still unusable though... Here is my final yarn.lock :

tweetnacl-util@^0.15.0, tweetnacl-util@^0.15.1:
  version "0.15.1"
  resolved "https://registry.yarnpkg.com/tweetnacl-util/-/tweetnacl-util-0.15.1.tgz#b80fcdb5c97bcc508be18c44a4be50f022eea00b"
  integrity sha512-RKJBIj8lySrShN4w6i/BonWp2Z/uxwC3h4y7xsRrpP59ZboCd0GpEVsOnMDYLMmKBpYhb5TgHzZXy7wTfYFBRw==

tweetnacl@^1.0.0, tweetnacl@^1.0.1, tweetnacl@^1.0.3:
  version "1.0.3"
  resolved "https://registry.yarnpkg.com/tweetnacl/-/tweetnacl-1.0.3.tgz#ac0af71680458d8a6378d0d0d050ab1407d35596"
  integrity sha512-6rt+RN7aOi1nGMyC4Xa5DdYiukl2UWCbcJft7YhxReBGQD7OAM8Pbxw6YMo4r2diNEA8FEmu32YOn9rhaiE5yw==

"eth-sig-util@https://github.com/xmaysonnave/eth-sig-util#v3.0.1":
  version "3.0.1"
  resolved "https://github.com/xmaysonnave/eth-sig-util#5dfbf631d2349891d64f2cdca166886a38c74d5f"
  dependencies:
    buffer "^5.2.1"
    elliptic "^6.4.0"
    ethereumjs-abi "0.6.5"
    ethereumjs-util "^5.1.1"
    tweetnacl "^1.0.3"
    tweetnacl-util "^0.15.1"

The eth-sig-util needs to be rebuild with the correct dependencies. I opened a request to get a browser component. I didn't get any answer. I you decide to rebuild the library, providing a browser component will be greatly appreciated. https://github.com/xmaysonnave/tiddlywiki-ipfs/issues/41

Thanks

xmaysonnave commented 3 years ago

The v8.1.10 fix the issue with Firefox however it doesn't work with Chrome. Chrome freezes while connecting....

Gudahtt commented 3 years ago

When you wrote that comment, v8.1.10 hadn't shipped in Chrome yet. Now that it's been published there, I suspect you won't see the UI freeze bug anymore.

Let me know if you're still experiencing this problem on the latest version of MetaMask. Thanks!

xmaysonnave commented 3 years ago

I built and tested the 8.1.10 with Chrome before Google shipped it. I think that the issue is different than the one fixed with the tweenacl library upgrade. I think that the Firefox issue has been addressed and as such this ticket could be closed. I will try to investigate this issue on Chrome. Thanks.

xmaysonnave commented 3 years ago

@Gudahtt Built v9.0.1 and tested under Chromium Version 83.0.4103.116 Got a crash while attempting to decrypt. Here is the stack trace. No issue with Firefox.

TypeError: Cannot read property 'name' of undefined
  at ConfirmDecryptMessage.renderBody (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:254246:56)
  at ConfirmDecryptMessage.render (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:254384:36)
  at finishClassComponent (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:181631:31)
  at updateClassComponent (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:181584:24)
  at beginWork$1 (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:183347:16)
  at HTMLUnknownElement.callCallback (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:163497:14)
  at Object.invokeGuardedCallbackDev (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:163546:16)
  at invokeGuardedCallback (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:163601:31)
  at beginWork$$1 (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:188941:7)
  at performUnitOfWork (chrome-extension://nganihjmiodjiaamcegndiflejojbekf/ui.js:187856:12)

Thanks

xmaysonnave commented 3 years ago

Starting from the v9.0.3 the UI crash has been fixed as described in the changelog and tested. However the UI is still freezing on Chrome (v9.0.3, v9.0.4), no issue so far with Firefox, cannot test on Metamask mobile as there is not encryption support yet. Most of the time the UI freezes with the Connection dialog box displayed, sometimes the Connection dialog box is displayed two times, not at the same time though, sometimes I go further and reach the decryption dialog box. Unfortunately unusable at this stage. Could be nice if we can tackle this issue as the Chromium family is the leading browser nowadays. Let me know at x.maysonave@gmail.com if we can peer work to debug. I'm also ready to send you a link with my standalone app. In that case send me a public encryption key. Thanks and keep up your good work.

vandan commented 2 years ago

Closing this issue. Please reopen with reproduction steps if needed.