NB: This post describes a very casual code review of a few cryptography functions used by MetaMask. It does not describe any vulnerabilities. If you’re the kind of person who likes a meandering and amateurish code review that goes absolutely nowhere, you’ll enjoy this post. Otherwise you might want to read something more exciting: I suggest Moxie’s post.
For reasons I can’t explain, I thought it might be fun to spend an hour or two investigating the cryptography used by MetaMask, a web-based cryptocurrency wallet used primarily for “web3” applications. My goal was an extremely casual code review: I didn’t use any tooling, and in most cases I didn’t even install the code on my computer. All I did was poke around various Github repositories to see if I could find anything unusual, or at least get a feel for the quality of the code. (In fact I did about half the work on my phone while eating a burrito bowl at Chipotle.)
After an hour or two of hunting through dependencies, I made the mistake of tweeting about my feelings:
I decided to look at MetaMask’s crypto, and oh wow I wish I could unlook.
— Matthew Green (@matthew_d_green) January 12, 2022
This offhand Tweet wasn’t intended to make anyone sad or scare them that their funds were at risk. Although I had a couple of brief scary moments, they were entirely on me. So let me be clear: I did not find any critical vulnerabilities in MetaMask’s crypto!
What I do have is some criticisms of the complexity and quality of the (current) crypto code and its dependency relationships. Some of this is fundamental to the setting (“untyped JavaScript crypto is bad.”) But some is specific to the design, and particularly some of it is a personal gripe: this code was not that easy to audit.
My biggest TL;DR is that finding and reviewing the correct code was much harder than it had to be. Moreover, there were far too many different organizations owning the dependencies that led to the crypto routines themselves. This made me uncomfortable, given how much money these routines are responsible for securing.
In this post I’m going to justify these opinions by walking you through a casual skim of MetaMask’s code and and crypto dependencies. To make this post more realistic, I’m going to discuss the process more or less in the order that I did it. (I’ll even include the embarrassing parts where I go totally off the rails and review the wrong code.)
If you enjoy reading crypto code for entertainment, this post might be for you. However if you’re hoping that this will actually lead to anywhere exciting, I suspect you’ll be very disappointed.
Quick explainer: what is MetaMask?
Since many readers have probably never used DeFi, I figure I should give a quick background here on “web3” and MetaMask in particular.
From a technical perspective, “web3” is pretty straightforward. It consists of many “decentralized apps” (dapps), each of which typically comprises a (usually Javascript-based) front-end web app, as well as some back-end business logic. Of course, that description would cover web2 as well. What makes dapps special is that (some of) the back-end stuff is decentralized, meaning that it’s implemented inside of a smart contract running in a network like Ethereum.
Web3 front-ends are typically not very interesting. They’re just web apps that are served to your browser via standard web infrastructure. (Security here generally means TLS, which means that anyone who pops a server or a Cloudflare API key can change an app’s code in malicious ways.)
Of course, web apps can’t communicate with blockchains, nor are they a good place to store private keys — which is why MetaMask exists. In its popular configuration, MetaMask is a browser extension for Chrome and Firefox that handles both tasks. When a web application needs to send a transaction off to a smart contract (e.g., because you want to deposit money into Aave), MetaMask is responsible for signing the transaction and managing your funds.
On the bright side, the actual cryptography in MetaMask is fairly limited: as a wallet, it must generate and store Ethereum public and private keys, and it also needs to handle “simple” operations like ECDSA signing. On the otherhand: we’re in a browser, and that means we should expect to be working in Javascript (or TypeScript) and dealing with all the vague and troubling limitations of that environment.
Getting started
To put some bounds on the effort required, I decided to only look at the implementation of ECDSA signing and key generation (excluding the curve operations.) This seemed like an easy task I could get done in an hour.
Finding the starting point for a review like this was harder than I expected. In short: it isn’t easy to grok the control flow of a complex event-based Javascript browser extension to find out exactly which calls are “real” and which are tests or dead code. To shortcut this problem, my approach amounts to “grepping and hoping”, starting from the “develop” branch of MetaMask’s extension repository.
A quick search for “sign” leads us to a promising call:
Sadly, following this call path quickly leads us into dependency hell.
First, we reach a library called eth-keyring-controller, which presumably manages Ethereum keyrings. I’ll spare you the details, but a quick scan shows it calling a second dependency: eth-sig-util. (Both are NPM packages developed by MetaMask.) We’ll jump right to the latter package, which calls… you guessed it, yet another package:
To find that call we need to leave MetaMask-owned code and set out for a package called “ethereum-jsutil” that’s maintained by the “Ethereum Javascript community“, whoever they are. (It doesn’t matter because we won’t stay here long enough to find out.) Here we have yet another layer of dependency indirection:
To find this routine, we need to head over to “js-ethereum-cryptography“, which holy cow is actually a repository maintained by the Ethereum project itself!
At this point I’m about halfway into my allotted time and asking myself questions like “why didn’t MetaMask just call this library directly” and “why do I make poor life choices”. But we’re almost there: surely we are going to see some actual crypto code soon! Except no, we are not. This is what we find in the Ethereum library:
So yet another call and yet another dependency: this time to something called “noble“. Here my quick-and-dirty approach to dependency resolution (Google “npm <package-name>”) fails me, since NPM says that “noble” is is a “Node.js Bluetooth Low Energy library.” As cool as that would be — access the blockchain through Bluetooth — I’m guessing this is not right.
A bit more searching leads me to believe that noble is actually the “Noble cryptography library“, which appears to have been developed by a guy named Paul Miller. And hey, this code looks promising! The page lists actual cryptography design goals that seem reasonable, and the code is written in TypeScript. Even better: the library has been subject to an audit by Cure53.
Nonetheless — with no disrespect to Paul or anyone else in this community — I would like to take a moment to complain about this dependency path:
- Resolving all these pointless dependencies has eaten up a lot more of my review time than you might imagine. (I’m leaving out all the times I accidentally visited the wrong libraries because they used some combination of “js” and “ethereum” and “cryptography” and just Googling is risky here.)
- More substantively, I can’t help but notice that there are a lot of code owners in this critical path. So far I’ve traveled through repositories owned by four different organizations, and the last one is someone’s personal Github account. Is this normal for a system that secures billions of dollars of user funds? Maybe it should not be.
But enough complaining. There is actual crypto code here! We can finally look at ECDSA.
Except… it turns out that we can’t do that, because I made a stupid mistake.
After speaking with Paul Miller on Twitter, I learned that this whole code review has been premised on a very foolish assumption: namely that the code in the main (development) branches of MetaMask and its dependencies is actually the code people are using in MetaMask today. That turns out to be wrong. In fact, Paul tells me, the noble library is slated for deployment in an upcoming release. The current release of MetaMask relies on a library called “elliptic“, which was written by Fedor Induntny.
I’m now scraping up those last bits of cheese in the burrito bowl.
Looking at elliptic’s ECDSA signing routine
I promise to come back to the newer code soon, but my goal in this review was to look at MetaMask as it exists in today’s releases. Apparently that means I need to look at Fedor’s elliptic library.
(Note: if I was being professional then what I would really do is review all the release branches of MetaMask and all of its dependencies just to make sure I’m in the right place. But life is too short: I’m going to trust Paul.)
The elliptic library is written in “plain old” Javascript, so types will tend to be confusing. On the bright side, it’s relatively compact. The core library supports Ed25519 (for EdDSA) and Secp256k1 for ECDSA on networks like Ethereum. Let’s focus on the latter.
I’m not a Javascript developer so certain things aggravate me about reviewing this code. One is that developers often put critical routines in stupidly-named files like “index.js”, which is where we finally reach the core signing implementation for ECDSA.
Signing
ECDSA is a stupid algorithm but it’s not a very complicated one. Leaving aside the curve operations, there are basically two places where ECDSA implementations tend to go wrong. The first is in key generation. The other is in signing.
In ECSDA signing the main security risk is in how nonces are generated: it’s critical that the ECDSA nonce (“k”) is sampled uniformly from the range 1 … n-1, where n is the group order. Critically, the same nonce must never be used with two distinct messages (really, message hashes.) If this ever happens, it’s possible to recover a private key from two signatures, something that’s generally frowned upon.
Let’s look at how signing is handled in this library:
So looking at the code above, how do we get “k“?
A first observation is that there is no actual randomness here, no call to a random number generator. Instead, the signing routine instantiates a deterministic random bit generator (DRBG) based on HMAC. That algorithm stretches a shorter “seed” into a longer sequence of pseudorandom bits that can then be used to obtain our random nonce.
I initially had a small heart attack when I (briefly!) misread the code and thought that the only seed for the DRBG was the ECDSA private key: this would definitely lead to nonce re-use. On closer inspection, the DRBG is seeded with two fields: entropy is set to the ECDSA private key, and nonce the (hashed) message to be signed. Within the underlying DRBG implementation both values get hashed into the DRBG seed: this should mean that each (private key, message) pair will get different random bits, and thus different nonces “k”.
While I don’t love a purely deterministic generation of “k” (and I’ll explain why below), this isn’t some roll-your-own idea: in fact this is appears very similar to the approach recommended by RFC 6979. And going one step farther, it appears that elliptic is actually directly implementing an algorithm from that RFC, specifically the one in section 3.3, “Alternate Description of the Generation of k.” I did not discover this fact from examining the code comments, which would have saved me a lot of time. I realized it only while reading through that RFC (in fairness, it’s briefly mentioned on elliptic’s README.)
(Developers: if you implement a standard algorithm please add detailed comments at the point where you’re using it, and comment each step so we can see exactly how it maps to the standard. For a good example of how to do this, see how noble implemented the same standard.)
I’m trying not to be too critical here, but there are a number of things I find less than optimal about this implementation.
A particularly bad one is that the caller can specify a function called options.k as part of the options argument. If present, this function overrides the built-in nonce generation logic and replaces it with logic the caller selects. While I can see the argument for allowing caller-provided entropy as an add-on to nonce generation, I can’t see why the caller would want to entirely override the signing routine’s secure implementation. This seems like a giant footgun and a also a good opportunity for someone to slip in malicious code. (I’m not sure which dependency actually calls this routine in the released code.)
Also, the options structure can arrive within the options argument of the routine as expected. But just for fun, it can also be passed in the enc argument as well, at which point it will be copied into options. This happens for… reasons, I guess.
A few smaller notes:
- The function
_truncateToN()seems poorly defined and serves two different purposes, depending on a boolean flag which is left out of some calls. In the first mode it simply truncates the message to ensure it is the same length as the group order n. This mode is only used to truncate nonces, which is not something that should ever need to happen — except when you’re using the crazyoptions.kfunction discussed above. - When passed an optional boolean flag, however,
_truncateToN()will also ensure that the truncated result is less than or equal to n, and if not it will subtract n. This appears to implement the bits2octets subroutine of RFC 6979, which is fine I guess but took too long to figure out. Neither of these two issues are critical, but they made the code harder to read.
As a final note: I would urge developers to avoid these purely deterministic ECDSA implementations, mostly because they make signing implementations very fragile. Since all of the “entropy” in this signing routine comes from the message and private key, this means that any non-determinism in the actual ECDSA implementation (e.g., caused by weird option flags or glitches in BigNum encoding) can potentially produce exploitable signatures if the same message is ever signed twice. (This could also happen if you install the same private key into two different wallet implementations and sign the same message.) I don’t see any obvious instances of this, but it makes me nervous.
In either case: adding some genuine entropy to the nonce could really help here. Section 3.6 of RFC 6979 shows how to do this by adding an additional random input.
Key generation
The last stop on this very brief tour is the key generation algorithm for ECDSA keypairs. This can be found slightly higher in the same file:
Once again we see that our friend HMAC-DRBG makes an appearance. Like in the signing routine we have a caller-supplied options structure, though fortunately this time it does not allow the caller to override the routine’s entropy generation. Even better: the input to the HMAC-DRBG appears to include actual randomness, drawn from the rand() call.
Does rand() produce actual random numbers in most browsers? I really could not tell you. This routine is implemented by a package called indutny/brorand, the entirety of which I’m pasting below, just because it made me burst into laughter:
Whatever, anyway, let’s just assume the entropy is good. The remainder of the (private) key generation function takes place in these lines:
This generates a random number priv between [0…2^{256} – 1] and then makes sure that priv is not greater than the group order n, in which case it re-generates priv. It then adds 1, which I think gives us a priv in the range [1…n+1], which seems wrong, since it should be in the range [1…n-1]. Am I doing my arithmetic backwards?
Anyway it doesn’t really matter. In either case, this seems more confusing than wrong.
Conclusions
Ok, I am basically just exhausted now. This was a lot of work to evaluate a tiny piece of a crypto library that, frankly, might not even be the actual crypto library that MetaMask is using. I’m not sure if any of this was worth it, and I’m starting to get indigestion from the Chipotle.
If I could summarize my overall findings above, this would be the list:
- I do not like reviewing Javascript for many reasons, not least of which is the lack of typing. This makes everything way more confusing than it should be.
- The stupid dependency tree in this codebase makes reviews much more difficult than they need to be, and adds too many points of trust.
- The crypto code may be well-written in a cryptographic sense, but it was not really optimized for humans to review. This made the process much harder.
If it was up to me I would re-write the entire codebase in TypeScript and would try to use more standard libraries. I would remove layers of dependencies. I would tighten up the crypto APIs to make sure malicious calls are harder to get away with. Finally, I would make sure every single major block in this code was absolutely clear about what it’s doing.
Then I would move this code back under the control of some more centralized organization(s), rather than leaving essential code in random personal repositories.
I don’t think this was entirely a waste of time. Although I don’t love everything about the way this code works, I’m now 15% more confident that MetaMask isn’t doing something utterly stupid with my cryptographic keys. That seems like a genuine win.
Next post: I’m going to review the noble-cryptography libraries to see what the new MetaMask code is planning to do.
Article Link: An extremely casual code review of MetaMask’s crypto – A Few Thoughts on Cryptographic Engineering