Use s2n-bignum P-256 scalar multiplication and Montgomery inverse #1877
Conversation
This replaces the general (fresh, not precomputed, point) scalar multiplication with the corresponding function p256_montjscalarmul or p256_montjscalarmul_alt from s2n-bignum, and also replaces the Fermat inverse in p256-nistz.c with the markedly faster and formally verified divstep-based code from s2n-bignum, bignum_montinv_p256.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1877 +/- ##
==========================================
- Coverage 78.48% 78.46% -0.02%
==========================================
Files 585 585
Lines 99524 99425 -99
Branches 14249 14234 -15
==========================================
- Hits 78112 78017 -95
+ Misses 20776 20773 -3
+ Partials 636 635 -1 ☔ View full report in Codecov by Sentry. |
Thanks to Torben
To land aws/aws-lc#1877, must filter new s2n-bignum symbols that are not supposed to have external linkage. Similar to e.g. #286
| 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 |
There was a problem hiding this comment.
All of this is unnecessary once #1903 is merged in.
| #if !defined(OPENSSL_NO_ASM) && \ | ||
| (defined(OPENSSL_LINUX) || defined(OPENSSL_APPLE)) && \ | ||
| ((defined(OPENSSL_X86_64) && \ | ||
| !defined(MY_ASSEMBLER_IS_TOO_OLD_FOR_512AVX)) || \ |
There was a problem hiding this comment.
Does any of the p256 s2n-bignum code use AVX 512? If not you could remove this.
| @@ -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 | |||
There was a problem hiding this comment.
Also not needed in the future.
| if (use_s2n_bignum_alt()) { p256_montjscalarmul_alt(res, scalar, point); } | ||
| else { p256_montjscalarmul(res, scalar, point); } |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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:
| 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)); |
| 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]; |
This replaces the general (fresh, not precomputed, point) scalar multiplication with the corresponding function p256_montjscalarmul or p256_montjscalarmul_alt from s2n-bignum, and also replaces the Fermat inverse in p256-nistz.c with the markedly faster and formally verified divstep-based code from s2n-bignum, bignum_montinv_p256.
Issues:
Resolves #ISSUE-NUMBER1
Addresses #ISSUE-NUMBER2
Description of changes:
Describe AWS-LC’s current behavior and how your code changes that behavior. If there are no issues this pr is resolving, explain why this change is necessary.
Call-outs:
Point out areas that need special attention or support during the review process. Discuss architecture or design changes.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.