Adding random tests against a naive implementation

elichai commented at 1:57 am on June 24, 2019: contributor

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

Started adding ecc-secp256k1 for cross testing 993637e67e

Added ecc-secp25k61 rust naive implementation to the Makefile 3e09aedb89

Updated the ecc_secp256k1 headers 6cca82441c

Updated the rust ecc-secp256k1 tests and added signing tests c69d5ee60b

in .gitignore:51 in c69d5ee60b outdated

47@@ -48,3 +48,4 @@ build-aux/compile
48 build-aux/test-driver
49 src/stamp-h1
50 libsecp256k1.pc
51+ecc-secp256k1

real-or-random commented at 8:24 am on June 24, 2019:

nit: GitHub says “No newline at end of file”

elichai commented at 2:00 pm on June 24, 2019:

Didn’t know newline at the end is the standard. fixed.

in Makefile.am:164 in c69d5ee60b outdated

160@@ -161,8 +161,9 @@ gen_%.o: src/gen_%.c
161 $(gen_context_BIN): $(gen_context_OBJECTS)
162 	$(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) $^ -o $@
163 
164+ECC_SECP=libecc_secp256k1.a

real-or-random commented at 8:28 am on June 24, 2019:

nit: ECC does not say much, easily confused. Maybe NAIVE or similar?

elichai commented at 2:00 pm on June 24, 2019:

fixed.

in Makefile.am:193 in c69d5ee60b outdated

188+$(ECC_SECP):
189+	git clone https://github.com/elichai/ecc-secp256k1.git -b v0.1.0 2> /dev/null | true
190+	cd ecc-secp256k1 && cargo build --release --features=ffi
191+	cp ./ecc-secp256k1/target/release/libecc_secp256k1.a  .
192+	cp ./ecc-secp256k1/ecc_secp256k1.h  ./src/
193+endif

real-or-random commented at 8:28 am on June 24, 2019:

newline at end of file

real-or-random commented at 8:29 am on June 24, 2019:

This needs additional rules for make clean.

elichai commented at 2:00 pm on June 24, 2019:

fixed.

elichai commented at 2:01 pm on June 24, 2019:

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

elichai commented at 2:02 pm on June 24, 2019:

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

in Makefile.am:189 in c69d5ee60b outdated

181@@ -181,3 +182,12 @@ endif
182 if ENABLE_MODULE_RECOVERY
183 include src/modules/recovery/Makefile.am.include
184 endif
185+
186+
187+if ENABLE_RUST_NAIVETESTS
188+$(ECC_SECP):
189+	git clone https://github.com/elichai/ecc-secp256k1.git -b v0.1.0 2> /dev/null | true

jonasnick commented at 8:29 am on June 24, 2019:

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 commented at 1:09 pm on June 24, 2019:

hmm so you suggest to hash the whole directory?

elichai commented at 1:10 pm on June 24, 2019:

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 commented at 1:21 pm on June 24, 2019:

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

in Makefile.am:192 in c69d5ee60b outdated

187+if ENABLE_RUST_NAIVETESTS
188+$(ECC_SECP):
189+	git clone https://github.com/elichai/ecc-secp256k1.git -b v0.1.0 2> /dev/null | true
190+	cd ecc-secp256k1 && cargo build --release --features=ffi
191+	cp ./ecc-secp256k1/target/release/libecc_secp256k1.a  .
192+	cp ./ecc-secp256k1/ecc_secp256k1.h  ./src/

jonasnick commented at 8:32 am on June 24, 2019:

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 commented at 1:08 pm on June 24, 2019:

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?

real-or-random commented at 8:36 am on June 24, 2019: contributor

(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.

jonasnick commented at 8:48 am on June 24, 2019: contributor

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).

elichai commented at 1:15 pm on June 24, 2019: contributor

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

Added cleanup and ifdefs for naive tests 44fd9c67da

elichai commented at 2:07 pm on June 24, 2019: contributor

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

When I disable these tests I don’t have a target for $(NAIVE_SECP) so it fails.
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

Added rust to travis and fixed the makefile b0c6f80aad

elichai commented at 9:35 pm on June 24, 2019: contributor

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 commented at 1:50 pm on June 25, 2019: contributor

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

real-or-random commented at 2:35 pm on June 25, 2019: contributor

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

real-or-random commented at 2:42 pm on June 25, 2019: contributor

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.

Added sha2 verification for ecc-secp256k1 rust lib 542840742c

elichai commented at 2:50 pm on June 25, 2019: contributor

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 commented at 3:29 pm on June 26, 2019: contributor

@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:

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

real-or-random commented at 5:30 pm on June 26, 2019: contributor

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?

jnewbery cross-referenced this on Jul 12, 2019 from issue Future PRs by jnewbery

elichai cross-referenced this on Nov 9, 2019 from issue cross-testing by real-or-random

elichai cross-referenced this on Apr 12, 2020 from issue Replace OpenSSL in cross-testing with micro-ecc(uECC) by elichai

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

BerryYoghurt cross-referenced this on May 16, 2022 from issue Cross testing against a naïve implementation by BerryYoghurt

jonasnick commented at 10:50 am on May 8, 2023: contributor

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).

jonasnick closed this on May 8, 2023

Adding random tests against a naive implementation #641