Operations in the DSA signing algorithm should run in constant time in
order to avoid side channel attacks. A flaw in the OpenSSL DSA
implementation means that a non-constant time codepath is followed for
certain operations. This has been demonstrated through a cache-timing
attack to be sufficient for an attacker to recover the private DSA key.
CVE-2016-2178
(Imported from upstream's 621eaf49a289bfac26d4cbcdb7396e796784c534 and
b7d0f2834e139a20560d64c73e2565e93715ce2b.)
We should eventually not depend on BN_FLG_CONSTTIME since it's a mess (seeing
as the original fix was wrong until we reported b7d0f2834e to them), but, for
now, go with the simplest fix.
Change-Id: I9ea15c1d1cc3a7e21ef5b591e1879ec97a179718
Reviewed-on: https://boringssl-review.googlesource.com/8172
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
More spring-cleaning of unnecessary incompatibilities. Since
OpenSSL_add_all_algorithms_conf doesn't specify a configuration file, it's
perfectly sound to have such a function.
Dear BoringSSL, please add all algorithms.
Uh, sure. They were already all there, but I have added them!
PS: Could you also load all your configuration files while you're at it.
...I don't have any. Fine. I have loaded all configuration files which I
recognize. *mutters under breath* why does everyone ask all these strange
questions...
Change-Id: I57f956933d9e519445bf22f89853bd5f56904172
Reviewed-on: https://boringssl-review.googlesource.com/8160
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Some files were named 𝑥_test.txt and some 𝑥_tests.txt. This change
unifies around the latter.
Change-Id: Id6f29bad8b998f3c3466655097ef593f7f18f82f
Reviewed-on: https://boringssl-review.googlesource.com/8150
Reviewed-by: David Benjamin <davidben@google.com>
Some OpenSSL consumers use them, so provide no-op versions to make porting code
easier.
Change-Id: I4348568c1cb08d2b2c0a9ec9a17e2c0449260965
Reviewed-on: https://boringssl-review.googlesource.com/8142
Reviewed-by: David Benjamin <davidben@google.com>
Make building against software that expects OpenSSL easier.
Change-Id: I1af090ae8208218d6e226ee0baf51053699d85cc
Reviewed-on: https://boringssl-review.googlesource.com/8141
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Windows is, not unreasonably, complaining that taking abs() of an unsigned is
ridiculous. But these values actually are signed and fit very easily in an int
anyway.
Change-Id: I34fecaaa3616732112e3eea105a7c84bd9cd0bae
Reviewed-on: https://boringssl-review.googlesource.com/8144
Reviewed-by: Adam Langley <agl@google.com>
Otherwise builds fail with:
crypto/newhope/newhope_statistical_test.cc:136:27: error: format specifies type 'long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat]
Change-Id: I85d5816c1d7ee71eef362bffe983b2781ce310a4
One of these tests the distribution of noise polynomials; the other
tests that that agreed-upon keys (prior to whitening) have roughly equal
numbers of 0s and 1s.
Along the way, expose a few more API bits.
Change-Id: I6b04708d41590de45d82ea95bae1033cfccd5d67
Reviewed-on: https://boringssl-review.googlesource.com/8130
Reviewed-by: Adam Langley <agl@google.com>
This commit adds coverage of the "offer" (first) step, as well as
testing all outputs of the "accept" (second) step, not just the shared
key.
Change-Id: Id11fe24029abc302442484a6c01fa496a1578b3a
Reviewed-on: https://boringssl-review.googlesource.com/8100
Reviewed-by: Adam Langley <agl@google.com>
The test vectors are taken from the reference implementation, modified
to output the results of its random-number generator, and the results of
key generation prior to SHA3. This allows the interoperability of the
two implementations to be tested somewhat.
To accomplish the testing, this commit creates a new, lower-level API
that leaves the generation of random numbers and all wire encoding and
decoding up to the caller.
Change-Id: Ifae3517696dde4be4a0b7c1998bdefb789bac599
Reviewed-on: https://boringssl-review.googlesource.com/8070
Reviewed-by: Adam Langley <agl@google.com>
Now that we no longer support Windows XP, this is available.
Unfortunately, the public header version of CRYPTO_MUTEX means we
still can't easily merge CRYPTO_MUTEX and CRYPTO_STATIC_MUTEX.
BUG=37
Change-Id: If309de3f06e0854c505083b72fd64d1dbb3f4563
Reviewed-on: https://boringssl-review.googlesource.com/8081
Reviewed-by: Adam Langley <agl@google.com>
Windows SRWLOCK requires you call different functions here. Split
them up in preparation for switching Windows from CRITICAL_SECTION.
BUG=37
Change-Id: I7b5c6a98eab9ae5bb0734b805cfa1ff334918f35
Reviewed-on: https://boringssl-review.googlesource.com/8080
Reviewed-by: Adam Langley <agl@google.com>
This code has caused a long history of problems. This change rewrites it
completely with something that is, hopefully, much simplier and robust
and adds more testing.
Change-Id: Ibeef51f9386afd95d5b73316e451eb3a2d7ec4e0
Reviewed-on: https://boringssl-review.googlesource.com/8033
Reviewed-by: Adam Langley <agl@google.com>
The key schedule in TLS 1.3 requires a separate Extract and Expand phase
for the cryptographic computations.
Change-Id: Ifdac1237bda5212de5d4f7e8db54e202151d45ec
Reviewed-on: https://boringssl-review.googlesource.com/7983
Reviewed-by: David Benjamin <davidben@google.com>
CECPQ1 is a new key exchange that concatenates the results of an X25519
key agreement and a NEWHOPE key agreement.
Change-Id: Ib919bdc2e1f30f28bf80c4c18f6558017ea386bb
Reviewed-on: https://boringssl-review.googlesource.com/7962
Reviewed-by: David Benjamin <davidben@google.com>
Use of strdup, close, lseek, read, and write prevent linking
statically againt libcmt.lib.
Change-Id: I04f7876ec0f03f29f000bbcc6b2ccdec844452d2
Reviewed-on: https://boringssl-review.googlesource.com/8010
Reviewed-by: David Benjamin <davidben@google.com>
This is consistent with the new convention in ssl_ecdh.c.
Along the way, change newhope_test.c to not iterate 1000 times over each
test.
Change-Id: I7a500f45b838eba8f6df96957891aa8e880ba089
Reviewed-on: https://boringssl-review.googlesource.com/8012
Reviewed-by: David Benjamin <davidben@google.com>
BUF_MEM is actually a rather silly API for the SSL stack. There's separate
length and max fields, but init_buf effectively treats length as max and max as
nothing.
We possibly don't want to be using it long-term anyway (if nothing else, the
char*/uint8_t* thing is irritating), but in the meantime, it'll be easier to
separately fix up get_message's book-keeping and state tracking from where the
handshake gets its messages from.
Change-Id: I9e56ea008173991edc8312ec707505ead410a9ee
Reviewed-on: https://boringssl-review.googlesource.com/7947
Reviewed-by: Adam Langley <agl@google.com>
This function will return whether BoringSSL was built with
OPENSSL_NO_ASM. This will allow us to write a test in our internal
codebase which asserts that normal builds should always have assembly
code included.
Change-Id: Ib226bf63199022f0039d590edd50c0cc823927b9
Reviewed-on: https://boringssl-review.googlesource.com/7960
Reviewed-by: David Benjamin <davidben@google.com>
The performance measurements seem to be very out-of-date. Also, the
idea for optimizing the case of an even modulus is interesting, but it
isn't useful because we never use an even modulus.
Change-Id: I012eb37638cda3c63db0e390c8c728f65b949e54
Reviewed-on: https://boringssl-review.googlesource.com/7733
Reviewed-by: David Benjamin <davidben@google.com>
This function is only really useful for DSA signature verification,
which is something that isn't performance-sensitive. Replace its
optimized implementation with a naïve implementation that's much
simpler.
Note that it would be simpler to use |BN_mod_mul| in the new
implementation; |BN_mod_mul_montgomery| is used instead only to be
consistent with other work being done to replace uses of non-Montgomery
modular reduction with Montgomery modular reduction.
Change-Id: If587d463b73dd997acfc5b7ada955398c99cc342
Reviewed-on: https://boringssl-review.googlesource.com/7732
Reviewed-by: David Benjamin <davidben@google.com>
sk_FOO_num may be called on const stacks. Given that was wrong, I suspect no
one ever uses a const STACK_OF(T)...
Other macros were correctly const, but were casting the constness a way (only
to have it come back again).
Also remove the extra newline after a group. It seems depending on which
version of clang-format was being used, we'd either lose or keep the extra
newline. The current file doesn't have them, so settle on that.
Change-Id: I19de6bc85b0a043d39c05ee3490321e9f0adec60
Reviewed-on: https://boringssl-review.googlesource.com/7946
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
When *pp is NULL, don't write garbage, return an unexpected pointer
or leak memory on error.
(Imported from upstream's 36c37944909496a123e2656ad1f651769a7cc72f.)
This calling convention...
Change-Id: Ic733092cfb942a3e1d3ceda6797222901ad55bef
Reviewed-on: https://boringssl-review.googlesource.com/7944
Reviewed-by: Adam Langley <agl@google.com>
|BN_mod_exp_mont_word| is only useful when the base is a single word
in length and timing side channel protection of the exponent is not
needed. That's never the case in real life.
Keep the function in the API, but removes its single-word-base
optimized implementation with a call to |BN_mod_exp_mont|.
Change-Id: Ic25f6d4f187210b681c6ee6b87038b64a5744958
Reviewed-on: https://boringssl-review.googlesource.com/7731
Reviewed-by: David Benjamin <davidben@google.com>
|BN_mod_exp_mont| will forward to |BN_mod_exp_mont_consttime|, so this
is a no-op semantically. However, this allows the linker to drop the
implementation of |BN_mod_exp_mont| even when the DH code is in use.
Change-Id: I0cb8b260224ed661ede74923bd134acb164459c1
Reviewed-on: https://boringssl-review.googlesource.com/7730
Reviewed-by: David Benjamin <davidben@google.com>
Also add a test.
This is the last of the openssl/asn1.h includes from the directories that are
to be kept in the core libcrypto library. (What remains is to finish sorting
out the crypto/obj stuff. We'll also want to retain a decoupled version of the
PKCS#12 stuff.)
Functions that need to be audited for reuse:
i2d_DHparams
BUG=54
Change-Id: Ibef030a98d3a93ae26e8e56869f14858ec75601b
Reviewed-on: https://boringssl-review.googlesource.com/7900
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Previously, the verification was only done when using the CRT method,
as the CRT method has been shown to be extremely sensitive to fault
attacks. However, there's no reason to avoid doing the verification
when the non-CRT method is used (performance-sensitive applications
should always be using the CRT-capable keys).
Previously, when we detected a fault (attack) through this verification,
libcrypto would fall back to the non-CRT method and assume that the
non-CRT method would give a correct result, despite having just
detecting corruption that is likely from an attack. Instead, just give
up, like NSS does.
Previously, the code tried to handle the case where the input was not
reduced mod rsa->n. This is (was) not possible, so avoid trying to
handle that. This simplifies the equality check and lets us use
|CRYPTO_memcmp|.
Change-Id: I78d1e55520a1c8c280cae2b7256e12ff6290507d
Reviewed-on: https://boringssl-review.googlesource.com/7582
Reviewed-by: David Benjamin <davidben@google.com>
Only treat an ASN1_ANY type as an integer if it has the V_ASN1_INTEGER
tag: V_ASN1_NEG_INTEGER is an internal only value which is never used
for on the wire encoding.
(Imported from upstream's d4b25980020821d4685752ecb9105c0902109ab5.)
This is redundant with our fb2c6f8c85 which I
think is a much better fix (having two notions of "type" depending on whether
we're in an ASN1_TYPE or an ASN1_STRING is fragile), so I think we should keep
our restriction too. Still, this is also worth doing.
Change-Id: I6ea54aae7b517a59c6e563d8c993d0ee22e25bee
Reviewed-on: https://boringssl-review.googlesource.com/7848
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 172c6e1e14defe7d49d62f5fc9ea6a79b225424f, but note our
values have different types. In particular, because we put in_len in a size_t
and C implicitly requires that all valid buffers' lengths fit in a ptrdiff_t
(signed), the overflow was impossible, assuming EVP_ENCODE_CTX::length is
untouched externally.
More importantly, this function is stuck taking an int output and has no return
value, so the only plausible contract is the caller is responsible for ensuring
the length fits anyway. Indeed, callers all call EVP_EncodeUpdate in bounded
chunks, so upstream's analysis is off.
Anyway, in theory that logic could locally overflow, so tweak it slightly. Tidy
up some of the variable names while I'm here.
Change-Id: Ifa78707cc26c11e0d67019918a028531b3d6738c
Reviewed-on: https://boringssl-review.googlesource.com/7847
Reviewed-by: Adam Langley <agl@google.com>
This adds an explicit limit to the size of an X509_NAME structure. Some
part of OpenSSL (e.g. TLS) already effectively limit the size due to
restrictions on certificate size.
See also upstream's 65cb92f4da37a3895437f0c9940ee0bcf9f28c8a, although this is
different from upstream's. Upstream's version bounds both the X509_NAME *and*
any data after it in the immediately containing structure. While adding a bound
on all of crypto/asn1 is almost certainly a good idea (will look into that for
a follow-up), it seems bizarre and unnecessary to have X509_NAME affect its
parent.
Change-Id: Ica2136bcd1455d7c501ccc6ef2a19bc5ed042501
Reviewed-on: https://boringssl-review.googlesource.com/7846
Reviewed-by: Adam Langley <agl@google.com>
An overflow can occur in the EVP_EncryptUpdate function. If an attacker is
able to supply very large amounts of input data after a previous call to
EVP_EncryptUpdate with a partial block then a length check can overflow
resulting in a heap corruption.
Following an analysis of all OpenSSL internal usage of the
EVP_EncryptUpdate function all usage is one of two forms.
The first form is like this:
EVP_EncryptInit()
EVP_EncryptUpdate()
i.e. where the EVP_EncryptUpdate() call is known to be the first called
function after an EVP_EncryptInit(), and therefore that specific call
must be safe.
The second form is where the length passed to EVP_EncryptUpdate() can be seen
from the code to be some small value and therefore there is no possibility of
an overflow. [BoringSSL: We also have code that calls EVP_CIPHER functions by
way of the TLS/SSL3 "AEADs". However, there we know the inputs are bounded by
2^16.]
Since all instances are one of these two forms, I believe that there can
be no overflows in internal code due to this problem.
It should be noted that EVP_DecryptUpdate() can call EVP_EncryptUpdate()
in certain code paths. Also EVP_CipherUpdate() is a synonym for
EVP_EncryptUpdate(). Therefore I have checked all instances of these
calls too, and came to the same conclusion, i.e. there are no instances
in internal usage where an overflow could occur.
This could still represent a security issue for end user code that calls
this function directly.
CVE-2016-2106
Issue reported by Guido Vranken.
(Imported from upstream's 3ab937bc440371fbbe74318ce494ba95021f850a.)
Change-Id: Iabde896555c39899c7f0f6baf7a163a7b3c2f3d6
Reviewed-on: https://boringssl-review.googlesource.com/7845
Reviewed-by: Adam Langley <agl@google.com>
Upstream decided to reset *pp on error and to later fix up the other i2d
functions to behave similarly. See upstream's
c5e603ee182b40ede7713c6e229c15a8f3fdb58a.
Change-Id: I01f82b578464060d0f2be5460fe4c1b969124c8e
Reviewed-on: https://boringssl-review.googlesource.com/7844
Reviewed-by: Adam Langley <agl@google.com>
Sanity check field lengths and sums to avoid potential overflows and reject
excessively large X509_NAME structures.
Issue reported by Guido Vranken.
(Imported from upstream's 9b08619cb45e75541809b1154c90e1a00450e537.)
Change-Id: Ib2e1e7cd086f9c3f0d689d61947f8ec3e9220049
Reviewed-on: https://boringssl-review.googlesource.com/7842
Reviewed-by: Adam Langley <agl@google.com>
Reject zero length buffers passed to X509_NAME_oneline().
Issue reported by Guido Vranken.
(Imported from upstream's 66e731ab09f2c652d0e179df3df10d069b407604.)
Tweaked slightly to use <= 0 instead of == 0 since the length is signed.
Change-Id: I5ee54d77170845e4699fda7df5e94538c8e55ed9
Reviewed-on: https://boringssl-review.googlesource.com/7841
Reviewed-by: Adam Langley <agl@google.com>
The traditional private key encryption algorithm doesn't function
properly if the IV length of the cipher is zero. These ciphers
(e.g. ECB mode) are not suitable for private key encryption
anyway.
(Imported from upstream's 4436299296cc10c6d6611b066b4b73dc0bdae1a6.)
Change-Id: I218c9c1d11274ef11b7c0cfce380521efa415215
Reviewed-on: https://boringssl-review.googlesource.com/7840
Reviewed-by: Adam Langley <agl@google.com>
In the past we have needed the ability to deploy security fixes to our
frontend systems without leaking them in source code or in published
binaries.
This change adds a function that provides some infrastructure for
supporting this in BoringSSL while meeting our internal build needs. We
do not currently have any specific patch that requires this—this is
purely preparation.
Change-Id: I5c64839e86db4e5ea7419a38106d8f88b8e5987e
Reviewed-on: https://boringssl-review.googlesource.com/7849
Reviewed-by: David Benjamin <davidben@google.com>
BUG=43
Change-Id: I46ad1ca62b8921a03fae51f5d7bbe1c68fc0b170
Reviewed-on: https://boringssl-review.googlesource.com/7821
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The logic to reset *pp doesn't actually work if pp is NULL. (It also doesn't
work if *pp is NULL, but that didn't work before either.) Don't bother
resetting it. This is consistent with the template-based i2d functions which do
not appear to leave *pp alone on error.
Will send this upstream.
Change-Id: I9fb5753e5d36fc1d490535720b8aa6116de69a70
Reviewed-on: https://boringssl-review.googlesource.com/7812
Reviewed-by: Adam Langley <agl@google.com>
See upstream's 34b9acbd3f81b46967f692c0af49020c8c405746.
Change-Id: I88d5b3cfbbe87e883323a9e6e1bf85227ed9576e
Reviewed-on: https://boringssl-review.googlesource.com/7811
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
See also upstream's 91fb42ddbef7a88640d1a0f853c941c20df07de7, though that has a
bug if |out| was non-NULL on entry. (I'll send them a patch.)
Change-Id: I807f23007b89063c23e02dac11c4ffb41f847fdf
Reviewed-on: https://boringssl-review.googlesource.com/7810
Reviewed-by: David Benjamin <davidben@google.com>
GCC gets unhappy if we don't initialize the padding.
Change-Id: I084ffee1717d9025dcb10d8f32de0da2339c7f01
Reviewed-on: https://boringssl-review.googlesource.com/7797
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If we're to allow the buggy CPU workaround to fire when __ARM_NEON__ is set,
CRYPTO_is_NEON_capable also needs to be aware of it. Also add an API to export
this value out of BoringSSL, so we can get some metrics on how prevalent this
chip is.
BUG=chromium:606629
Change-Id: I97d65a47a6130689098b32ce45a8c57c468aa405
Reviewed-on: https://boringssl-review.googlesource.com/7796
Reviewed-by: Adam Langley <agl@google.com>
The getauxval (and friends) code would be filling that in anyway. The default
only serves to enable NEON even if the OS is old enough to be missing getauxval
(and everything else).
Notably, this unbreaks the has_buggy_neon code when __ARM_NEON__ is set, as is
the case in Chrome for Android, as of M50. Before, the default
OPENSSL_armcap_P value was getting in the way.
Arguably, this doesn't make a whole lot of sense. We're saying we'll let the
CPU run compiler-generated NEON code, but not our hand-crafted stuff. But, so
far, we only have evidence of the hand-written NEON tickling the bug and not
the compiler-generated stuff, so avoid the unintentional regression. (Naively,
I would expect the hand-crafted NEON is better at making full use of the
pipeline and is thus more likely to tickle the CPU bug.)
This is not the fix for M50, as in the associated Chromium bug, but it will fix
master and M51. M50 will instead want to revert
https://codereview.chromium.org/1730823002.
BUG=chromium:606629
Change-Id: I394f97fea2f09891dd8fa30e0ec6fc6b1adfab7a
Reviewed-on: https://boringssl-review.googlesource.com/7794
Reviewed-by: Adam Langley <agl@google.com>
This reverts commits:
- 9158637142
- a90aa64302
- c0d8b83b44
It turns out code outside of BoringSSL also mismatches Init and Update/Final
functions. Since this is largely cosmetic, it's probably not worth the cost to
do this.
Change-Id: I14e7b299172939f69ced2114be45ccba1dbbb704
Reviewed-on: https://boringssl-review.googlesource.com/7793
Reviewed-by: Adam Langley <agl@google.com>
As with SHA512_Final, use the different APIs rather than store md_len.
Change-Id: Ie1150de6fefa96f283d47aa03de0f18de38c93eb
Reviewed-on: https://boringssl-review.googlesource.com/7722
Reviewed-by: Adam Langley <agl@google.com>
This is in preparation for taking md_len out of SHA256_CTX by allowing us to do
something similar to SHA512_CTX. md32_common.h now emits a static "finish"
function which Final composes with the extraction step.
Change-Id: I314fb31e2482af642fd280500cc0e4716aef1ac6
Reviewed-on: https://boringssl-review.googlesource.com/7721
Reviewed-by: Adam Langley <agl@google.com>
Rather than store md_len, factor out the common parts of SHA384_Final and
SHA512_Final and then extract the right state. Also add a missing
SHA384_Transform and be consistent about "1" vs "one" in comments.
This also removes the NULL output special-case which no other hash function
had.
Change-Id: If60008bae7d7d5b123046a46d8fd64139156a7c5
Reviewed-on: https://boringssl-review.googlesource.com/7720
Reviewed-by: Adam Langley <agl@google.com>
There was only one function that required BoringSSL to know how to read
directories. Unfortunately, it does have some callers and it's not immediately
obvious whether the code is unreachable. Rather than worry about that, just
toss it all into decrepit.
In doing so, do away with the Windows and PNaCl codepaths. Only implement
OPENSSL_DIR_CTX on Linux.
Change-Id: Ie64d20254f2f632fadc3f248bbf5a8293ab2b451
Reviewed-on: https://boringssl-review.googlesource.com/7661
Reviewed-by: Adam Langley <agl@google.com>
The copy of mingw-w64 used by Android isn't new enough and is missing half of
the INIT_ONCE definitions. (But not the other half, strangely.) Work around
this for now.
Change-Id: I5c7e89db481f932e03477e50cfb3cbacaeb630e6
Reviewed-on: https://boringssl-review.googlesource.com/7790
Reviewed-by: Adam Langley <agl@google.com>
Rather than use an internal function in a test (which would need an
OPENSSL_EXPORT to work in a shared-library build), this change corrupts
the secret key directly.
Change-Id: Iee501910b23a0affaa0639dcc773d6ea2d0c5a82
Reviewed-on: https://boringssl-review.googlesource.com/7780
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <agl@google.com>
C and C++ disagree on the sizes of empty structs, which can be rather bad for
structs embedded in public headers. Stick a char in them to avoid issues. (It
doesn't really matter for CRYPTO_STATIC_MUTEX, but it's easier to add a char in
there too.)
Thanks to Andrew Chi for reporting this issue.
Change-Id: Ic54fff710b688decaa94848e9c7e1e73f0c58fd3
Reviewed-on: https://boringssl-review.googlesource.com/7760
Reviewed-by: David Benjamin <davidben@google.com>
(Imported from upstream's 2442382e11c022aaab4fdc6975bd15d5a75c4db2 and
0ca67644ddedfd656d43a6639d89a6236ff64652)
Change-Id: I601ef07e39f936e8f3e30412fd90cd339d712dc4
Reviewed-on: https://boringssl-review.googlesource.com/7742
Reviewed-by: David Benjamin <davidben@google.com>
The i2d_X509() function can return a negative value on error. Therefore
we should make sure we check it.
Issue reported by Yuan Jochen Kang.
(Imported from upstream's 8f43c80bfac15544820739bf035df946eeb603e8)
Change-Id: If247d5bf1d792eb7c6dc179b606ed21ea0ccdbb8
Reviewed-on: https://boringssl-review.googlesource.com/7743
Reviewed-by: David Benjamin <davidben@google.com>
If the ASN.1 BIO is presented with a large length field read it in
chunks of increasing size checking for EOF on each read. This prevents
small files allocating excessive amounts of data.
CVE-2016-2109
Thanks to Brian Carpenter for reporting this issue.
(Imported from upstream's f32774087f7b3db1f789688368d16d917757421e)
Change-Id: Id1b0d4436c4879d0ba7d3b7482b937cafffa28f7
Reviewed-on: https://boringssl-review.googlesource.com/7741
Reviewed-by: David Benjamin <davidben@google.com>
Forgot to mark something static.
Change-Id: I497075d0ad27e2062f84528fb568b333e72a7d3b
Reviewed-on: https://boringssl-review.googlesource.com/7753
Reviewed-by: David Benjamin <davidben@google.com>
It's not possible to encode an OID with only one component, so some of
the NIDs do not have encodings. The logic to actually encode OIDs checks
for this (before calling der_it), but not the logic to compute the
sorted OID list.
Without this, OBJ_obj2nid, when given an empty OID, returns something
arbitrary based on the binary search implementation instead of
NID_undef.
Change-Id: Ib68bae349f66eff3d193616eb26491b6668d4b0a
Reviewed-on: https://boringssl-review.googlesource.com/7752
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
C gets grumpy when you shift into a sign bit. Replace it with a different bit
trick.
BUG=chromium:603502
Change-Id: Ia4cc2e2d68675528b7c0155882ff4d6230df482b
Reviewed-on: https://boringssl-review.googlesource.com/7740
Reviewed-by: Adam Langley <agl@google.com>
We already had coverage for our new EVP_PKEY parsers, but it's good to have
some that cover them directly. The initial corpus was generated manually with
der-ascii and should cover most of the insanity around EC key serialization.
BUG=15
Change-Id: I7aaf56876680bfd5a89f5e365c5052eee03ba862
Reviewed-on: https://boringssl-review.googlesource.com/7728
Reviewed-by: Adam Langley <agl@google.com>
The x86-64 version of this assembly doesn't include this function. It's
in decrepit/rc4 as a compatibility backfill but that means that 32-bit
builds end up with two definitions of this symbol.
Change-Id: Ib6da6b91aded8efc679ebbae6d60c96a78f3dc4e
Reviewed-on: https://boringssl-review.googlesource.com/7734
Reviewed-by: David Benjamin <davidben@google.com>
Avoid calculating the affine Y coordinate when the caller didn't ask
for it, as occurs, for example, in ECDH.
For symmetry and clarity, avoid calculating the affine X coordinate in
the hypothetical case where the caller only asked for the Y coordinate.
Change-Id: I69f5993fa0dfac8b010c38e695b136cefc277fed
Reviewed-on: https://boringssl-review.googlesource.com/7590
Reviewed-by: David Benjamin <davidben@google.com>
This is purely hypothetical, as in real life nobody cares about the
|y| component without also caring about the |x| component, but it
clarifies the code and makes a future change clearer.
Change-Id: Icaa4de83c87b82a8e68cd2942779a06e5db300c3
Reviewed-on: https://boringssl-review.googlesource.com/7588
Reviewed-by: David Benjamin <davidben@google.com>
The result would not be correct if, on input, |x->neg != 0| or
|y->neg != 0|.
Change-Id: I645566a78c2e18e42492fbfca1df17baa05240f7
Reviewed-on: https://boringssl-review.googlesource.com/7587
Reviewed-by: David Benjamin <davidben@google.com>
Use only Montgomery math in |ec_GFp_mont_point_get_affine_coordinates|.
In particular, avoid |BN_mod_sqr| and |BN_mod_mul|.
Change-Id: I05c8f831d2865d1b105cda3871e9ae67083f8399
Reviewed-on: https://boringssl-review.googlesource.com/7586
Reviewed-by: David Benjamin <davidben@google.com>
usleep is guarded by feature macro insanity. Use nanosleep which looks to be
less unfriendly.
Change-Id: I75cb2284f26cdedabb19871610761ec7440b6ad3
Reviewed-on: https://boringssl-review.googlesource.com/7710
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Now that we no longer support Windows XP, this function is available. In doing
so, remove the odd run_once_arg_t union and pass in a pointer to a function
pointer which is cleaner and still avoids C's silly rule where function
pointers can't be placed in a void*.
BUG=37
Change-Id: I44888bb3779dacdb660706debd33888ca389ebd5
Reviewed-on: https://boringssl-review.googlesource.com/7613
Reviewed-by: David Benjamin <davidben@google.com>
The existing tests never actually tested this case.
Change-Id: Idb9cf0cbbe32fdf5cd353656a95fbedbaac09376
Reviewed-on: https://boringssl-review.googlesource.com/7612
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is avoids pulling in BIGNUM for doing a straight-forward addition on a
block-sized value, and avoids a ton of mallocs. It's also -Wconversion-clean,
unlike the old one.
In doing so, this replaces the HMAC_MAX_MD_CBLOCK with EVP_MAX_MD_BLOCK_SIZE.
By having the maximum block size available, most of the temporary values in the
key derivation don't need to be malloc'd.
BUG=22
Change-Id: I940a62bba4ea32bf82b1190098f3bf185d4cc7fe
Reviewed-on: https://boringssl-review.googlesource.com/7688
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also switch the EVP_CIPHER copy to cut down on how frequently we need to cast
back and forth.
BUG=22
Change-Id: I9af1e586ca27793a4ee6193bbb348cf2b28a126e
Reviewed-on: https://boringssl-review.googlesource.com/7689
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The EVP_MD versions do, so the types should bubble up.
BUG=22
Change-Id: Ibccbc9ff35bbfd3d164fc28bcdd53ed97c0ab338
Reviewed-on: https://boringssl-review.googlesource.com/7687
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Require the public exponent to be available unless
|RSA_FLAG_NO_BLINDING| is set on the key. Also, document this.
If the public exponent |e| is not available, then we could compute it
from |p|, |q|, and |d|. However, there's no reasonable situation in
which we'd have |p| or |q| but not |e|; either we have all the CRT
parameters, or we have (e, d, n), or we have only (d, n). The
calculation to compute |e| exposes the private key to risk of side
channel attacks.
Also, it was particularly wasteful to compute |e| for each
|BN_BLINDING| created, instead of just once before the first
|BN_BLINDING| was created.
|BN_BLINDING| now no longer needs to contain a duplicate copy of |e|,
so it is now more space-efficient.
Note that the condition |b->e != NULL| in |bn_blinding_update| was
always true since commit cbf56a5683.
Change-Id: Ic2fd6980e0d359dcd53772a7c31bdd0267e316b4
Reviewed-on: https://boringssl-review.googlesource.com/7594
Reviewed-by: David Benjamin <davidben@google.com>
This reduces the chance of double-frees.
BUG=10
Change-Id: I11a240e2ea5572effeddc05acb94db08c54a2e0b
Reviewed-on: https://boringssl-review.googlesource.com/7583
Reviewed-by: David Benjamin <davidben@google.com>
When |rsa->e == NULL| we cannot verify the result, so using the CRT
would leave the key too vulnerable to fault attacks.
Change-Id: I154622cf6205ba4d5fb219143db6072a787c2d1f
Reviewed-on: https://boringssl-review.googlesource.com/7581
Reviewed-by: David Benjamin <davidben@google.com>
|CRYPTO_memcmp| isn't necessary because there is no secret data being
acted on here.
Change-Id: Ib678d5d4fc16958aca409a93df139bdff8cb73fb
Reviewed-on: https://boringssl-review.googlesource.com/7465
Reviewed-by: David Benjamin <davidben@google.com>
Use the common pattern of returning early instead of |goto err;| when
there's no cleanup to do yet. Also, move the error checking of
|BN_CTX_get| failure closer to the the calls to |BN_CTX_get|. Avoid
calling |OPENSSL_cleanse| on public data. Clarify when/why |buf| is not
freed.
Change-Id: I9df833db7eb7041c5af9349c461297372b988f98
Reviewed-on: https://boringssl-review.googlesource.com/7464
Reviewed-by: David Benjamin <davidben@google.com>
The same check is already done in |RSA_verify_raw|, so |RSA_verify|
doesn't need to do it.
Also, move the |RSA_verify_raw| check earlier.
Change-Id: I15f7db0aad386c0f764bba53e77dfc46574f7635
Reviewed-on: https://boringssl-review.googlesource.com/7463
Reviewed-by: David Benjamin <davidben@google.com>
We do not need to support engine-provided verification methods.
Change-Id: Iaad8369d403082b728c831167cc386fdcabfb067
Reviewed-on: https://boringssl-review.googlesource.com/7311
Reviewed-by: David Benjamin <davidben@google.com>
I don't think I ever look at that output. This way our builds are nice and
silent.
Change-Id: Idb215e3702f530a8b8661622c726093530885c91
Reviewed-on: https://boringssl-review.googlesource.com/7700
Reviewed-by: Adam Langley <agl@google.com>
In OpenSSL, socket BIOs only used recv/send on Windows and read/write on POSIX.
Align our socket BIOs with that behavior. This should be a no-op, but avoids
frustrating consumers overly sensitive to the syscalls used now that SSL_set_fd
has switched to socket BIOs to align with OpenSSL. b/28138582.
Change-Id: Id4870ef8e668e587d6ef51c5b5f21e03af66a288
Reviewed-on: https://boringssl-review.googlesource.com/7686
Reviewed-by: Adam Langley <agl@google.com>
One of the codepaths didn't free the group. Found by libFuzzer.
BUG=chromium:603893
Change-Id: Icb81f2f89a8c1a52e29069321498986b193a0e56
Reviewed-on: https://boringssl-review.googlesource.com/7685
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from b9077d85b0042d3d5d877d5cf7f06a8a8c035673.)
Change-Id: I6df3b3d0913b001712a78671c69b9468e059047f
Reviewed-on: https://boringssl-review.googlesource.com/7682
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
There was only one function that required BoringSSL to know how to read
directories. Unfortunately, it does have some callers and it's not immediately
obvious whether the code is unreachable. Rather than worry about that, just
toss it all into decrepit.
In doing so, do away with the Windows and PNaCl codepaths. Only implement
OPENSSL_DIR_CTX on Linux.
Change-Id: I3eb55b098e3aa042b422bb7da115c0812685553e
This slipped through, but all the callers are now using
EVP_aead_chacha20_poly1305, so we can remove this version.
Change-Id: I76eb3a4481aae4d18487ca96ebe3776e60d6abe8
Reviewed-on: https://boringssl-review.googlesource.com/7650
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: Id181957956ccaacc6c29b641a1f1144886d442c0
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7630
Reviewed-by: David Benjamin <davidben@google.com>
BUG=chromium:499653
Change-Id: I4e8d4af3129dbf61d4a8846ec9db685e83999d5e
Reviewed-on: https://boringssl-review.googlesource.com/7565
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Instead, embed the (very short) encoding of the OID into built_in_curve.
BUG=chromium:499653
Change-Id: I0db36f83c71fbd3321831f54fa5022f8304b30cd
Reviewed-on: https://boringssl-review.googlesource.com/7564
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
A lot of consumers of obj.h only want the NID values. Others didn't need
it at all. This also removes some OBJ_nid2sn and OBJ_nid2ln calls in EVP
error paths which isn't worth pulling a large table in for.
BUG=chromium:499653
Change-Id: Id6dff578f993012e35b740a13b8e4f9c2edc0744
Reviewed-on: https://boringssl-review.googlesource.com/7563
Reviewed-by: David Benjamin <davidben@google.com>
obj_mac.h is missing #include guards, so one cannot use NIDs without
pulling in the OBJ_* functions which depend on the giant OID table. Give
it #include guards, tidy up the style slightly, and also rename it to
nid.h which is a much more reasonable name.
obj_mac.h is kept as a forwarding header as, despite it being a little
screwy, some code #includes it anyway.
BUG=chromium:499653
Change-Id: Iec0b3f186c02e208ff1f7437bf27ee3a5ad004b7
Reviewed-on: https://boringssl-review.googlesource.com/7562
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Whatever compiler settings AOSP is using warns that this is a GNU extension.
Change-Id: Ife395d2b206b607b14c713cbb5a94d479816dad0
Reviewed-on: https://boringssl-review.googlesource.com/7604
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This reverts commit 6f0c4db90e except for the
imported assembly files, which are left as-is but unused. Until upstream fixes
https://rt.openssl.org/Ticket/Display.html?id=4483, we shouldn't ship this
code. Once that bug has been fixed, we'll restore it.
Change-Id: I74aea18ce31a4b79657d04f8589c18d6b17f1578
Reviewed-on: https://boringssl-review.googlesource.com/7602
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The documentation in |RSA_METHOD| says that the |ctx| parameter to
|mod_exp| can be NULL, however the default implementation doesn't
handle that case. That wouldn't matter since internally it is always
called with a non-NULL |ctx| and it is static, but an external
application could get a pointer to |mod_exp| by extracting it from
the default |RSA_METHOD|. That's unlikely, but making that impossible
reduces the chances that future refactorings will cause unexpected
trouble.
Change-Id: Ie0e35e9f107551a16b49c1eb91d0d3386604e594
Reviewed-on: https://boringssl-review.googlesource.com/7580
Reviewed-by: David Benjamin <davidben@google.com>
|BN_mod_mul_montgomery| has better constant-time behavior (usually)
than |BN_mod_mul| and |BN_mod_sqr| and on platforms where we have
assembly language optimizations (when |OPENSSL_BN_ASM_MONT| is set in
crypto/bn/montgomery.c) it is faster. While doing so, reorder and
rename the |BN_MONT_CTX| parameters of the blinding functions to match
the order normally used in Montgomery math functions.
As a bonus, remove a redundant copy of the RSA public modulus from the
|BN_BLINDING| structure, which reduces memory usage.
Change-Id: I70597e40246429c7964947a1dc46d0d81c7530ef
Reviewed-on: https://boringssl-review.googlesource.com/7524
Reviewed-by: David Benjamin <davidben@google.com>
In VS2015's debug runtime, the C runtime has been unloaded by the time
DLL_PROCESS_DETACH is called and things crash. Instead, don't run destructors
at that point.
This means we do *not* free memory associated with any remaining thread-locals
on application shutdown, only shutdown of individual threads. This is actually
desirable since it's consistent with pthreads. If an individual thread calls
pthread_exit, destructors are run. If the entire process exits, they are not.
(It's also consistent with thread_none.c which never bothers to free
anything.)
BUG=chromium:595795
Change-Id: I3e64d46ea03158fefff583c1e3e12dfa0c0e172d
Reviewed-on: https://boringssl-review.googlesource.com/7601
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In the case |BN_CTX_get| failed, the function returned without calling
|BN_CTX_end|. Fix that.
Change-Id: Ia24cba3256e2cec106b539324e9679d690048780
Reviewed-on: https://boringssl-review.googlesource.com/7592
Reviewed-by: David Benjamin <davidben@google.com>
It looks like we started reformatting that function and adding curly braces,
etc., but forget to finish it. This is corroborated by the diff. Although git
thinks I removed the EAY-style one and tweaked the #if-0'd one, I actually
clang-formatted the EAY-style one anew and deleted the #if-0'd one after
tweaking the style to match. Only difference is the alignment stuff is
uintptr_t rather than intptr_t since the old logic was using unsigned
arithmetic.
Change-Id: Ia244e4082a6b6aed3ef587d392d171382c32db33
Reviewed-on: https://boringssl-review.googlesource.com/7574
Reviewed-by: David Benjamin <davidben@google.com>
This code is only used in ec_montgomery.c, so |field_encode| and
|field_decode| are never NULL.
Change-Id: I42a3ad5744d4ed6f0be1707494411e7efcf930ff
Reviewed-on: https://boringssl-review.googlesource.com/7585
Reviewed-by: David Benjamin <davidben@google.com>
It is only used in ec_montgomery.c, so move it there.
Change-Id: Ib189d5579d6363bdc1da89b775ad3df824129758
Reviewed-on: https://boringssl-review.googlesource.com/7584
Reviewed-by: David Benjamin <davidben@google.com>
These only affect the tests.
Change-Id: If22d047dc98023501c771787b485276ece92d4a2
Reviewed-on: https://boringssl-review.googlesource.com/7573
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Patch from https://mta.openssl.org/pipermail/openssl-dev/2016-March/005625.html.
Upstream has yet to make a decision on aliasing requirements for their
assembly. If they choose to go with the stricter aliasing requirement rather
than land this patch, we'll probably want to tweak EVP_AEAD's API guarantees
accordingly and then undiverge.
In the meantime, import this to avoid a regression on x86 from when we had
compiler-vectorized code on GCC platforms. Per our assembly coverage tools and
pending multi-CPU-variant tests, we have good coverage here. Unlike Poly1305
(which is currently waiting on yet another upstream bugfix), where there is
risk of missed carries everywhere, it is much more difficult to accidentally
make a ChaCha20 implementation that fails based on the data passed into it.
This restores a sizeable speed improvement on x86.
Before:
Did 1131000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000205us (1130768.2 ops/sec): 18.1 MB/s
Did 161000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1006136us (160018.1 ops/sec): 216.0 MB/s
Did 28000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1023264us (27363.4 ops/sec): 224.2 MB/s
Did 1166000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000447us (1165479.0 ops/sec): 18.6 MB/s
Did 160000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1004818us (159232.8 ops/sec): 215.0 MB/s
Did 30000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1016977us (29499.2 ops/sec): 241.7 MB/s
After:
Did 2208000 ChaCha20-Poly1305 (16 bytes) seal operations in 1000031us (2207931.6 ops/sec): 35.3 MB/s
Did 402000 ChaCha20-Poly1305 (1350 bytes) seal operations in 1001717us (401310.9 ops/sec): 541.8 MB/s
Did 97000 ChaCha20-Poly1305 (8192 bytes) seal operations in 1005394us (96479.6 ops/sec): 790.4 MB/s
Did 2444000 ChaCha20-Poly1305-Old (16 bytes) seal operations in 1000089us (2443782.5 ops/sec): 39.1 MB/s
Did 459000 ChaCha20-Poly1305-Old (1350 bytes) seal operations in 1000563us (458741.7 ops/sec): 619.3 MB/s
Did 97000 ChaCha20-Poly1305-Old (8192 bytes) seal operations in 1007942us (96235.7 ops/sec): 788.4 MB/s
Change-Id: I976da606dae062a776e0cc01229ec03a074035d1
Reviewed-on: https://boringssl-review.googlesource.com/7561
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I24d0179ca5019e82ca1494c8773f373f8c09ce82
Reviewed-on: https://boringssl-review.googlesource.com/7566
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I0effe99d244c4ccdbb0e34db6e01a59c9463cb15
Reviewed-on: https://boringssl-review.googlesource.com/7572
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Some ARMv8 Android devices don't have AT_HWCAP2. This means, when running in
32-bit mode (ARM capability APIs on Linux are different between AArch32 and
AArch64), we can't discover the various nice instructions.
On a Nexus 6P, this gives a, uh, minor performance win when running in 32-bit
mode.
Before:
Did 1085000 AES-128-GCM (16 bytes) seal operations in 1000003us (1084996.7 ops/sec): 17.4 MB/s
Did 60000 AES-128-GCM (1350 bytes) seal operations in 1013416us (59205.7 ops/sec): 79.9 MB/s
Did 11000 AES-128-GCM (8192 bytes) seal operations in 1019778us (10786.7 ops/sec): 88.4 MB/s
Did 1009000 AES-256-GCM (16 bytes) seal operations in 1000650us (1008344.6 ops/sec): 16.1 MB/s
Did 49000 AES-256-GCM (1350 bytes) seal operations in 1015698us (48242.7 ops/sec): 65.1 MB/s
Did 9394 AES-256-GCM (8192 bytes) seal operations in 1071104us (8770.4 ops/sec): 71.8 MB/s
Did 1557000 SHA-1 (16 bytes) operations in 1000317us (1556506.6 ops/sec): 24.9 MB/s
Did 762000 SHA-1 (256 bytes) operations in 1000527us (761598.6 ops/sec): 195.0 MB/s
Did 45000 SHA-1 (8192 bytes) operations in 1013773us (44388.6 ops/sec): 363.6 MB/s
Did 1459000 SHA-256 (16 bytes) operations in 1000271us (1458604.7 ops/sec): 23.3 MB/s
Did 538000 SHA-256 (256 bytes) operations in 1000990us (537467.9 ops/sec): 137.6 MB/s
Did 26000 SHA-256 (8192 bytes) operations in 1008403us (25783.3 ops/sec): 211.2 MB/s
After:
Did 1890000 AES-128-GCM (16 bytes) seal operations in 1000068us (1889871.5 ops/sec): 30.2 MB/s
Did 509000 AES-128-GCM (1350 bytes) seal operations in 1000112us (508943.0 ops/sec): 687.1 MB/s
Did 110000 AES-128-GCM (8192 bytes) seal operations in 1007966us (109130.7 ops/sec): 894.0 MB/s
Did 1960000 AES-256-GCM (16 bytes) seal operations in 1000303us (1959406.3 ops/sec): 31.4 MB/s
Did 460000 AES-256-GCM (1350 bytes) seal operations in 1001873us (459140.0 ops/sec): 619.8 MB/s
Did 97000 AES-256-GCM (8192 bytes) seal operations in 1005337us (96485.1 ops/sec): 790.4 MB/s
Did 1927000 SHA-1 (16 bytes) operations in 1000429us (1926173.7 ops/sec): 30.8 MB/s
Did 1151000 SHA-1 (256 bytes) operations in 1000425us (1150511.0 ops/sec): 294.5 MB/s
Did 87000 SHA-1 (8192 bytes) operations in 1003089us (86732.1 ops/sec): 710.5 MB/s
Did 2357390 SHA-256 (16 bytes) operations in 1000116us (2357116.6 ops/sec): 37.7 MB/s
Did 1410000 SHA-256 (256 bytes) operations in 1000176us (1409751.9 ops/sec): 360.9 MB/s
Did 101000 SHA-256 (8192 bytes) operations in 1007007us (100297.2 ops/sec): 821.6 MB/s
BUG=chromium:596156
Change-Id: Iacc1f8d8a07e991d4615f2e12c5c54923fb31aa2
Reviewed-on: https://boringssl-review.googlesource.com/7507
Reviewed-by: David Benjamin <davidben@google.com>
This removes the thread-unsafe SIGILL-based detection and the
multi-consumer-hostile CRYPTO_set_NEON_capable API. (Changing
OPENSSL_armcap_P after initialization is likely to cause problems.)
The right way to detect ARM features on Linux is getauxval. On aarch64,
we should be able to rely on this, so use it straight. Split this out
into its own file. The #ifdefs in the old cpu-arm.c meant it shared all
but no code with its arm counterpart anyway.
Unfortunately, various versions of Android have different missing APIs, so, on
arm, we need a series of workarounds. Previously, we used a SIGILL fallback
based on OpenSSL's logic, but this is inherently not thread-safe. (SIGILL also
does not tell us if the OS knows how to save and restore NEON state.) Instead,
base the behavior on Android NDK's cpu-features library, what Chromium
currently uses with CRYPTO_set_NEON_capable:
- Android before API level 20 does not provide getauxval. Where missing,
we can read from /proc/self/auxv.
- On some versions of Android, /proc/self/auxv is also not readable, so
use /proc/cpuinfo's Features line.
- Linux only advertises optional features in /proc/cpuinfo. ARMv8 makes NEON
mandatory, so /proc/cpuinfo can't be used without additional effort.
Finally, we must blacklist a particular chip because the NEON unit is broken
(https://crbug.com/341598).
Unfortunately, this means CRYPTO_library_init now depends on /proc being
available, which will require some care with Chromium's sandbox. The
simplest solution is to just call CRYPTO_library_init before entering
the sandbox.
It's worth noting that Chromium's current EnsureOpenSSLInit function already
depends on /proc/cpuinfo to detect the broken CPU, by way of base::CPU.
android_getCpuFeatures also interally depends on it. We were already relying on
both of those being stateful and primed prior to entering the sandbox.
BUG=chromium:589200
Change-Id: Ic5d1c341aab5a614eb129d8aa5ada2809edd6af8
Reviewed-on: https://boringssl-review.googlesource.com/7506
Reviewed-by: David Benjamin <davidben@google.com>
Many of the compatibility issues are described at
https://msdn.microsoft.com/en-us/library/mt612856.aspx. The macros
that suppressed warnings on a per-function basis no longer work in
Update 1, so replace them with #pragmas. Update 1 warns when |size_t|
arguments to |printf| are casted, so stop doing that casting.
Unfortunately, this requires an ugly hack to continue working in
MSVC 2013 as MSVC 2013 doesn't support "%zu". Finally, Update 1 has new
warnings, some of which need to be suppressed.
---
Updated by davidben to give up on suppressing warnings in crypto/x509 and
crypto/x509v3 as those directories aren't changed much from upstream. In each
of these cases, upstream opted just blindly initialize the variable, so do the
same. Also switch C4265 to level 4, per Microsoft's recommendation and work
around a bug in limits.h that happens to get fixed by Google include order
style.
(limits.h is sensitive to whether corecrt.h, pulled in by stddef.h and some
other headers, is included before it. The reason it affected just one file is
we often put the file's header first, which means base.h is pulling in
stddef.h. Relying on this is ugly, but it's no worse than what everything else
is doing and this doesn't seem worth making something as tame as limits.h so
messy to use.)
Change-Id: I02d1f935356899f424d3525d03eca401bfa3e6cd
Reviewed-on: https://boringssl-review.googlesource.com/7480
Reviewed-by: David Benjamin <davidben@google.com>
Conscrypt, thanks to Java's RSAPrivateKeySpec API, must be able to use RSA keys
with only modulus and exponent. This is kind of silly and breaks the blinding
code so they, both in OpenSSL and BoringSSL, had to explicitly turn blinding
off.
Add a test for this as we're otherwise sure to break it on accident.
We may wish to avoid the silly rsa->flags modification, I'm not sure. For now,
keep the requirement in so other consumers do not accidentally rely on this.
(Also add a few missing ERR_clear_error calls. Functions which are expected to
fail should be followed by an ERR_clear_error so later unexpected failures
don't get confused.)
BUG=boringssl:12
Change-Id: I674349821f1f59292b8edd085f21dc37e8bcaa75
Reviewed-on: https://boringssl-review.googlesource.com/7560
Reviewed-by: David Benjamin <davidben@google.com>
In |bn_blinding_update| the condition |b->e != NULL| would never be
true (probably), but the test made reasoning about the correctness of
the code confusing. That confusion was amplified by the circuitous and
unusual way in which |BN_BLINDING|s are constructed. Clarify all this
by simplifying the construction of |BN_BLINDING|s, making it more like
the construction of other structures.
Also, make counter unsigned as it is no longer ever negative.
Change-Id: I6161dcfeae19a80c780ccc6762314079fca1088b
Reviewed-on: https://boringssl-review.googlesource.com/7530
Reviewed-by: David Benjamin <davidben@google.com>
Simplify the code by always caching Montgomery contexts in the RSA
structure, regardless of the |RSA_FLAG_CACHE_PUBLIC| and
|RSA_FLAG_CACHE_PRIVATE| flags. Deprecate those flags.
Now that we do this no more than once per key per RSA exponent, the
private key exponents better because the initialization of the
Montgomery contexts isn't perfectly side-channel protected.
Change-Id: I4fbcfec0f2f628930bfeb811285b0ae3d103ac5e
Reviewed-on: https://boringssl-review.googlesource.com/7521
Reviewed-by: David Benjamin <davidben@google.com>
The function wants the expected value first.
Change-Id: I6d3e21ebfa55d6dd99a34fe8380913641b4f5ff6
Reviewed-on: https://boringssl-review.googlesource.com/7501
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Imported from patch attached to
https://rt.openssl.org/Ticket/Display.html?id=4439.
But with the extra vs $extra typo fixed.
The root problem appears to be that lazy_reduction tries to use paddd instead
of paddq when they believe the sum will not overflow a u32. In the final call
to lazy_reduction, this is not true. svaldez and I attempted to work through
the bounds, but the bounds derived from the cited paper imply paddd is always
fine. Empirically in a debugger, the bounds are exceeded in the test case.
I requested more comments from upstream on the bug. When upstream lands their
final fix (hopefully with comments), I will update this code. In the meantime,
let's stop carrying known-broken stuff.
(vlazy_reduction is probably something similar, but since we don't enable that
code, we haven't bothered analyzing it.)
Also add the smaller of the two test cases that catch the bug. (The other uses
an update pattern which isn't quite what poly1305_test does.)
Change-Id: I446ed47c21f10b41a0745de96ab119a3f6fd7801
Reviewed-on: https://boringssl-review.googlesource.com/7544
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: If76154c8d255600e925a408acdc674fc7dad0359
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7526
Reviewed-by: David Benjamin <davidben@google.com>
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>
There is a potential double free in EVP_DigestInit_ex. This is believed
to be reached only as a result of programmer error - but we should fix it
anyway.
(Imported from upstream's e78dc7e279ed98e1ab9845a70d14dafdfdc88f58)
Change-Id: I1da7be7db7afcbe9f30f168df000d64ed73d7edd
Reviewed-on: https://boringssl-review.googlesource.com/7541
Reviewed-by: David Benjamin <davidben@google.com>
We recently gained -Werror=missing-prototypes. (See also, we really need to get
those Android bots...)
Change-Id: I3962d3050bccf5f5a057d029b5cbff1695ca1a03
Reviewed-on: https://boringssl-review.googlesource.com/7540
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The fields of the |bn_blinding_st| are not updated atomically.
Consequently, one field (|A| or |Ai|) might get updated while the
other field (|Ai| or |A|) doesn't get updated, if an error occurs in
the middle of updating. Deal with this by reseting the counter so that
|A| and |Ai| will both get recreated the next time the blinding is
used.
Fix a separate but related issue by resetting the counter to zero after
calling |bn_blinding_create_param| only if |bn_blinding_create_param|
succeeded. Previously, regardless of whether an error occured in
|bn_blinding_create_param|, |b->counter| would get reset to zero. The
consequence of this was that potentially-bad blinding values would get
used 32 times instead of (32 - |b->counter|) times.
Change-Id: I236cdb6120870ef06cba129ed86619f593cbcf3d
Reviewed-on: https://boringssl-review.googlesource.com/7520
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>
Since the function doesn't call |BN_CTX_get|, it doesn't need to call
|BN_CTX_start|/|BN_CTX_end|.
Change-Id: I6cb954d3fee2959bdbc81b9b97abc52bb6f7704c
Reviewed-on: https://boringssl-review.googlesource.com/7469
Reviewed-by: David Benjamin <davidben@google.com>
As far as I can tell, this is the last place within libcrypto where
this type of check is missing.
Change-Id: I3d09676abab8c9f6c4e87214019a382ec2ba90ee
Reviewed-on: https://boringssl-review.googlesource.com/7519
Reviewed-by: David Benjamin <davidben@google.com>
Partially fixes build with -Wmissing-prototypes -Wmissing-declarations.
Change-Id: I51209c30f532899f57cfdd9a50cff0a8ee3da5b5
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7512
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>
Change-Id: I44aa9be26ad9aea6771cb46a886a721b4bc28fde
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7510
Reviewed-by: David Benjamin <davidben@google.com>
(Imported from upstream's 2460c7f13389d766dd65fa4e14b69b6fbe3e4e3b.)
This is a no-op for us, but avoid a diff with upstream.
Change-Id: I6e875704a38dcd9339371393a4dd523647aeef44
Reviewed-on: https://boringssl-review.googlesource.com/7491
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
(Imported from upstream's 01c32b5e448f6d42a23ff16bdc6bb0605287fa6f.)
Change-Id: Ib52278dbbac1ed1ad5c80f0ad69e34584d411cec
Reviewed-on: https://boringssl-review.googlesource.com/7461
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Not all assemblers of "gas" flavour handle binary constants, e.g.
seasoned MacOS Xcode doesn't, so give them a hand.
(Imported from upstream's ba26fa14556ba49466d51e4d9e6be32afee9c465.)
Change-Id: I35096dc8035e06d2fbef2363b869128da206ff9d
Reviewed-on: https://boringssl-review.googlesource.com/7459
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I6267c9bfb66940d0b6fe5368514210a058ebd3cc
Reviewed-on: https://boringssl-review.googlesource.com/7494
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
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>
I messed up a few of these.
ASN1_R_UNSUPPORTED_ALGORITHM doesn't exist. X509_R_UNSUPPORTED_ALGORITHM does
exist as part of X509_PUBKEY_set, but the SPKI parser doesn't emit this. (I
don't mind the legacy code having really weird errors, but since EVP is now
limited to things we like, let's try to keep that clean.) To avoid churn in
Conscrypt, we'll keep defining X509_R_UNSUPPORTED_ALGORITHM, but not actually
do anything with it anymore. Conscrypt was already aware of
EVP_R_UNSUPPORTED_ALGORITHM, so this should be fine. (I don't expect
EVP_R_UNSUPPORTED_ALGORITHM to go away. The SPKI parsers we like live in EVP
now.)
A few other ASN1_R_* values didn't quite match upstream, so make those match
again. Finally, I got some of the rsa_pss.c values wrong. Each of those
corresponds to an (overly specific) RSA_R_* value in upstream. However, those
were gone in BoringSSL since even the initial commit. We placed the RSA <-> EVP
glue in crypto/evp (so crypto/rsa wouldn't depend on crypto/evp) while upstream
placed them in crypto/rsa.
Since no one seemed to notice the loss of RSA_R_INVALID_SALT_LENGTH, let's undo
all the cross-module errors inserted in crypto/rsa. Instead, since that kind of
specificity is not useful, funnel it all into X509_R_INVALID_PSS_PARAMETERS
(formerly EVP_R_INVALID_PSS_PARAMETERS, formerly RSA_R_INVALID_PSS_PARAMETERS).
Reset the error codes for all affected modules.
(That our error code story means error codes are not stable across this kind of
refactoring is kind of a problem. Hopefully this will be the last of it.)
Change-Id: Ibfb3a0ac340bfc777bc7de6980ef3ddf0a8c84bc
Reviewed-on: https://boringssl-review.googlesource.com/7458
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This reverts commit ba70118d8e. Reverting this
did not resolve the regression and the cause is now known.
BUG=593963
Change-Id: Ic5e24b74e8f16b01d9fdd80f267a07ef026c82cf
Reviewed-on: https://boringssl-review.googlesource.com/7454
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
People seem to condition on these a lot. Since this code has now been moved
twice, just make them all cross-module errors rather than leave a trail of
renamed error codes in our wake.
Change-Id: Iea18ab3d320f03cf29a64a27acca119768c4115c
Reviewed-on: https://boringssl-review.googlesource.com/7431
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If the values of any of the coordinates in the output point |r| were
negative during nistz256 multiplication, then the calls to
|bn_set_word| would result in the wrong coordinates being returned
(the negatives of the correct coordinates would be returned instead).
Fix that.
Change-Id: I6048e62f76dca18f625650d11ef5a051c9e672a4
Reviewed-on: https://boringssl-review.googlesource.com/7442
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>
We never heap-allocate a GCM128_CONTEXT.
Change-Id: I7e89419ce4d81c1598a4b3a214c44dbbcd709651
Reviewed-on: https://boringssl-review.googlesource.com/7430
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Although exactly one iteration of cmov_cached will always initialize selected,
it ends up messing with uninitialized memory. Initialize |selected| before the
loop.
BUG=593540
Change-Id: I5921843f68c6dd1dc7f752538825bc43ba75df4a
Reviewed-on: https://boringssl-review.googlesource.com/7415
Reviewed-by: Arnar Birgisson <arnarb@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
