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.

Sign up for GitHub

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

fix and verify compressed argument in _eckey_pubkey_serialize calls #300

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix and verify compressed argument in _eckey_pubkey_serialize calls #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/eckey_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include "eckey.h"

#include "util.h"
#include "scalar.h"
#include "field.h"
#include "group.h"
Expand All @@ -35,6 +36,8 @@ static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char
}

static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *pub, size_t *size, int compressed) {
VERIFY_CHECK(compressed == 0 || compressed == 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we want to touch this upstream file. It's also a nice goal to keep the diff to upstream minimal. Ideally, the diff to upstream would just be added modules and changes to build system, README, etc... But yeah, we've modified upstream files in the past. If we think that this is a reasonable change, we could PR it to upstream, of course.

Same is true for the modification to secp256k1.c


if (secp256k1_ge_is_infinity(elem)) {
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion src/modules/ecdsa_adaptor/main_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ int secp256k1_ecdsa_adaptor_recover(const secp256k1_context* ctx, unsigned char
/* We declassify non-secret enckey_expected_ge to allow using it as a
* branch point. */
secp256k1_declassify(ctx, &enckey_expected_ge, sizeof(enckey_expected_ge));
if (!secp256k1_eckey_pubkey_serialize(&enckey_expected_ge, enckey_expected33, &size, SECP256K1_EC_COMPRESSED)) {
if (!secp256k1_eckey_pubkey_serialize(&enckey_expected_ge, enckey_expected33, &size, 1)) {
/* Unreachable from tests (and other VERIFY builds) and therefore this
* branch should be ignored in test coverage analysis.
*
Expand Down
2 changes: 1 addition & 1 deletion src/modules/musig/session_impl.h
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should just rebase on upstream's musig module now.

Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ int secp256k1_musig_nonce_gen(const secp256k1_context* ctx, secp256k1_musig_secn
if (!secp256k1_pubkey_load(ctx, &pk, pubkey)) {
return 0;
}
pk_serialize_success = secp256k1_eckey_pubkey_serialize(&pk, pk_ser, &pk_ser_len, SECP256K1_EC_COMPRESSED);
pk_serialize_success = secp256k1_eckey_pubkey_serialize(&pk, pk_ser, &pk_ser_len, 1);

#ifdef VERIFY
/* A pubkey cannot be the point at infinity */
Expand Down
8 changes: 4 additions & 4 deletions src/modules/whitelist/whitelist_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ static int secp256k1_whitelist_hash_pubkey(secp256k1_scalar* output, secp256k1_g
secp256k1_ge_set_gej(&ge, pubkey);

secp256k1_sha256_initialize(&sha);
if (!secp256k1_eckey_pubkey_serialize(&ge, c, &size, SECP256K1_EC_COMPRESSED)) {
if (!secp256k1_eckey_pubkey_serialize(&ge, c, &size, 1)) {
return 0;
}
secp256k1_sha256_write(&sha, c, size);
Expand Down Expand Up @@ -94,7 +94,7 @@ static int secp256k1_whitelist_compute_keys_and_message(const secp256k1_context*
secp256k1_pubkey_load(ctx, &subkey_ge, sub_pubkey);

/* commit to sub-key */
if (!secp256k1_eckey_pubkey_serialize(&subkey_ge, c, &size, SECP256K1_EC_COMPRESSED)) {
if (!secp256k1_eckey_pubkey_serialize(&subkey_ge, c, &size, 1)) {
return 0;
}
secp256k1_sha256_write(&sha, c, size);
Expand All @@ -105,12 +105,12 @@ static int secp256k1_whitelist_compute_keys_and_message(const secp256k1_context*

/* commit to fixed keys */
secp256k1_pubkey_load(ctx, &offline_ge, &offline_pubkeys[i]);
if (!secp256k1_eckey_pubkey_serialize(&offline_ge, c, &size, SECP256K1_EC_COMPRESSED)) {
if (!secp256k1_eckey_pubkey_serialize(&offline_ge, c, &size, 1)) {
return 0;
}
secp256k1_sha256_write(&sha, c, size);
secp256k1_pubkey_load(ctx, &online_ge, &online_pubkeys[i]);
if (!secp256k1_eckey_pubkey_serialize(&online_ge, c, &size, SECP256K1_EC_COMPRESSED)) {
if (!secp256k1_eckey_pubkey_serialize(&online_ge, c, &size, 1)) {
return 0;
}
secp256k1_sha256_write(&sha, c, size);
Expand Down
2 changes: 1 addition & 1 deletion src/secp256k1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *o
ARG_CHECK(pubkey != NULL);
ARG_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_COMPRESSION);
if (secp256k1_pubkey_load(ctx, &Q, pubkey)) {
ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, flags & SECP256K1_FLAGS_BIT_COMPRESSION);
ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, !!(flags & SECP256K1_FLAGS_BIT_COMPRESSION));
if (ret) {
*outputlen = len;
}
Expand Down
Loading