redo fuzz target #264

apoelstra · 2020-12-22T21:27:59Z

The existing fuzz target code had a number of problems

It unnecessarily reimplemented parsing and serialization logic that we could just fuzz directly
It didn't implement any of the Schnorr stuff
It wasn't internally consistent, in the sense that all the unit tests would fail with it

This PR reimplements signing/verification for both Schnorr and ECDSA, such that a fuzzer should be able to forge "valid" signatures.

It does not implement ECDH, since I couldn't find a reasonable way to get a commutative function of (secret key, public key) that the fuzzer could plausibly fake, nor did I implement the tweak_add and tweak_add_check API for the same reason. I think the ECDH case is probably okay because you never "verify" an ECDH secret without recomputing it, so the fuzzer will know what value to target.

elichai · 2020-12-23T13:26:50Z

One of the doc tests is failing with the fuzzing enabled

   Doc-tests secp256k1

running 5 tests
test src/lib.rs - Secp256k1<C>::verify (line 703) ... ok
test src/lib.rs - (line 39) ... ok
test src/ecdh.rs - ecdh::SharedSecret::new_with_hash (line 129) ... ok
test src/lib.rs - (line 75) ... FAILED
test src/lib.rs - (line 59) ... ok

failures:

---- src/lib.rs - (line 75) stdout ----
Test executable failed (exit code 101).

stderr:
thread 'main' panicked at 'assertion failed: secp.verify(&message, &sig, &public_key).is_ok()', src/lib.rs:34:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

elichai · 2020-12-23T13:32:27Z

Also, just making sure I understand correctly, this only overrides sign/verify for fuzzing, it doesn't override context creation and relevant code, right?

apoelstra · 2020-12-23T15:29:40Z

@elichai correct. To override context creation would require I override pubkey_create, which would require I come up with a whole new group law, which would require I override pubkey_parse and pubkey_serialize, and then at that point I've rewritten every function. Which is what the original code tried to do.

As for the failing doctest, there seems to be a Cargo bug where I can't propagate --cfg flags into the doc tests :( so I don't know how to fix this.

apoelstra · 2020-12-23T17:44:47Z

Got it! There is a second variable RUSTDOCFLAGS I had to set.

You should be able to run cargo test --all-features now with the fuzztarget on.

apoelstra · 2020-12-23T17:53:07Z

BTW, in getting this to work, I notice that we broke the old fuzz implemntation with #233. So this PR is release-blocking.

We can now run unit tests with the fuzz feature on, and they'll pass, which is some assurance that fuzzing with the feature on won't lead to spurious failures due to the fuzz harness inadequately simulating message signing.

TheBlueMatt · 2020-12-23T19:53:58Z

It does not implement ECDH, since I couldn't find a reasonable way to get a commutative function of (secret key, public key) that the fuzzer could plausibly fake, nor did I implement the tweak_add and tweak_add_check API for the same reason

I believe rust-lightning's fuzz stuff needs both? Maybe only the tweak stuff, though, I'd have to check.

apoelstra · 2020-12-23T20:11:31Z

It would surprise me if you were using tweak_add in any way that the fuzzer would've had difficulty with, since the original code was not consistent (if you tweaked a secret key and public key with the same tweak, the resulting secret/public keys would not match).

One interesting thing we could try would be to compile libsecp with an order-13 group (rather than the ~2^256-order group that we normally use). And then not mock any functions.

TheBlueMatt · 2020-12-23T20:18:21Z

It would surprise me if you were using tweak_add in any way that the fuzzer would've had difficulty with, since the original code was not consistent

I'd buy that it meant our fuzzers have been bad, but still need to be able to call it.

One interesting thing we could try would be to compile libsecp with an order-13 group (rather than the ~2^256-order group that we normally use). And then not mock any functions.

Hmm, this may make sense - forcing the fuzzer to guess two bytes instead of one kinda sucks, but if it means we get exact behavior maybe its still worth it.

apoelstra · 2020-12-23T20:20:08Z

You can call everything in the library now with this PR. Before you couldn't because many functions were unimplemented!().

TheBlueMatt · 2020-12-23T20:23:23Z

Oh, oops, I misread that as order-2^13, not order-13, maybe lets just do that?

apoelstra · 2020-12-23T20:25:45Z

Will need a couple hours to play with this -- our order-13 implementation apparently rejects secret keys outside of the range [1, 12] so I'm seeing if I can work around that or if I should just patch the vendored copy of rust-secp to be more forgiving :P.

apoelstra · 2020-12-23T20:36:13Z

Hmmm. So, I can definitely do the order-13 thing but it breaks a lot of our unit tests because half the valid pubkeys on secp are invalid on secp-13, and vice-versa.

I can change the overflow logic to check against the secp curve order,, or I can be more clever (and say, reject half of all keys). The former would mean fewer unit tests break, but the latter would give us better fuzz-test coverage.

No matter what, fuzz test failures are going to be weird and confusing since all of the public curvepoints will live on a different curve from secp.

apoelstra · 2020-12-23T20:41:32Z

But even with all these considerations, I think we should do it. What do you think @elichai @TheBlueMatt?

To be clear, my proposal is:

Rather than mocking functionality in the test lib, have the rust_secp_fuzz feature enable the upstream EXHAUSTIVE_TEST_ORDER=13, which switches libsecp to work on a different EC group of order 13.
Modify the overflow logic to complain about secret keys in excess of 0f00000....0000 say, rather than either 13 or the actual secp order. This means that the fuzzer will be able to easily find out-of-range keys even if it's getting them from hash functions.
Disable something like 50% of all the unit tests when the fuzz feature is enabled since almost no fixed-vector tests (for secret keys or public keys) will work anymore.

By using a group of order 13 but allowing secret keys > 13, we break some invariants ... such as there being only one secret key corresponding to each public key and the negate function on secret keys not giving you the original secret key when you run it twice.

I think we should try it, and if shit hits the fan we'll just return to the proposal from this PR (where almost all unit tests pass so we know it's being sane) in a patch release.

TheBlueMatt · 2020-12-23T20:47:50Z

This means that the fuzzer will be able to easily find out-of-range keys even if it's getting them from hash functions.

I don't think this is a good idea. Its generally not unreasonable to panic if a hash function generates an invalid secp private key, whereas suddenly it will be commonplace.

Otherwise I tend to agree.

apoelstra · 2020-12-23T20:48:51Z

Yeah, that's reasonable. I'll change it to check against the actual secp order (or at least, something so big that it's cryptographically inaccessible).

apoelstra · 2020-12-23T21:13:09Z

Added a commit which demonstrates what this'd look like. Currently the tests fail because schnorr signing fails a high proportion of the time when you run it on a compromised group (since BIP340 was defined assuming that out-of-range nonces are cryptographically inaccessible). Also I directly edited the vendored copy of libsecp rather than adding a patch to the vendor script, which I need to fix before this is mergeable.

apoelstra · 2020-12-23T22:22:22Z

Hmm, CI should not have passed there. Looks like it's not actually running the test matrix. Sigh.

apoelstra · 2020-12-26T22:57:56Z

Removed the order-13 commit. We should revisit this in the future but it'll take a fair bit more work than just enabling the order-13 group.

Let's try to get this in now so we can have a release.

elichai · 2020-12-28T16:50:04Z

secp256k1-sys/src/recovery.rs

+    ) -> c_int {
+        let sig_sl = slice::from_raw_parts(sig as *const u8, 65);
+        let msg_sl = slice::from_raw_parts(msg32 as *const u8, 32);
+            println!("HMM0");


You forgot this print here

elichai · 2020-12-28T16:51:12Z

secp256k1-sys/src/recovery.rs

+        sig_sl.swap(32, 64);
+        sig_sl[64] -= 2;


Oh you're using the pubkey parity(0x02/0x03) for a Recover parity, clever :)

elichai · 2020-12-28T16:53:38Z

secp256k1-sys/src/recovery.rs

+        let msg_sl = slice::from_raw_parts(msg32 as *const u8, 32);
+            println!("HMM0");
+
+        if sig_sl[64] > 4 {


I think that should be >= 4, because the VERIFY_CHECK makes sure that recid < 4, so it should return 0 on 4.

…em in CI

elichai

utACK

apoelstra force-pushed the 2020-12--no-extsymb2 branch 2 times, most recently from 68755f3 to 904ddad Compare December 22, 2020 22:04

apoelstra force-pushed the 2020-12--no-extsymb2 branch from 4eaf672 to 335687a Compare December 23, 2020 17:48

apoelstra added 2 commits December 23, 2020 17:59

ffi: make function types nullable

40c3134

apoelstra force-pushed the 2020-12--no-extsymb2 branch from 335687a to b1916a7 Compare December 23, 2020 18:00

apoelstra force-pushed the 2020-12--no-extsymb2 branch from c919480 to b1916a7 Compare December 26, 2020 22:56

elichai reviewed Dec 28, 2020

View reviewed changes

apoelstra added 2 commits December 28, 2020 18:42

fuzz: implement recoverable signatures, get all tests passing, run th…

96862b6

…em in CI

fuzz: disable sig-grinding tests, prevent spin-looping

d1714ce

apoelstra force-pushed the 2020-12--no-extsymb2 branch from fff07a6 to 238328d Compare December 28, 2020 18:57

fuzz: fix elichai's nits

75d7177

apoelstra force-pushed the 2020-12--no-extsymb2 branch from 238328d to 75d7177 Compare December 28, 2020 19:01

elichai approved these changes Dec 28, 2020

View reviewed changes

apoelstra merged commit 867b920 into master Dec 28, 2020

apoelstra deleted the 2020-12--no-extsymb2 branch December 28, 2020 19:48

TheBlueMatt mentioned this pull request Jan 15, 2021

#264 restircted fuzz coverage due to conversion to real private -> public key conversion #271

Closed

TheBlueMatt mentioned this pull request Feb 19, 2021

Use cfg(fuzzing) instead of a fuzztarget feature and don't allow 0-hashes. rust-bitcoin/bitcoin_hashes#111

Merged

apoelstra mentioned this pull request Sep 8, 2021

replicate fuzz rust-bitcoin/rust-miniscript#250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redo fuzz target #264

redo fuzz target #264

apoelstra commented Dec 22, 2020 •

edited

elichai commented Dec 23, 2020

elichai commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020 •

edited

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 26, 2020

elichai Dec 28, 2020

elichai Dec 28, 2020

elichai Dec 28, 2020

elichai left a comment

redo fuzz target #264

redo fuzz target #264

Conversation

apoelstra commented Dec 22, 2020 • edited

elichai commented Dec 23, 2020

elichai commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

TheBlueMatt commented Dec 23, 2020 • edited

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 23, 2020

apoelstra commented Dec 26, 2020

elichai Dec 28, 2020

Choose a reason for hiding this comment

elichai Dec 28, 2020

Choose a reason for hiding this comment

elichai Dec 28, 2020

Choose a reason for hiding this comment

elichai left a comment

Choose a reason for hiding this comment

apoelstra commented Dec 22, 2020 •

edited

TheBlueMatt commented Dec 23, 2020 •

edited