Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding random tests against a naive implementation #641

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@ build-aux/compile
build-aux/test-driver
src/stamp-h1
libsecp256k1.pc
ecc-secp256k1
elichai marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 11 additions & 1 deletion Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,9 @@ gen_%.o: src/gen_%.c
$(gen_context_BIN): $(gen_context_OBJECTS)
$(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) $(LDFLAGS_FOR_BUILD) $^ -o $@

ECC_SECP=libecc_secp256k1.a
elichai marked this conversation as resolved.
Show resolved Hide resolved
$(libsecp256k1_la_OBJECTS): src/ecmult_static_context.h
$(tests_OBJECTS): src/ecmult_static_context.h
$(tests_OBJECTS): src/ecmult_static_context.h $(ECC_SECP)
$(bench_internal_OBJECTS): src/ecmult_static_context.h
$(bench_ecmult_OBJECTS): src/ecmult_static_context.h

Expand All @@ -181,3 +182,12 @@ endif
if ENABLE_MODULE_RECOVERY
include src/modules/recovery/Makefile.am.include
endif


if ENABLE_RUST_NAIVETESTS
$(ECC_SECP):
git clone https://github.com/elichai/ecc-secp256k1.git -b v0.1.0 2> /dev/null | true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm so you suggest to hash the whole directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cd ecc-secp256k1 && cargo build --release --features=ffi
cp ./ecc-secp256k1/target/release/libecc_secp256k1.a .
cp ./ecc-secp256k1/ecc_secp256k1.h ./src/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

endif
elichai marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs additional rules for make clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

13 changes: 13 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ AC_ARG_ENABLE(openssl_tests,
[enable_openssl_tests=$enableval],
[enable_openssl_tests=auto])

AC_ARG_ENABLE(rust_naivetests,
AS_HELP_STRING([--enable-rust-naivetests],[enable tests against naive rust implementation [default=auto]]),
[enable_rust_naivetests=$enableval],
[enable_rust_naivetests=auto])

AC_ARG_ENABLE(experimental,
AS_HELP_STRING([--enable-experimental],[allow experimental configure options [default=no]]),
[use_experimental=$enableval],
Expand Down Expand Up @@ -442,10 +447,17 @@ if test x"$use_tests" = x"yes"; then
AC_MSG_ERROR([OpenSSL tests requested but OpenSSL with EC support is not available])
fi
fi
if test x"$enable_rust_naivetests" != x"no"; then
AC_DEFINE(ENABLE_RUST_NAIVETESTS, 1, [Define this symbol if you want to build the rust tests])
SECP_TEST_LIBS="$SECP_TEST_LIBS -ldl -lpthread -lm -lecc_secp256k1 -L."
fi
else
if test x"$enable_openssl_tests" = x"yes"; then
AC_MSG_ERROR([OpenSSL tests requested but tests are not enabled])
fi
if test x"$enable_rust_naivetests" = x"yes"; then
AC_MSG_ERROR([Rust naiveTests requested but tests are not enabled])
fi
fi

if test x"$use_jni" != x"no"; then
Expand Down Expand Up @@ -534,6 +546,7 @@ AM_CONDITIONAL([ENABLE_MODULE_RECOVERY], [test x"$enable_module_recovery" = x"ye
AM_CONDITIONAL([USE_JNI], [test x"$use_jni" = x"yes"])
AM_CONDITIONAL([USE_EXTERNAL_ASM], [test x"$use_external_asm" = x"yes"])
AM_CONDITIONAL([USE_ASM_ARM], [test x"$set_asm" = x"arm"])
AM_CONDITIONAL([ENABLE_RUST_NAIVETESTS], [test x"$enable_rust_naivetests" != x"no"])

dnl make sure nothing new is exported so that we don't break the cache
PKGCONFIG_PATH_TEMP="$PKG_CONFIG_PATH"
Expand Down
61 changes: 61 additions & 0 deletions src/ecc_secp256k1.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* Sign an ECDSA Signature
* The message should be a hashed 32 bytes.
* Input: msg -> pointer to 32 bytes message.
* privkey -> pointer to 32 bytes private key.
* Output: sig_out -> pointer to a 64 bytes buffer.
* Returns:
* 1 - Finished successfully.
* 0 - Failed.
*/
int ecc_secp256k1_ecdsa_sign(unsigned char *sig_out,
const unsigned char *msg,
const unsigned char *privkey);

/**
* Verify a ECDSA Signature
* Accepts either compressed(33 btes) or uncompressed(65 bytes) public key. using the flag (1==compressed, 0==uncompressed).
* Input: sig -> pointer to 64 bytes signature.
* msg -> 32 bytes result of a hash. (***Make Sure you hash the message yourself! otherwise it's easily broken***)
* pubkey -> pointer to 33 or 65 bytes pubkey depending on the compressed flag.
* compressed -> 1 for compressed, 0 for uncompressed.
* Returns:
* 1 - The signature is valid.
* 0 - Signature is not valid.
* -1 - Some other problem.
*/
int ecc_secp256k1_ecdsa_verify(const unsigned char *sig,
const unsigned char *msg,
const unsigned char *pubkey,
int compressed);

/**
* Sign a Schnorr Signature
* The message should be a hashed 32 bytes.
* Input: msg -> pointer to 32 bytes message.
* privkey -> pointer to 32 bytes private key.
* Output: sig_out -> pointer to a 64 bytes buffer.
* Returns:
* 1 - Finished successfully.
* 0 - Failed.
*/
int ecc_secp256k1_schnorr_sign(unsigned char *sig_out,
const unsigned char *msg,
const unsigned char *privkey);

/**
* Verify a Schnorr Signature
* Accepts either compressed(33 btes) or uncompressed(65 bytes) public key. using the flag (1==compressed, 0==uncompressed).
* Input: sig -> pointer to 64 bytes signature.
* msg -> 32 bytes result of a hash. (***Make Sure you hash the message yourself! otherwise it's easily broken***)
* pubkey -> pointer to 33 or 65 bytes pubkey depending on the compressed flag.
* compressed -> 1 for compressed, 0 for uncompressed.
* Returns:
* 1 - The signature is valid.
* 0 - Signature is not valid.
* -1 - Some other problem.
*/
int ecc_secp256k1_schnorr_verify(const unsigned char *sig,
const unsigned char *msg,
const unsigned char *pubkey,
int compressed);
38 changes: 38 additions & 0 deletions src/tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "include/secp256k1.h"
#include "include/secp256k1_preallocated.h"
#include "testrand_impl.h"
#include "ecc_secp256k1.h"

#ifdef ENABLE_OPENSSL_TESTS
#include "openssl/bn.h"
Expand Down Expand Up @@ -4134,11 +4135,48 @@ void test_ecdsa_sign_verify(void) {
CHECK(!secp256k1_ecdsa_sig_verify(&ctx->ecmult_ctx, &sigr, &sigs, &pub, &msg));
}

void test_ecdsa_sign_verify_rust(void) {

secp256k1_scalar msg, key;
unsigned char raw_key[32];
unsigned char raw_msg[32];
unsigned char raw_pubkey[65];
unsigned char raw_sig[64];
size_t pubkey_len = sizeof(raw_pubkey);
secp256k1_pubkey pubkey;
secp256k1_ecdsa_signature sig;

random_scalar_order_test(&msg);
random_scalar_order_test(&key);

secp256k1_scalar_get_b32(raw_msg, &msg);
secp256k1_scalar_get_b32(raw_key, &key);

CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, raw_key) == 1);
secp256k1_ec_pubkey_serialize(ctx, raw_pubkey, &pubkey_len, &pubkey, SECP256K1_EC_UNCOMPRESSED);
/* Sign with secp256k1 */
CHECK(secp256k1_ecdsa_sign(ctx, &sig, raw_msg, raw_key, NULL, NULL) == 1);
CHECK(secp256k1_ecdsa_verify(ctx, &sig, raw_msg, &pubkey) == 1);
secp256k1_ecdsa_signature_serialize_compact(ctx, raw_sig, &sig);
/* Verify with ecc-secp256k1(rust) */
CHECK(ecc_secp256k1_ecdsa_verify(raw_sig, raw_msg, raw_pubkey, 0) == 1);
/* Sign with ecc-secp256k1(rust) */
CHECK(ecc_secp256k1_ecdsa_sign(raw_sig, raw_msg, raw_key) == 1);
/* Verify with secp256k1 */
CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, &sig, raw_sig) == 1);
CHECK(secp256k1_ecdsa_verify(ctx, &sig, raw_msg, &pubkey) == 1);
}


void run_ecdsa_sign_verify(void) {
int i;
for (i = 0; i < 10*count; i++) {
test_ecdsa_sign_verify();
}

for (i = 0; i < 10*count; i++) {
test_ecdsa_sign_verify_rust();
}
}

/** Dummy nonce generation function that just uses a precomputed nonce, and fails if it is not accepted. Use only for testing. */
Expand Down