Switch from sjcl to noble-curves and noble-hashes #26
Conversation
|
I have tried implementing ristretto, but it takes a lot of time. There's little-endian instead of big-endian and bit fiddling (clearing bits in scalars), etc. Noble curves with DST that accepts Uint8Array should be out later. Other than that, the PR is ready for review. |
Co-authored-by: Armando Faz <armfazh@users.noreply.github.com>
There was a problem hiding this comment.
Thanks @paulmillr for the contribution! These look like great libraries. After having a closer look at noble-curves (I have not had a chance to look at noble-hashes yet), the selling points of these implementations are indeed quite attractive.
My initial concern is that there was no effort to make the field arithmetic constant-time. As the README rightly points out, we can't reasonably expect a JS implementation of things like scalar multiplication to mitigate all possible side channels (e.g., an attacker that shares CPU cache with the process). That said, it may be reasonable to expect such code to resist less sophisticated threats (e.g., an attacker making requests to the process from the network). Put simply: orders of magnitude matter.
Before taking this PR, I'd like to suggest we assess whether there are regressions in side-channel resistance. In particular, if the SJCL made any effort to make the field arithmetic constant time, then I think we should keep this dependency for now.
I should emphasize however that I'm not really an expert on how to write low-level crypto securely (@armfazh is) and I'm not an expert in JS. Which leads me to a question: What accounts for the speed up of noble-curves over SJCL? (E.g., nearly 3.5x for P-256 scalar multiplication, if I'm reading your benchmarks correctly.) Perhaps there side-channel mitigations built into SJCL's field arithmetic that make it inherently slower?
|
No. Sjcl does not have ct mitigations. Noble curves do. Sjcl is basically unsupported buggy garbage these days. Maybe they were great in 2012.
Native bigint versus non native bigint (which is also non ct) and tons of other hand-picked optimizations |
Bugs:
You're saying "noble is bad" because we don't have ct fields (sjcl fields are also not ct), however, noble has things which are a lot more important: algorithmically CT multiplication using second accumulator, complete addition formulas. Just do some EC ct tests, you'll see what is where. |
To be clear I'm not suggesting noble is bad. My questions were meant to assess whether we would lose any CT code by taking this change, which you are saying we don't. I appreciate you taking the time to point this out. |
|
I haven't studied the topic constant-time javascript in much detail yet, but it scares me. For instance, JITs commonly specialise functions on commonly used arguments, and that's a bummer if the argument is meant to be secret. With that big caveat out of the way: noble relies on |
|
@bwesterb I wrote this paragraph on MDN. Pull request 20272 in repository mdn/content. |
|
Folks, you literally have dumpster fire right now with regards to timing attacks (curves themselves) and you're talking about some theoretical nonsense. Did you check the library you've linked? There is no CT inversion there. This doesn't make any sense. |
|
Thanks @paulmillr for taking the time to prepare and send this PR! We're investigating this change and will get back to this thread when we have time. Please be patient. In the meantime, if you could clean up conflicts, that would help make our assessment easier. Please also be mindful of our code of conduct when engaging here. Let's keep the discussion and tone professional and focus on the technical matters at hand. We look forward to your continued, constructive contributions! |
|
Understood. To make decision easier for you, check out how sjcl calculation time depends on private key bits, effectively leaking all the timings: Reproducible with this code: // npm init -y && npm install micro-bmark sjcl-including-ecc
const bmark = require('micro-bmark');
const sjcl = require('sjcl-including-ecc');
const curve = sjcl.ecc.curves.k256;
// Generate new key
// var k2 = sjcl.ecc.ecdsa.generateKeys(curve, 0); console.log(k2.sec.serialize())
const privA = '1000000000000000000000000000000000000000000000000000000000000000';
const privB = '0000000000000000000000000000010000000000000000000000000000000000';
const privC = '0000000000000000000000000000000000000000000000000000000000000001';
bmark.run(async () => {
console.log(curve.G.mult(privA).isIdentity);
await bmark.mark('sjcl private key A', 110, () => curve.G.mult(privA));
await bmark.mark('sjcl private key B', 110, () => curve.G.mult(privB));
await bmark.mark('sjcl private key C', 110, () => curve.G.mult(privC));
})This is catastrophically bad result and it won't happen with curves. By the way, curves got audited, I will publish it in the repo asap. |
|
It has been around 1 month since the PR was created. I have shown you how your current cryptography is not secure and is bleeding with vulnerability leaking timings that could be exploited today, given enough resources. On a contrary, curves were audited by a third party. Is this going to be reviewed, or did I do some unnecessary investment of 7 hours of free work in cloudflare repository that you've agreed upon before? This is not how open source is developed. Either you don't want it and say in advance, or at least leave some feedback to OSS contributors. |
|
As I said above, @paulmillr, we are still investigating this PR amongst many other competing priorities. Please be patient. Being disrespectful will not expedite the process. |
|
sjcl will be crushed by noble-curves any day now. Watch for weekly downloads and dependents who've made their pick at this url: |
|
This seems a clear improvement. An MIT licensed (can always fork) open source lib by a passionate author vs an ancient lib that requires vendoring and patching. Not to mention the clear performance gains! |
|
Update: in #35 , there is a new way to make the group implementation plug-able. This enable users to provide a different (than sjcl) implementation. |
|
Would you be fine with bundling a @noble implementation in this package? (and updating the test files to test it) Can prefix it with experimental (to start?) Also, while I'm at it messing with package.json "exports", I would like to sort out commonjs support (see #30 ) |
I have removed
InnerScalarhack becauseprivateproperty access (s.k) may fail at some point in the future when typescript will start compiling it to actual private property.Benchmark results on M2 / node v18.14.0. Before:
After: