From 68b20c00659c16b65ae2fcfc120e1407a68de04f Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 12 Feb 2018 14:27:02 +0000 Subject: [PATCH] More style fixes to Curve448 code based on review feedback Reviewed-by: Bernd Edlinger (Merged from https://github.com/openssl/openssl/pull/5105) --- crypto/ec/curve448/arch_32/arch_intrinsics.h | 8 +-- crypto/ec/curve448/arch_32/f_impl.h | 14 +++-- crypto/ec/curve448/curve448.c | 61 ++++++++++---------- crypto/ec/curve448/curve448_lcl.h | 6 +- crypto/ec/curve448/curve448utils.h | 8 +-- crypto/ec/curve448/ed448.h | 9 ++- crypto/ec/curve448/eddsa.c | 21 +++---- crypto/ec/curve448/f_generic.c | 3 + crypto/ec/curve448/field.h | 8 +-- crypto/ec/curve448/point_448.h | 6 +- crypto/ec/curve448/word.h | 6 +- test/curve448_internal_test.c | 4 +- 12 files changed, 81 insertions(+), 73 deletions(-) diff --git a/crypto/ec/curve448/arch_32/arch_intrinsics.h b/crypto/ec/curve448/arch_32/arch_intrinsics.h index 1f5d2d7751..48081c7717 100644 --- a/crypto/ec/curve448/arch_32/arch_intrinsics.h +++ b/crypto/ec/curve448/arch_32/arch_intrinsics.h @@ -10,10 +10,10 @@ * Originally written by Mike Hamburg */ -#include "internal/constant_time_locl.h" +#ifndef HEADER_ARCH_32_ARCH_INTRINSICS_H +# define HEADER_ARCH_32_ARCH_INTRINSICS_H -#ifndef __ARCH_ARCH_32_ARCH_INTRINSICS_H__ -# define __ARCH_ARCH_32_ARCH_INTRINSICS_H__ +#include "internal/constant_time_locl.h" # define ARCH_WORD_BITS 32 @@ -24,4 +24,4 @@ static ossl_inline uint64_t widemul(uint32_t a, uint32_t b) return ((uint64_t)a) * b; } -#endif /* __ARCH_ARCH_32_ARCH_INTRINSICS_H__ */ +#endif /* HEADER_ARCH_32_ARCH_INTRINSICS_H */ diff --git a/crypto/ec/curve448/arch_32/f_impl.h b/crypto/ec/curve448/arch_32/f_impl.h index 7c87e972e2..217f9947e9 100644 --- a/crypto/ec/curve448/arch_32/f_impl.h +++ b/crypto/ec/curve448/arch_32/f_impl.h @@ -9,12 +9,16 @@ * * Originally written by Mike Hamburg */ -#define GF_HEADROOM 2 -#define LIMB(x) (x)&((1<<28)-1), (x)>>28 -#define FIELD_LITERAL(a,b,c,d,e,f,g,h) \ + +#ifndef HEADER_ARCH_32_F_IMPL_H +# define HEADER_ARCH_32_F_IMPL_H + +# define GF_HEADROOM 2 +# define LIMB(x) (x)&((1<<28)-1), (x)>>28 +# define FIELD_LITERAL(a,b,c,d,e,f,g,h) \ {{LIMB(a),LIMB(b),LIMB(c),LIMB(d),LIMB(e),LIMB(f),LIMB(g),LIMB(h)}} -#define LIMB_PLACE_VALUE(i) 28 +# define LIMB_PLACE_VALUE(i) 28 void gf_add_RAW(gf out, const gf a, const gf b) { @@ -54,3 +58,5 @@ void gf_weak_reduce(gf a) a->limb[i] = (a->limb[i] & mask) + (a->limb[i - 1] >> 28); a->limb[0] = (a->limb[0] & mask) + tmp; } + +#endif /* HEADER_ARCH_32_F_IMPL_H */ diff --git a/crypto/ec/curve448/curve448.c b/crypto/ec/curve448/curve448.c index bfeaf51bd4..eb8752db38 100644 --- a/crypto/ec/curve448/curve448.c +++ b/crypto/ec/curve448/curve448.c @@ -167,6 +167,7 @@ static void sub_niels_from_pt(curve448_point_t d, const niels_t e, int before_double) { gf a, b, c; + gf_sub_nr(b, d->y, d->x); /* 3+e */ gf_mul(a, e->b, b); gf_add_nr(b, d->x, d->y); /* 2+e */ @@ -207,9 +208,9 @@ c448_bool_t curve448_point_eq(const curve448_point_t p, const curve448_point_t q) { mask_t succ; + gf a, b; /* equality mod 2-torsion compares x/y */ - gf a, b; gf_mul(a, p->y, q->x); gf_mul(b, q->y, p->x); succ = gf_eq(a, b); @@ -220,8 +221,8 @@ c448_bool_t curve448_point_eq(const curve448_point_t p, c448_bool_t curve448_point_valid(const curve448_point_t p) { mask_t out; - gf a, b, c; + gf_mul(a, p->x, p->y); gf_mul(b, p->z, p->t); out = gf_eq(a, b); @@ -248,17 +249,16 @@ void curve448_precomputed_scalarmul(curve448_point_t out, const curve448_precomputed_s * table, const curve448_scalar_t scalar) { - int i; - unsigned j, k; + unsigned int i, j, k; const unsigned int n = COMBS_N, t = COMBS_T, s = COMBS_S; niels_t ni; - curve448_scalar_t scalar1x; + curve448_scalar_add(scalar1x, scalar, precomputed_scalarmul_adjustment); curve448_scalar_halve(scalar1x, scalar1x); - for (i = s - 1; i >= 0; i--) { - if (i != (int)s - 1) + for (i = s; i > 0; i--) { + if (i != s) point_double_internal(out, out, 0); for (j = 0; j < n; j++) { @@ -266,7 +266,8 @@ void curve448_precomputed_scalarmul(curve448_point_t out, mask_t invert; for (k = 0; k < t; k++) { - unsigned int bit = i + s * (k + j * t); + unsigned int bit = (i - 1) + s * (k + j * t); + if (bit < C448_SCALAR_BITS) { tab |= (scalar1x->limb[bit / WBITS] >> (bit % WBITS) & 1) << k; @@ -281,8 +282,8 @@ void curve448_precomputed_scalarmul(curve448_point_t out, 1 << (t - 1), tab); cond_neg_niels(ni, invert); - if ((i != (int)s - 1) || j) { - add_niels_to_pt(out, ni, j == n - 1 && i); + if ((i != s) || j != 0) { + add_niels_to_pt(out, ni, j == n - 1 && i != 1); } else { niels_to_pt(out, ni); } @@ -297,10 +298,10 @@ void curve448_point_mul_by_ratio_and_encode_like_eddsa( uint8_t enc[EDDSA_448_PUBLIC_BYTES], const curve448_point_t p) { - - /* The point is now on the twisted curve. Move it to untwisted. */ gf x, y, z, t; curve448_point_t q; + + /* The point is now on the twisted curve. Move it to untwisted. */ curve448_point_copy(q, p); { @@ -354,9 +355,7 @@ c448_error_t curve448_point_decode_like_eddsa_and_mul_by_ratio( enc2[EDDSA_448_PRIVATE_BYTES - 1] &= ~0x80; succ = gf_deserialize(p->y, enc2, 1, 0); -#if 0 == 0 succ &= word_is_zero(enc2[EDDSA_448_PRIVATE_BYTES - 1]); -#endif gf_sqr(p->x, p->y); gf_sub(p->z, ONE, p->x); /* num = 1-y^2 */ @@ -371,8 +370,9 @@ c448_error_t curve448_point_decode_like_eddsa_and_mul_by_ratio( gf_copy(p->z, ONE); { - /* 4-isogeny 2xy/(y^2-ax^2), (y^2+ax^2)/(2-y^2-ax^2) */ gf a, b, c, d; + + /* 4-isogeny 2xy/(y^2-ax^2), (y^2+ax^2)/(2-y^2-ax^2) */ gf_sqr(c, p->x); gf_sqr(a, p->y); gf_add(d, c, a); @@ -478,6 +478,7 @@ void curve448_point_mul_by_ratio_and_encode_like_x448(uint8_t const curve448_point_t p) { curve448_point_t q; + curve448_point_copy(q, p); gf_invert(q->t, q->x, 0); /* 1/x */ gf_mul(q->z, q->t, q->y); /* y/x */ @@ -523,7 +524,7 @@ struct smvt_control { # define NUMTRAILINGZEROS numtrailingzeros static uint32_t numtrailingzeros(uint32_t i) { - unsigned int tmp; + uint32_t tmp; uint32_t num = 31; if (i == 0) @@ -549,7 +550,8 @@ static uint32_t numtrailingzeros(uint32_t i) i = tmp; num -= 2; } - if ((i << 1) != 0) + tmp = i << 1; + if (tmp != 0) num--; return num; @@ -593,7 +595,7 @@ static int recode_wnaf(struct smvt_control *control, int32_t delta = odd & mask; assert(position >= 0); - if (odd & 1 << (table_bits + 1)) + if (odd & (1 << (table_bits + 1))) delta -= (1 << (table_bits + 1)); current -= delta << pos; control[position].power = pos + 16 * (w - 1); @@ -606,9 +608,9 @@ static int recode_wnaf(struct smvt_control *control, position++; n = table_size - position; - for (i = 0; i < n; i++) { + for (i = 0; i < n; i++) control[i] = control[i + position]; - } + return n - 1; } @@ -649,8 +651,8 @@ void curve448_base_double_scalarmul_non_secret(curve448_point_t combo, const curve448_point_t base2, const curve448_scalar_t scalar2) { - const int table_bits_var = C448_WNAF_VAR_TABLE_BITS, - table_bits_pre = C448_WNAF_FIXED_TABLE_BITS; + const int table_bits_var = C448_WNAF_VAR_TABLE_BITS; + const int table_bits_pre = C448_WNAF_FIXED_TABLE_BITS; struct smvt_control control_var[C448_SCALAR_BITS / (C448_WNAF_VAR_TABLE_BITS + 1) + 3]; struct smvt_control control_pre[C448_SCALAR_BITS / @@ -682,37 +684,36 @@ void curve448_base_double_scalarmul_non_secret(curve448_point_t combo, } for (i--; i >= 0; i--) { - int cv = (i == control_var[contv].power), cp = - (i == control_pre[contp].power); + int cv = (i == control_var[contv].power); + int cp = (i == control_pre[contp].power); + point_double_internal(combo, combo, i && !(cv || cp)); if (cv) { assert(control_var[contv].addend); - if (control_var[contv].addend > 0) { + if (control_var[contv].addend > 0) add_pniels_to_pt(combo, precmp_var[control_var[contv].addend >> 1], i && !cp); - } else { + else sub_pniels_from_pt(combo, precmp_var[(-control_var[contv].addend) >> 1], i && !cp); - } contv++; } if (cp) { assert(control_pre[contp].addend); - if (control_pre[contp].addend > 0) { + if (control_pre[contp].addend > 0) add_niels_to_pt(combo, curve448_wnaf_base[control_pre[contp].addend >> 1], i); - } else { + else sub_niels_from_pt(combo, curve448_wnaf_base[(-control_pre [contp].addend) >> 1], i); - } contp++; } } diff --git a/crypto/ec/curve448/curve448_lcl.h b/crypto/ec/curve448/curve448_lcl.h index 8513d93a2d..2bc3bd84c8 100644 --- a/crypto/ec/curve448/curve448_lcl.h +++ b/crypto/ec/curve448/curve448_lcl.h @@ -6,7 +6,9 @@ * in the file LICENSE in the source distribution or at * https://www.openssl.org/source/license.html */ -#include "curve448utils.h" +#ifndef HEADER_CURVE448_LCL_H +# define HEADER_CURVE448_LCL_H +# include "curve448utils.h" int X448(uint8_t out_shared_key[56], const uint8_t private_key[56], const uint8_t peer_public_value[56]); @@ -32,3 +34,5 @@ int ED448ph_verify(const uint8_t hash[64], const uint8_t signature[114], int ED448_public_from_private(uint8_t out_public_key[57], const uint8_t private_key[57]); + +#endif /* HEADER_CURVE448_LCL_H */ diff --git a/crypto/ec/curve448/curve448utils.h b/crypto/ec/curve448/curve448utils.h index 2243889594..95b8c26da8 100644 --- a/crypto/ec/curve448/curve448utils.h +++ b/crypto/ec/curve448/curve448utils.h @@ -10,8 +10,8 @@ * Originally written by Mike Hamburg */ -#ifndef __C448_COMMON_H__ -# define __C448_COMMON_H__ 1 +#ifndef HEADER_CURVE448UTILS_H +# define HEADER_CURVE448UTILS_H # include @@ -58,10 +58,10 @@ typedef int64_t c448_dsword_t; # endif /* C448_TRUE = -1 so that C448_TRUE & x = x */ -static const c448_bool_t C448_TRUE = 0 - (c448_bool_t)1; +# define C448_TRUE (0 - (c448_bool_t)1) /* C448_FALSE = 0 so that C448_FALSE & x = 0 */ -static const c448_bool_t C448_FALSE = 0; +# define C448_FALSE 0 /* Another boolean type used to indicate success or failure. */ typedef enum { diff --git a/crypto/ec/curve448/ed448.h b/crypto/ec/curve448/ed448.h index eec66296fe..87c1aed1eb 100644 --- a/crypto/ec/curve448/ed448.h +++ b/crypto/ec/curve448/ed448.h @@ -10,8 +10,8 @@ * Originally written by Mike Hamburg */ -#ifndef __C448_ED448_H__ -# define __C448_ED448_H__ 1 +#ifndef HEADER_ED448_H +# define HEADER_ED448_H # include "point_448.h" @@ -108,8 +108,7 @@ c448_error_t c448_ed448_sign_prehash( * * For Ed25519, it is unsafe to use the same key for both prehashed and * non-prehashed messages, at least without some very careful protocol-level - * disambiguation. For Ed448 it is safe. The C++ wrapper is designed to make - * it harder to screw this up, but this C code gives you no seat belt. + * disambiguation. For Ed448 it is safe. */ c448_error_t c448_ed448_verify(const uint8_t signature[EDDSA_448_SIGNATURE_BYTES], @@ -196,4 +195,4 @@ c448_error_t c448_ed448_convert_private_key_to_x448( uint8_t x[X448_PRIVATE_BYTES], const uint8_t ed[EDDSA_448_PRIVATE_BYTES]); -#endif /* __C448_ED448_H__ */ +#endif /* HEADER_ED448_H */ diff --git a/crypto/ec/curve448/eddsa.c b/crypto/ec/curve448/eddsa.c index aac8a9e728..556edf07b9 100644 --- a/crypto/ec/curve448/eddsa.c +++ b/crypto/ec/curve448/eddsa.c @@ -169,7 +169,7 @@ c448_error_t c448_ed448_sign( expanded + EDDSA_448_PRIVATE_BYTES, EDDSA_448_PRIVATE_BYTES) || !EVP_DigestUpdate(hashctx, message, message_len)) { - OPENSSL_cleanse(expanded, sizeof(expanded)); + OPENSSL_cleanse(expanded, sizeof(expanded)); goto err; } OPENSSL_cleanse(expanded, sizeof(expanded)); @@ -191,9 +191,8 @@ c448_error_t c448_ed448_sign( curve448_point_t p; curve448_scalar_halve(nonce_scalar_2, nonce_scalar); - for (c = 2; c < C448_EDDSA_ENCODE_RATIO; c <<= 1) { + for (c = 2; c < C448_EDDSA_ENCODE_RATIO; c <<= 1) curve448_scalar_halve(nonce_scalar_2, nonce_scalar_2); - } curve448_precomputed_scalarmul(p, curve448_precomputed_base, nonce_scalar_2); @@ -207,10 +206,10 @@ c448_error_t c448_ed448_sign( /* Compute the challenge */ if (!hash_init_with_dom(hashctx, prehashed, 0, context, context_len) - || !EVP_DigestUpdate(hashctx, nonce_point, sizeof(nonce_point)) - || !EVP_DigestUpdate(hashctx, pubkey, EDDSA_448_PUBLIC_BYTES) - || !EVP_DigestUpdate(hashctx, message, message_len) - || !EVP_DigestFinalXOF(hashctx, challenge, sizeof(challenge))) + || !EVP_DigestUpdate(hashctx, nonce_point, sizeof(nonce_point)) + || !EVP_DigestUpdate(hashctx, pubkey, EDDSA_448_PUBLIC_BYTES) + || !EVP_DigestUpdate(hashctx, message, message_len) + || !EVP_DigestFinalXOF(hashctx, challenge, sizeof(challenge))) goto err; curve448_scalar_decode_long(challenge_scalar, challenge, @@ -313,12 +312,8 @@ c448_error_t c448_ed448_verify_prehash( const uint8_t hash[64], const uint8_t *context, uint8_t context_len) { - c448_error_t ret; - - ret = c448_ed448_verify(signature, pubkey, hash, 64, 1, context, - context_len); - - return ret; + return c448_ed448_verify(signature, pubkey, hash, 64, 1, context, + context_len); } int ED448_sign(uint8_t *out_sig, const uint8_t *message, size_t message_len, diff --git a/crypto/ec/curve448/f_generic.c b/crypto/ec/curve448/f_generic.c index 81c75761a8..341eb3f3b0 100644 --- a/crypto/ec/curve448/f_generic.c +++ b/crypto/ec/curve448/f_generic.c @@ -46,6 +46,7 @@ void gf_serialize(uint8_t serial[SER_BYTES], const gf x, int with_hibit) mask_t gf_hibit(const gf x) { gf y; + gf_add(y, x, x); gf_strong_reduce(y); return 0 - (y->limb[0] & 1); @@ -55,6 +56,7 @@ mask_t gf_hibit(const gf x) mask_t gf_lobit(const gf x) { gf y; + gf_copy(y, x); gf_strong_reduce(y); return 0 - (y->limb[0] & 1); @@ -168,6 +170,7 @@ mask_t gf_eq(const gf a, const gf b) mask_t gf_isr(gf a, const gf x) { gf L0, L1, L2; + gf_sqr(L1, x); gf_mul(L2, x, L1); gf_sqr(L1, L2); diff --git a/crypto/ec/curve448/field.h b/crypto/ec/curve448/field.h index 5bc16bc2be..18d91f3600 100644 --- a/crypto/ec/curve448/field.h +++ b/crypto/ec/curve448/field.h @@ -10,8 +10,8 @@ * Originally written by Mike Hamburg */ -#ifndef __GF_H__ -# define __GF_H__ +#ifndef HEADER_FIELD_H +# define HEADER_FIELD_H # include "internal/constant_time_locl.h" # include @@ -23,7 +23,7 @@ # define SER_BYTES 56 # if defined(__GNUC__) || defined(__clang__) -# define INLINE_UNUSED __inline__ __attribute__((unused,always_inline)) +# define INLINE_UNUSED __inline__ __attribute__((__unused__,__always_inline__)) # define RESTRICT __restrict__ # define ALIGNED __attribute__((aligned(32))) # else @@ -169,4 +169,4 @@ static ossl_inline void gf_cond_swap(gf x, gf_s * RESTRICT y, mask_t swap) } } -#endif /* __GF_H__ */ +#endif /* HEADER_FIELD_H */ diff --git a/crypto/ec/curve448/point_448.h b/crypto/ec/curve448/point_448.h index 90a7c3ba05..33e4001020 100644 --- a/crypto/ec/curve448/point_448.h +++ b/crypto/ec/curve448/point_448.h @@ -10,8 +10,8 @@ * Originally written by Mike Hamburg */ -#ifndef __C448_POINT_448_H__ -# define __C448_POINT_448_H__ 1 +#ifndef HEADER_POINT_448_H +# define HEADER_POINT_448_H # include "curve448utils.h" # include "field.h" @@ -280,4 +280,4 @@ void curve448_scalar_destroy(curve448_scalar_t scalar); /* Overwrite point with zeros. */ void curve448_point_destroy(curve448_point_t point); -#endif /* __C448_POINT_448_H__ */ +#endif /* HEADER_POINT_448_H */ diff --git a/crypto/ec/curve448/word.h b/crypto/ec/curve448/word.h index a180850df8..ff85c4c458 100644 --- a/crypto/ec/curve448/word.h +++ b/crypto/ec/curve448/word.h @@ -10,8 +10,8 @@ * Originally written by Mike Hamburg */ -#ifndef __WORD_H__ -# define __WORD_H__ +#ifndef HEADER_WORD_H +# define HEADER_WORD_H # include @@ -168,4 +168,4 @@ static ossl_inline void ignore_result(c448_bool_t boo) (void)boo; } -#endif /* __WORD_H__ */ +#endif /* HEADER_WORD_H */ diff --git a/test/curve448_internal_test.c b/test/curve448_internal_test.c index f01d81bbd1..aac4818965 100644 --- a/test/curve448_internal_test.c +++ b/test/curve448_internal_test.c @@ -587,8 +587,8 @@ static const uint8_t *dohash(EVP_MD_CTX *hashctx, const uint8_t *msg, static uint8_t hashout[64]; if (!EVP_DigestInit_ex(hashctx, EVP_shake256(), NULL) - || !EVP_DigestUpdate(hashctx, msg, msglen) - || !EVP_DigestFinalXOF(hashctx, hashout, sizeof(hashout))) + || !EVP_DigestUpdate(hashctx, msg, msglen) + || !EVP_DigestFinalXOF(hashctx, hashout, sizeof(hashout))) return NULL; return hashout; -- 2.25.1