Skip to content

Commit

Permalink
Remove neg parameter; always check with both signs; adjust blacklist
Browse files Browse the repository at this point in the history
  • Loading branch information
@jedisct1
jedisct1 committed Oct 22, 2017
1 parent d0e009e commit afabd7e
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 28 deletions.
27 changes: 3 additions & 24 deletions src/libsodium/crypto_core/curve25519/ref10/curve25519_ref10.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2127,7 +2127,7 @@ ge_is_on_main_subgroup(const ge_p3 *p)
}

int
ge_has_small_order(const unsigned char s[32], unsigned char neg)
ge_has_small_order(const unsigned char s[32])
{
CRYPTO_ALIGN(16)
static const unsigned char blacklist[][32] = {
Expand All @@ -2149,14 +2149,6 @@ ge_has_small_order(const unsigned char s[32], unsigned char neg)
{ 0xc7, 0x17, 0x6a, 0x70, 0x3d, 0x4d, 0xd8, 0x4f, 0xba, 0x3c, 0x0b,
0x76, 0x0d, 0x10, 0x67, 0x0f, 0x2a, 0x20, 0x53, 0xfa, 0x2c, 0x39,
0xcc, 0xc6, 0x4e, 0xc7, 0xfd, 0x77, 0x92, 0xac, 0x03, 0x7a },
/* p-1 (order 2) */
{ 0x13, 0xe8, 0x95, 0x8f, 0xc2, 0xb2, 0x27, 0xb0, 0x45, 0xc3, 0xf4,
0x89, 0xf2, 0xef, 0x98, 0xf0, 0xd5, 0xdf, 0xac, 0x05, 0xd3, 0xc6,
0x33, 0x39, 0xb1, 0x38, 0x02, 0x88, 0x6d, 0x53, 0xfc, 0x85 },
/* p (order 4) */
{ 0xb4, 0x17, 0x6a, 0x70, 0x3d, 0x4d, 0xd8, 0x4f, 0xba, 0x3c, 0x0b,
0x76, 0x0d, 0x10, 0x67, 0x0f, 0x2a, 0x20, 0x53, 0xfa, 0x2c, 0x39,
0xcc, 0xc6, 0x4e, 0xc7, 0xfd, 0x77, 0x92, 0xac, 0x03, 0xfa },
/* p+1 (order 1) */
{ 0xec, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Expand All @@ -2170,30 +2162,17 @@ ge_has_small_order(const unsigned char s[32], unsigned char neg)
(order 8) */
{ 0xee, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f },
/* 2p-1 (order 2) */
{ 0xd9, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
/* 2p (order 4) */
{ 0xda, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
/* 2p+1 (order 1) */
{ 0xdb, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f }
};
size_t i, j;
unsigned char c;

neg <<= 7;
for (i = 0; i < sizeof blacklist / sizeof blacklist[0]; i++) {
c = 0;
for (j = 0; j < 31; j++) {
c |= s[j] ^ blacklist[i][j];
}
c |= s[j] ^ blacklist[i][j] ^ neg;
c |= (s[j] & 0x7f) ^ blacklist[i][j];
if (c == 0) {
return 1;
}
Expand Down
4 changes: 2 additions & 2 deletions src/libsodium/crypto_sign/ed25519/ref10/keypair.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ crypto_sign_ed25519_scalarmult(unsigned char *q, const unsigned char *n,
ge_p3 Q;
ge_p3 P;

if (ge_has_small_order(p, 1) != 0 ||
if (ge_has_small_order(p) != 0 ||
ge_frombytes_negate_vartime(&P, p) != 0 ||
ge_is_on_main_subgroup(&P) == 0) {
return -1;
Expand Down Expand Up @@ -78,7 +78,7 @@ crypto_sign_ed25519_pk_to_curve25519(unsigned char *curve25519_pk,
fe x;
fe one_minus_y;

if (ge_has_small_order(ed25519_pk, 1) != 0 ||
if (ge_has_small_order(ed25519_pk) != 0 ||
ge_frombytes_negate_vartime(&A, ed25519_pk) != 0 ||
ge_is_on_main_subgroup(&A) == 0) {
return -1;
Expand Down
2 changes: 1 addition & 1 deletion src/libsodium/crypto_sign/ed25519/ref10/open.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ _crypto_sign_ed25519_verify_detached(const unsigned char *sig,

#ifndef ED25519_COMPAT
if (sc_is_less_than_L(sig + 32) == 0 ||
ge_has_small_order(sig, 0) != 0) {
ge_has_small_order(sig) != 0) {
return -1;
}
#else
Expand Down
2 changes: 1 addition & 1 deletion src/libsodium/include/sodium/private/curve25519_ref10.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ extern void ge_scalarmult(ge_p3 *,const unsigned char *,const ge_p3 *);
extern void ge_scalarmult_vartime(ge_p3 *,const unsigned char *,const ge_p3 *);
extern int ge_is_on_curve(const ge_p3 *p);
extern int ge_is_on_main_subgroup(const ge_p3 *p);
extern int ge_has_small_order(const unsigned char s[32], unsigned char neg);
extern int ge_has_small_order(const unsigned char s[32]);

/*
The set of scalars is \Z/l
Expand Down

3 comments on commit afabd7e

@daira
Copy link

@daira daira commented on afabd7e Jan 15, 2018

Choose a reason for hiding this comment

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

This change will cause a problem for Zcash in upgrading to libsodium 1.0.16 (we're currently on 1.0.15). Any change in which signatures are considered valid can potentially be used to provoke a Zcash chain fork. We can work around it by reverting this patch, but that's obviously not ideal for maintenance. Is it possible to have a LIBSODIUM_STABLE_VALIDATION #define that would disable any changes like this?

@daira
Copy link

@daira daira commented on afabd7e Jan 15, 2018

Choose a reason for hiding this comment

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

Filed #662, since this is probably easier to keep track of as an issue.

@daira
Copy link

@daira daira commented on afabd7e Jan 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This turns out not to affect which points are valid. (The motivation is to avoid certain side channel attacks, for which it matters that low-order points are rejected before the scalar multiplication. They were rejected afterward in any case.)

Please sign in to comment.