Use |size_t| for array indexes. Use |int| for boolean flags. Declare
the variables that had their types changed closer to where they are
used.
Previously, some `for` loops depended on `i` being signed, so their
structure had to be changed to work with the unsigned type.
Change-Id: I247e4f04468419466733b6818d81d28666da0ad3
Reviewed-on: https://boringssl-review.googlesource.com/7468
Reviewed-by: David Benjamin <davidben@google.com>
Make it match how it is done in p224-64.c. Note in particular that
|size| may be 17, so presumably |pre_comp[16]| is accessed, which one
would not expect when it was declared |precomp[16][3]|.
Change-Id: I54c1555f9e20ccaacbd4cd75a7154b483b4197b7
Reviewed-on: https://boringssl-review.googlesource.com/7467
Reviewed-by: David Benjamin <davidben@google.com>
Partially fixes build with -Wmissing-prototypes -Wmissing-declarations.
Change-Id: I6048f5b7ef31560399b25ed9880156bc7d8abac2
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7511
Reviewed-by: David Benjamin <davidben@google.com>
Having a different API for this case than upstream is more trouble than is
worth it. This is sad since the new API avoids incomplete EC_GROUPs at least,
but I don't believe supporting this pair of functions will be significantly
more complex than supporting EC_GROUP_new_arbitrary even when we have static
EC_GROUPs.
For now, keep both sets of APIs around, but we'll be able to remove the scar
tissue once Conscrypt's complex dependencies are resolved.
Make the restored EC_GROUP_set_generator somewhat simpler than before by
removing the ability to call it multiple times and with some parameters set to
NULL. Keep the test.
Change-Id: I64e3f6a742678411904cb15c0ad15d56cdae4a73
Reviewed-on: https://boringssl-review.googlesource.com/7432
Reviewed-by: David Benjamin <davidben@google.com>
The (internal) constant-time callers of this function already do a
constant-time reduction before calling. And, nobody should be calling
this function with out-of-range coordinates anyway. So, just require
valid coordinates as input.
Further, this function is rarely called, so don't bother with the
optimization to avoid encoding Montgomery encoding of 1 for the Z
coordinate.
Change-Id: I637ffaf4d39135ca17214915b9a8582ea052eea8
Reviewed-on: https://boringssl-review.googlesource.com/7441
Reviewed-by: David Benjamin <davidben@google.com>
Don't try to fix a bad |x| coordinate by reducing it. Instead, just
fail. This also makes the code clearer; in particular, it was confusing
why |x_| was used for some calculations when it seems like |x| was just
as good or better.
Change-Id: I9a6911f0d2bd72852a26b46f3828eb5ba3ef924f
Reviewed-on: https://boringssl-review.googlesource.com/7440
Reviewed-by: David Benjamin <davidben@google.com>
The points are only converted to affine form when there are at least
three points being multiplied (in addition to the generator), but there
never is more than one point, so this is all dead code.
Also, I doubt that the comments "...point at infinity (which normally
shouldn't happen)" in the deleted code are accurate. And, the
projective->affine conversions that were removed from p224-64.c and
p256-64.c didn't seem to properly account for the possibility that any of
those points were at infinity.
Change-Id: I611d42d36dcb7515eabf3abf1857e52ff3b45c92
Reviewed-on: https://boringssl-review.googlesource.com/7100
Reviewed-by: David Benjamin <davidben@google.com>
If the function returns early due to an error, then the coordinates of the
result will have their |top| value set to a value beyond what has actually
been been written. Fix that, and make it easier to avoid such issues in the
future by refactoring the code.
As a bonus, avoid a false positive MSVC 64-bit opt build "potentially
uninitialized value used" warning.
Change-Id: I8c48deb63163a27f739c8797962414f8ca2588cd
Reviewed-on: https://boringssl-review.googlesource.com/6579
Reviewed-by: David Benjamin <davidben@google.com>
Having |Z_is_one| be out of sync with |Z| could potentially be a very
bad thing, and in the past there have been multiple bugs of this sort,
including one currently in p256-x86_64.c (type confusion: Montgomery-
encoded vs unencoded). Avoid the issue entirely by getting rid of
|Z_is_one|.
Change-Id: Icb5aa0342df41d6bc443f15f952734295d0ee4ba
Reviewed-on: https://boringssl-review.googlesource.com/6576
Reviewed-by: David Benjamin <davidben@google.com>
See also 1b0c438e1a.
Change-Id: Ifcfe15caa4d0db8ef725f8dacd0e8c5c94b00a09
Reviewed-on: https://boringssl-review.googlesource.com/7390
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
(Imported from upstream's 64333004a41a9f4aa587b8e5401420fb70d00687.)
RT#4284.
This case should be impossible to hit because |EC_POINT_add| doesn't use
this function and trying to add equal inputs should never occur during a
multiplication. Support for this exists because the pattern has been
copied from the first 64-bit P-224 and P-256 work that Emilia, Bodo and
I did. There it seemed like a reasonable defense-in-depth in case the
code changed in the future.
Change-Id: I7ff138669c5468b7d7a5153429bec728cb67e338
Reviewed-on: https://boringssl-review.googlesource.com/7246
Reviewed-by: David Benjamin <davidben@google.com>
Callers of this function are not checking for the -1 result. Change
the semantics to match their expectations and to match the common
semantics of most other parts of BoringSSL.
Change-Id: I4ec537d7619e20e8ddfee80c72125e4c02cfaac1
Reviewed-on: https://boringssl-review.googlesource.com/7125
Reviewed-by: David Benjamin <davidben@google.com>
(imported from upstream's 2b80d00e3ac652377ace84c51b53f51a1b7e1ba2)
Change-Id: Iee5a8d85d276033b6ac8bc9ac87e157916a1a29a
Reviewed-on: https://boringssl-review.googlesource.com/7212
Reviewed-by: David Benjamin <davidben@google.com>
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>
The documentation for |BN_CTX_get| states: "Once |BN_CTX_get| has
returned NULL, all future calls will also return NULL until
|BN_CTX_end| is called." Some code takes advantage of that guarantee
by only checking the return value of the last call to |BN_CTX_get| in a
series of calls. That is correct and the most efficient way of doing
it. However, that pattern is inconsistent with most of the other uses
of |BN_CTX_get|. Also, static analysis tools like Coverity cannot
understand that pattern. This commit removes the instances of that
pattern that Coverity complained about when scanning *ring*.
Change-Id: Ie36d0223ea1caee460c7979547cf5bfd5fb16f93
Reviewed-on: https://boringssl-review.googlesource.com/5611
Reviewed-by: Adam Langley <agl@google.com>
This isn't exhaustive. There are still failures in some tests which probably
ought to get C++'d first.
Change-Id: Iac58df9d98cdfd94603d54374a531b2559df64c3
Reviewed-on: https://boringssl-review.googlesource.com/4795
Reviewed-by: Adam Langley <agl@google.com>
Currently far from passing and I haven't even tried with a leak checker yet.
Also bn_test is slow.
Change-Id: I4fe2783aa5f7897839ca846062ae7e4a367d2469
Reviewed-on: https://boringssl-review.googlesource.com/4794
Reviewed-by: Adam Langley <agl@google.com>
This change converts the reference counts in crypto/ to use
|CRYPTO_refcount_t|. The reference counts in |X509_PKEY| and |X509_INFO|
were never actually used and so were dropped.
Change-Id: I75d572cdac1f8c1083c482e29c9519282d7fd16c
Reviewed-on: https://boringssl-review.googlesource.com/4772
Reviewed-by: Adam Langley <agl@google.com>
This change exposes the functions needed to support arbitrary elliptic
curve groups. The Java API[1] doesn't allow a provider to only provide
certain elliptic curve groups. So if BoringSSL is an ECC provider on
Android, we probably need to support arbitrary groups because someone
out there is going to be using it for Bitcoin I'm sure.
Perhaps in time we can remove this support, but not yet.
[1] https://docs.oracle.com/javase/7/docs/api/java/security/spec/ECParameterSpec.html
Change-Id: Ic1d76de96f913c9ca33c46b451cddc08c5b93d80
Reviewed-on: https://boringssl-review.googlesource.com/4740
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Change |EC_KEY_new_by_curve_name| to report |ERR_R_MALLOC_FAILURE|
itself, so that reporting of |EC_R_UNKNOWN_GROUP| is not confused by
the caller's addition of a spurious |ERR_R_MALLOC_FAILURE|.
Change-Id: Id3f5364f01eb8e3597bcddd6484bc03d5578befb
Reviewed-on: https://boringssl-review.googlesource.com/4690
Reviewed-by: Adam Langley <agl@google.com>
BoringSSL always uses uncompressed points. This function aborts if
another form is requested or does nothing if uncompressed points are
requested.
Change-Id: I80bc01444cdf9c789c9c75312b5527bf4957361b
MSVC seems to dislike the zero-array trick in C++, but not C. Turns out there
was no need for the include, so that's an easy fix.
Change-Id: I6def7b430a450c4ff7eeafa3611f0d40f5fc5945
Reviewed-on: https://boringssl-review.googlesource.com/4580
Reviewed-by: Adam Langley <agl@google.com>
RFC 5915 requires the use of the I2OSP primitive as defined in RFC 3447
for encoding ECPrivateKey. Fix this and add a test.
See also upstream's 30cd4ff294252c4b6a4b69cbef6a5b4117705d22, though it mixes
up degree and order.
Change-Id: I81ba14da3c8d69e3799422c669fab7f16956f322
Reviewed-on: https://boringssl-review.googlesource.com/4469
Reviewed-by: Adam Langley <agl@google.com>
CRYPTO_MUTEX was the wrong size. Fortunately, Apple was kind enough to define
pthread_rwlock_t unconditionally, so we can be spared fighting with feature
macros. Some of the stdlib.h removals were wrong and clang is pick about
multiply-defined typedefs. Apparently that's a C11 thing?
BUG=478598
Change-Id: Ibdcb8de9e5d83ca28e4c55b2979177d1ef0f9721
Reviewed-on: https://boringssl-review.googlesource.com/4404
Reviewed-by: Adam Langley <agl@google.com>
This is taken from upstream, although it originally came from us. This
will only take effect on 64-bit systems (x86-64 and aarch64).
Before:
Did 1496 ECDH P-256 operations in 1038743us (1440.2 ops/sec)
Did 2783 ECDSA P-256 signing operations in 1081006us (2574.5 ops/sec)
Did 2400 ECDSA P-256 verify operations in 1059508us (2265.2 ops/sec)
After:
Did 4147 ECDH P-256 operations in 1061723us (3905.9 ops/sec)
Did 9372 ECDSA P-256 signing operations in 1040589us (9006.4 ops/sec)
Did 4114 ECDSA P-256 verify operations in 1063478us (3868.4 ops/sec)
Change-Id: I11fabb03239cc3a7c4a97325ed4e4c97421f91a9
Instead, each module defines a static CRYPTO_EX_DATA_CLASS to hold the values.
This makes CRYPTO_cleanup_all_ex_data a no-op as spreading the
CRYPTO_EX_DATA_CLASSes across modules (and across crypto and ssl) makes cleanup
slightly trickier. We can make it do something if needbe, but it's probably not
worth the trouble.
Change-Id: Ib6f6fd39a51d8ba88649f0fa29c66db540610c76
Reviewed-on: https://boringssl-review.googlesource.com/4375
Reviewed-by: Adam Langley <agl@google.com>
Callers are required to use the wrappers now. They still need OPENSSL_EXPORT
since crypto and ssl get built separately in the standalone shared library
build.
Change-Id: I61186964e6099b9b589c4cd45b8314dcb2210c89
Reviewed-on: https://boringssl-review.googlesource.com/4372
Reviewed-by: Adam Langley <agl@google.com>
OpenSSH, especially, does some terrible things that mean that it needs
the EVP_CIPHER structure to be exposed ☹. Damian is open to a better API
to replace this, but only if OpenSSL agree too. Either way, it won't be
happening soon.
Change-Id: I393b7a6af6694d4d2fe9ebcccd40286eff4029bd
Reviewed-on: https://boringssl-review.googlesource.com/4330
Reviewed-by: Adam Langley <agl@google.com>
Beyond generally eliminating unnecessary includes, eliminate as many
includes of headers that declare/define particularly error-prone
functionality like strlen, malloc, and free. crypto/err/internal.h was
added to remove the dependency on openssl/thread.h from the public
openssl/err.h header. The include of <stdlib.h> in openssl/mem.h was
retained since it defines OPENSSL_malloc and friends as macros around
the stdlib.h functions. The public x509.h, x509v3.h, and ssl.h headers
were not changed in order to minimize breakage of source compatibility
with external code.
Change-Id: I0d264b73ad0a720587774430b2ab8f8275960329
Reviewed-on: https://boringssl-review.googlesource.com/4220
Reviewed-by: Adam Langley <agl@google.com>
A previous change in BoringSSL renamed ERR_print_errors_fp to
BIO_print_errors_fp as part of refactoring the code to improve the
layering of modules within BoringSSL. Rename it back for better
compatibility with code that was using the function under the original
name. Move its definition back to crypto/err using an implementation
that avoids depending on crypto/bio.
Change-Id: Iee7703bb1eb4a3d640aff6485712bea71d7c1052
Reviewed-on: https://boringssl-review.googlesource.com/4310
Reviewed-by: Adam Langley <agl@google.com>
Prior to this change, when EC_GROUP_get0_generator fails, BN_CTX_end
would get called even though BN_CTX_start hadn't been called yet, in
the case where the caller-supplied |ctx| is not NULL.
Change-Id: I6f728e74f0167193891cdb6f122b20b0770283dc
Reviewed-on: https://boringssl-review.googlesource.com/4271
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: If15f2f0e2b4627318c9cdfbc76d5ca56a6894e3f
Reviewed-on: https://boringssl-review.googlesource.com/4270
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Reported by the LibreSSL project as a follow on to CVE-2015-0209
(Imported from upstream's 5e5d53d341fd9a9b9cc0a58eb3690832ca7a511f.)
Change-Id: Ic2e5dc5c96e316c55f76bedc6ea55b416be3287a
Reviewed-on: https://boringssl-review.googlesource.com/4049
Reviewed-by: Adam Langley <agl@google.com>
Upstream decided to make the caller free the scratch space rather than the
callee. May as well match. (Existing code is pretty inconsistent. This API
pattern needs to go.)
See upstream's 9e442d485008046933cdc7da65080f436a4af089.
Change-Id: I7c9fcae5778a74d6ae8e9f546e03fb2cf6e48426
Reviewed-on: https://boringssl-review.googlesource.com/3671
Reviewed-by: Adam Langley <agl@google.com>
EC_GROUP_copy is an rather unfriendly function; it doesn't work if the groups
have different[*] underlying EC_METHODs, but this notion is not exposed through
the API. I found no callers of EC_GROUP_copy in external code.
This leaves the precompute_mult functions as the remaining mutable API exposed
through EC_GROUP.
[*] Though, of the two EC_METHODs right now, simple.c is entirely unused.
Change-Id: Iabb52518005250fb970e12b3b0ea78b4f6eff4a0
Reviewed-on: https://boringssl-review.googlesource.com/3631
Reviewed-by: Adam Langley <agl@google.com>
They do the same thing. This removes all callers of EC_GROUP_copy outside
EC_GROUP_dup.
Change-Id: I65433ee36040de79e56483dfece774e01e2e2743
Reviewed-on: https://boringssl-review.googlesource.com/3630
Reviewed-by: Adam Langley <agl@google.com>
(I got this wrong when reading the OpenSSL code.)
Change-Id: Ib289ef41d0ab5a3157ad8b9454d2de96d1f86c22
Reviewed-on: https://boringssl-review.googlesource.com/3620
Reviewed-by: Adam Langley <agl@google.com>
It was a mistake to remove this in the first place.
Change-Id: Icd97b4db01e49151daa41dd892f9da573ddc2842
Reviewed-on: https://boringssl-review.googlesource.com/3541
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Caught by malloc valgrind tests on Basic-Client-Sync. Also one by inspection
and verified with valgrind. Those should pass now with the exception of
CRYPTO_free_ex_data being internally implemented with malloc.
(Clearly we next should make our malloc tests assert that the containing
function fails to catch when we fail to check for some error and things
silently move one.)
Change-Id: I56c51dc8a32a7d3c7ac907d54015dc241728c761
Reviewed-on: https://boringssl-review.googlesource.com/3440
Reviewed-by: Adam Langley <agl@google.com>
Previously, error strings were kept in arrays for each subdirectory and
err.c would iterate over them all and insert them at init time to a hash
table.
This means that, even if you have a shared library and lots of processes
using that, each process has ~30KB of private memory from building that
hash table.
This this change, all the error strings are built into a sorted list and
are thus static data. This means that processes can share the error
information and it actually saves binary space because of all the
pointer overhead in the old scheme. Also it saves the time taken
building the hash table at startup.
This removes support for externally-supplied error string data.
Change-Id: Ifca04f335c673a048e1a3e76ff2b69c7264635be
Including string.h in base.h causes any file that includes a BoringSSL
header to include string.h. Generally this wouldn't be a problem,
although string.h might slow down the compile if it wasn't otherwise
needed. However, it also causes problems for ipsec-tools in Android
because OpenSSL didn't have this behaviour.
This change removes string.h from base.h and, instead, adds it to each
.c file that requires it.
Change-Id: I5968e50b0e230fd3adf9b72dd2836e6f52d6fb37
Reviewed-on: https://boringssl-review.googlesource.com/3200
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
