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

Use s2n-bignum P-256 scalar multiplication and Montgomery inverse #1877

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,9 @@ if(BORINGSSL_PREFIX AND BORINGSSL_PREFIX_SYMBOLS AND GO_EXECUTABLE)
COMMAND sed -i.bak '/ edwards25519_/d' ${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols.h
COMMAND sed -i.bak '/ edwards25519_/d' ${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols_asm.h
COMMAND sed -i.bak '/ edwards25519_/d' ${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols_nasm.inc
COMMAND sed -i.bak '/ p256_montjscalarmul/d' ${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols.h
COMMAND sed -i.bak '/ p256_montjscalarmul/d' ${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols_asm.h
COMMAND sed -i.bak '/ p256_montjscalarmul/d' ${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols_nasm.inc
Comment on lines +226 to +228
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this is unnecessary once #1903 is merged in.

COMMAND ${CMAKE_COMMAND} -E remove
${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols.h.bak
${CMAKE_CURRENT_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols_asm.h.bak
Expand Down
4 changes: 4 additions & 0 deletions crypto/fipsmodule/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,10 @@ if((((ARCH STREQUAL "x86_64") AND NOT MY_ASSEMBLER_IS_TOO_OLD_FOR_512AVX) OR
set(
S2N_BIGNUM_ASM_SOURCES

p256/bignum_montinv_p256.S
p256/p256_montjscalarmul_alt.S
p256/p256_montjscalarmul.S

p384/bignum_add_p384.S
p384/bignum_sub_p384.S
p384/bignum_neg_p384.S
Expand Down
73 changes: 60 additions & 13 deletions crypto/fipsmodule/ec/p256-nistz.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@
#include "internal.h"
#include "p256-nistz.h"

#if defined(EC_P256_USE_S2N_BIGNUM)
# include "../../../third_party/s2n-bignum/include/s2n-bignum_aws-lc.h"
#endif

#if !defined(OPENSSL_NO_ASM) && \
(defined(OPENSSL_X86_64) || defined(OPENSSL_AARCH64)) && \
!defined(OPENSSL_SMALL)
Expand All @@ -47,19 +51,6 @@ static const BN_ULONG ONE[P256_LIMBS] = {
// Precomputed tables for the default generator
#include "p256-nistz-table.h"

// Recode window to a signed digit, see |ec_GFp_nistp_recode_scalar_bits| in
// util.c for details
static crypto_word_t booth_recode_w5(crypto_word_t in) {
crypto_word_t s, d;

s = ~((in >> 5) - 1);
d = (1 << 6) - in - 1;
d = (d & s) | (in & ~s);
d = (d >> 1) + (d & 1);

return (d << 1) + (s & 1);
}

static crypto_word_t booth_recode_w7(crypto_word_t in) {
crypto_word_t s, d;

Expand Down Expand Up @@ -119,6 +110,16 @@ static BN_ULONG is_not_zero(BN_ULONG in) {
// ecp_nistz256_mod_inverse_sqr_mont sets |r| to (|in| * 2^-256)^-2 * 2^256 mod
// p. That is, |r| is the modular inverse square of |in| for input and output in
// the Montgomery domain.

#if defined(EC_P256_USE_S2N_BIGNUM)
static void ecp_nistz256_mod_inverse_sqr_mont(BN_ULONG r[P256_LIMBS],
const BN_ULONG in[P256_LIMBS]) {
BN_ULONG z2[P256_LIMBS];
ecp_nistz256_sqr_mont(z2,in);
bignum_montinv_p256(r,z2);
}

#else
static void ecp_nistz256_mod_inverse_sqr_mont(BN_ULONG r[P256_LIMBS],
const BN_ULONG in[P256_LIMBS]) {
// This implements the addition chain described in
Expand Down Expand Up @@ -185,7 +186,50 @@ static void ecp_nistz256_mod_inverse_sqr_mont(BN_ULONG r[P256_LIMBS],
ecp_nistz256_sqr_mont(r, ret); // 2^256 - 2^224 + 2^192 + 2^96 - 2^2
}

#endif


// r = p * p_scalar

#if defined(EC_P256_USE_S2N_BIGNUM)

static void ecp_nistz256_windowed_mul(const EC_GROUP *group, P256_POINT *r,
const EC_JACOBIAN *p,
const EC_SCALAR *p_scalar) {
uint64_t s2n_point[12], s2n_result[12];
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does 12 come from?


assert(p != NULL);
assert(p_scalar != NULL);
assert(group->field.N.width == P256_LIMBS);

OPENSSL_memcpy(s2n_point,p->X.words,32);
OPENSSL_memcpy(s2n_point+4,p->Y.words,32);
OPENSSL_memcpy(s2n_point+8,p->Z.words,32);
Comment on lines +205 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

Can some of this use constants/known sizes? I think this 32 could be sizeof(p->X.words), also would be handy to figure out what the 4 and 8 are coming from. Also spacing is a little off, convention is spaces after punctuation/symbols:

Suggested change
OPENSSL_memcpy(s2n_point,p->X.words,32);
OPENSSL_memcpy(s2n_point+4,p->Y.words,32);
OPENSSL_memcpy(s2n_point+8,p->Z.words,32);
OPENSSL_memcpy(s2n_point, p->X.words, sizeof(p->X.words));
OPENSSL_memcpy(s2n_point + ????,p->Y.words, sizeof(p->Y.words));
OPENSSL_memcpy(s2n_point + ????,p->Z.words, sizeof(p->Z.words));


p256_montjscalarmul_selector(s2n_result,(uint64_t*)p_scalar,s2n_point);

OPENSSL_memcpy(r->X,s2n_result,32);
OPENSSL_memcpy(r->Y,s2n_result+4,32);
OPENSSL_memcpy(r->Z,s2n_result+8,32);
}

#else

// Recode window to a signed digit, see |ec_GFp_nistp_recode_scalar_bits| in
// util.c for details
static crypto_word_t booth_recode_w5(crypto_word_t in) {
crypto_word_t s, d;

s = ~((in >> 5) - 1);
d = (1 << 6) - in - 1;
d = (d & s) | (in & ~s);
d = (d >> 1) + (d & 1);

return (d << 1) + (s & 1);
}

// r = p * p_scalar

static void ecp_nistz256_windowed_mul(const EC_GROUP *group, P256_POINT *r,
const EC_JACOBIAN *p,
const EC_SCALAR *p_scalar) {
Expand Down Expand Up @@ -279,6 +323,9 @@ static void ecp_nistz256_windowed_mul(const EC_GROUP *group, P256_POINT *r,
ecp_nistz256_point_add(r, r, aligned_h);
}

#endif


static crypto_word_t calc_first_wvalue(size_t *index, const uint8_t p_str[33]) {
static const size_t kWindowSize = 7;
static const crypto_word_t kMask = (1 << (7 /* kWindowSize */ + 1)) - 1;
Expand Down
7 changes: 7 additions & 0 deletions crypto/fipsmodule/ec/p256-nistz.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@
extern "C" {
#endif

#if !defined(OPENSSL_NO_ASM) && \
(defined(OPENSSL_LINUX) || defined(OPENSSL_APPLE)) && \
((defined(OPENSSL_X86_64) && \
!defined(MY_ASSEMBLER_IS_TOO_OLD_FOR_512AVX)) || \
Copy link
Contributor

Choose a reason for hiding this comment

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

Does any of the p256 s2n-bignum code use AVX 512? If not you could remove this.

defined(OPENSSL_AARCH64))
#define EC_P256_USE_S2N_BIGNUM
#endif

#if !defined(OPENSSL_NO_ASM) && \
(defined(OPENSSL_X86_64) || defined(OPENSSL_AARCH64)) && \
Expand Down
2 changes: 1 addition & 1 deletion tests/ci/common_posix_setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function verify_symbols_prefixed {
# * "\(bignum\|curve25519_x25519\)": match string of either "bignum" or "curve25519_x25519".
# Recall that the option "-v" reverse the pattern matching. So, we are really
# filtering out lines that contain either "bignum" or "curve25519_x25519".
cat "$BUILD_ROOT"/symbols_final_crypto.txt "$BUILD_ROOT"/symbols_final_ssl.txt | grep -v -e '^_\?\(bignum\|curve25519_x25519\|edwards25519\)' > "$SRC_ROOT"/symbols_final.txt
cat "$BUILD_ROOT"/symbols_final_crypto.txt "$BUILD_ROOT"/symbols_final_ssl.txt | grep -v -e '^_\?\(bignum\|curve25519_x25519\|edwards25519\|p256_montjscalarmul\)' > "$SRC_ROOT"/symbols_final.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not needed in the future.

# Now filter out every line that has the unique prefix $CUSTOM_PREFIX. If we
# have any lines left, then some symbol(s) weren't prefixed, unexpectedly.
if [ $(grep -c -v ${CUSTOM_PREFIX} "$SRC_ROOT"/symbols_final.txt) -ne 0 ]; then
Expand Down
13 changes: 13 additions & 0 deletions third_party/s2n-bignum/include/s2n-bignum_aws-lc.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,19 @@ static inline uint8_t use_s2n_bignum_alt(void) {
}
#endif

// Montgomery inverse modulo p_256 = 2^256 - 2^224 + 2^192 + 2^96 - 1
// Input x[4]; output z[4]
extern void bignum_montinv_p256(uint64_t z[static 4],const uint64_t x[static 4]);

// Montgomery-Jacobian form scalar multiplication for P-256
// Input scalar[4], point[12]; output res[12]
extern void p256_montjscalarmul(uint64_t res[static 12],const uint64_t scalar[static 4],const uint64_t point[static 12]);
extern void p256_montjscalarmul_alt(uint64_t res[static 12],const uint64_t scalar[static 4],const uint64_t point[static 12]);
static inline void p256_montjscalarmul_selector(uint64_t res[static 12], const uint64_t scalar[static 4], const uint64_t point[static 12]) {
if (use_s2n_bignum_alt()) { p256_montjscalarmul_alt(res, scalar, point); }
else { p256_montjscalarmul(res, scalar, point); }
Comment on lines +62 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: in a far future change would it make sense to rename these functions something more descriptive like p256_montjscalarmul_wide_multiplier? Such that in the future if we have a third implementation it's not p256_montjscalarmul_alt2 or p256_montjscalarmul_alt_alt.

}

// Add modulo p_384, z := (x + y) mod p_384, assuming x and y reduced
// Inputs x[6], y[6]; output z[6]
extern void bignum_add_p384(uint64_t z[static 6], const uint64_t x[static 6], const uint64_t y[static 6]);
Expand Down
Loading