Adding random tests against a naive implementation

[elichai]

Hi,
As suggested in #635
This adds ECDSA Signing + Verifying tests against a naive rust implementation (https://github.com/elichai/ecc-secp256k1)

What I added:

• New configure flag called rust-naivetests
• Cloning and building ecc-secp256k1 from sources in the Makefile.
• Signing(ECDSA) with this library and verifying in ecc-secp256k1.
• Signing(ECDSA) with ecc-secp256k1 and verifying with this library

[real-or-random]

(co-reviewed with @jonasnick)

Approach ACK.

That's a nice start. I think this can be expanded a lot, e.g., using it for the corner case tests, (implementing deterministic verification and) comparing signatures byte-by-byte, comparing results of internal operations, etc... But all of this can be in a further PR if we agree here that we like this approach.

[real-or-random]

This needs additional rules for make clean.

[elichai]

fixed.
Do you think we should use cargo clean for cleanup or manually delete the target/ dir?

[elichai]

or should it just remove the whole ecc-secp256k1 directory?

[jonasnick]

Thank you. This could be useful - in particular if we want to fuzz test equality of signing (would require deterministic nonces in your lib) or scalar multiplication. Using your library seems fine as there are only few pure rust secp256k1 implementations (most projects seem to use rust-secp256k1 which are just bindings to C-libsecp).

[jonasnick]

Makefile needs to check that this matches a fixed hash. Otherwise malicious code can be easily added to your repo which would then be run on our machines.

nit: ./configure could check if git and cargo are available.

[elichai]

hmm so you suggest to hash the whole directory?

[elichai]

I'll look into checking that git and cargo are available, i'm pretty new to autotools so this was mostly me shooting in the dark :)

[real-or-random]

Maybe it would be simpler to just vendor the library, i.e., copy the source code into a subfolder.

[jonasnick]

Copying libraries and header files around will be confusing. Can't you avoid it by directly linking to the files in the ecc-secp256k1 dir?

[elichai]

I thought it was simpler by yeah it's possible to link directly into the files there.
about the header, don't you want it to be under source control here?

[elichai]

The main reason I wrote that library was to learn so I tried to minimize dependencies and write everything myself,
I'll try to implement hmac myself there for the deterministic nonce later this week.

And then I'll add more tests in a later PR

[elichai]

Two problems I have right now that I need to debug:

1. When I disable these tests I don't have a target for $(NAIVE_SECP) so it fails.
2. It can try to link against libecc_secp256k1.a before it finishes building it.

P.S. I need to try and play with travis see what's the best way to make it compile for C and Rust

[elichai]

Fixed almost all the tests.
What should I do with distcheck? https://travis-ci.org/elichai/secp256k1/jobs/549941744#L1126
And still not sure why the HOST are failing

[elichai]

I implemented deterministic nonce in my implementation so in a future PR i'll add a bit by bit signature comparison

[real-or-random]

distcheck: Probably depends on whether we want to vendor (copy to our tree) the files

[real-or-random]

I have no idea why configure fails on the HOST (cross-compiling) builds; this works for me locally.

One issue: Your crate has a few dependencies, so we pull all of these whenever we build the test. That's potentially dangerous.

[elichai]

I have no idea why configure fails on the HOST (cross-compiling) builds; this works for me locally.

One issue: Your crate has a few dependencies, so we pull all of these whenever we build the test. That's potentially dangerous.

Hmm, technically the dependencies get verified against the hashes in Cargo.lock.
I can work on decreasing the dependencies (right now it's just sha2 and a big integer library, so I could work on implementing those myself(actually on my TODO, and I started implementing a U256 type but it isn't as easy as I thought))

And that's kind of the situation right now generally in rust.

[elichai]

@real-or-random I removed almost all the dependencies in my library.
I currently only have https://crates.io/crates/rug.
If i'll have the time to implement the big number part myself I'll remove that too.
But even now it's just these:

$ cargo clean && cargo build
Compiling libc v0.2.58
Compiling rug v1.4.0
Compiling ecc-secp256k1 v0.2.0 (/media/hdd/CLionProjects/elichai/ecc-secp256k1)
Compiling dirs v1.0.5
Compiling gmp-mpfr-sys v1.1.13
Finished release [optimized] target(s) in 3.96s

[real-or-random]

That's super nice. I'm not saying that this is a strict requirement -- but I won't stop your motivation for removing them. :)

@sipa @gmaxwell @apoelstra
Can you comment on this approach in general to make sure @elichai is not wasting time going into the wrong direction here?

[jonasnick]

Closing this for now. Naive cross-testing of ecdsa and BIP 340 is already covered by CryptoFuzz (and Wycheproof). Also, if we add cross-testing then it would be much preferable to have a C implementation (which contradicts what I said above, sorry).