Functions which lose object reuse and need auditing:
- d2i_ECParameters
- d2i_ECPrivateKey
This adds a handful of bytestring-based APIs to handle EC key
serialization. Deprecate all the old serialization APIs. Notes:
- An EC_KEY has additional state that controls its encoding, enc_flags
and conv_form. conv_form is left alone, but enc_flags in the new API
is an explicit parameter.
- d2i_ECPrivateKey interpreted its T** argument unlike nearly every
other d2i function. This is an explicit EC_GROUP parameter in the new
function.
- The new specified curve code is much stricter and should parse enough
to uniquely identify the curve.
- I've not bothered with a new version of i2d_ECParameters. It just
writes an OID. This may change later when decoupling from the giant
OID table.
- Likewise, I've not bothered with new APIs for the public key since the
EC_POINT APIs should suffice.
- Previously, d2i_ECPrivateKey would not call EC_KEY_check_key and it
was possible for the imported public and private key to mismatch. It
now calls it.
BUG=499653
Change-Id: I30b4dd2841ae76c56ab0e1808360b2628dee0615
Reviewed-on: https://boringssl-review.googlesource.com/6859
Reviewed-by: Adam Langley <agl@google.com>
In c0d9484902, we had to add support for
recognizing specified versions of named curves. I believe the motivation
was an ECPrivateKey encoded by OpenSSL without the EC_KEY's asn1_flag
set to OPENSSL_EC_NAMED_CURVE. Annoyingly, it appears OpenSSL's API
defaulted to the specified form while the tool defaulted to the named
form.
Add tests for this at the ECPrivateKey and the PKCS#8 level. The latter
was taken from Chromium's ec_private_key_unittest.cc which was the
original impetus for this.
Change-Id: I53a80c842c3fc9598f2e0ee7bf2d86b2add9e6c4
Reviewed-on: https://boringssl-review.googlesource.com/7072
Reviewed-by: Adam Langley <agl@google.com>
C has implicit conversion of |void *| to other pointer types so these
casts are unnecessary. Clean them up to make the code easier to read
and to make it easier to find dangerous casts.
Change-Id: I26988a672e8ed4d69c75cfbb284413999b475464
Reviewed-on: https://boringssl-review.googlesource.com/7102
Reviewed-by: David Benjamin <davidben@google.com>
|a_is_minus_3| is calculated in |ec_GFp_simple_group_set_curve|, so
the custom |group_init| functions are unnecessary. Just as in
commit 9f1f04f313, it is never the case
that custom parameters are passed to the |group_set_curve| method for
these curves.
Change-Id: I18a38b104bc332e44cc2053c465cf234f4c5163b
Reviewed-on: https://boringssl-review.googlesource.com/7090
Reviewed-by: David Benjamin <davidben@google.com>
This slightly simplifies the SSL_ECDH code and will be useful later on
in reimplementing the key parsing logic.
Change-Id: Ie41ea5fd3a9a734b3879b715fbf57bd991e23799
Reviewed-on: https://boringssl-review.googlesource.com/6858
Reviewed-by: Adam Langley <agl@google.com>
These are never called. Group parameters are not secret anyway. This is
a remnant of upstream's EC_GROUP_clear_free.
Change-Id: I23a4076eae8e4561abddbe74d0ba72641532f229
Reviewed-on: https://boringssl-review.googlesource.com/6823
Reviewed-by: Adam Langley <alangley@gmail.com>
node.js is, effectively, another bindings library. However, it's better
written than most and, with these changes, only a couple of tiny fixes
are needed in node.js. Some of these changes are a little depressing
however so we'll need to push node.js to use APIs where possible.
Changes:
∙ Support verify_recover. This is very obscure and the motivation
appears to be https://github.com/nodejs/node/issues/477 – where it's
not clear that anyone understands what it means :(
∙ Add a few, no-op #defines
∙ Add some members to |SSL_CTX| and |SSL| – node.js needs to not
reach into these structs in the future.
∙ Add EC_get_builtin_curves.
∙ Add EVP_[CIPHER|MD]_do_all_sorted – these functions are limited to
decrepit.
Change-Id: I9a3566054260d6c4db9d430beb7c46cc970a9d46
Reviewed-on: https://boringssl-review.googlesource.com/6952
Reviewed-by: Adam Langley <agl@google.com>
Since the error string logic was rewritten, this hasn't done anything.
Change-Id: Icb73dca65e852bb3c7d04c260d591906ec72c15f
Reviewed-on: https://boringssl-review.googlesource.com/6961
Reviewed-by: Adam Langley <agl@google.com>
MSVC doesn't have stdalign.h and so doesn't support |alignas| in C
code. Define |alignas(x)| as a synonym for |__decltype(align(x))|
instead for it.
This also fixes -Wcast-qual warnings in rsaz_exp.c.
Change-Id: Ifce9031724cb93f5a4aa1f567e7af61b272df9d5
Reviewed-on: https://boringssl-review.googlesource.com/6924
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is only for Conscrypt which always calls the pair in succession. (Indeed
it wouldn't make any sense to not call it.) Remove those two APIs and replace
with a single merged API. This way incomplete EC_GROUPs never escape outside
our API boundary and EC_GROUPs may *finally* be made immutable.
Also add a test for this to make sure I didn't mess it up.
Add a temporary BORINGSSL_201512 define to ease the transition for Conscrypt.
Conscrypt requires https://android-review.googlesource.com/#/c/187801/ before
picking up this change.
Change-Id: I3706c2ceac31ed2313175ba5ee724bd5c74ef6e1
Reviewed-on: https://boringssl-review.googlesource.com/6550
Reviewed-by: Adam Langley <agl@google.com>
Fix casts from const to non-const where dropping the constness is
completely unnecessary. The changes to chacha_vec.c don't result in any
changes to chacha_vec_arm.S.
Change-Id: I2f10081fd0e73ff5db746347c5971f263a5221a6
Reviewed-on: https://boringssl-review.googlesource.com/6923
Reviewed-by: David Benjamin <davidben@google.com>
clang scan-build is annoyed it's not obvious the sizeof line matches the
pointer type. This is easy to fix and makes it be quiet.
Change-Id: Iec80d2a087f81179c88cae300f56d3f76b32b347
Reviewed-on: https://boringssl-review.googlesource.com/6701
Reviewed-by: Adam Langley <agl@google.com>
This callback is never used. The one caller I've ever seen is in Android
code which isn't built with BoringSSL and it was a no-op.
It also doesn't actually make much sense. A callback cannot reasonably
assume that it sees every, say, SSL_CTX created because the index may be
registered after the first SSL_CTX is created. Nor is there any point in
an EX_DATA consumer in one file knowing about an SSL_CTX created in
completely unrelated code.
Replace all the pointers with a typedef to int*. This will ensure code
which passes NULL or 0 continues to compile while breaking code which
passes an actual function.
This simplifies some object creation functions which now needn't worry
about CRYPTO_new_ex_data failing. (Also avoids bouncing on the lock, but
it's taking a read lock, so this doesn't really matter.)
BUG=391192
Change-Id: I02893883c6fa8693682075b7b130aa538a0a1437
Reviewed-on: https://boringssl-review.googlesource.com/6625
Reviewed-by: Adam Langley <agl@google.com>
In some cases it would be good to restrict the input range of scalars
given to |EC_METHOD::mul| to be [0, order-1]. This is a first step
towards that goal.
Change-Id: I58a25db06f6c7a68a0ac1fe79794b04f7a173b23
Reviewed-on: https://boringssl-review.googlesource.com/6562
Reviewed-by: Adam Langley <agl@google.com>
|EC_GROUP_get0_order| doesn't require any heap allocations and never
fails, so it is much more convenient and more efficient for callers to
call.
Change-Id: Ic60f768875e7bc8e74362dacdb5cbbc6957b05a6
Reviewed-on: https://boringssl-review.googlesource.com/6532
Reviewed-by: Adam Langley <agl@google.com>
Previously, both crypto/dh and crypto/ec defined |TOBN| macros that did
the same thing, but which took their arguments in the opposite order.
This change makes the code consistently use the same macro. It also
makes |STATIC_BIGNUM| available for internal use outside of crypto/bn.
Change-Id: Ide57f6a5b74ea95b3585724c7e1a630c82a864d9
Reviewed-on: https://boringssl-review.googlesource.com/6528
Reviewed-by: Adam Langley <agl@google.com>
Without |EC_POINTs_mul|, there's never more than one variable point
passed to a |EC_METHOD|'s |mul| method. This allows them to be
simplified considerably. In this commit, the p256-x86_64 implementation
has been simplified to eliminate the heap allocation and looping
related that was previously necessary to deal with the possibility of
there being multiple input points. The other implementations were left
mostly as-is; they should be similarly simplified in the future.
Change-Id: I70751d1d5296be2562af0730e7ccefdba7a1acae
Reviewed-on: https://boringssl-review.googlesource.com/6493
Reviewed-by: Adam Langley <agl@google.com>
This makes similar fixes as were done in the following OpenSSL commits:
c028254b12a8ea0d0f8a677172eda2e2d78073f3: Correctly set Z_is_one on
the return value in the NISTZ256 implementation.
e22d2199e2a5cc9b243f45c2b633d1e31fadecd7: Error checking and memory
leak leak fixes in NISTZ256.
4446044a793a9103a4bc70c0214005e6a4463767: NISTZ256: set Z_is_one to
boolean 0/1 as is customary.
a4d5269e6d0dba0c276c968448a3576f7604666a: NISTZ256: don't swallow
malloc errors.
The fixes aren't exactly the same. In particular, the comments "This is
an unusual input, we don't guarantee constant-timeness" and the changes
to |ecp_nistz256_mult_precompute| (which isn't in BoringSSL) were
omitted.
Change-Id: Ia7bb982daa62fb328e8bd2d4dd49a8857e104096
Reviewed-on: https://boringssl-review.googlesource.com/6492
Reviewed-by: Adam Langley <agl@google.com>
This moves us closer to having |EC_GROUP| and |EC_KEY| being immutable.
The functions are left as no-ops for backward compatibility.
Change-Id: Ie23921ab0364f0771c03aede37b064804c9f69e0
Reviewed-on: https://boringssl-review.googlesource.com/6485
Reviewed-by: Adam Langley <agl@google.com>
This extends 9f1f04f313 to the other
implementations.
|EC_GFp_nistp224_method| and |EC_GFp_nistp256_method| are not marked
|OPENSSL_EXPORT|. |EC_GROUP_set_generator| doesn't allow the generator
to be changed for any |EC_GROUP| for built-in curves. Consequently,
there's no way (except some kind of terrible abuse) that this code
could be executed with a non-default generator.
Change-Id: I5d9b6be4e6f9d384159cb3d708390a8e3c69f23f
Reviewed-on: https://boringssl-review.googlesource.com/6489
Reviewed-by: Adam Langley <agl@google.com>
|EC_POINT_point2oct| would encode ∞, which is surprising, and
|EC_POINT_oct2point| would decode ∞, which is insane. This change
removes both behaviours.
Thanks to Brian Smith for pointing it out.
Change-Id: Ia89f257dc429a69b9ea7b7b15f75454ccc9c3bdd
Reviewed-on: https://boringssl-review.googlesource.com/6488
Reviewed-by: Adam Langley <agl@google.com>
In the case of a compressed point, the decompression ensures that the
point is on the curve. In the uncompressed case,
|EC_POINT_set_affine_coordinates_GFp| checks that the point is on the
curve as of 38feb990a1.
Change-Id: Icd69809ae396838b4aef4fa89b3b354560afed55
Reviewed-on: https://boringssl-review.googlesource.com/6487
Reviewed-by: Brian Smith <brian@briansmith.org>
Reviewed-by: Adam Langley <agl@google.com>
|EC_GFp_nistz256_method| is not marked |OPENSSL_EXPORT| so only the
built-in P-256 curve uses it. |EC_GROUP_set_generator| doesn't allow
the generator to be changed for any |EC_GROUP| for a built-in curve.
Consequently, there's no way (except some kind of terrible abuse) that
the nistz code could be executed with a non-default generator.
Change-Id: Ib22f00bc74c103b7869ed1e35032b1f3d26cdad2
Reviewed-on: https://boringssl-review.googlesource.com/6446
Reviewed-by: Adam Langley <agl@google.com>
Chromium's toolchains may now assume C++11 library support, so we may freely
use C++11 features. (Chromium's still in the process of deciding what to allow,
but we use Google's style guide directly, toolchain limitations aside.)
Change-Id: I1c7feb92b7f5f51d9091a4c686649fb574ac138d
Reviewed-on: https://boringssl-review.googlesource.com/6465
Reviewed-by: Adam Langley <agl@google.com>
This removes a sharp corner in the API where |ECDH_compute_key| assumed
that callers were either using ephemeral keys, or else had already
checked that the public key was on the curve.
A public key that's not on the curve can be in a small subgroup and thus
the result can leak information about the private key.
This change causes |EC_POINT_set_affine_coordinates_GFp| to require that
points are on the curve. |EC_POINT_oct2point| already does this.
Change-Id: I77d10ce117b6efd87ebb4a631be3a9630f5e6636
Reviewed-on: https://boringssl-review.googlesource.com/5861
Reviewed-by: Adam Langley <agl@google.com>
MSVC unhelpfuly says: warning C4146: unary minus operator applied to
unsigned type, result still unsigned.
Change-Id: Ia1e6b9fc415908920abb1bcd98fc7f7a5670c2c7
This change incorporates Intel's P-256 implementation. The record of
Intel's submission under CLA is in internal bug number 25330687.
Before:
Did 3582 ECDH P-256 operations in 1049114us (3414.3 ops/sec)
Did 8525 ECDSA P-256 signing operations in 1028778us (8286.5 ops/sec)
Did 3487 ECDSA P-256 verify operations in 1008996us (3455.9 ops/sec)
build/tool/bssl is 1434704 bytes after strip -s
After:
Did 8618 ECDH P-256 operations in 1027884us (8384.2 ops/sec)
Did 21000 ECDSA P-256 signing operations in 1049490us (20009.7 ops/sec)
Did 8268 ECDSA P-256 verify operations in 1079481us (7659.2 ops/sec)
build/tool/bssl is 1567216 bytes after strip -s
Change-Id: I147971a8e19849779c8ed7e20310d41bd4962299
Reviewed-on: https://boringssl-review.googlesource.com/6371
Reviewed-by: Adam Langley <agl@google.com>
These functions ultimately return the result of |BN_num_bits|, and that
function's return type is |unsigned|. Thus, these functions' return
type should also be |unsigned|.
Change-Id: I2cef63e6f75425857bac71f7c5517ef22ab2296b
Reviewed-on: https://boringssl-review.googlesource.com/6170
Reviewed-by: Adam Langley <alangley@gmail.com>
Intel's P-256 code has very large tables and things like Chromium just
don't need that extra size. However, servers generally do so this change
adds an OPENSSL_SMALL define that currently just drops the 64-bit P-224
but will gate Intel's P-256 in the future too.
Change-Id: I2e55c6e06327fafabef9b96d875069d95c0eea81
Reviewed-on: https://boringssl-review.googlesource.com/6362
Reviewed-by: Adam Langley <alangley@gmail.com>
If the application is only using the P-256 implementation in p256-64.c,
then the WNAF code would all be dead code. The change reorganizes the
code so that all modern toolchains should be able to recognize that
fact and eliminate the WNAF-based code when it is unused.
Change-Id: I9f94bd934ca7d2292de4c29bb89e17c940c7cd2a
Reviewed-on: https://boringssl-review.googlesource.com/6173
Reviewed-by: Adam Langley <alangley@gmail.com>
None of these methods vary per group. Factoring these out of
|EC_METHOD| should help some toolchains to do a better job optimizing
the code for size.
Change-Id: Ibd22a52992b4d549f12a8d22bddfdb3051aaa891
Reviewed-on: https://boringssl-review.googlesource.com/6172
Reviewed-by: Adam Langley <alangley@gmail.com>
It's very annoying having to remember the right incant every time I want
to switch around between my build, build-release, build-asan, etc.,
output directories.
Unfortunately, this target is pretty unfriendly without CMake 3.2+ (and
Ninja 1.5+). This combination gives a USES_TERMINAL flag to
add_custom_target which uses Ninja's "console" pool, otherwise the
output buffering gets in the way. Ubuntu LTS is still on an older CMake,
so do a version check in the meantime.
CMake also has its own test mechanism (CTest), but this doesn't use it.
It seems to prefer knowing what all the tests are and then tries to do
its own output management and parallelizing and such. We already have
our own runners. all_tests.go could actually be converted tidily, but
generate_build_files.py also needs to read it, and runner.go has very
specific needs.
Naming the target ninja -C build test would be nice, but CTest squats
that name and CMake grumps when you use a reserved name, so I've gone
with run_tests.
Change-Id: Ibd20ebd50febe1b4e91bb19921f3bbbd9fbcf66c
Reviewed-on: https://boringssl-review.googlesource.com/6270
Reviewed-by: Adam Langley <alangley@gmail.com>
This imports the Google-authored P-224 implementation by Emilia Käsper
and Bodo Möller that is also in upstream OpenSSL.
Change-Id: I16005c74a2a3e374fb136d36f3f6569dab9d8919
Reviewed-on: https://boringssl-review.googlesource.com/6145
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 728bcd59d3d41e152aead0d15acc51a8958536d3.)
Actually this one was reported by us, but the commit message doesn't
mention this.
This is slightly modified from upstream's version to fix some problems
noticed in import. Specifically one of d2i_X509_AUX's success paths is
bust and d2i_PrivateKey still updates on one error path. Resolve the
latter by changing both it and d2i_AutoPrivateKey to explicitly hit the
error path on ret == NULL. This lets us remove the NULL check in
d2i_AutoPrivateKey.
We'll want to report the problems back upstream.
Change-Id: Ifcfc965ca6d5ec0a08ac154854bd351cafbaba25
Reviewed-on: https://boringssl-review.googlesource.com/5948
Reviewed-by: Adam Langley <agl@google.com>
arm_arch.h is included from ARM asm files, but lives in crypto/, not
openssl/include/. Since the asm files are often built from a different
location than their position in the source tree, relative include paths
are unlikely to work so, rather than having crypto/ be a de-facto,
second global include path, this change moves arm_arch.h to
include/openssl/.
It also removes entries from many include paths because they should be
needed as relative includes are always based on the locations of the
source file.
Change-Id: I638ff43d641ca043a4fc06c0d901b11c6ff73542
Reviewed-on: https://boringssl-review.googlesource.com/5746
Reviewed-by: Adam Langley <agl@google.com>
MSAN appears to have a bug that causes this code to be miscompiled when
compiled with optimisations. In order to prevent that bug from holding
everything up, this change disables that code when MEMORY_SANITIZER is
defined. The generic elliptic-curve code can pick up the slack in that
case.
Change-Id: I7ce26969b3ee0bc0b0496506f06a8cf9b2523cfa
Previously, |x| was reset to the value of the cofactor for no reason,
and there was an unnecessary copy made of |order|.
Change-Id: Ib6b06f651e280838299dff534c38726ebf4ccc97
Reviewed-on: https://boringssl-review.googlesource.com/4447
Reviewed-by: Adam Langley <agl@google.com>
