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>
The logic is a little hairy, partly because we used to support multiple
certificate slots.
Change-Id: Iee8503e61f5e0e91b7bcb15f526e9ef7cc7ad860
Reviewed-on: https://boringssl-review.googlesource.com/7823
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>
Sync a few changes with the upstream one, notably get_toolchain_if_necessary.py
needs GYP_MSVS_VERSION set. Also pull the variables that change up to the top.
This diverges a bit more from the upstream one, but we're already heavily
diverged. If we ever need to support two concurrent toolchains, I'll bring us
closer to parity.
Change-Id: I6db7fbaccd5dddd92ad2deee15bd6dd3e28841f7
Reviewed-on: https://boringssl-review.googlesource.com/7830
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Track the Chromium requirements. This makes our bots build with 2015 instead of
2013.
BUG=43
Change-Id: Id5329900a5d1d5fae4b5b22299ed47bc1b947dd8
Reviewed-on: https://boringssl-review.googlesource.com/7820
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
See util/bot/UPDATING for where they come from.
Change-Id: Ib2eae6efc737dd8c4e5fb001fd4b478102e0ad6a
Reviewed-on: https://boringssl-review.googlesource.com/7822
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>
Chromium uses GN's default configs feature which makes all targets default to a
set of configs. It then expects third_party code to take one of them
(chromium_code) out and put in a different one (no_chromium_code).
Because of that, we need a way to tell the template to emit -= lines. Add a
separate option for that.
(It may be worth making us clean against the chromium_code config rather than
the no_chromium_code one, but I'll explore that separately in case making the C
code clean ends up being a rabbithole.)
BUG=chromium:607294
Change-Id: I2aa179665ab17439cc123fc86a7af9690cd4bcd6
Reviewed-on: https://boringssl-review.googlesource.com/7795
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 term “#define”, when the line breaking happens to put at the
beginning of a line, confuses markdown. This change escapes the '#'.
Change-Id: I8300324f9e8c7561f32aba6fa29c0132a188a58b
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>
This is another thing that MSVC can't cope with:
..\tool\speed.cc(537) : error C2536: 'SpeedNewHope::<⋯>::SpeedNewHope::<⋯>::clientmsg' : cannot specify explicit initializer for arrays
Change-Id: I6b4cb430895f7794e9cef1b1c12b57ba5d537c64
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>
aosp-master has been updated past the point that this is necessary. Sadly, all
the other hacks still are. I'll try to get things rolling so we can ditch the
others in time.
Change-Id: If7b3aad271141fb26108a53972d2d3273f956e8d
Reviewed-on: https://boringssl-review.googlesource.com/7751
Reviewed-by: Adam Langley <agl@google.com>
Due to Android's complex branching scheme, we have to keep building against a
snapshotted version of wpa_supplicant. wpa_supplicant, in preparation for
OpenSSL 1.1.0, added compatibility versions of some accessors that we, in
working towards opaquification, have imported. This causes a conflict (C does
not like having static and non-static functions share a name).
Add a hack in the headers to suppress the conflicting accessors when
BORINGSSL_SUPPRESS_ACCESSORS is defined. Android releases which include an
updated BoringSSL will also locally carry this #define in wpa_supplicant build
files. Once we can be sure releases of BoringSSL will only see a new enough
wpa_supplicant (one which includes a to-be-submitted patch), we can ditch this.
Change-Id: I3e27fde86bac1e59077498ee5cbd916cd880821e
Reviewed-on: https://boringssl-review.googlesource.com/7750
Reviewed-by: Adam Langley <agl@google.com>
These source files previously didn't have the ISC license on them.
Change-Id: Ic0a2047d23b28d9d7f0a85b2fedb67574bdcab25
Reviewed-on: https://boringssl-review.googlesource.com/7735
Reviewed-by: David Benjamin <davidben@google.com>
