Initialization by multiple consumers on ARM is still problematic due to
CRYPTO_set_NEON_{capable,functional}, until we reimplement that in-library, but
if that is called before the first CRYPTO_library_init, this change makes it
safe.
BUG=556462
Change-Id: I5845d09cca909bace8293ba7adf09a3bd0d4f943
Reviewed-on: https://boringssl-review.googlesource.com/6519
Reviewed-by: Adam Langley <agl@google.com>
The |ri| field was only used in |BN_MONT_CTX_set|, so make it a local
variable of that function.
Change-Id: Id8c3d44ac2e30e3961311a7b1a6731fe2c33a0eb
Reviewed-on: https://boringssl-review.googlesource.com/6526
Reviewed-by: Adam Langley <agl@google.com>
The comment in |BN_mod_inverse_ex| makes it clear that |BN_BITS2| was
intended. Besides fixing the code to match the comment, remove
the now-unused |BN_BITS| and the already-unused |BN_MASK| to prevent
future confusion of this sort.
On MSVC builds there seems to be very little difference in performance
between the two code paths according to |bssl speed|.
Change-Id: I765b7b3d464e2057b1d7952af25b6deb2724976a
Reviewed-on: https://boringssl-review.googlesource.com/6525
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>
This ensures the run_tests target updates those binaries.
Change-Id: I32b68026da4852424b5621e014e71037c8a5754c
Reviewed-on: https://boringssl-review.googlesource.com/6513
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>
Nexus 7 goes from 1002.8 ops/sec to 4704.8 at a cost of 10KB of code.
(It'll actually save code if built with -mfpu=neon because then the
generic version can be discarded by the compiler.)
Change-Id: Ia6d02efb2c2d1bb02a07eb56ec4ca3b0dba99382
Reviewed-on: https://boringssl-review.googlesource.com/6524
Reviewed-by: Adam Langley <agl@google.com>
If -mfpu=neon is passed then we don't need to worry about checking for
NEON support at run time. This change allows |CRYPTO_is_NEON_capable| to
statically return 1 in this case. This then allows the compiler to
discard generic code in several cases.
Change-Id: I3b229740ea3d5cb0a304f365c400a0996d0c66ef
Reviewed-on: https://boringssl-review.googlesource.com/6523
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
MSVC doesn't like unary minus on unsigned types. Also, the speed test
always failed because the inputs were all zeros and thus had small
order.
Change-Id: Ic2d3c2c9bd57dc66295d93891396871cebac1e0b
It can fail on FreeBSD when library is not linked against either
threading library and results in init routine not being executed
at all, leading to errors in other parts of the code.
Change-Id: I1063f6940e381e6470593c063fbfecf3f47991cd
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6522
Reviewed-by: Adam Langley <agl@google.com>
Relevant code was removed in 5d5e39f5d2.
Change-Id: I198844064030c04f88e5541f2bbaa29ae13d14bb
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6521
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
stdint.h already has macros for this. The spec says that, in C++,
__STDC_CONSTANT_MACROS is needed, so define it for bytestring_test.cc.
Chromium seems to use these macros without trouble, so I'm assuming we
can rely on them.
Change-Id: I56d178689b44d22c6379911bbb93d3b01dd832a3
Reviewed-on: https://boringssl-review.googlesource.com/6510
Reviewed-by: Adam Langley <agl@google.com>
Until we've done away with the d2i_* stack completely, boundaries need
to be mindful of the type mismatch. d2i_* takes a long, not a size_t.
Change-Id: If02f9ca2cfde02d0929ac18275d09bf5df400f3a
Reviewed-on: https://boringssl-review.googlesource.com/6491
Reviewed-by: Adam Langley <agl@google.com>
Triggered by RT#3989.
(Imported from upstream's fbab8baddef8d3346ae40ff068871e2ddaf10270. This
doesn't seem to affect us, but avoid getting out of sync.)
Change-Id: I164e2a72e4b75e286ceaa03745ed9bcbf6c3e32e
Reviewed-on: https://boringssl-review.googlesource.com/6512
Reviewed-by: Adam Langley <agl@google.com>
To no great surprise, ASAN didn't like this test and I suspect that
Chromium, with its crashing allocator, won't like it either. Oh well.
Change-Id: I235dbb965dbba186f8f37d7df45f8eac9addc7eb
Reviewed-on: https://boringssl-review.googlesource.com/6496
Reviewed-by: Adam Langley <agl@google.com>
People expect to do:
CBB foo;
if (!CBB_init(&foo, 100) ||
…
…) {
CBB_cleanup(&foo);
return 0;
}
However, currently, if the allocation of |initial_capacity| fails in
|CBB_init| then |CBB_cleanup| will operate on uninitialised values. This
change makes the above pattern safe.
Change-Id: I3e002fda8f0a3ac18650b504e7e84a842d4165ca
Reviewed-on: https://boringssl-review.googlesource.com/6495
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSH calls |RAND_seed| before jailing in the expectation that that
will be sufficient to ensure that later RAND calls are successful.
See internal bug 25695426.
Change-Id: I9d3f5665249af6610328ac767cb83059bb2953dd
Reviewed-on: https://boringssl-review.googlesource.com/6494
Reviewed-by: David Benjamin <davidben@chromium.org>
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>
There's a few things that will be kind of a nuisance and possibly not worth it
(crypto/asn1 dumps a lot of undeclared things, etc.). But it caught some
mistakes. Even without the warning, making sure to include the externs before
defining a function helps catch type mismatches.
Change-Id: I3dab282aaba6023e7cebc94ed7a767a5d7446b08
Reviewed-on: https://boringssl-review.googlesource.com/6484
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>
Found with -Wtype-limits.
Change-Id: I5580f179425bc6b09ff2a8559fce121b0cc8ae14
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6463
Reviewed-by: Adam Langley <agl@google.com>
Found with -Wtype-limits.
Change-Id: I41cdbb7e6564b715dfe445877a89594371fdeef0
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/6462
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>
dh.c had a 10k-bit limit but it wasn't quite correctly enforced. However,
that's still 1.12s of jank on the IO thread, which is too long. Since the SSL
code consumes DHE groups from the network, it should be responsible for
enforcing what sanity it needs on them.
Costs of various bit lengths on 2013 Macbook Air:
1024 - 1.4ms
2048 - 14ms
3072 - 24ms
4096 - 55ms
5000 - 160ms
10000 - 1.12s
UMA says that DHE groups are 0.2% 4096-bit and otherwise are 5.5% 2048-bit and
94% 1024-bit and some noise. Set the limit to 4096-bit to be conservative,
although that's already quite a lot of jank.
BUG=554295
Change-Id: I8e167748a67e4e1adfb62d73dfff094abfa7d215
Reviewed-on: https://boringssl-review.googlesource.com/6464
Reviewed-by: Adam Langley <agl@google.com>
The file armv8-mont.pl is taken from upstream. The speed ups are fairly
modest (~30%) but seem worthwhile.
Before:
Did 231 RSA 2048 signing operations in 1008671us (229.0 ops/sec)
Did 11208 RSA 2048 verify operations in 1036997us (10808.1 ops/sec)
Did 342 RSA 2048 (3 prime, e=3) signing operations in 1021545us (334.8 ops/sec)
Did 32000 RSA 2048 (3 prime, e=3) verify operations in 1016162us (31491.0 ops/sec)
Did 45 RSA 4096 signing operations in 1039805us (43.3 ops/sec)
Did 3608 RSA 4096 verify operations in 1060283us (3402.9 ops/sec)
After:
Did 300 RSA 2048 signing operations in 1009772us (297.1 ops/sec)
Did 12740 RSA 2048 verify operations in 1075413us (11846.6 ops/sec)
Did 408 RSA 2048 (3 prime, e=3) signing operations in 1016139us (401.5 ops/sec)
Did 33000 RSA 2048 (3 prime, e=3) verify operations in 1017510us (32432.1 ops/sec)
Did 52 RSA 4096 signing operations in 1067678us (48.7 ops/sec)
Did 3408 RSA 4096 verify operations in 1062863us (3206.4 ops/sec)
Change-Id: Ife74fac784067fce3668b5c87f51d481732ff855
Reviewed-on: https://boringssl-review.googlesource.com/6444
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 52e028b9de371da62c1e51b46592517b1068d770.)
Change-Id: If980d774671b9b5ba997db3fd7d4043525a85609
Reviewed-on: https://boringssl-review.googlesource.com/6445
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
When building in OPENSSL_NO_ASM mode, MSVC complains about unreachable
code. The redundant initialization of |i| is the main problem. The
skipping of the first test of the condition |i < num| with |goto| was
also confusing.
It turns out that |bn_mul_mont| is only called when assembly language
optimizations are available, but in that case the assmebly language
versions will always be used instead. Although this code will be
compiled in |OPENSSL_NO_ASM| builds, it is never called in
|OPENSSL_NO_ASM| builds. Thus, it can just be removed.
Change-Id: Id551899b2602824978edc1a1cb0703b76516808d
Reviewed-on: https://boringssl-review.googlesource.com/5550
Reviewed-by: Adam Langley <agl@google.com>
The previous logic only defined
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| when the assembly language
optimizations were enabled, but
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| is also useful when the C
implementations are used.
If support for ARM processors that don't support unaligned access is
important, then it might be better to condition the enabling of
|SHA512_BLOCK_CAN_MANAGE_UNALIGNED_DATA| on ARM based on more specific
flags.
Change-Id: Ie8c37c73aba308c3ccf79371ce5831512e419989
Reviewed-on: https://boringssl-review.googlesource.com/6402
Reviewed-by: Adam Langley <agl@google.com>
Most functions can take this in as const. Note this changes an
RSA_METHOD hook, though one I would not expect anyone to override.
Change-Id: Ib70ae65e5876b01169bdc594e465e3e3c4319a8b
Reviewed-on: https://boringssl-review.googlesource.com/6419
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>
This change fixes up several comments (many of which were spotted by
Kenny Root) and also changes doc.go to detect cases where comments don't
start with the correct word. (This is a common error.)
Since we have docs builders now, these errors will be found
automatically in the future.
Change-Id: I58c6dd4266bf3bd4ec748763c8762b1a67ae5ab3
Reviewed-on: https://boringssl-review.googlesource.com/6440
Reviewed-by: Adam Langley <agl@google.com>
This function allows one to extract the current IVs from an SSL
connection. This is needed for the CBC cipher suites with implicit IVs
because, for those, the IV can't be extracted from the handshake key
material.
Change-Id: I247a1d0813b7a434b3cfc88db86d2fe8754344b6
Reviewed-on: https://boringssl-review.googlesource.com/6433
Reviewed-by: Adam Langley <agl@google.com>
The documentation in md32_common.h is now (more) correct with respect
to the most important details of the layout of |HASH_CTX|. The
documentation explaining why sha512.c doesn't use md32_common.h is now
more accurate as well.
Before, the C implementations of HASH_BLOCK_DATA_ORDER took a pointer
to the |HASH_CTX| and the assembly language implementations took a
pointer to the hash state |h| member of |HASH_CTX|. (This worked
because |h| is always the first member of |HASH_CTX|.) Now, the C
implementations take a pointer directly to |h| too.
The definitions of |MD4_CTX|, |MD5_CTX|, and |SHA1_CTX| were changed to
be consistent with |SHA256_CTX| and |SHA512_CTX| in storing the hash
state in an array. This will break source compatibility with any
external code that accesses the hash state directly, but will not
affect binary compatibility.
The second parameter of |HASH_BLOCK_DATA_ORDER| is now of type
|const uint8_t *|; previously it was |void *| and all implementations
had a |uint8_t *data| variable to access it as an array of bytes.
This change paves the way for future refactorings such as automatically
generating the |*_Init| functions and/or sharing one I-U-F
implementation across all digest algorithms.
Change-Id: I6e9dd09ff057c67941021d324a4fa1d39f58b0db
Reviewed-on: https://boringssl-review.googlesource.com/6405
Reviewed-by: Adam Langley <agl@google.com>
Although those are only created by code owned by RSA_METHOD, custom RSA_METHODs
shouldn't be allowed to squat our internal fields and then change how you free
things.
Remove 'method' from their names now that they're not method-specific.
Change-Id: I9494ef9a7754ad59ac9fba7fd463b3336d826e0b
Reviewed-on: https://boringssl-review.googlesource.com/6423
Reviewed-by: Adam Langley <agl@google.com>
This restores the original semantics of the finished hook.
Change-Id: I70da393c7e66fb6e3be1e2511e08b34bb54fc0b4
Reviewed-on: https://boringssl-review.googlesource.com/6422
Reviewed-by: Adam Langley <agl@google.com>
Having a single RSA_METHOD means they all get pulled in. Notably, RSA key
generation pulls in the primality-checking code.
Change-Id: Iece480113754da090ddf87b64d8769f01e05d26c
Reviewed-on: https://boringssl-review.googlesource.com/6389
Reviewed-by: Adam Langley <agl@google.com>
This will allow a static linker (with -ffunction-sections since things aren't
split into files) to drop unused parts of DH and DSA. Notably, the parameter
generation bits pull in primality-checking code.
Change-Id: I25087e4cb91bc9d0ab43bcb267c2e2c164e56b59
Reviewed-on: https://boringssl-review.googlesource.com/6388
Reviewed-by: Adam Langley <agl@google.com>
Removing the function codes continued to sample __func__ for compatibility with
ERR_print_errors_cb, but not ERR_error_string_n. We can just emit
OPENSSL_internal for both. ERR_print_errors_cb already has the file and line
number available which is strictly more information than the function name.
(ERR_error_string_n does not, but we'd already turned that to
OPENSSL_internal.)
This shaves 100kb from a release build of the bssl tool.
In doing so, put an unused function code parameter back into ERR_put_error to
align with OpenSSL. We don't need to pass an additional string in anymore, so
OpenSSL compatibility with anything which uses ERR_LIB_USER or
ERR_get_next_error_library costs nothing. (Not that we need it.)
Change-Id: If6af34628319ade4145190b6f30a0d820e00b20d
Reviewed-on: https://boringssl-review.googlesource.com/6387
Reviewed-by: Adam Langley <agl@google.com>
