The generic code special-cases affine points, but this leaks
information. (Of course, the generic code also doesn't have a
constant-time multiply and other problems, but one thing at a time.)
The optimization in point doubling is not useful. Point multiplication
more-or-less never doubles an affine point. The optimization in point
addition *is* useful because the wNAF code converts the tables to
affine. Accordingly, align with the P-256 code which adds a 'mixed'
parameter.
(I haven't aligned the formally-verified point formulas themselves yet;
initial testing suggests that the large number of temporaries take a
perf hit with BIGNUM. I'll check the results in EC_FELEM, which will be
stack-allocated, to see if we still need to help the compiler out.)
Strangly, it actually got a bit faster with this change. I'm guessing
because now it doesn't need to bother with unnecessary comparisons and
maybe was kinder to the branch predictor?
Before:
Did 2201 ECDH P-384 operations in 3068341us (717.3 ops/sec)
Did 4092 ECDSA P-384 signing operations in 3076981us (1329.9 ops/sec)
Did 3503 ECDSA P-384 verify operations in 3024753us (1158.1 ops/sec)
Did 992 ECDH P-521 operations in 3017884us (328.7 ops/sec)
Did 1798 ECDSA P-521 signing operations in 3059000us (587.8 ops/sec)
Did 1581 ECDSA P-521 verify operations in 3033142us (521.2 ops/sec)
After:
Did 2310 ECDH P-384 operations in 3092648us (746.9 ops/sec)
Did 4080 ECDSA P-384 signing operations in 3044588us (1340.1 ops/sec)
Did 3520 ECDSA P-384 verify operations in 3056070us (1151.8 ops/sec)
Did 992 ECDH P-521 operations in 3012779us (329.3 ops/sec)
Did 1792 ECDSA P-521 signing operations in 3019459us (593.5 ops/sec)
Did 1600 ECDSA P-521 verify operations in 3047749us (525.0 ops/sec)
Bug: 239
Change-Id: If5d13825fc98e4c58bdd1580cf0245bf7ce93a82
Reviewed-on: https://boringssl-review.googlesource.com/27004
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for representing field elements with
stack-allocated types in the generic code. While there is likely little
benefit in threading all the turned field arithmetic through all the
generic code, and the P-224 logic, in particular, does not have a tight
enough abstraction for this, the current implementations depend on
BN_div, which is not compatible with stack-allocating things and avoiding
malloc.
This also speeds things up slightly, now that benchmarks cover point
validation.
Before:
Did 82786 ECDH P-224 operations in 10024326us (8258.5 ops/sec)
After:
Did 89991 ECDH P-224 operations in 10012429us (8987.9 ops/sec)
Change-Id: I468483b49f5dc69187aebd62834365ce5caab795
Reviewed-on: https://boringssl-review.googlesource.com/26971
Reviewed-by: Adam Langley <agl@google.com>
I'm not sure why I separated "fixed" and "quick_ctx" names. That's
annoying and doesn't generalize well to, say, adding a bn_div_consttime
function for RSA keygen.
Change-Id: I751d52b30e079de2f0d37a952de380fbf2c1e6b7
Reviewed-on: https://boringssl-review.googlesource.com/26364
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
As the EC code will ultimately want to use these in "words" form by way
of EC_FELEM, and because it's much easier, I've implement these as
low-level words-based functions that require all inputs have the same
width. The BIGNUM versions which RSA and, for now, EC calls are
implemented on top of that.
Unfortunately, doing such things in constant-time and accounting for
undersized inputs requires some scratch space, and these functions don't
take BN_CTX. So I've added internal bn_mod_*_quick_ctx functions that
take a BN_CTX and the old functions now allocate a bit unnecessarily.
RSA only needs lshift (for BN_MONT_CTX) and sub (for CRT), but the
generic EC code wants add as well.
The generic EC code isn't even remotely constant-time, and I hope to
ultimately use stack-allocated EC_FELEMs, so I've made the actual
implementations here implemented in "words", which is much simpler
anyway due to not having to take care of widths.
I've also gone ahead and switched the EC code to these functions,
largely as a test of their performance (an earlier iteration made the EC
code noticeably slower). These operations are otherwise not
performance-critical in RSA.
The conversion from BIGNUM to BIGNUM+BN_CTX should be dropped by the
static linker already, and the unused BIGNUM+BN_CTX functions will fall
off when EC_FELEM happens.
Update-Note: BN_mod_*_quick bounce on malloc a bit now, but they're not
really used externally. The one caller I found was wpa_supplicant
which bounces on malloc already. They appear to be implementing
compressed coordinates by hand? We may be able to convince them to
call EC_POINT_set_compressed_coordinates_GFp.
Bug: 233, 236
Change-Id: I2bf361e9c089e0211b97d95523dbc06f1168e12b
Reviewed-on: https://boringssl-review.googlesource.com/25261
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This has no behavior change, but it has a semantic one. This CL is an
assertion that all BIGNUM functions tolerate non-minimal BIGNUMs now.
Specifically:
- Functions that do not touch top/width are assumed to not care.
- Functions that do touch top/width will be changed by this CL. These
should be checked in review that they tolerate non-minimal BIGNUMs.
Subsequent CLs will start adjusting the widths that BIGNUM functions
output, to fix timing leaks.
Bug: 232
Change-Id: I3a2b41b071f2174452f8d3801bce5c78947bb8f7
Reviewed-on: https://boringssl-review.googlesource.com/25257
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The order (and later the field) are used to size stack-allocated fixed
width word arrays. They're also entirely public, so this is fine.
Bug: 232
Change-Id: Ie98869cdbbdfea92dcad64a300f7e0b47bef6bf2
Reviewed-on: https://boringssl-review.googlesource.com/25256
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
p224-64.c can just write straight into the EC_POINT, as the other files
do, which saves the mess around BN_CTX. It's also more correct.
ec_point_set_Jprojective_coordinates_GFp abstracts out field_encode, but
then we would want to abstract out field_decode too when reading.
That then allows us to inline ec_point_set_Jprojective_coordinates_GFp
into ec_GFp_simple_point_set_affine_coordinates and get rid of an
unnecessary tower of helper functions. Also we can use the precomputed
value of one rather than recompute it each time.
Change-Id: I8282dc66a4a437f5a3b6a1a59cc39be4cb71ccf9
Reviewed-on: https://boringssl-review.googlesource.com/24687
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Id12ab478b6ba441fb1b6f4c2f9479384fc3fbdb6
Reviewed-on: https://boringssl-review.googlesource.com/23144
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Currently we only check that the underlying EC_METHODs match, which
avoids the points being in different forms, but not that the points are
on the same curves. (We fixed the APIs early on so off-curve EC_POINTs
cannot be created.)
In particular, this comes up with folks implementating Java's crypto
APIs with ECDH_compute_key. These APIs are both unfortunate and should
not be mimicked, as they allow folks to mismatch the groups on the two
multiple EC_POINTs. Instead, ECDH APIs should take the public value as a
byte string.
Thanks also to Java's poor crypto APIs, we must support custom curves,
which makes this particularly gnarly. This CL makes EC_GROUP_cmp work
with custom curves and adds an additional subtle requirement to
EC_GROUP_set_generator.
Annoyingly, this change is additionally subtle because we now have a
reference cycle to hack around.
Change-Id: I2efbc4bd5cb65fee5f66527bd6ccad6b9d5120b9
Reviewed-on: https://boringssl-review.googlesource.com/22245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I really need to resurrect the CL to make them entirely static
(https://crbug.com/boringssl/20), but, in the meantime, to make
replacing the EC_METHOD pointer in EC_POINT with EC_GROUP not
*completely* insane, make them refcounted.
OpenSSL did not do this because their EC_GROUPs are mutable
(EC_GROUP_set_asn1_flag and EC_GROUP_set_point_conversion_form). Ours
are immutable but for the two-function dance around custom curves (more
of OpenSSL's habit of making their objects too complex), which is good
enough to refcount.
Change-Id: I3650993737a97da0ddcf0e5fb7a15876e724cadc
Reviewed-on: https://boringssl-review.googlesource.com/22244
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
crypto/{asn1,x509,x509v3,pem} were skipped as they are still OpenSSL
style.
Change-Id: I3cd9a60e1cb483a981aca325041f3fbce294247c
Reviewed-on: https://boringssl-review.googlesource.com/19504
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The names in the P-224 code collided with the P-256 code and thus many
of the functions and constants in the P-224 code have been prefixed.
Change-Id: I6bcd304640c539d0483d129d5eaf1702894929a8
Reviewed-on: https://boringssl-review.googlesource.com/15847
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
