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>
Otherwise it's confusing if you mistype the test name.
Change-Id: Idf32081958f85f3b5aeb8993a07f6975c27644f8
Reviewed-on: https://boringssl-review.googlesource.com/7500
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>
We failed to correctly parse files that executed from the very start of
the file due to a missing '- line XXX'. We now use the 'Ir' indicator to
recognize the beginning of a file.
Change-Id: I529fae9458ac634bf7bf8af61ef18f080e808535
Reviewed-on: https://boringssl-review.googlesource.com/7542
Reviewed-by: Steven Valdez <svaldez@google.com>
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>
Maintain the max_len values in foo.options files which ClusterFuzz can process.
Also recompute the recommended client and server lengths as they've since
gotten much more extensive.
Change-Id: Ie87a80d8a4a0c41e215f0537c8ccf82b38c4de09
Reviewed-on: https://boringssl-review.googlesource.com/7509
Reviewed-by: Mike Aizatsky <aizatsky@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Most code already dereferences it directly.
Change-Id: I227fa91ecbf25a19077f7cfba21b0abd2bc2bd1d
Reviewed-on: https://boringssl-review.googlesource.com/7422
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>
Change-Id: Ieba81f114483095f3657e87f669c7562ff75b58c
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7516
Reviewed-by: David Benjamin <davidben@google.com>
Partially fixes build with -Wmissing-declarations.
Change-Id: Ia563063fb077cda79244c21f02fd1c0f550353c2
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7515
Reviewed-by: David Benjamin <davidben@google.com>
Partially fixes build with -Wmissing-prototypes.
Change-Id: I828bcfb49b23c5a9ea403038bc3fb76750556ef8
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7514
Reviewed-by: David Benjamin <davidben@google.com>
Partially fixes build with -Wmissing-prototypes.
Change-Id: If04d8fe7cbf068883485e95bd5ea6cdab6743e46
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7513
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>
Change-Id: I9c5e66c34d0f1b735c69d033daee5d312e3c2fe7
Reviewed-on: https://boringssl-review.googlesource.com/7410
Reviewed-by: Steven Valdez <svaldez@google.com>
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>
For time_t and struct tm.
BUG=595118
Change-Id: I6c7f05998887ed2bd3fb56c83ac543894ef27fe6
Reviewed-on: https://boringssl-review.googlesource.com/7462
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: Nico Weber <thakis@chromium.org>
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>
Also fix a long/unsigned-long cast. (ssl_get_message returns long. It really
shouldn't, but ssl_get_message needs much more work than just a long -> size_t
change, so leave it as long for now.)
Change-Id: Ice8741f62a138c0f35ca735eedb541440f57e114
Reviewed-on: https://boringssl-review.googlesource.com/7457
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Found by libFuzzer.
Change-Id: Ifa343a184cc65f71fb6591d290b2d47d24a2be80
Reviewed-on: https://boringssl-review.googlesource.com/7456
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL historically made some poor API decisions. Rather than returning a
status enum in SSL_read, etc., these functions must be paired with
SSL_get_error which determines the cause of the last error's failure. This
requires SSL_read communicate with SSL_get_error with some stateful flag,
rwstate.
Further, probably as workarounds for bugs elsewhere, SSL_get_error does not
trust rwstate. Among other quirks, if the error queue is non-empty,
SSL_get_error overrides rwstate and returns a value based on that. This
requires that SSL_read, etc., be called with an empty error queue. (Or we hit
one of the spurious ERR_clear_error calls in the handshake state machine,
likely added as further self-workarounds.)
Since requiring callers consistently clear the error queue everywhere is
unreasonable (crbug.com/567501), clear ERR_clear_error *once* at the entry
point. Until/unless[*] we make SSL_get_error sane, this is the most reasonable
way to get to the point that clearing the error queue on error is optional.
With those in place, the calls in the handshake state machine are no longer
needed. (I suspect all the ERR_clear_system_error calls can also go, but I'll
investigate and think about that separately.)
[*] I'm not even sure it's possible anymore, thanks to the possibility of
BIO_write pushing to the error queue.
BUG=567501,593963
Change-Id: I564ace199e5a4a74b2554ad3335e99cd17120741
Reviewed-on: https://boringssl-review.googlesource.com/7455
Reviewed-by: Steven Valdez <svaldez@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>
Align all unexpected messages on SSL_R_UNEXPECTED_MESSAGE. Make the SSL 3.0
case the exceptional case. In doing so, make sure the SSL 3.0
SSL_VERIFY_FAIL_IF_NO_PEER_CERT case has its own test as that's a different
handshake shape.
Change-Id: I1a539165093fbdf33e2c1b25142f058aa1a71d83
Reviewed-on: https://boringssl-review.googlesource.com/7421
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If we're doing substring matching, we should at least include the delimiter.
Change-Id: I98bee568140d0304bbb6a2788333dbfca044114c
Reviewed-on: https://boringssl-review.googlesource.com/7420
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
