chore(WIP): migrate to @noble/curves and eciesjs #1236
Conversation
|
I thought we were using GCM also, according to the old encryption audit. Is the CBC code actually used for anything? https://request.network/en/2020/07/07/request-encryption-audit-completed-by-cure53/
|
|
@MantisClone There are two encryption stages. First, the request is encrypted using AES-256-GCM (symmetric encryption). The generated encryption key is later referred to as the channel key. This channel key is then encrypted with each shareholder's public key using ECIES (asymmetric encryption). This two-stage encryption scheme aims to reduce the size of the encrypted data; otherwise, we would have to encrypt the whole request as many times as there are shareholders. Instead, we encrypt the channel key multiple times, which is a minimal overhead—more details over here. This PR only concentrates on the ECIES part. I didn't know this before, but ECIES also uses AES under the hood, and |
|
@alexandre-abrioux now that ecies/js added support for AES-256-CBC in ecies/js#748, do you still think that it's possible to migrate to @noble/curves and ecies/js? Are we blocked by the fact that ecies/js does not support the SHA512 key derivation function?
|
|
@MantisClone I still think it's possible. We will probably have to implement our own decryption fallback class for supporting old requests with SHA512, but I haven't found time yet to look deeper. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| const hmacSha256Verify = (key: Uint8Array, msg: Uint8Array, sig: Uint8Array): boolean => { | ||
| const expectedSig = hmac(sha256, key, msg); | ||
| return equalConstTime(expectedSig, sig); | ||
| }; | ||
|
|
||
| // Compare two buffers in constant time to prevent timing attacks. | ||
| const equalConstTime = (b1: Uint8Array, b2: Uint8Array): boolean => { | ||
| if (b1.length !== b2.length) { | ||
| return false; | ||
| } | ||
| let res = 0; | ||
| for (let i = 0; i < b1.length; i++) { | ||
| res |= b1[i] ^ b2[i]; | ||
| } | ||
| return res === 0; | ||
| }; |
There was a problem hiding this comment.
By using the Web Crypto API (available in Node.js and in browsers) this whole code (both methods hmacSha256Verify + equalConstTime) could be replaced by a single line:
globalThis.crypto.subtle.verify("HMAC", key, sig, msg)Pros:
- no need to use a library (
@noble/hashes) - the Web Crypto API is async so non-thread blocking
Cons
- the Web Crypto API is async so the calling code needs to be async too (but that's OK since
ecEncryptandecDecryptwere already async in the previous implementation)
- the Web Crypto API is only available in secure context (= HTTPS) (this could be a pain point inlocalhostenvironments)
Let me know if you would prefer it, I can easily make the switch.
alexandre-abrioux
changed the title
| "moduleResolution": "node", | ||
| "moduleResolution": "nodenext", |
There was a problem hiding this comment.
Needed to load @ecies/ciphers/aes. The value node is deprecated anyway, we shouldn't use it, see https://www.typescriptlang.org/tsconfig/#moduleResolution
alexandre-abrioux
changed the title
Description of the changes
Get rid of:
secp256k1which uses C++ bindings on Node.js andellipticin the browser.@toruslabs/eccrypto(eccrypto's fork) which is pure JS and unauditedReplace it with:
ethersfor signatures, which uses@noble/curvesunder the hood starting in v6eciesjsfor encryption/decryption, which uses@noble/curvesunder the hood in browsers and thecryptomodule in Node.jsNote:
@noble/curvesis an audited library and now the industry standard used byethersandviem.Breaking change
The data encryption format changes. This is a breaking change as old versions of the protocol will not be able to decrypt new Requests. The impact should be limited to integrations where actors encrypting and decrypting the Request are different and not using the same version of the protocol, which should, I think, be negligible in the current state of our ecosystem.
The new encryption still uses ECIES but uses the following parameters:
secp256k1AES-256-GCMHDKF; versus previous configuration:
secp256k1AES-256-CBCwithHMACauthenticationSHA512Note:
AES-256-GCMis considered safer thanAES-256-CBCas it contains authentification without needing additionalHMACverification. See https://security.stackexchange.com/questions/184305/why-would-i-ever-use-aes-256-cbc-if-aes-256-gcm-is-more-secureRelated
In ecies/js#747 I've discussed the possibility of supporting the legacy
AES-CBCalgorithm for decryption, and this has been added by the maintainer. In the end, I did not use the library's feature because it does not fit our needs. Indeed,eccryptowas usingAES-CBC-MAC(not justAES-CBC) so verifying the authentication is important before decryption. Furthermore, we have been serializing the encrypted data in a certain format (iv + publicKey + mac + ciphersee here) which is not the expected format needed by the library (publicKey + iv + cipher) and there is no way to pass them down individually. For this reason, I have reimplemented the logic from scratch inutils/crypto/ec-utils-legacy.tsand made sure to verify theHMACauthentication for security.