This function returns a tri-state -1 on error. We should check this.
Change-Id: I6fe130c11d10690923aac5ac7a6dfe3e3ff3f5e9
Reviewed-on: https://boringssl-review.googlesource.com/8490
Reviewed-by: Adam Langley <agl@google.com>
It was already nearly clean. Just one undeclared variable.
(Imported from upstream's abeae4d3251181f1cedd15e4433e79406b766155.)
Change-Id: I3b8f20034f914fc44faabf165d1553d4084c87cc
Reviewed-on: https://boringssl-review.googlesource.com/8393
Reviewed-by: Adam Langley <agl@google.com>
We were missing this case. It is possible to receive an early unencrypted
ChangeCipherSpec alert in DTLS because they aren't ordered relative to the
handshake. Test this case. (ChangeCipherSpec in DTLS is kind of pointless.)
Change-Id: I84268bc1821734f606fb20bfbeda91abf372f32c
Reviewed-on: https://boringssl-review.googlesource.com/8460
Reviewed-by: Adam Langley <agl@google.com>
With IPv6, splitting a colon-separated host/port becomes more complicated.
Change-Id: I5073a5cbaa0714f2f8b9c837bb0809dd20304a3c
Reviewed-on: https://boringssl-review.googlesource.com/8441
Reviewed-by: Adam Langley <agl@google.com>
The payload comments aren't necessary now that our parsing code is readable in
itself. The check is impossible to hit.
Change-Id: Ib41ad606babda903a9fab50de3189f97e99cac2f
Reviewed-on: https://boringssl-review.googlesource.com/8248
Reviewed-by: Adam Langley <agl@google.com>
That both exist with nearly the same name is unfortunate. This also does away
with cert_req being unnecessarily tri-state.
Change-Id: Id83e13d0249b80700d9258b363d43b15d22898d8
Reviewed-on: https://boringssl-review.googlesource.com/8247
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.2 has a long series of optional messages within a flight. We really
should send and process these synchronously. In the meantime, the 'skip'
pattern is probably the best we can get away with. Otherwise we have too many
state transitions to think about. (The business with CCS, NPN, and ChannelID is
particularly a headache. Session tickets aren't great either.)
Change-Id: I84e391a6410046372cf9c6989be056a27606ad19
Reviewed-on: https://boringssl-review.googlesource.com/8246
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This functionally pulls in a number of changes from upstream, including:
4e3d2866b6e8e7a700ea22e05840a093bfd7a4b1
1eb12c437bbeb2c748291bcd23733d4a59d5d1ca
6a4ea0022c475bbc2c7ad98a6f05f6e2e850575b
c25278db8e4c21772a0cd81f7873e767cbc6d219
e0a651945cb5a70a2abd9902c0fd3e9759d35867
d405aa2ff265965c71ce7331cf0e49d634a06924
ce3d25d3e5a7e82fd59fd30dff7acc39baed8b5e
9ba96fbb2523cb12747c559c704c58bd8f9e7982
Notably, c25278db8e4c21772a0cd81f7873e767cbc6d219 makes it enable 'use strict'.
To avoid having to deal with complex conflicts, this was done by taking a diff
of our copy of the file with the point just before
c25278db8e4c21772a0cd81f7873e767cbc6d219, and reapplying the non-reverting
parts of our diff on top of upstream's current version.
Confirmed with generate_build_files.py that this makes no changes *except*
d405aa2ff265965c71ce7331cf0e49d634a06924 causes this sort of change throughout
chacha-x86_64.pl's nasm output:
@@ -1179,7 +1179,7 @@ $L$oop8x:
vpslld ymm14,ymm0,12
vpsrld ymm0,ymm0,20
vpor ymm0,ymm14,ymm0
- vbroadcasti128 ymm14,YMMWORD[r11]
+ vbroadcasti128 ymm14,XMMWORD[r11]
vpaddd ymm13,ymm13,ymm5
vpxor ymm1,ymm13,ymm1
vpslld ymm15,ymm1,12
This appears to be correct. vbroadcasti128 takes a 128-bit-wide second
argument, so it wants XMMWORD, not YMMWORD. I suppose nasm just didn't care.
(Looking at a diff-diff may be a more useful way to review this CL.)
Change-Id: I61be0d225ddf13b5f05d1369ddda84b2f322ef9d
Reviewed-on: https://boringssl-review.googlesource.com/8392
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This is the only codepath where ssl->version can get a garbage value, which is
a little concerning. Since, in all these cases, the peer is failing to connect
and speaks so low a version we don't even accept it anymore, there is probably
not much value in letting them distinguish protocol_version from a record-layer
version number mismatch, where enforced (which will give a version-related
error anyway).
Should we get a decode_error or so just before version negotiation, we'd have
this behavior already.
Change-Id: I9b3e5685ab9c9ad32a7b7e3129363cd1d4cdaaf4
Reviewed-on: https://boringssl-review.googlesource.com/8420
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This implements the cipher suite constraints in "fake TLS 1.3". It also makes
bssl_shim and runner enable it by default so we can start adding MaxVersion:
VersionTLS12 markers to tests as 1.2 vs. 1.3 differences begin to take effect.
Change-Id: If1caf6e43938c8d15b0a0f39f40963b8199dcef5
Reviewed-on: https://boringssl-review.googlesource.com/8340
Reviewed-by: David Benjamin <davidben@google.com>
They were defined with the wrong MAC.
Change-Id: I531678dccd53850221d271c79338cfe37d4bb298
Reviewed-on: https://boringssl-review.googlesource.com/8422
Reviewed-by: Nick Harper <nharper@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In order to ensure that we don't randomly interoperate with
implementations that don't mask scalars correctly, always generate
scalars with the wrong fixed bits.
Change-Id: I82536a856f034cfe4464fc545a99c21b3cff1691
Reviewed-on: https://boringssl-review.googlesource.com/8391
Reviewed-by: David Benjamin <davidben@google.com>
It's always one. We don't support other kinds of curves with this framework.
(Curve25519 uses a much simpler API.) This also allows us to remove the
check_pub_key_order logic.
Change-Id: Ic15e1ecd68662b838c76b1e0aa15c3a93200d744
Reviewed-on: https://boringssl-review.googlesource.com/8350
Reviewed-by: Adam Langley <agl@google.com>
This unifies a bunch of tests and also adds a few missing ones.
Change-Id: I91652bd010da6cdb62168ce0a3415737127e1577
Reviewed-on: https://boringssl-review.googlesource.com/8360
Reviewed-by: Nick Harper <nharper@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
DSA is deprecated, but get this aligned with some of the BN_FLG_CONSTTIME work
going on elsewhere.
Change-Id: I676ceab298a69362bef1b61d6f597c5c90da2ff0
Reviewed-on: https://boringssl-review.googlesource.com/8309
Reviewed-by: Adam Langley <agl@google.com>
It's a prime, so computing a constant-time mod inverse is straight-forward.
Change-Id: Ie09b84363c3d5da827989300a844c470437fd8f2
Reviewed-on: https://boringssl-review.googlesource.com/8308
Reviewed-by: Adam Langley <agl@google.com>
The case where ec_group_get_mont_data is NULL is only for arbitrary groups
which we now require to be prime order. BN_mod_exp_mont is fine with a NULL
BN_MONT_CTX. It will just compute it. Saves a bit of special-casing.
Also don't mark p-2 as BN_FLG_CONSTTIME as the exponent is public anyway.
Change-Id: Ie868576d52fc9ae5f5c9f2a4039a729151bf84c7
Reviewed-on: https://boringssl-review.googlesource.com/8307
Reviewed-by: Adam Langley <agl@google.com>
Check against the write encryption state, not the read state.
Change-Id: Ib3d8e02800e37bd089ef02c67a0b7e5dc009b1a5
Reviewed-on: https://boringssl-review.googlesource.com/8330
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
The Conscrypt revert cycled in long ago.
Change-Id: If3cdb211d7347dca88bd70bdc643f80b19a7e528
Reviewed-on: https://boringssl-review.googlesource.com/8306
Reviewed-by: Adam Langley <agl@google.com>
Make |BN_mod_inverse_ex| symmetric with |BN_mod_inverse_no_branch| in
this respect.
Change-Id: I4a5cbe685edf50e13ee1014391bc4001f5371fec
Reviewed-on: https://boringssl-review.googlesource.com/8316
Reviewed-by: David Benjamin <davidben@google.com>
The alignas in NEWPOLY_POLY told the compiler that it could assume a
certain alignment. However, values were allocated with malloc with no
specific alignment.
We could try and allocate aligned memory but the alignment doesn't have
a performance impact (on x86-64) so this is the simpler change. (Also,
Windows doesn't have |posix_memalign|. The cloest thing is
_alligned_alloc but then one has to use a special free function.)
Change-Id: I53955a88862160c02aa5436d991b1b797c3c17db
Reviewed-on: https://boringssl-review.googlesource.com/8315
Reviewed-by: David Benjamin <davidben@google.com>
There's no use doing the remaining work if we're going to fail due to
there being no inverse.
Change-Id: Ic6d7c92cbbc2f7c40c51e6be2de3802980d32543
Reviewed-on: https://boringssl-review.googlesource.com/8310
Reviewed-by: David Benjamin <davidben@google.com>
In TLS 1.3, the iv_length is equal to the explicit AEAD nonce length,
and is required to be at least 8 bytes.
Change-Id: Ib258f227d0a02c5abfc7b65adb4e4a689feffe33
Reviewed-on: https://boringssl-review.googlesource.com/8304
Reviewed-by: David Benjamin <davidben@google.com>
This ensures that the test is not flaky after lots of iterations.
Along the way, change newhope_test.cc to C++.
Change-Id: I4ef139444b8c8a98db53d075105eb6806f6c5fc7
Reviewed-on: https://boringssl-review.googlesource.com/8110
Reviewed-by: Adam Langley <agl@google.com>
(This change will be sent upstream. Since the legacy X.509 stack is just
kept around for compatibility, if they decide to fix it in a different
way, we may wish to revert this and apply their fix.)
Dating back to SSLeay, X509_LOOKUP_METHOD had this X509_LU_RETRY
machinery. But it's not documented and it appears to have never worked.
Problems with the existing logic:
- X509_LU_* is not sure whether it is a type enum (to be passed into
X509_LOOKUP_by_*) or a return enum (to be retained by those same
functions).
- X509_LOOKUP_by_* is not sure whether it returns 0/1 or an X509_LU_*
value. Looking at the functions themselves, one might think it's the
latter, but for X509_LOOKUP_by_subject returning both 0 and
X509_LU_FAIL. But looking at the call sites, some expect 0/1 (such as
X509_STORE_get1_certs) while others expect an X509_LU_* enum (such as
X509_STORE_CTX_get1_issuer). It is very fortunate that FAIL happens to
be 0 and X509 happens to be 1.
These functions primarily call to X509_LOOKUP_METHOD hooks. Looking
through OpenSSL itself and code checked into Google, I found no
evidence that any hooks have been implemented except for
get_by_subject in by_dir.c. We take that one as definitive and observe
it believes it returns 0/1. Notably, it returns 1 on success even if
asked for a type other than X509_LU_X509. (X509_LU_X509 = 1. Others are
different.) I found another piece of third-party software which corroborates
this worldview.
- X509_STORE_get_by_subject's handling of X509_LU_RETRY (it's the j < 0
check) is broken. It saves j into vs->current_method where it probably
meant to save i. (This bug has existed since SSLeay.)
It also returns j (supposedly X509_LU_RETRY) while all callers of
X509_STORE_get_by_subject expect it to return 0/1 by checking with !
instead of <= 0. (Note that all other codepaths return 0 and 1 so this
function did not actually believe it returned X509_LU_* most of the
time.)
This, in turn, gives us a free of uninitialized pointers in
X509_STORE_get1_certs and other functions which expect that *ret is
filled in if X509_STORE_get_by_subject returns success. GCC 4.9 with
optimizations from the Android NDK noticed this, which trigged this
saga.
(It's only reachable if any X509_LOOKUP_METHOD returned
X509_LU_RETRY.)
- Although the code which expects X509_STORE_get_by_subject return 0/1
does not date to SSLeay, the X509_STORE_get_by_subject call in
X509_STORE_CTX_get1_issuer *does* (though, at the time, it was inline
in X509_verify_cert. That code believes X509_STORE_get_by_subject
returns an X509_LU_* enum, but it doesn't work either! It believes
*ret is filled in on X509_LU_RETRY, thus freeing another uninitialized
pointer (GCC noticed this too).
Since this "retry" code has clearly never worked, from SSLeay onwards,
unwind it completely rather than attempt to fix it. No
X509_LOOKUP_METHOD can possibly have depended on it.
Matching all non-broken codepaths X509_LOOKUP_by_* now returns 0/1 and
X509_STORE_get_by_subject returns 0/1. X509_LU_* is purely a type enum
with X509_LU_{REJECT,FAIL} being legacy constants to keep old code
compiling. (Upstream is recommended to remove those values altogether
for 1.1.0.)
On the off chance any get_by_* X509_LOOKUP_METHOD implementations did
not return 0/1 (I have found no evidence anywhere of this, and I believe
it wouldn't have worked anyway), the X509_LOOKUP_by_* wrapper functions
will coerce the return values back to 0/1 before passing up to the
callers which want 0/1. This both avoids the error-prone -1/0/1 calling
convention and, more importantly, avoids problems with third-party
callers which expect a X509_LU_* return code. 0/1 collide with FAIL/X509
while -1 will collide with RETRY and might confuse things.
Change-Id: I98ecf6fa7342866b9124dc6f0b422cb9ce4a1ae7
Reviewed-on: https://boringssl-review.googlesource.com/8303
Reviewed-by: Adam Langley <agl@google.com>
I did the same change in NaCl in
https://codereview.chromium.org/2070533002/. I thought NaCl is the only
place where this was needed, but at least it's due to SecureZeroMemory()
again. So it's two files now, but at least there's only one function we
know of that needs this, and it's only called in three files total in
all projects used by Chromium.
BUG=chromium:592745
Change-Id: I07ed197869e26ec70c1f4b75d91fd64abae5015e
Reviewed-on: https://boringssl-review.googlesource.com/8320
Reviewed-by: David Benjamin <davidben@google.com>
Both messages go between CCS and Finished. We weren't testing their relative
order and one of the state machine edges. Also test resume + NPN since that too
is a different handshake shape.
Change-Id: Iaeaf6c2c9bfd133103e2fb079d0e5a86995becfd
Reviewed-on: https://boringssl-review.googlesource.com/8196
Reviewed-by: Adam Langley <agl@google.com>
I named the compatibility function wrong.
Change-Id: Idc289c317c5826c338c1daf58a2d3b26b09a7e49
Reviewed-on: https://boringssl-review.googlesource.com/8301
Reviewed-by: Adam Langley <agl@google.com>
SSL_set_bio has some rather complex ownership story because whether rbio/wbio
are both owning depends on whether they are equal. Moreover, whether
SSL_set_bio(ssl, rbio, wbio) frees ssl->rbio depends on whether rbio is the
existing rbio or not. The current logic doesn't even get it right; see tests.
Simplify this. First, rbio and wbio are always owning. All the weird ownership
cases which we're stuck with for compatibility will live in SSL_set_bio. It
will internally BIO_up_ref if necessary and appropriately no-op the left or
right side as needed. It will then call more well-behaved ssl_set_rbio or
ssl_set_wbio functions as necessary.
Change-Id: I6b4b34e23ed01561a8c0aead8bb905363ee413bb
Reviewed-on: https://boringssl-review.googlesource.com/8240
Reviewed-by: Adam Langley <agl@google.com>
Their implementations expose a lot of really weird SSL_set_bio behavior. Note
that one test must be disabled as it doesn't even work. The subsequent commit
will re-enable it.
Change-Id: I4b7acadd710b3be056951886fc3e073a5aa816de
Reviewed-on: https://boringssl-review.googlesource.com/8272
Reviewed-by: Adam Langley <agl@google.com>
Include all internal headers in |test_support_sources|, since that's
easier than enumerating the ones specifically required for each test.
This incidentally removes test headers from |crypto_internal_headers|
and |ssl_internal_headers|.
Require the crypto and ssl libraries to be passed as arguments to
create_tests(), rather than hardcoding the names :crypto and :ssl
Change-Id: Idcc522298c5baca2a84635ad3a7fdcf6e4968a5a
Reviewed-on: https://boringssl-review.googlesource.com/8260
Reviewed-by: David Benjamin <davidben@google.com>
These are more remnants of CMS. Nothing uses them directly. Removing them means
more code we don't have to think about when importing upstream patches.
Also take out a bunch of dead prototypes nearby.
Change-Id: Ife094d9d2078570006d1355fa4e3323f435be608
Reviewed-on: https://boringssl-review.googlesource.com/8244
Reviewed-by: David Benjamin <davidben@google.com>
These are more pretty-printers for generic ASN.1 structures. They're never
called externally and otherwise are only used in the X509V3_EXT_PARSE_UNKNOWN
mode for the X509 pretty-print functions. That makes unknown extensions
pretty-print as ASN.1 structures.
This is a rather useless feature, so have that fall through to
X509V3_EXT_DUMP_UNKNOWN which does a hexdump instead.
(The immediate trigger is I don't know what |op| is in upstream's
8c918b7b9c93ba38790ffd1a83e23c3684e66f57 and don't think it is worth the time
to puzzle that out and verify it. Better ditch this code completely.)
Change-Id: I0217906367d83056030aea64ef344d4fedf74763
Reviewed-on: https://boringssl-review.googlesource.com/8243
Reviewed-by: David Benjamin <davidben@google.com>
These functions are never instantiated. (They're a remnant of the PKCS#7 and
CMS bits.) Next time upstream touches this code, we don't have to puzzle
through the diff and import it.
Change-Id: I67c2102ae13e8e0527d858e1c63637dd442a4ffb
Reviewed-on: https://boringssl-review.googlesource.com/8242
Reviewed-by: David Benjamin <davidben@google.com>
This is not very satisfactory.
Change-Id: I7e7a86f921e66f8f830c72eac084e9fea5ffd4d9
Reviewed-on: https://boringssl-review.googlesource.com/8270
Reviewed-by: David Benjamin <davidben@google.com>
By corrupting the X25519 and Newhope parts separately, the test shows
that both are in use. Possibly excessive?
Change-Id: Ieb10f46f8ba876faacdafe70c5561c50a5863153
Reviewed-on: https://boringssl-review.googlesource.com/8250
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 3892b95750b6aa5ed4328a287068f7cdfb9e55bc.)
More reasonable would have been to drop |to| altogether and act on from[len-1],
but I suppose this works.
Change-Id: I280b4991042b4d330ba034f6a631f8421ddb2643
Reviewed-on: https://boringssl-review.googlesource.com/8241
Reviewed-by: Adam Langley <agl@google.com>
We already require GCC 4.8+, so -std=c++11 should work fine.
Change-Id: I07d46d7dcccb695b5df97a702f0d5007fdff3385
Reviewed-on: https://boringssl-review.googlesource.com/8245
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The minimum version is purely based on what we've patched out of the perlasm
files. I'm assuming they're accurate.
Change-Id: I5ae176cf793512125fa78f203a1314396e8a14d7
Reviewed-on: https://boringssl-review.googlesource.com/8238
Reviewed-by: Adam Langley <agl@google.com>
The functions it calls all pass through <= 0 as error codes, not < 0.
Change-Id: I9d0d6b1df0065efc63f2d3a5e7f3497b2c28453a
Reviewed-on: https://boringssl-review.googlesource.com/8237
Reviewed-by: Adam Langley <agl@google.com>
There's a __pragma expression which allows this. Android builds us Windows with
MinGW for some reason, so we actually do have to tolerate non-MSVC-compatible
Windows compilers. (Clang for Windows is much more sensible than MinGW and
intentionally mimicks MSVC.)
MinGW doesn't understand MSVC's pragmas and warns a lot. #pragma warning is
safe to suppress, so wrap those to shush them. This also lets us do away with a
few ifdefs.
Change-Id: I1f5a8bec4940d4b2d947c4c1cc9341bc15ec4972
Reviewed-on: https://boringssl-review.googlesource.com/8236
Reviewed-by: Adam Langley <agl@google.com>
Switch one for loop to the new spelling as a canary. All our compilers seem to
support it fine, except GCC needs to be told to build with -std=c99. (And, upon
doing so, it'll require _XOPEN_SOURCE=700 for pthread_rwlock_t.)
We'll let this sit for a bit until it's gotten into downstreams without issue
and then open the floodgates.
BUG=47
Change-Id: I1c69d4b2df8206e0b55f30aa59b5874d82fca893
Reviewed-on: https://boringssl-review.googlesource.com/8235
Reviewed-by: Adam Langley <agl@google.com>
This reverts commit 762e1d039c. We no longer need
to support out < in. Better to keep the assembly aligned with upstream.
Change-Id: I345bf822953bd0e1e79ad5ab4d337dcb22e7676b
Reviewed-on: https://boringssl-review.googlesource.com/8232
Reviewed-by: Adam Langley <agl@google.com>
