Replace OpenSSL in cross-testing with micro-ecc(uECC)

elichai commented at 2:36 pm on April 12, 2020: contributor

Adding cross testing against non-openssl implementation was suggested multiple times #691 I attempted to do it in #641 but there wasn’t good consensus to add rust even to the testing suite. @gmaxwell suggested in #716 that we can use https://github.com/kmackay/micro-ecc for that.

So I integrated uECC into our tests build system and replaced the current openssl cross testing with uECC tests.

This is IMHO better for a couple of reasons:

uECC is a simpler implementation, so this allows us to test that the different approaches don’t produce anything wrong.
uECC has a deterministic ECDSA implementation so we also test signatures equivalence(there was a small bug there I had to fix).
We replace an external dependency(OpenSSL) with a small vendored one, which makes things simpler.
We could also add BIP-Schnorr impl into uECC when they get merged.

cc #736 https://github.com/bitcoin-core/secp256k1/issues/734

vendor kmackay/micro-ecc commit: 601bd11062c551b108adbb43ba99f199b840777c with source files only 44ad4d739e

Modify micro-ecc to compile under C89 and use correct endianess in RFC6979 f5327bc8ae

Integrate building micro-ecc into autotools c61e974f4e

Implement cross testing with micro-ecc d47aee3f53

Remove openssl cross tests and benchmarks 50eaa53d04

real-or-random commented at 3:06 pm on April 12, 2020: contributor

Hey, great stuff!

I haven’t looked at the code yet but here are some quick initial thoughts and discussion points:

Do you think upstream wants to integrate your second commit?
Adding µΕCC is great but it doesn’t necessarily mean that we need to remove OpenSSL. But I think we should remove it for the reasons you mention (no external dep, easier to implement Schnorr sigs, …) Did you look at the performance of µΕCC vs OpenSSL? It’s not crucial but of course helpful if we want to run many tests.
Do we want to vendor this or integrate this differently? I think vendoring it is fine for our purpose. Have you considered git subtree or even https://github.com/brettlangdon/git-vendor (old but works very and in the end is just a wrapper around subtree)?

elichai commented at 3:12 pm on April 12, 2020: contributor

Do you think upstream wants to integrate your second commit?

It sounds like upstream knows about this, not sure why they decided to do that https://github.com/kmackay/micro-ecc/blob/master/uECC.c#L1374

Adding µΕCC is great but it doesn’t necessarily mean that we need to remove OpenSSL. But I think we should remove it for the reasons you mention (no external dep, easier to implement Schnorr sigs, …) Did you look at the performance of µΕCC vs OpenSSL? It’s not crucial but of course helpful if we want to run many tests.

I got something along these lines: a bit faster than openssl but not by much.

0ecdsa_verify: min 52.7us / avg 55.7us / max 58.4us        
1ecdsa_verify_openssl: min 369us / avg 386us / max 414us                                                                                 
2benchmark_verify_uECC: min 323us / avg 333us / max 348us

Do we want to vendor this or integrate this differently? I think vendoring it is fine for our purpose. Have you considered git subtree or even https://github.com/brettlangdon/git-vendor (old but works very and in the end is just a wrapper around subtree)?

That’s a good question, I’m fine however, I did it like this to minimize the amount of junk I’m adding here, but if people prefer I can do a subtree instead

real-or-random commented at 3:23 pm on April 12, 2020: contributor

That’s a good question, I’m fine however, I did it like this to minimize the amount of junk I’m adding here, but if people prefer I can do a subtree instead

I think subtree is a good option since it has been integrated in git.

real-or-random commented at 3:25 pm on April 12, 2020: contributor

Do you think upstream wants to integrate your second commit?

It sounds like upstream knows about this, not sure why they decided to do that https://github.com/kmackay/micro-ecc/blob/master/uECC.c#L1374

I guess it’s about performance and simplicity. Endianness is arguably not so interesting for a random integer. :D

sipa commented at 5:22 pm on April 12, 2020: contributor

Subtree won’t work if we need local modifications.

real-or-random commented at 6:47 pm on April 12, 2020: contributor

Oh right! I confused subtree with this thing here https://github.com/ingydotnet/git-subrepo that I used once in a different project. It’s very neat but not sure if it’s overkill for us. It’s something like a dev dependency (only people that want to use subrepo commands need to install it, you don’t need it for a normal checkout and build.)

gmaxwell commented at 10:47 pm on April 12, 2020: contributor

@elichai It’s offtopic, but since you edited the microecc code, I’m confused by: “* We just use H(m) directly rather than bits2octets(H(m)) (it is not reduced modulo curve_n).” IIRC, in RFC6979 you’re supposed to truncate to ceil(log2(n)) bits then use rejection sampling. For some curves like the nist160 curve that microecc supports, the order is extremely far from a power of two, and so the rejection sampling actually avoids a security vulnerability. There isn’t one hidden there is there?

elichai commented at 10:56 pm on April 12, 2020: contributor

@elichai It’s offtopic, but since you edited the microecc code, I’m confused by: “* We just use H(m) directly rather than bits2octets(H(m)) (it is not reduced modulo curve_n).” IIRC, in RFC6979 you’re supposed to truncate to ceil(log2(n)) bits then use rejection sampling. For some curves like the nist160 curve that microecc supports, the order is extremely far from a power of two, and so the rejection sampling actually avoids a security vulnerability. There isn’t one hidden there is there?

It’s late here so maybe I’m not reading right, but reading RFC6979 bits2octets also reduces mod q. And skipping bits2octets is also explicit in section 3.6 Variants in the first variant.

Maybe you’re confusing the result of the HMAC (which is k) rather than the H(m) which get fed into the HMAC engine? or am I misunderstanding you?

gmaxwell commented at 11:35 pm on April 12, 2020: contributor

Ah, indeed that’s right! I don’t see how to exploit that particular behaviour, either. Thanks for answering my question!

real-or-random cross-referenced this on Apr 13, 2020 from issue build: fix OpenSSL EC detection on macOS by fanquake

real-or-random commented at 9:23 am on April 13, 2020: contributor

Hm I looked at the status of micro-ecc, and it’s not very satisfying. It’s not actively maintained (I guess that’s okay for us) but there are a lot of bug reports about incorrect computations:

https://github.com/kmackay/micro-ecc/issues/145
https://github.com/kmackay/micro-ecc/issues/128
https://github.com/kmackay/micro-ecc/issues/161
https://github.com/kmackay/micro-ecc/issues/160
https://github.com/kmackay/micro-ecc/issues/163 (not sure, seems to be a usage issue)

Cross-testing is a good issue but cross-testing against a known-to-be-buggy library will probably create too much more headache to be useful.

gmaxwell commented at 9:36 am on April 13, 2020: contributor

@real-or-random if it’s buggy and the cross tests pass that in and of itself is extremely informative as the tests should be catching real bugs. … though the first link is that it doesn’t produce low-S signatures and isn’t actually a bug. Cross-testing with openssl turned up real bugs in openssl: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-3570

If it’s too buggy then sure it might not be worth using if it takes too much effort to fix it, but the exercise of exposing the bugs would presumably improve the tests a lot.

real-or-random commented at 9:47 am on April 13, 2020: contributor

@real-or-random if it’s buggy and the cross tests pass that in and of itself is extremely informative as the tests should be catching real bugs. … […] Cross-testing with openssl turned up real bugs in openssl: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-3570

I can’t really parse your first sentence, sorry. I think the goal is to find bugs in our library and not in micro-ecc, no? I mean of course we want to find discrepancies first and then see who’s wrong but it seems to me that micro-ecc is quite often wrong, e.g. 128 and 160 are for sure bugs in micro-ecc.

though the first link is that it doesn’t produce low-S signatures and isn’t actually a bug.

Okay indeed.

gmaxwell commented at 10:08 am on April 13, 2020: contributor

The main purpose of cross testing is to find (or avoid) deficiencies in the tests and prevent them from concealing complementary bugs in the library. If those bugs in microecc are real (and not reporting errors) then the tests in libsecp256k1 are deficient if it passes crosstesting.

It’s not necessary that the thing being crosstested against is great. OpenSSL wasn’t great, it was– in fact– outright defective. For crosstesting it’s important that it fails differently. If it has some bugs too, they’re areas that libsecp256k1’s test should make sure to test in case libsecp256k1 eventually gains similar bugs. If it’s too broken and too hard to fix, then sure it could be a waste of time.

I think you didn’t mean 160? It’s not a bug at all, it’s someone asking about bugs. 128 and 161 are bugs that if they exists libsecp256k1 tests should totally catch, they’re obvious test vectors. 163 is so bad its hard to believe its correct (and if its doing it on nonces too, it’s pretty much a total break– kinda perplexing that it would have that issue for keygen and not nonces).

elichai commented at 10:20 am on April 13, 2020: contributor

@real-or-random if it’s buggy and the cross tests pass that in and of itself is extremely informative as the tests should be catching real bugs.

A future PR can integrate this into all of our edge cases tests. ie wrap the signing/verifying function and every time it’s called call both libsecp and micro-ecc and compare results. this should make us test all the current test vectors / edge cases against both and will probably discover bugs if they’re there.

about the low-S as @gmaxwell that’s not a bug and I handle it in the tests: https://github.com/bitcoin-core/secp256k1/blob/50eaa53d04bd04a937f3925aa46e812227089152/src/tests.c#L5128 https://github.com/bitcoin-core/secp256k1/blob/50eaa53d04bd04a937f3925aa46e812227089152/src/tests.c#L5136

elichai commented at 10:26 am on April 13, 2020: contributor

And yeah https://github.com/kmackay/micro-ecc/issues/163 doesn’t make a lot of sense. only here I test 640 times signing+verifying and comparing the deterministic signatures. And computing pubkeys+comparing, which uses the EccPoint_mult function he’s talking about

real-or-random commented at 10:39 am on April 13, 2020: contributor

I think you didn’t mean 160? It’s not a bug at all, it’s someone asking about bugs. 128 and 161 are bugs that if they exists libsecp256k1 tests should totally catch, they’re obvious test vectors. 163 is so bad its hard to believe its correct (and if its doing it on nonces too, it’s pretty much a total break– kinda perplexing that it would have that issue for keygen and not nonces).

Yes, I meant 161.

Having those as test vectors is certainly useful. What’s not clear in my mind yet is how this should work in the end. Let me try to say it in my own words:

Assume the cross-tests fail, we discover some differences and add test vectors. Then the cross-tests still fail. So we need to exclude the failing instances from the cross-tests (as we can’t hope for micro-ecc to fix them). Excluding is okay for cases like 161, 128 because these happen with negligible probability on random vectors. If it’s a bug that we run into now and then even on random inputs, that’s probably more annoying. But hopefully this won’t happen.

Does this match your ideas?

A future PR can integrate this into all of our edge cases tests. ie wrap the signing/verifying function and every time it’s called call both libsecp and micro-ecc and compare results. this should make us test all the current test vectors / edge cases against both and will probably discover bugs if they’re there.

I feel we should do be doing this right now for OpenSSL.

elichai commented at 10:44 am on April 13, 2020: contributor

I feel we should do be doing this right now for OpenSSL.

I can look into it if it helps, but too many parts of the tests use internal APIs for no reason right now. So it will probably require some refactoring

wpeckr commented at 4:35 pm on April 13, 2020: none

You should be extremely cautious doing anything with this library, by all means test it and break it, but never trust it.

Privately we’ve had concerns about how it operates for quite a while, and this particular piece of code has shown up in a large number of places recently. It is used in Sony products, Intel products, numerous Bitcoin hardware wallets, security sensitive bootloaders, and even security appliances.

The code has no meaningful tests at all.
Functions for signing operations are confusingly named, leading to errors in implementation. Static analysis of one usage of this code shows that the author was confused by the existence of a function named uECC_sign(), which uses a random nonce, when they really wanted to be using uECC_sign_deterministic().
The RNG code supplied for embedded platforms is a joke.

wpeckr commented at 4:39 pm on April 13, 2020: none

This to me is a pretty good demonstration of code laundering. A library that nobody would ever have considered using in its current state is being included in a large number of projects, and its existence in those is being used as a justification of its safety. if studying it does not result in at least a dozen CVEs I would be very surprised.

gmaxwell commented at 7:07 pm on April 13, 2020: contributor

So we need to exclude the failing instances from the cross-tests (as we can’t hope for micro-ecc to fix them).

Just fix ’em locally. Unless it takes a heroic effort to fix them, in which case that would be an argument against using it as a comparison point.

Another way to look at it: is that if the project had unlimited resources it would be attractive to go and get two non-overlapping set of developers and get them to implement the same functionality without ever looking at libsecp256k1; and forbid one of them from using a list of techniques that are in libsecp256k1. Then use both of those as comparison points. The resources to do that don’t exist, but grabbing some already existing code is a good approximation. It may share some bugs with libsecp256k1 but its unlikely to share all. Having to apply fixes takes some work, but less than writing the whole thing from scratch. Applying fixes may increase the failure correlation but it won’t make it worse than not having the comparison at all.

real-or-random commented at 7:16 am on April 14, 2020: contributor

I feel we should do be doing this right now for OpenSSL.

I can look into it if it helps, but too many parts of the tests use internal APIs for no reason right now. So it will probably require some refactoring

I think this will be very useful and then we could have a more generic thing that is able to call a number of different implementations (OpenSSL, micro-ecc, …?) and get cross-tests for free for a lot of (future) tests. Then this will actually a reason to keep the OpenSSL tests first. But I will be good to have some more opinions on the entire thing. @sipa @jonasnick @apoelstra

Just fix ’em locally.

Yeah, this is what I’m most skeptical about in terms of effort but I guess we should give it a try.

real-or-random commented at 7:20 am on April 14, 2020: contributor

You should be extremely cautious doing anything with this library, by all means test it and break it, but never trust it.

It’s safe to say that this PR is in line with not trusting it.

elichai commented at 7:06 pm on April 15, 2020: contributor

Another way to look at it: is that if the project had unlimited resources it would be attractive to go and get two non-overlapping set of developers and get them to implement the same functionality without ever looking at libsecp256k1;

FWIW this is what I’ve done in #641, I wrote my impl without looking at the code here, or honestly any code, I intentionally implemented it from papers only. there are 2 problems though.

It’s in Rust.
Using gmp(self compiling+statically linked) for the scalar+field arithmetic, which kinda suck, but had my reasons for that.

real-or-random cross-referenced this on Sep 28, 2021 from issue [RFC] Remove OpenSSL testing support by sipa

real-or-random cross-referenced this on Oct 17, 2021 from issue cross-testing by real-or-random

real-or-random commented at 2:48 pm on May 8, 2023: contributor

Some thoughts after discussion with @sipa and @jonasnick: It’s not entirely clear how useful cross-testing only ECDSA is: The confidence in our ECDSA code is pretty good. There’s cryptofuzz and there’s Wycheproof vectors now (which are explicitly based on differential testing or built to catch differences.) So, the approach in this PR would probably be more useful if it covered more than just ECDSA. But even if this approach covered more, we’d need to see if it’s worth the hassle. @elichai

Are you still convinced that this is a useful approach?
If yes, do you plan to work on it?
If yes, do you plan to add more than ECDSA?

If the answer to all of these three questions is yes, then we should probably keep talking. If not, we should close this.

real-or-random added the label assurance on May 8, 2023

Replace OpenSSL in cross-testing with micro-ecc(uECC) #738