This is to make sure all of libssl's consumers' have sufficiently
reasonable toolchains. Once this bakes, we can go about moving
libssl to C++.
This is just starting with libssl for now because libcrypto has more
consumers and libssl would benefit more from C++ than libcrypto (though
libcrypto also has code that would benefit).
BUG=132
Change-Id: Ie02f7b0a8a95defd289cc7e62451d4b16408ca2a
Reviewed-on: https://boringssl-review.googlesource.com/13161
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
(cherry picked from commit 1252f8758a)
As a client, we must tolerate this to avoid interoperability failures
with allowed server behaviors.
BUG=117
Change-Id: I9c40a2a048282e2e63ab5ee1d40773fc2eda110a
Reviewed-on: https://boringssl-review.googlesource.com/12311
Reviewed-by: David Benjamin <davidben@google.com>
The draft 18 implementation did not compute scts_requested correctly. As
a result, it always believed SCTs were requested. Fix this and add tests
for unsolicited OCSP responses and SCTs at all versions.
Thanks to Daniel Hirche for the report.
Change-Id: Ifc59c5c4d7edba5703fa485c6c7a4055b15954b4
Reviewed-on: https://boringssl-review.googlesource.com/12305
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Having that logic in two different places is a nuisance when we go to
add new checks like resumption stuff. Along the way, this adds missing
tests for the ClientHello cipher/session consistency check. (We'll
eventually get it for free once the cipher/resumption change is
unblocked, but get this working in the meantime.)
This also fixes a bug where the session validity checks happened in the
wrong order relative to whether tickets_supported or renew_ticket was
looked at. Fix that by lifting that logic closer to the handshake.
Change-Id: I3f4b59cfe01064f9125277dc5834e62a36e64aae
Reviewed-on: https://boringssl-review.googlesource.com/12230
Reviewed-by: Adam Langley <agl@google.com>
The other field operations have an explicit _mont suffix to denote their
inputs and outputs are in the Montgomery domain, aside from
ecp_nistz256_neg which works either way. Do the same here.
Change-Id: I63741adaeba8140e29fb0b45dff72273e231add7
Reviewed-on: https://boringssl-review.googlesource.com/12224
Reviewed-by: Adam Langley <agl@google.com>
The Poly1305 state defined in the header file is just a 512-byte buffer.
The vector code aligns to 64 bytes but the non-vector code did not.
Since we have lots of space to spare, this change causes the non-vector
code to also align to 64 bytes.
Change-Id: I77e26616a709e770d6eb23df47d9e292742625d7
Reviewed-on: https://boringssl-review.googlesource.com/12201
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Some old assemblers can't cope with r0 in address. It's actually
sensible thing to do, because r0 is shunted to 0 in address arithmetic
and by refusing r0 assembler effectively makes you understand that.
(Imported from upstream's a54aba531327285f64cf13a909bc129e9f9d5970.)
This also pulls in a trailing whitespace fix from upstream's
609b0852e4d50251857dbbac3141ba042e35a9ae.
Change-Id: Ieec0bc8d24b98f86ce4fc9ee6ce5126d127cf452
Reviewed-on: https://boringssl-review.googlesource.com/12188
Reviewed-by: Adam Langley <agl@google.com>
The ASN1_GENERALIZEDTIME_adj() function leaks an ASN1_GENERALIZEDTIME
object on an error path.
(Imported from upstream's fe71bb3ad97ed01ccf92812891cc2bc3ef3dce76.)
Thanks to Jinguang Dong for pointing out the bug.
Change-Id: I2c14662bb03b0cf957bd277bda487f05f07e89e7
Reviewed-on: https://boringssl-review.googlesource.com/12185
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 2a7dd548a6f5d6f7f84a89c98323b70a2822406e and
9ebcbbba81eba52282df9ad8902f047e2d501f51.)
This is only in the ADX assembly codepath which we do not enable. See
$addx = 0 at the top of the file. Nonetheless, import the test vector
and fix since we still have the code in there.
Upstream's test vector only compares a*b against b*a. The expected
answer was computed using Python.
Change-Id: I3a21093978c5946d83f2d6f4f8399f69d78202cf
Reviewed-on: https://boringssl-review.googlesource.com/12186
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We have at least three different external build definitions for the
fuzzers. That's enough that requiring each of them account for the split
fuzzer mode is probably too much turbulence.
Change-Id: I96dbb12a2b4f70bfa1b04cd0d15fda918bbf51d6
Reviewed-on: https://boringssl-review.googlesource.com/12183
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This change renames |peer| to |x509_peer| and |cert_chain| to
|x509_chain| in |SSL_SESSION|. It also renames |x509| to |x509_leaf| and
|chain| to |x509_chain| in |CERT|. (All with an eye to maybe making
them lazily initialised in the future).
This a) catches anyone who might be accessing these members directly and
b) makes space for |CRYPTO_BUFFER|-based values to take the unprefixed
names.
Change-Id: I10573304fb7d6f1ea03f9e645f7fc0acdaf71ac2
Reviewed-on: https://boringssl-review.googlesource.com/12162
Reviewed-by: David Benjamin <davidben@google.com>
In transition to removing it altogether, set SSL_MODE_NO_AUTO_CHAIN by
default. If we find some consumer was relying on it, this will allow
them to revert locally with SSL_(CTX_)clear_mode, but hopefully this was
just unused.
BUG=42
Change-Id: Iaf70a436a3324ce02e02dfb18213b6715c034ff2
Reviewed-on: https://boringssl-review.googlesource.com/12180
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
3c6a1ea674 switched what layer handled
the DTLS version mapping but forgot to correct the HelloVerifyRequest
logic to account for this.
Thanks to Jed Davis for noticing this.
Change-Id: I94ea18fc43a7ba15dd7250bfbcf44dbb3361b3ce
Reviewed-on: https://boringssl-review.googlesource.com/11984
Reviewed-by: David Benjamin <davidben@google.com>
This is already manually released at the end of the handshake. With this
change, it can happen implicitly, and SSL3_STATE shrinks further by
another pointer.
Change-Id: I94b9f2e4df55e8f2aa0b3a8799baa3b9a34d7ac1
Reviewed-on: https://boringssl-review.googlesource.com/12121
Reviewed-by: Adam Langley <agl@google.com>
Fuzzer mode explores the handshake, but at the cost of losing coverage
on the record layer. Add a separate build flag and client/server
corpora for this mode.
Note this requires tweaks in consumers' fuzzer build definitions.
BUG=111
Change-Id: I1026dc7301645e165a761068a1daad6eedc9271e
Reviewed-on: https://boringssl-review.googlesource.com/12108
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The distinction for full handshakes is not meaningful (the timestamp is
currently the start of the handshake), but for renewed sessions, we
currently retain the timestamp of the original issuance.
Instead, when minting or receiving tickets, adjust session->time and
session->timeout so that session->time is the ticket issuance time.
This is still not our final TLS 1.3 behavior (which will need a both
renewable and non-renewable times to honor the server ticket lifetime),
but it gets us closer and unblocks handling ticket_age_add from TLS 1.3
draft 18 and sends the correct NewSessionTicket lifetime.
This fixes the ticket lifetime hint which we emit on the server to
mirror the true ticket lifetime. It also fixes the TLS 1.3 server code
to not set the ticket lifetime hint. There is no need to waste ticket
size with it, it is no longer a "hint" in TLS 1.3, and even in the TLS
1.3 code we didn't fill it in on the server.
Change-Id: I140541f1005a24e53e1b1eaa90996d6dada1c3a1
Reviewed-on: https://boringssl-review.googlesource.com/12105
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
If there is a malloc failure while assembling the ticket, call
CBB_cleanup. Also return -1 instead of 0; zero means EOF in the old
state machine and -1 means error. (Except enough of the stack gets it
wrong that consumers handle both, but we should fix this.)
Change-Id: I98541a9fa12772ec159f9992d1f9f53e5ca4cc5a
Reviewed-on: https://boringssl-review.googlesource.com/12104
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
There's no sense in flushing twice in one flight. This means when
writing a message is finally synchronous, we don't need the intermediate
state at all.
Change-Id: Iaca60d64917f82dce0456a8b15de4ee00f2d557b
Reviewed-on: https://boringssl-review.googlesource.com/12103
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 clarifies that a ticket lifetime of zero means the session is
unusable. We don't currently pay attention to that field (to be fixed in
later changes) but, in preparation for this, switch the >= to a >.
Change-Id: I0e67a0d97bc8def04914f121e84d3e7a2d640d2c
Reviewed-on: https://boringssl-review.googlesource.com/12102
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
These don't make sense and mean some SSL_SESSIONs serialize and
deserialize as different values. If we ever managed to create an
SSL_SESSION without a time, it would never expire because time always
gets set to time(NULL). If we ever created an SSL_SESSION with a zero
timeout, the timeout would be... three? Once we start adjusting
time/timeout to issuance time, driving timeout to zero is actually
plausible, so it should work properly.
Instead, make neither field optional. We always fill both out, so this
shouldn't have any effects. If it does, the only effect would be to
decline to resume some existing tickets which must have been so old that
we'd want them to have expired anyway.
Change-Id: Iee3620658c467dd6d96a2b695fec831721b03b5b
Reviewed-on: https://boringssl-review.googlesource.com/12101
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
A renewed session does not refresh the timeout. Add tests for this in
preparation for future changes which will revise this logic.
Specifically, TLS 1.3 draft 18's ticket_age_add logic will require some
tweaks in lifetime tracking to record when the ticket was minted. We'll
also likely wish to tweak the parameters for 1.3 to account for (a)
ECDHE-PSK means we're only worried about expiring a short-circuited
authentication rather than forward secrecy and (b) two hours is too
short for a QUIC 0-RTT replacement.
Change-Id: I0f1edd09151e7fcb5aee2742ef8600fbd7080df6
Reviewed-on: https://boringssl-review.googlesource.com/12002
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The naming breaks layering, but it seems we're stuck with it. We don't
seem to have bothered making first-party code call it BIO_print_errors
(I found no callers of BIO_print_errors), so let's just leave it at
ERR_print_errors.
Change-Id: Iddc22a6afc2c61d4b94ac555be95079e0f477171
Reviewed-on: https://boringssl-review.googlesource.com/11960
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is the last blocker within BoringSSL itself to opaquifying SSL.
(There are still blockers in consumers, of course.)
BUG=6
Change-Id: Ie3b8dcb78eeaa9aea7311406c5431a8625d60401
Reviewed-on: https://boringssl-review.googlesource.com/12061
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 ciphers are now always enabled and come with a hard-coded
preference order.
BUG=110
Change-Id: Idd9cb0d75fb6bf2676ecdee27d88893ff974c4a3
Reviewed-on: https://boringssl-review.googlesource.com/12025
Reviewed-by: Adam Langley <agl@google.com>
They will get very confused about which key they're using. Any caller
using exporters must either (a) leave renegotiation off or (b) be very
aware of when renegotiations happen anyway. (You need to somehow
coordinate with the peer about which epoch's exporter to use.)
Change-Id: I921ad01ac9bdc88f3fd0f8283757ce673a47ec75
Reviewed-on: https://boringssl-review.googlesource.com/12003
Reviewed-by: Adam Langley <agl@google.com>
The existing tests for this codepath require us to reconfigure the shim.
This will not work when TLS 1.3 cipher configuration is detached from
the old cipher language. It also doesn't hit codepaths like sessions
containing a TLS 1.3 version but TLS 1.2 cipher.
Instead, add some logic to the runner to rewrite tickets and build tests
out of that.
Change-Id: I57ac5d49c3069497ed9aaf430afc65c631014bf6
Reviewed-on: https://boringssl-review.googlesource.com/12024
Reviewed-by: Adam Langley <agl@google.com>
HTTP/2 places requirements on the cipher suite. So that servers can
decline HTTP/2 when these requirements aren't met, defer ALPN
negotiation.
See also b/32553041.
Change-Id: Idbcf049f9c8bda06a8be52a0154fe76e84607268
Reviewed-on: https://boringssl-review.googlesource.com/11982
Reviewed-by: Adam Langley <agl@google.com>
Bazel has moved their primary site to bazel.build.
(Thanks to Damien Martin-guillerez for the change.)
Change-Id: Ifb29dbb79f1e1d9611f40992a3e75e0fb5a3722a
Reviewed-on: https://boringssl-review.googlesource.com/11961
Reviewed-by: David Benjamin <davidben@google.com>
This should never happen, but the SSL_AEAD_CTX_new layer should enforce
key sizes as it's not locally obvious at the call site the caller didn't
get confused. There's still a mess of asserts below, but those should be
fixed by cutting the SSL_CIPHER/SSL_AEAD_CTX boundary differently.
(enc_key_len is validated by virtue of being passed into EVP_AEAD.)
BUG=chromium:659593
Change-Id: I8c91609bcef14ca1509c87aab981bbad6556975f
Reviewed-on: https://boringssl-review.googlesource.com/11940
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
These were forward-declared for SSL3_STATE but with that hidden, it's no
longer necessary.
Change-Id: I8c548822f56f6172b4033b2fa89c038adcec2caa
Reviewed-on: https://boringssl-review.googlesource.com/11860
Reviewed-by: Adam Langley <agl@google.com>
Tagging non-pointer return types const doesn't do anything and makes
some compilers grumpy. Thanks to Daniel Hirche for the report.
Change-Id: I157ddefd8f7e604b4d8317ffa2caddb8f0dd89de
Reviewed-on: https://boringssl-review.googlesource.com/11849
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is still rather a mess with how it's tied to SSL_AEAD_CTX_new
(probably these should get encapsulated in an SSL_AEAD struct), but this
avoids running the TLS 1.3 nonce logic on fake AEADs. This is impossible
based on cipher version checks, but we shouldn't need to rely on it.
It's also a little tidier since out_mac_secret_len is purely a function
of algorithm_mac.
BUG=chromium:659593
Change-Id: Icc24d43c54a582bcd189d55958e2d232ca2db4dd
Reviewed-on: https://boringssl-review.googlesource.com/11842
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This shouldn't happen, but it is good to check to avoid the potential
underflow in ssl_session_is_time_valid.
This required tweaking the mock clock in bssl_shim to stop going back in
time.
Change-Id: Id3ab8755139e989190d0b53d4bf90fe1ac203022
Reviewed-on: https://boringssl-review.googlesource.com/11841
Reviewed-by: David Benjamin <davidben@google.com>
We currently preferentially sign the largest hash available and
advertise such a preference for signatures we accept. We're just as
happy with SHA-256 and, all else equal, a smaller hash would be epsilon
more performant. We also currently claim, in TLS 1.3, we prefer P-384
over P-256 which is off.
Instead order SHA-256 first, next the larger SHA-2 hashes, and leave
SHA-1 at the bottom. Within a hash, order ECDSA > RSA-PSS > RSA-PKCS1.
This has the added consequence that we will preferentially pair P-256
with SHA-256 in signatures we generate instead of larger hashes that get
truncated anyway.
Change-Id: If4aee068ba6829e8c0ef7948f56e67a5213e4c50
Reviewed-on: https://boringssl-review.googlesource.com/11821
Reviewed-by: Adam Langley <agl@google.com>
SSL_CTX_set1_curves was being called with the size of the input data in
bytes rather than in ints.
BUG=chromium:659361
Change-Id: I90da1c6d60e92423c6b7d9efd744ae70ff589172
Reviewed-on: https://boringssl-review.googlesource.com/11840
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
SSL_write has messy semantics around retries. As a sanity-check, it does
pointer and length checks and requires the original and retry SSL_write
pass the same buffer pointer.
In some cases, buffer addresses may change but still include the
original data as a prefix on the retry. Callers then set
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER to skip the pointer check. But, in
that case, the pointer may have been freed so doing a comparison is
undefined behavior.
Short-circuiting the pointer equality check avoids this problem.
Change-Id: I76cb8f7d45533504cd95287bc53897ca636af51d
Reviewed-on: https://boringssl-review.googlesource.com/11760
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Unhandled critical CRL extensions were not detected if they appeared
after the handled ones. (Upstream GitHub issue 1757). Thanks to John
Chuah for reporting this.
(Imported from upstream's 3ade92e785bb3777c92332f88e23f6ce906ee260.)
This additionally adds a regression test for this issue, generated with
der-ascii. The signatures on the CRLs were repaired per notes in
https://github.com/google/der-ascii/blob/master/samples/certificates.md
Change-Id: I74a77f92710e6ef7f46dcde5cb6ae9350084ddcb
Reviewed-on: https://boringssl-review.googlesource.com/11720
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
On the client we'll leave it off by default until the change has made it
through Chrome's release process. For TLS 1.3, there is no existing
breakage risk, so always do it. This saves us the trouble of having to
manually turn it on in servers.
See [0] for a data point of someone getting it wrong.
[0] https://hg.mozilla.org/projects/nss/rev/9dbc21b1c3cc
Change-Id: I74daad9e7efd2040e9d66d72d558b31f145e6c4c
Reviewed-on: https://boringssl-review.googlesource.com/11680
Reviewed-by: Adam Langley <agl@google.com>
Macros need a healthy dose of parentheses to avoid expression-level
misparses. Most of this comes from the clang-tidy CL here:
https://android-review.googlesource.com/c/235696/
Also switch most of the macros to use do { ... } while (0) to avoid all
the excessive comma operators and statement-level misparses.
Change-Id: I4c2ee51e347d2aa8c74a2d82de63838b03bbb0f9
Reviewed-on: https://boringssl-review.googlesource.com/11660
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This reverts commit 7b9bbd9639. This seems
to cause some problem linking with gold in Chromium:
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: Cannot export local symbol 'free'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: Cannot export local symbol 'malloc'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: Cannot export local symbol 'realloc'
../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating warnings as errors
The same error in https://crbug.com/368351 implies we're actually
causing the compiler to make some assumptions it shouldn't make. The
obvious fix of marking things as visible causes crashes when built with
ASan (ASan's malloc interceptors and ours are conflicting somehow).
Revert this for now. We should study how ASan's interceptors work and
figure out how to make these two coexist.
BUG=655938
Change-Id: Iaad245d1028c442bd924d46519b20115d37a57c4
Reviewed-on: https://boringssl-review.googlesource.com/11604
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Data allocated in one fuzzer iteration and then freed in the next
complicates the leak checker. Avoid this by dropping hidden global state
at the end of each run.
Change-Id: Ice79704f2754a6b1f40e288df9b97ddd5b3b97d5
Reviewed-on: https://boringssl-review.googlesource.com/11600
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This clears the last of Android's build warnings from BoringSSL. These
pragmas aren't actually no-ops, but it just means that MinGW consumers
(i.e. just Android) need to explicitly list the dependency (which they
do).
There may be something to be said for removing those and having everyone
list dependencies, but I don't really want to chase down every
consumer's build files. Probably not worth the trouble.
Change-Id: I8fcff954a6d5de9471f456db15c54a1b17cb937a
Reviewed-on: https://boringssl-review.googlesource.com/11573
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is part of TLS 1.3 draft 16 but isn't much of a wire format change,
so go ahead and add it now. When rolling into Chromium, we'll want to
add an entry to the error mapping.
Change-Id: I8fd7f461dca83b725a31ae19ef96c890d603ce53
Reviewed-on: https://boringssl-review.googlesource.com/11563
Reviewed-by: David Benjamin <davidben@google.com>
We only save them at TLS 1.0 through 1.2. This saves 104 bytes of
per-connection memory.
Change-Id: If397bdc10e40f0194cba01024e0e9857d6b812f0
Reviewed-on: https://boringssl-review.googlesource.com/11571
Reviewed-by: Adam Langley <agl@google.com>
We need to retain a pair of Finished messages for renegotiation_info.
SSL 3.0's is actually larger than TLS 1.2's (always 12 bytes). Take
renegotiation out in preparation for trimming them to size.
Change-Id: I2e238c48aaf9be07dd696bc2a6af75e9b0ead299
Reviewed-on: https://boringssl-review.googlesource.com/11570
Reviewed-by: Adam Langley <agl@google.com>
tls-unique isn't defined at TLS 1.3 yet. (Given that it was too small in
1.2, they may just define a new one entirely?) SSL_get_(peer_)finished
doesn't work at 1.3 and is only used in lieu of computing tls-unique,
also undefined at SSL 3.0.
This is in preparation for trimming the copies of the Finished messages
we retain.
Change-Id: Iace99f2baea92c511c4041c592300dfbbe7226e2
Reviewed-on: https://boringssl-review.googlesource.com/11568
Reviewed-by: Adam Langley <agl@google.com>
SSL_HANDSHAKE is dropped after the handshake, so I've removed the logic
around smaller sizes. It's much simpler when we can use CBS_stow and
CBB_finish without extra bounds-checking.
Change-Id: Idafaa5d69e171aed9a8759f3d44e52cb01c40f39
Reviewed-on: https://boringssl-review.googlesource.com/11567
Reviewed-by: Adam Langley <agl@google.com>
Now not only the pointers but also the list itself is released after the
handshake completes.
Change-Id: I8b568147d2d4949b3b0efe58a93905f77a5a4481
Reviewed-on: https://boringssl-review.googlesource.com/11528
Reviewed-by: Adam Langley <agl@google.com>
It's weird and makes things more confusing. Only use it for local
preferences as there is a default. Peer preferences can be read
directly. Also simplify the logic for requiring a non-empty peer group
list for ECDHE. The normal logic will give us this for free.
Change-Id: I1916155fe246be988f20cbf0b1728380ec90ff3d
Reviewed-on: https://boringssl-review.googlesource.com/11527
Reviewed-by: Adam Langley <agl@google.com>
This function is now only ever called as a client, so there are no peer
preferences to check against. It is also now only called on peer curves,
so it only needs to be compared against local preferences.
Change-Id: I87f5b10cf4fe5fef9a9d60aff36010634192e90c
Reviewed-on: https://boringssl-review.googlesource.com/11526
Reviewed-by: Adam Langley <agl@google.com>
These functions are only called once. It ends up being not much code if
just done inline.
Change-Id: Ic432b313a6f7994ff9f51436cffbe0c3686a6c7c
Reviewed-on: https://boringssl-review.googlesource.com/11525
Reviewed-by: Adam Langley <agl@google.com>
This is in preparation for simplifying tls1_check_group_id, called by
tls1_check_ec_cert, which, in turn, is in preparation for moving the
peer group list to SSL_HANDSHAKE.
It also helps with bug #55. Move the key usage check to the certificate
configuration sanity check. There's no sense in doing it late. Also
remove the ECDSA peer curve check as we configure certificates
externally. With only one certificate, there's no sense in trying to
remove it.
BUG=55
Change-Id: I8c116337770d96cc9cfd4b4f0ca7939a4f05a1a9
Reviewed-on: https://boringssl-review.googlesource.com/11524
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This releases memory associated with them after the handshake. Note this
changes the behavior of |SSL_get0_certificate_types| and
|SSL_get_client_CA_list| slightly. Both functions now return NULL
outside of the handshake. But they were already documented to return
something undefined when not called at the CertificateRequest.
A survey of callers finds none that would care. (Note
SSL_get_client_CA_list is used both as a getter for the corresponding
server config setter and to report client handshake properties. Only the
latter is affected.) It's also pretty difficult to imagine why a caller
would wish to query this stuff at any other time, and there are clear
benefits to dropping the CA list after the handshake (some servers send
ABSURDLY large lists).
Change-Id: I3ac3b601ff0cfa601881ce77ae33d99bb5327004
Reviewed-on: https://boringssl-review.googlesource.com/11521
Reviewed-by: Adam Langley <agl@google.com>
EnableAllCiphers is problematic since some (version, cipher)
combinations aren't even defined and crash. Instead, use the
SendCipherSuite bug to mask the true cipher (which is becomes arbitrary)
for failure tests. The shim should fail long before we get further.
This lets us remove a number of weird checks in the TLS 1.3 code.
This also fixes the UnknownCipher tests which weren't actually testing
anything. EnableAllCiphers is now AdvertiseAllConfiguredCiphers and
does not filter out garbage values.
Change-Id: I7102fa893146bb0d096739e768c5a7aa339e51a8
Reviewed-on: https://boringssl-review.googlesource.com/11481
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This finally removes the last Android hack. Both Chromium and Android
end up needing this thing (Chromium needs it for WebCrypto but currently
uses the EVP_AEAD version and Android needs it by way of
wpa_supplicant).
On the Android side, the alternative is we finish upstream's
NEED_INTERNAL_AES_WRAP patch, but then it just uses its own key-wrap
implementation. This seems a little silly, considering we have a version
of key-wrap under a different API anyway.
It also doesn't make much sense to leave the EVP_AEAD API around if we
don't want people to use it and Chromium's the only consumer. Remove it
and I'll switch Chromium to the new---er, old--- APIs next roll.
Change-Id: I23a89cda25bddb6ac1033e4cd408165f393d1e6c
Reviewed-on: https://boringssl-review.googlesource.com/11410
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is just to reduce the diff with upstream's files so it's easier to
tell what's going on. Upstream's files have lots and lots of trailing
whitespace. We were also missing a comment.
Change-Id: Icfc3b52939823a046a3744fd8e619b5bd6160715
Reviewed-on: https://boringssl-review.googlesource.com/11408
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
ConflictingVersionNegotiation really should be about, say 1.1 and 1.2
since those may be negotiated via either mechanism. (Those two cases are
actually kinda weird and we may wish to change the spec. But, in the
meantime, test that we have the expected semantics.)
Also test that we ignore true TLS 1.3's number for now, until we use it,
and that TLS 1.3 suitably ignores ClientHello.version.
Change-Id: I76c660ddd179313fa68b15a6fda7a698bef4d9c9
Reviewed-on: https://boringssl-review.googlesource.com/11407
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
They weren't updated for the new version negotiation. (Though right now
they're just testing that we *don't* implement the downgrade detection
because it's a draft version.)
Change-Id: I4c983ebcdf3180d682833caf1e0063467ea41544
Reviewed-on: https://boringssl-review.googlesource.com/11406
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Breaking from inside the inner loop doesn't do what the code wants.
Instead the outer loop will continue running and it's possible for it to
read off the end of the buffer. (Found with libFuzzer.)
Next change will update the other abort points in this code to match.
Change-Id: I006dca0cd4c31db1c4b5e84b996fe24b2f1e6c13
Reviewed-on: https://boringssl-review.googlesource.com/11421
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
If asn1_item_ex_combine_new fails in one of the ASN1_template_new calls
just before the ASN1_OP_NEW_POST call, ASN1_item_ex_free will free the
temporary object which ultimately calls ASN1_OP_FREE_POST. This means
that ASN1_OP_FREE_POST needs to account for zero-initialized objects.
Change-Id: I56fb63bd5c015d9dfe3961606449bc6f5b1259e3
Reviewed-on: https://boringssl-review.googlesource.com/11403
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This function is used by NGINX to enable specific curves for ECDH from a
configuration file. However when building with BoringSSL, since it's not
implmeneted, it falls back to using EC_KEY_new_by_curve_name() wich doesn't
support X25519.
Change-Id: I533df4ef302592c1a9f9fc8880bd85f796ce0ef3
Reviewed-on: https://boringssl-review.googlesource.com/11382
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This change adds AES and GHASH assembly from upstream, with the aim of
speeding up AES-GCM.
The PPC64LE assembly matches the interface of the ARMv8 assembly so I've
changed the prefix of both sets of asm functions to be the same
("aes_hw_").
Otherwise, the new assmebly files and Perlasm match exactly those from
upstream's c536b6be1a (from their master branch).
Before:
Did 1879000 AES-128-GCM (16 bytes) seal operations in 1000428us (1878196.1 ops/sec): 30.1 MB/s
Did 61000 AES-128-GCM (1350 bytes) seal operations in 1006660us (60596.4 ops/sec): 81.8 MB/s
Did 11000 AES-128-GCM (8192 bytes) seal operations in 1072649us (10255.0 ops/sec): 84.0 MB/s
Did 1665000 AES-256-GCM (16 bytes) seal operations in 1000591us (1664016.6 ops/sec): 26.6 MB/s
Did 52000 AES-256-GCM (1350 bytes) seal operations in 1006971us (51640.0 ops/sec): 69.7 MB/s
Did 8840 AES-256-GCM (8192 bytes) seal operations in 1013294us (8724.0 ops/sec): 71.5 MB/s
After:
Did 4994000 AES-128-GCM (16 bytes) seal operations in 1000017us (4993915.1 ops/sec): 79.9 MB/s
Did 1389000 AES-128-GCM (1350 bytes) seal operations in 1000073us (1388898.6 ops/sec): 1875.0 MB/s
Did 319000 AES-128-GCM (8192 bytes) seal operations in 1000101us (318967.8 ops/sec): 2613.0 MB/s
Did 4668000 AES-256-GCM (16 bytes) seal operations in 1000149us (4667304.6 ops/sec): 74.7 MB/s
Did 1202000 AES-256-GCM (1350 bytes) seal operations in 1000646us (1201224.0 ops/sec): 1621.7 MB/s
Did 269000 AES-256-GCM (8192 bytes) seal operations in 1002804us (268247.8 ops/sec): 2197.5 MB/s
Change-Id: Id848562bd4e1aa79a4683012501dfa5e6c08cfcc
Reviewed-on: https://boringssl-review.googlesource.com/11262
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for implementing the version extension and is
probably what we should have done from the beginning as it makes
intolerance bugs simpler.
This means knobs like SendClientVersion and SendServerVersion deal with
the wire values while knobs like NegotiateVersion and MaxVersion deal
with logical versions. (This matches how the bugs have always worked.
SendFoo is just a weird post-processing bit on the handshake messages
while NegotiateVersion actually changes how BoGo behaves.)
BUG=90
Change-Id: I7f359d798d0899fa2742107fb3d854be19e731a4
Reviewed-on: https://boringssl-review.googlesource.com/11300
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This GREASEs cipher suites, groups, and extensions. For now, we'll
always place them in a hard-coded position. We can experiment with more
interesting strategies later.
If we add new ciphers and curves, presumably we prefer them over current
ones, so place GREASE values at the front. This prevents implementations
from parsing only the first value and ignoring the rest.
Add two new extensions, one empty and one non-empty. Place the empty one
in front (IBM WebSphere can't handle trailing empty extensions) and the
non-empty one at the end.
Change-Id: If2e009936bc298cedf2a7a593ce7d5d5ddbb841a
Reviewed-on: https://boringssl-review.googlesource.com/11241
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The high-level documentation for CBB describes using CBB_flush when a
child goes out of scope, but the function level documentation for
CBB_flush is less clear that CBB_flush will result in the CBB being
safe to use after the children go out of scope.
Change-Id: I58bf9e59a87d2be31a969097455aeeae6381efb3
Reviewed-on: https://boringssl-review.googlesource.com/11261
Reviewed-by: David Benjamin <davidben@google.com>
We used upstream's reformat script, but they had stuck hyphens
everywhere to tell indent to leave a comment alone. Fix this one since
it was especially hard to read.
Change-Id: I9f43bd57dbcf66b79b775ad10ee67867d815ed33
Reviewed-on: https://boringssl-review.googlesource.com/11243
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This was done by first minimizing the existing set and then merging in a
fresh recording from runner. Glancing through LCOV output does not
reveal anything anomolous. Fuzzer mode seems to be working as expected.
Change-Id: Ife0959a5e16e3c7e2e5a2deb0c32539ff2bc740b
Reviewed-on: https://boringssl-review.googlesource.com/11229
Reviewed-by: Adam Langley <agl@google.com>
This allows the fuzzer to discover server-side resumption paths by
simply supplying what we'd like the ticket to decrypt to in the clear.
We also have a natural way to get transcripts out of runner. We record
the runner-side transcripts, so all resumption handshakes will replay
the shim-created unencrypted tickets.
BUG=104
Change-Id: Icf9cbf4af520077d38e2c8c2766b6f8bfa3c9ab5
Reviewed-on: https://boringssl-review.googlesource.com/11224
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We want to ensure -fuzzer passes tests, except for the tests it
intentionally fails on. This ensures that we don't lose our ability to
refresh the fuzzer transcripts.
Change-Id: I761856c30379a3934fd46a24627ef8415b136f93
Reviewed-on: https://boringssl-review.googlesource.com/11221
Reviewed-by: Adam Langley <agl@google.com>
Apparently we never wrote one of those. Also send a decrypt_error alert
to be consistent with all the other signature checks.
Change-Id: Ib5624d098d1e3086245192cdce92f5df26005064
Reviewed-on: https://boringssl-review.googlesource.com/11180
Reviewed-by: David Benjamin <davidben@google.com>
SSL_peek works fine for us, but OpenSSL 1.1.0 regressed this
(https://github.com/openssl/openssl/issues/1563), and we don't have
tests either. Fix this.
SSL_peek can handle all weird events that SSL_read can, so use runner
and tell bssl_shim to do a SSL_peek + SSL_peek + SSL_read instead of
SSL_read. Then add tests for all the events we may discover.
Change-Id: I9e8635e3ca19653a02a883f220ab1332d4412f98
Reviewed-on: https://boringssl-review.googlesource.com/11090
Reviewed-by: Adam Langley <agl@google.com>
We'd previously been assuming we'd want to predict P-256 and X25519 but,
on reflection, that's nonsense. Although, today, P-256 is widespread and
X25519 is less so, that's not the right question to ask. Those servers
are all 1.2.
The right question is whether we believe enough servers will get to TLS
1.3 before X25519 to justify wasting 64 bytes on all other connections.
Given that OpenSSL has already shipped X25519 and Microsoft was doing
interop testing on X25519 around when we were shipping it, I think the
answer is no.
Moreover, if we are wrong, it will be easier to go from predicting one
group to two rather than the inverse (provided we send a fake one with
GREASE). I anticipate prediction-miss HelloRetryRequest logic across the
TLS/TCP ecosystem will be largely untested (no one wants to pay an RTT),
so taking a group out of the predicted set will likely be a risky
operation.
Only predicting one group also makes things a bit simpler. I haven't
done this here, but we'll be able to fold the 1.2 and 1.3 ecdh_ctx's
together, even.
Change-Id: Ie7e42d3105aca48eb9d97e2e05a16c5379aa66a3
Reviewed-on: https://boringssl-review.googlesource.com/10960
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The old numbers violate a MUST-level requirement in TLS 1.2 to not
advertise anonymous (0x0700 ends in 0x00). The spec has been updated
with new allocations which avoid these.
BUG=webrtc:6342
Change-Id: Ia5663ada98fa1ebf0f8a7f50fe74a0e9206c4194
Reviewed-on: https://boringssl-review.googlesource.com/11131
Reviewed-by: Adam Langley <agl@google.com>
Upstream added these functions after we did but decided to change the
names slightly. I'm not sure why they wanted to add the "proto" in
there, but align with them nonetheless so the ecosystem only has one set
of these functions.
BUG=90
Change-Id: Ia9863c58c9734374092051f02952b112806040cc
Reviewed-on: https://boringssl-review.googlesource.com/11123
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for using the supported_versions extension to
experiment with draft TLS 1.3 versions, since we don't wish to restore
the fallback. With versions begin opaque values, we will want
version_from_wire to reject unknown values, not attempt to preserve
order in some way.
This means ClientHello.version processing needs to be separate code.
That's just written out fully in negotiate_version now. It also means
SSL_set_{min,max}_version will notice invalid inputs which aligns us
better with upstream's versions of those APIs.
This CL doesn't replace ssl->version with an internal-representation
version, though follow work should do it once a couple of changes land
in consumers.
BUG=90
Change-Id: Id2f5e1fa72847c823ee7f082e9e69f55e51ce9da
Reviewed-on: https://boringssl-review.googlesource.com/11122
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Passing --quiet makes valgrind only print out errors, so we don't need
to suppress things. Combine that with checking valgrind's dedicated exit
code so we notice errors that happen before the "---DONE---" marker.
This makes that marker unnecessary for valgrind. all_tests.go was not
sensitive to this, but still would do well to have valgrind be silent.
Change-Id: I841edf7de87081137e38990e647e989fd7567295
Reviewed-on: https://boringssl-review.googlesource.com/11128
Reviewed-by: Adam Langley <agl@google.com>
Runner needs to implement fuzzer mode as well so we can record
transcripts from it. A bunch of tests were failing:
- C and Go disagreed on what fuzzer mode did to TLS 1.3 padding. So we
fuzz more code, align Go with C. Fuzzer mode TLS 1.3 still pads but
just skips the final AEAD.
- The deterministic RNG should be applied per test, not per exchange. It
turns out, if your RNG is deterministic, one tends to pick the same
session ID over and over which confuses clients. (Resumption is
signaled by echoing the session ID.)
Now the only failing tests are the ones one would expect to fail.
BUG=79
Change-Id: Ica23881a6e726adae71e6767730519214ebcd62a
Reviewed-on: https://boringssl-review.googlesource.com/11126
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Plain PSK omits the ServerKeyExchange when there is no hint and includes
it otherwise (it should have always sent it), while other PSK ciphers
like ECDHE_PSK cannot omit the hint. Having different capabilities here
is odd and RFC 4279 5.2 suggests that all PSK ciphers are capable of
"[not] provid[ing] an identity hint".
Interpret this to mean no identity hint and empty identity hint are the
same state. Annoyingly, this gives a plain PSK implementation two
options for spelling an empty hint. The spec isn't clear and this is not
really a battle worth fighting, so I've left both acceptable and added a
test for this case.
See also https://android-review.googlesource.com/c/275217/. This is also
consistent with Android's PskKeyManager API, our only consumer anyway.
https://developer.android.com/reference/android/net/PskKeyManager.html
Change-Id: I8a8e6cc1f7dd1b8b202cdaf3d4f151bebfb4a25b
Reviewed-on: https://boringssl-review.googlesource.com/11087
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Android uses MinGW for some host tools on Windows. That toolchain
doesn't support the #pragma tricks we use for thread-local destructors,
but does appear to support pthreads.
This also lets us remove the INIT_ONCE workaround, although that's
removable anyway since Android's MinGW is now new enough.
Change-Id: I8d1573923fdaac880a50d84acbebbf87461c50d2
Reviewed-on: https://boringssl-review.googlesource.com/11125
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Kenny Root <kroot@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
C99 decided that, like PRI* macros, UINT64_C and friends should be
conditioned on __STDC_CONSTANT_MACROS in C++. C++11 then decided this
was ridiculous and overruled this decision. However, Android's headers
in older NDKs mistakenly followed the C99 rules for C++, so work around
this.
This fixes the android_arm bots.
Change-Id: I3b49e8dfc20190ebfa78876909bd0dccd3e210ea
Reviewed-on: https://boringssl-review.googlesource.com/11089
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Android currently implements this manually (see NativeBN_putULongInt) by
reaching into BIGNUM's internals. BN_ULONG is a somewhat unfortunate API
anyway as the size is platform-dependent, so add a platform-independent
way to do this.
The other things Android needs are going to need more work, but this
one's easy.
BUG=97
Change-Id: I4af4dc29f9845bdce0f0663c379b4b5d3e1dc46e
Reviewed-on: https://boringssl-review.googlesource.com/11088
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
clang's integrated assembler does not appear to support debug symbols
yet. (And somehow CMake has another statefulness bug that causes this to
only manifest on a second run, so the try bots aren't noticing.)
Also don't clobber the existing CMAKE_ASM_FLAGS value.
Change-Id: Ia04345f0dc2f9833ac13912d761cfab3479c10cd
Reviewed-on: https://boringssl-review.googlesource.com/11083
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
access_denied is only used to indicate client cert errors and Chrome
maps it to ERR_SSL_BAD_CLIENT_AUTH_CERT accordingly:
access_denied
A valid certificate was received, but when access control was
applied, the sender decided not to proceed with negotiation. This
message is always fatal.
We don't appear to be the cause of Chrome's recent
ERR_SSL_BAD_CLIENT_AUTH_CERT spike, but we should send these correctly
nonetheless.
If the early callback fails, handshake_failure seems the most
appropriate ("I was unable to find suitable parameters"). There isn't
really an alert that matches DoS, but internal_error seems okay?
internal_error
An internal error unrelated to the peer or the correctness of the
protocol (such as a memory allocation failure) makes it impossible
to continue. This message is always fatal.
There's nothing wrong, per se, with your ClientHello, but I just can't
deal with it right now. Please go away.
Change-Id: Icd1c998c09dc42daa4b309c1a4a0f136b85eb69d
Reviewed-on: https://boringssl-review.googlesource.com/11084
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This function (actually a macro in OpenSSL) is used by several projects
(e.g. OpenResty, OpenVPN, ...) so it can useuful to provide it for
compatibility.
However, depending on the semantics of the BIO type (e.g. BIO_pair), the
return value can be meaningless, which might explain why it was removed.
Change-Id: I0e432c92222c267eb994d32b0bc28e999c4b40a7
Reviewed-on: https://boringssl-review.googlesource.com/11020
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reason for revert: Right now in TLS 1.3, certificate_auth is exactly
the same as whether we're doing resumption. With the weird reauth
stuff punted to later in the spec, having extra state is just more
room for bugs to creep in.
Original issue's description:
> Determining certificate_auth and key_exchange based on SSL.
>
> This allows us to switch TLS 1.3 to use non-cipher based negotiation
> without needing to use separate functions between 1.3 and below.
>
> BUG=77
>
> Change-Id: I9207e7a6793cb69e8300e2c15afe3548cbf82af2
> Reviewed-on: https://boringssl-review.googlesource.com/10803
> Reviewed-by: David Benjamin <davidben@google.com>
> Commit-Queue: David Benjamin <davidben@google.com>
> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
>
Change-Id: I240e3ee959ffd1f2481a06eabece3af554d20ffa
Reviewed-on: https://boringssl-review.googlesource.com/11008
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It's easy to forget to check those. Unfortunately, it's also easy to
forget to check inner structures, which is going to be harder to stress,
but do these to start with. In doing, so fix up and unify some
error-handling, and add a missing check when parsing TLS 1.2
CertificateRequest.
This was also inspired by the recent IETF posting.
Change-Id: I27fe3cd3506258389a75d486036388400f0a33ba
Reviewed-on: https://boringssl-review.googlesource.com/10963
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This was done just by grepping for 'size_t i;' and 'size_t j;'. I left
everything in crypto/x509 and friends alone.
There's some instances in gcm.c that are non-trivial and pulled into a
separate CL for ease of review.
Change-Id: I6515804e3097f7e90855f1e7610868ee87117223
Reviewed-on: https://boringssl-review.googlesource.com/10801
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Really the only thing we should be doing with these ciphers is hastening
their demise, but it was the weekend and this seemed like fun.
EVP_tls_cbc_copy_mac needs to rotate a buffer by a secret amount. (It
extracts the MAC, but rotated.) We have two codepaths for this. If
CBC_MAC_ROTATE_IN_PLACE is defined (always on), we make some assumptions
abuot cache lines, play games with volatile, and hope that doesn't leak
anything. Otherwise, we do O(N^2) work to constant-time select the
rotation incidences.
But we can do O(N lg N). Rotate by powers of two and constant-time
select by the offset's bit positions. (Handwaivy lower-bound: an array
position has N possible values, so, armed with only a constant-time
select, we need O(lg N) work to resolve it. There's N array positions,
so O(N lg N).)
A microbenchmark of EVP_tls_cbc_copy_mac shows this is 27% faster than
the old one, but still 32% slower than the in-place version.
in-place:
Did 15724000 CopyFromMAC operations in 20000744us (786170.8 ops/sec)
N^2:
Did 8443000 CopyFromMAC operations in 20001582us (422116.6 ops/sec)
N lg N:
Did 10718000 CopyFromMAC operations in 20000763us (535879.6 ops/sec)
This results in the following the CBC ciphers. I measured
AES-128-CBC-SHA1 and AES-256-CBC-SHA384 which are, respectively, the
cipher where the other bits are the fastest and the cipher where N is
largest.
in-place:
Did 2634000 AES-128-CBC-SHA1 (16 bytes) open operations in 10000739us (263380.5 ops/sec): 4.2 MB/s
Did 1424000 AES-128-CBC-SHA1 (1350 bytes) open operations in 10002782us (142360.4 ops/sec): 192.2 MB/s
Did 531000 AES-128-CBC-SHA1 (8192 bytes) open operations in 10002460us (53086.9 ops/sec): 434.9 MB/s
N^2:
Did 2529000 AES-128-CBC-SHA1 (16 bytes) open operations in 10001474us (252862.7 ops/sec): 4.0 MB/s
Did 1392000 AES-128-CBC-SHA1 (1350 bytes) open operations in 10006659us (139107.4 ops/sec): 187.8 MB/s
Did 528000 AES-128-CBC-SHA1 (8192 bytes) open operations in 10001276us (52793.3 ops/sec): 432.5 MB/s
N lg N:
Did 2531000 AES-128-CBC-SHA1 (16 bytes) open operations in 10003057us (253022.7 ops/sec): 4.0 MB/s
Did 1390000 AES-128-CBC-SHA1 (1350 bytes) open operations in 10003287us (138954.3 ops/sec): 187.6 MB/s
Did 531000 AES-128-CBC-SHA1 (8192 bytes) open operations in 10002448us (53087.0 ops/sec): 434.9 MB/s
in-place:
Did 1249000 AES-256-CBC-SHA384 (16 bytes) open operations in 10001767us (124877.9 ops/sec): 2.0 MB/s
Did 879000 AES-256-CBC-SHA384 (1350 bytes) open operations in 10009244us (87818.8 ops/sec): 118.6 MB/s
Did 344000 AES-256-CBC-SHA384 (8192 bytes) open operations in 10025897us (34311.1 ops/sec): 281.1 MB/s
N^2:
Did 1072000 AES-256-CBC-SHA384 (16 bytes) open operations in 10008090us (107113.3 ops/sec): 1.7 MB/s
Did 780000 AES-256-CBC-SHA384 (1350 bytes) open operations in 10007787us (77939.3 ops/sec): 105.2 MB/s
Did 333000 AES-256-CBC-SHA384 (8192 bytes) open operations in 10016332us (33245.7 ops/sec): 272.3 MB/s
N lg N:
Did 1168000 AES-256-CBC-SHA384 (16 bytes) open operations in 10007671us (116710.5 ops/sec): 1.9 MB/s
Did 836000 AES-256-CBC-SHA384 (1350 bytes) open operations in 10001536us (83587.2 ops/sec): 112.8 MB/s
Did 339000 AES-256-CBC-SHA384 (8192 bytes) open operations in 10018522us (33837.3 ops/sec): 277.2 MB/s
TLS CBC performance isn't as important as it was before, and the costs
aren't that high, so avoid making assumptions about cache lines. (If we
care much about CBC open performance, we probably should get the malloc
out of EVP_tls_cbc_digest_record at the end.)
Change-Id: Ib8d8271be4b09e5635062cd3b039e1e96f0d9d3d
Reviewed-on: https://boringssl-review.googlesource.com/11003
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
In OpenSSL, they're used in the 32-bit x86 Blowfish, CAST, DES, and RC5
assembly bits. We don't have any of those.
Change-Id: I36f22ca873842a200323cd3f398d2446f7bbabca
Reviewed-on: https://boringssl-review.googlesource.com/10780
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This simplifies the logic around SSL_clear to reset the state for a new
handshake. The state around here is still a little iffy, but this is a
slight improvement.
The SSL_ST_CONNECT and SSL_ST_ACCEPT states are still kept separate to
avoid problems with the info callback reporting SSL_ST_INIT. Glancing
through info callback consumers, although they're all debugging, they
tend to assume that all intermediate states either have only
SSL_ST_CONNECT set or only SSL_ST_ACCEPT set.
(They also all look identical which makes me think it's copy-and-pasted
from OpenSSL command-line tool or something.)
Change-Id: I55503781e52b51b4ca829256c14de6f5942dae51
Reviewed-on: https://boringssl-review.googlesource.com/10760
Reviewed-by: Adam Langley <agl@google.com>
This mechanism is incompatible with deploying draft versions of TLS 1.3.
Suppose a draft M client talks to a draft N server, M != N. (Either M or
N could also be the final standard revision should there be lingering
draft clients or servers.) The server will notice the mismatch and
pretend ClientHello.version is TLS 1.2, not TLS 1.3. But this will
trigger anti-downgrade signal and cause an interop failure! And if it
doesn't trigger, all the clever tricks around ServerHello.random being
signed in TLS 1.2 are moot.
We'll put this back when the dust has settled.
Change-Id: Ic3cf72b7c31ba91e5cca0cfd7a3fca830c493a43
Reviewed-on: https://boringssl-review.googlesource.com/11005
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We need time.h for clock_gettime on Linux. Previously, scoped_types.h
was pulling in everything and getting it for us, but now it doesn't
exist. We seem to have been pulling it in on accident anyway but
it seems Android's system headers end up not doing so.
Change-Id: I0a04495b1462649d77d62018e317c416803ac58d
Reviewed-on: https://boringssl-review.googlesource.com/11000
Reviewed-by: Matt Braithwaite <mab@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The compiler complains about:
error: explicit specialization of
'bssl::internal::Deleter<evp_pkey_st>' after instantiation
This is because, although the deleter's operator() is not instantiated
without emitting std::unique_ptr's destructor, the deleter itself *is*.
Deleters are allowed to have non-zero size, so a std::unique_ptr
actually embeds a copy of the deleter, so it needs the size of the
deleter.
As with all problems in computer science, we fix this with a layer of
indirection. Instead of specializing the deleter, we specialize
bssl::internal::DeleterImpl which, when specialized, has a static method
Free. That is only instantiated inside
bssl::internal::Deleter::operator(), giving us the desired properties.
(Did I mention forward decls are terrible? I wish people wouldn't want
them so much.)
Also appease clang-format.
Change-Id: I9a07b2fd13e8bdfbd204e225ac72c52d20a397dc
Reviewed-on: https://boringssl-review.googlesource.com/10964
Reviewed-by: Matt Braithwaite <mab@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Although RFC 6066 recommends against it, some servers send a warning
alert prior to ServerHello on SNI mismatch, and, per spec, TLS 1.2
allows it.
We're fine here, but add a test for it. It interacts interestingly with
TLS 1.3 forbidding warning alerts because it happens before version
negotiation.
Change-Id: I0032313c986c835b6ae1aa43da6ee0dad17a97c2
Reviewed-on: https://boringssl-review.googlesource.com/10800
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Add a test that RSA-PSS is available in TLS 1.2 by default, both for
signing and verifying. Note that if a custom SSL_PRIVATE_KEY_METHOD is
used and it sets signing preferences, it won't use RSA-PSS if it doesn't
know about it. (See *-Sign-Negotiate-* tests.)
Change-Id: I3776a0c95480188a135795f7ebf31f2b0e0626cc
Reviewed-on: https://boringssl-review.googlesource.com/10723
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Changing parameters on renegotiation makes all our APIs confusing. This
one has no reason to change, so lock it down. In particular, our
preference to forbid Token Binding + renego may be overridden at the
IETF, even though it's insane. Loosening it will be a bit less of a
headache if EMS can't change.
https://www.ietf.org/mail-archive/web/unbearable/current/msg00690.html
claims that this is already in the specification and enforced by NSS. I
can't find anything to this effect in the specification. It just says
the client MUST disable renegotiation when EMS is missing, which is
wishful thinking. At a glance, NSS doesn't seem to check, though I could
be misunderstanding the code.
Nonetheless, locking this down is a good idea anyway. Accurate or not,
take the email as an implicit endorsement of this from Mozilla.
Change-Id: I236b05991d28bed199763dcf2f47bbfb9d0322d7
Reviewed-on: https://boringssl-review.googlesource.com/10721
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This removes all but the generic C RC4 implementation. At this point we
want to optimize for size/simplicity rather than speed.
See also upstream's 3e9e810f2e047effb1056211794d2d12ec2b04e7 which
removed the RC4_CHUNK code and standardized on RC4_INDEX. A
since-removed comment says that it was implemented for "pre-21164a Alpha
CPUs don't have byte load/store instructions" and helps with SPARC and
MIPS.
This also removes all the manual loop unrolling.
Change-Id: I91135568483260b2e1e675f190fb00ce8f9eff3d
Reviewed-on: https://boringssl-review.googlesource.com/10720
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This and the following commits will import NIST's ECC test vectors.
Right now all our tests pass if I make P-224 act like P-521, which is
kind of embarrassing. (Other curves are actually tested, but only
because runner.go tests them against BoGo.)
Change-Id: Id0b20451ebd5f10f1d09765a810ad140bea28fa0
Reviewed-on: https://boringssl-review.googlesource.com/10700
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
We may need to implement high tag number form someday. CBS_get_asn1 has
an unsigned output to allow for this, but CBB_add_asn1 takes a uint8_t
(I think this might be my fault). Fix that which also fixes a
-Wconversion warning.
Simply leaving room in tag representation will still cause troubles
because the class and constructed bits overlap with bits for tag numbers
above 31. Probably the cleanest option would be to shift them to the top
3 bits of a u32 and thus not quite match the DER representation. Then
CBS_get_asn1 and CBB_add_asn1 will internally munge that into the DER
representation and consumers may continue to write things like:
tag_number | CBS_ASN1_CONTEXT_SPECIFIC
I haven't done that here, but in preparation for that, document that
consumers need to use the values and should refrain from assuming the
correspond to DER.
Change-Id: Ibc76e51f0bc3b843e48e89adddfe2eaba4843d12
Reviewed-on: https://boringssl-review.googlesource.com/10502
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
peer_sigalgs should live on SSL_HANDSHAKE. This both releases a little
bit of memory after the handshake is over and also avoids the bug where
the sigalgs get dropped if SSL_set_SSL_CTX is called at a bad time. See
also upstream's 14e14bf6964965d02ce89805d9de867f000095aa.
This only affects consumers using the old SNI callback and not
select_certificate_cb.
Add a test that the SNI callback works as expected. In doing so, add an
SSL_CTX version of the signing preferences API. This is a property of
the cert/key pair (really just the key) and should be tied to that. This
makes it a bit easier to have the regression test work with TLS 1.2 too.
I thought we'd fixed this already, but apparently not... :-/
BUG=95
Change-Id: I75b02fad4059e6aa46c3b05183a07d72880711b3
Reviewed-on: https://boringssl-review.googlesource.com/10445
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
958aaf1ea1, imported from upstream, had an
off-by-one error. Reproducing the failure is fairly easy as it can't
even serialize 1. See also upstream's
099e2968ed3c7d256cda048995626664082b1b30.
Rewrite the function completely with CBB and add a basic test.
BUG=chromium:639740
Change-Id: I41a91514c4bb9e83854824ed5258ffe4e49d9491
Reviewed-on: https://boringssl-review.googlesource.com/10540
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I'll hold on regenerating the transcripts until either the protocol has
stablized more or we're ready to start actually deploying some of this,
but we can get this in now.
Confirmed these #ifdef points are covered by tests:
- BadFinished-*-TLS13
- *-InvalidSignature-*-TLS13
BUG=79
Change-Id: I5f6b9d0f50ac33d5cc79688928fb3fdf6df845ae
Reviewed-on: https://boringssl-review.googlesource.com/10500
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Having two copies of this is confusing. This field is inherently tied to
the certificate chain, which lives on SSL_SESSION, so this should live
there too. This also wasn't getting reset correctly on SSL_clear, but
this is now resolved.
Change-Id: I22b1734a93320bb0bf0dc31faa74d77a8e1de906
Reviewed-on: https://boringssl-review.googlesource.com/10283
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
As documented by OpenSSL, it does not interact with session resumption
correctly:
https://www.openssl.org/docs/manmaster/ssl/SSL_set_verify_result.html
Sadly, netty-tcnative calls it, but we should be able to get them to
take it out because it doesn't do anything. Two of the three calls are
immediately after SSL_new. In OpenSSL and BoringSSL as of the previous
commit, this does nothing.
The final call is in verify_callback (see SSL_set_verify). This callback
is called in X509_verify_cert by way of X509_STORE_CTX_set_verify_cb.
As soon as X509_verify_cert returns, ssl->verify_result is clobbered
anyway, so it doesn't do anything.
Within OpenSSL, it's used in testdane.c. As far as I can tell, it does
not actually do a handshake and just uses this function to fake having
done one. (Regardless, we don't need to build against that.)
This is done in preparation for removing ssl->verify_result in favor of
session->verify_result.
Change-Id: I7e32d7f26c44f70136c72e58be05a3a43e62582b
Reviewed-on: https://boringssl-review.googlesource.com/10485
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
In TLS 1.3 draft 14, due to resumption using a different cipher, this
is actually not too hard to mess up. (In fact BoGo didn't quite get it
right.)
Fortunately, the new cipher suite negotiation in draft 15 should make
this reasonable again once we implement it. In the meantime, test it.
Change-Id: I2eb948eeaaa051ecacaa9095b66ff149582ea11d
Reviewed-on: https://boringssl-review.googlesource.com/10442
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
9498e74 changed the default value of verify_result to an error. This
tripped up NGINX, which depends on a bug[1] in OpenSSL. netty-tcnative
also uses this behavior, though it currently isn't tripped up by 9498e74
because it calls |SSL_set_verify_result|. However, we would like to
remove |SSL_set_verify_result| and with two data points, it seems this
is behavior we must preserve.
This change sets |verify_result| to |X509_V_OK| when a) no client
certificate is requested or b) none is given and it's optional.
[1] See BUGS in https://www.openssl.org/docs/manmaster/ssl/SSL_get_verify_result.html
Change-Id: Ibd33660ae409bfe272963a8c39b7e9aa83c3d635
Reviewed-on: https://boringssl-review.googlesource.com/9067
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I found an earlier reference for an algorithm for the optimized
computation of n0 that is very similar to the one in the "Montgomery
Multiplication" paper cited in the comments. Add a reference to it.
Henry S. Warren, Jr. pointed out that his "Montgomery Multiplication"
paper is not a chapter of his book, but a supplement to the book.
Correct the reference to it.
Change-Id: Iadeb148c61ce646d1262ccba0207a31ebdad63e9
Reviewed-on: https://boringssl-review.googlesource.com/10480
Reviewed-by: Adam Langley <agl@google.com>
This was causing some Android breakage. The real bug is actually
entirely in Android for getting its error-handling code wrong and not
handling multiple errors. I'll fix that. (See b/30917411.)
That said, BN_R_NO_INVERSE is a perfectly legitimate reason for those
operations to fail, so ERR_R_INTERNAL_ERROR isn't really a right thing
to push in front anyway. We're usually happy enough with single-error
returns (I'm still a little skeptical of this queue idea), so let's just
leave it at that.
Change-Id: I469b6e2b5987c6baec343e2cfa52bdcb6dc42879
Reviewed-on: https://boringssl-review.googlesource.com/10483
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The TLS 1.3 state machine is actually less in need of the aggressive
state machine coverage tests, but nonetheless, we should cover all
handshake shapes. PSK resumption and HelloRetryRequest were missing.
We were also accidentally running "DTLS" versions of the TLS 1.3 tests
but silently running TLS 1.2.
Change-Id: I65db4052b89d770db7e47738e73aaadde9634236
Reviewed-on: https://boringssl-review.googlesource.com/10441
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
If an oversize BIGNUM is presented to BN_bn2dec() it can cause
BN_div_word() to fail and not reduce the value of 't' resulting
in OOB writes to the bn_data buffer and eventually crashing.
Fix by checking return value of BN_div_word() and checking writes
don't overflow buffer.
Thanks to Shi Lei for reporting this bug.
CVE-2016-2182
(Imported from upstream's e36f27ddb80a48e579783bc29fb3758988342b71.)
Change-Id: Ib9078921b4460952c4aa5a6b03ec39a03704bb90
Reviewed-on: https://boringssl-review.googlesource.com/10367
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
RT#4530
(Imported from upstream's 7123aa81e9fb19afb11fdf3850662c5f7ff1f19c.)
We've yet to enable this code, but this confirms that we do indeed need
to get our future all-variants stuff working on Windows as well as
Linux and find an AVX2-capable CI setup on each.
The crash here is caused by some win64-only code using %rax as a frame
pointer (perlasm injects a mov rax,rsp in the prologue of every win64
function).
Change-Id: Ifbe59ceb6ae29266d9cf8a461920344a32b6e555
Reviewed-on: https://boringssl-review.googlesource.com/10366
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Check for error return in BN_div_word().
(Imported from upstream's d871284aca5524c85a6460119ac1b1e38f7e19c6.)
This function is only called from crypto/obj to convert strings like
"1.2.3.4.5" to OIDs. We may wish to see about rewriting it just so it's
out of the way.
Change-Id: Ia8379d2dd30606f6a81ce24dee8852312cb7f127
Reviewed-on: https://boringssl-review.googlesource.com/10365
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
These functions are unused. Upstream recently needed to limit recursion
depth on this function in 81f69e5b69b8e87ca5d7080ab643ebda7808542c. It
looks like deeply nested BER constructed strings could cause unbounded
stack usage. Delete the function rather than import the fix.
Change-Id: I7868080fae52b46fb9f9147543c0f7970d8fff98
Reviewed-on: https://boringssl-review.googlesource.com/10368
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The weird function thing is a remnant of OpenSSL and I think something
weird involving Windows and symbols exported from dlls. These aren't
exposed in the public API, so have everything point to the tables
directly.
This is in preparation for making built-in EC_GROUPs static. (The static
EC_GROUPs won't be able to call a function wrapper.)
BUG=20
Change-Id: If33888430f32e51f48936db4046769aa1894e3aa
Reviewed-on: https://boringssl-review.googlesource.com/10346
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
In TLS 1.2 and below, the server is not supposed to echo it, but I just
came across a BigIP server which does. Document this so we know to take
care before trying to flip it in the future.
(It's actually kind of odd that it wasn't allowed to be sent given TLS
1.2 makes supported_groups interact with ECDSA client certificates. Ah
well.)
Change-Id: I4b97266f461e85bb1ad9bb935470e027f926d4df
Reviewed-on: https://boringssl-review.googlesource.com/10320
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
These are probably a good idea to ship so long as we have the PSK
callbacks at all, but they're not *completely* standard yet and Android
tests otherwise need updating to know about them. We don't care enough
about PSK to be in a rush to ship them, and taking them out is an easier
default action until then.
Change-Id: Ic646053d29b69a114e2efea61d593d5e912bdcd0
Reviewed-on: https://boringssl-review.googlesource.com/10225
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
People seem to like adding ifdefs for us for random initialization
functions that are cheap enough to add no-ops stubs for.
Change-Id: I7fb4e978e035329cd81d9bf33ab0d64fde6cc05f
Reviewed-on: https://boringssl-review.googlesource.com/10280
Reviewed-by: Adam Langley <agl@google.com>
If cert_cb runs asynchronously, we end up repeating a large part of very
stateful ClientHello processing. This seems to be mostly fine and there
are few users of server-side cert_cb (it's a new API in 1.0.2), but it's
a little scary.
This is also visible to external consumers because some callbacks get
called multiple times. We especially should try to avoid that as there
is no guarantee that these callbacks are idempotent and give the same
answer each time.
Change-Id: I212b2325eae2cfca0fb423dace101e466c5e5d4e
Reviewed-on: https://boringssl-review.googlesource.com/10224
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It's odd that a function like ssl_bytes_to_cipher_list secretly has side
effects all over the place. This removes the need for the TLS 1.3 code
to re-query the version range, and it removes the requirement that the
RI extension be first.
Change-Id: Ic9af549db3aaa8880f3c591b8a13ba9ae91d6a46
Reviewed-on: https://boringssl-review.googlesource.com/10220
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Between TLS 1.2, TLS 1.3, and the early callback, we've got a lot of
ClientHello parsers. Unify everything on the early callback's parser. As
a side effect, this means we can parse a ClientHello fairly succinctly
from any function which will let us split up ClientHello states where
appropriate.
Change-Id: I2359b75f80926cc7d827570cf33f93029b39e525
Reviewed-on: https://boringssl-review.googlesource.com/10184
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Add the following cases:
- Maximal padding
- Maximal padding with each possible byte position wrong.
- When the input is not publicly too short to find a MAC, but the
unpadded value is too short. (This tests that
EVP_tls_cbc_remove_padding and EVP_tls_cbc_copy_mac coordinate
correctly. EVP_tls_cbc_remove_padding promises to also consider it
invalid padding if there is no room for a MAC.)
Change-Id: I8fe18121afb915e579a8236d0e3ef354f1f835bc
Reviewed-on: https://boringssl-review.googlesource.com/10182
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is more progress in letting other stacks use the test runner.
You can provide a per-shim configuration file that includes:
- A list of test patterns to be suppressed (presumably because
they don't work). This setting is ignored if -test is used.
- A translation table of expected errors to shim-specific errors.
BUG=92
Change-Id: I3c31d136e35c282e05d4919e18ba41d44ea9cf2a
Reviewed-on: https://boringssl-review.googlesource.com/9161
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The old implementation had a lot of size_t/int confusion. It also
accepted non-minimally-encoded OIDs. Unlike the old implementation, the
new one does not fall back to BIGNUMs and does not attempt to
pretty-print OIDs with components which do not fit in a uint64_t. Add
tests for these cases.
With this new implementation, hopefully we'll have a much easier time
enabling MSVC's size_t truncation warning later.
Change-Id: I602102b97cf9b02d874644f8ef67fe9bac70e45e
Reviewed-on: https://boringssl-review.googlesource.com/9131
Reviewed-by: Adam Langley <agl@google.com>
BN_mod_inverse_odd was always being used on 64-bit platforms and was being used
for all curves with an order of 450 bits or smaller (basically, everything but
P-521). We generally don't care much about minor differences in the speed of
verifying signatures using curves other than P-256 and P-384. It is better to
always use the same algorithm.
This also allows |bn_mod_inverse_general|, |bn_mod_inverse_no_branch|, and
|BN_mod_inverse| to be dropped from programs that can somehow avoid linking in
the RSA key generation and RSA CRT recovery code.
Change-Id: I79b94bff23d2b07d5e0c704f7d44538797f8c7a0
Reviewed-on: https://boringssl-review.googlesource.com/9103
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The main RSA public modulus size of concern is 2048 bits.
bn_mod_inverse_odd is already used for public moduli of 2048 bits and
smaller on 64-bit platforms, so for 64-bit it is a no-op. For 32-bit
x86, this seems to slightly decrease the speed of RSA signing, but not
by a lot, and plus we don't care about RSA signing performance much on
32-bit platforms. It's better to have all platforms using the same
algorithms.
Change-Id: I869dbfc98994e36a04a535c1fe63b14a902a4f13
Reviewed-on: https://boringssl-review.googlesource.com/9102
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for factoring out the binary Euclidean
implementation (the one used for odd numbers that aren't too big) for
direct use from outside of crypto/bn/gcd.c. The goal is to make the
resultant |BN_mod_inverse_odd|'s signature similar to
|BN_mod_inverse_blinded|. Thus, the logic for reducing the final result
isn't factored out because that yet-to-be-created |BN_mod_inverse_odd|
will need to do it itself.
Change-Id: Iaecb79fb17d13c774c4fb6ade8742937780b0006
Reviewed-on: https://boringssl-review.googlesource.com/9100
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
OBJ_obj2txt's implementation is kind of scary. Also it casts between int
and size_t a lot. In preparation for rewriting it, add a test.
Change-Id: Iefb1d0cddff58d67e5b04ec332477aab8aa687b6
Reviewed-on: https://boringssl-review.googlesource.com/9130
Reviewed-by: Adam Langley <agl@google.com>
We're starting to get quite a lot of these ALL-CAPS.md documents.
There's been enough questions around how to properly use types like
EVP_MD_CTX that we probably should write down some of these common
rules.
Change-Id: I125f4e82efb168a071b54ff76c5af34c42ff4800
Reviewed-on: https://boringssl-review.googlesource.com/9115
Reviewed-by: Adam Langley <agl@google.com>
tls13_process_certificate can take a boolean for whether anonymous is
allowed. This does change the error on the client slightly, but I think
this is correct anyway. It is not a syntax error for the server to send
no certificates in so far as the Certificate message allows it. It's
just illegal.
Change-Id: I1af80dacf23f50aad0b1fbd884bc068a40714399
Reviewed-on: https://boringssl-review.googlesource.com/9072
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
While the sanity check isn't insane (one should arrange for sessions to
be invalidated once client auth settings change, and a sid_ctx is one
way to do it), this check lives in a poor place to enforce configuration
mistakes. To be effective, it needs to happen at the start of the
handshake, independent of the ClientHello from the peer.
But the benefit this check gives is low compared to the trouble it will
be to continually maintain this difference from OpenSSL (our own
ssl_test and bssl_shim forget to set a dummy sid_ctx). Instead, remove
it so we don't have to duplicate it across TLS 1.2 and TLS 1.3. Also so
we don't have weird failures which only manifest once a resuming client
connects.
Change-Id: Ia7f88711701afde5e26b7782c2264ce78dccc89b
Reviewed-on: https://boringssl-review.googlesource.com/9112
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We broke this to varying degrees ages ago.
This is the logic to implement the variations of rules in TLS to discard
sessions after a failed connection, where a failed connection could be
one of:
- A connection that was not cleanly shut down.
- A connection that received a fatal alert.
The first one is nonsense since close_notify does not actually work in
the real world. The second is a vaguely more plausible but...
- A stateless ticket-based server can't drop sessions anyway.
- In TLS 1.3, a client may receive many tickets over the lifetime of a
single connection. With an external session cache like ours which may,
in theory, but multithreaded, this will be a huge hassle to track.
- A client may well attempt to establish a connection and reuse the
session before we receive the fatal alert, so any application state we
hope to manage won't really work.
- An attacker can always close the connection before the fatal alert, so
whatever security policy clearing the session gave is easily
bypassable.
Implementation-wise, this has basically never worked. The
ssl_clear_bad_session logic called into SSL_CTX_remove_session which
relied on the internal session cache. (Sessions not in the internal
session cache don't get removed.) The internal session cache was only
useful for a server, where tickets prevent this mechanism from doing
anything. For a client, we since removed the internal session cache, so
nothing got removed. The API for a client also did not work as it gave
the SSL_SESSION, not the SSL, so a consumer would not know the key to
invalidate anyway.
The recent session state splitting change further broke this.
Moreover, calling into SSL_CTX_remove_session logic like that is
extremely dubious because it mutates the not_resumable flag on the
SSL_SESSION which isn't thread-safe.
Spec-wise, TLS 1.3 has downgraded the MUST to a SHOULD.
Given all that mess, just remove this code. It is no longer necessary to
call SSL_shutdown just to make session caching work.
Change-Id: Ib601937bfc5f6b40436941e1c86566906bb3165d
Reviewed-on: https://boringssl-review.googlesource.com/9091
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We will now send tickets as a server and accept them as a
client. Correctly offering and resuming them in the handshake will be
implemented in a follow-up.
Now that we're actually processing draft 14 tickets, bump the draft
version.
Change-Id: I304320a29c4ffe564fa9c00642a4ace96ff8d871
Reviewed-on: https://boringssl-review.googlesource.com/8982
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
|BN_mod_exp_mont| uses |BN_nnmod| so it seems like
|BN_mod_exp_mont_consttime| should too. Further, I created
these test vectors by doing the math by hand, and the tests
passed for |BN_mod_exp_mont| but failed for
|BN_mod_exp_mont_consttime| without this change.
Change-Id: I7cffa1375e94dd8eaee87ada78285cd67fff1bac
Reviewed-on: https://boringssl-review.googlesource.com/9032
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
None of the SSL_VERIFY_FAIL_IF_NO_PEER_CERT codepaths will ever be
reached if SSL_VERIFY_PEER is unset. If we've gotten as far as getting a
Certificate message, consider SSL_VERIFY_FAIL_IF_NO_PEER_CERT alone
significant grounds for rejecting no peer certificate.
Change-Id: I2c6be4269d65b2467b86b1fc7d76ac47ca735553
Reviewed-on: https://boringssl-review.googlesource.com/9070
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The functions appear to try to handle negative inputs, but it isn't
clear how negative inputs are supposed to work and/or if these
functions work the way they are supposed to given negative inputs.
There seems to be no legitimate reason to pass these functions negative
inputs, so just document that negative inputs shouldn't be used. More
specifically, document that the inputs should be in the range [0, n)
where |n| is the Montgomery modulus.
Change-Id: Id8732fb89616f10e673704e6fa09d78926c402d8
Reviewed-on: https://boringssl-review.googlesource.com/9033
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
SSL_set_bio is a nightmare.
In f715c42322, we noticed that, among
other problems, SSL_set_bio's actual behavior did not match how
SSL_set_rfd was calling it due to an asymmetry in the rbio/wbio
handling. This resulted in SSL_set_fd/SSL_set_rfd calls to crash. We
decided that SSL_set_rfd's believed semantics were definitive and
changed SSL_set_bio.
Upstream, in 65e2d672548e7c4bcb28f1c5c835362830b1745b, decided that
SSL_set_bio's behavior, asymmetry and all, was definitive and that the
SSL_set_rfd crash was a bug in SSL_set_rfd. Accordingly, they switched
the fd callers to use the side-specific setters, new in 1.1.0.
Align with upstream's behavior and add tests for all of SSL_set_bio's
insanity. Also export the new side-specific setters in anticipation of
wanting to be mostly compatible with OpenSSL 1.1.0.
Change-Id: Iceac9508711f79750a3cc2ded081b2bb2cbf54d8
Reviewed-on: https://boringssl-review.googlesource.com/9064
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
If two CRLs are equivalent then use the one with a later lastUpdate field:
this will result in the newest CRL available being used.
(Imported from upstream's 325da8231c8d441e6bb7f15d1a5a23ff63c842e5 and
3dc160e9be6dcaeec9345fbb61b1c427d7026103.)
Change-Id: I8c722663b979dfe08728d091697d8b8204dc265c
Reviewed-on: https://boringssl-review.googlesource.com/8947
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Negative zeros are nuts, but it will probably be a while before we've
fixed everything that can create them. Fix both to consistently print
'-0' rather than '0' so failures are easier to diagnose (BN_cmp believes
the values are different.)
Change-Id: Ic38d90601b43f66219d8f44ca085432106cf98e3
Reviewed-on: https://boringssl-review.googlesource.com/9073
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Simplify the calculation of the Montgomery constants in
|BN_MONT_CTX_set|, making the inversion constant-time. It should also
be faster by avoiding any use of the |BIGNUM| API in favor of using
only 64-bit arithmetic.
Now it's obvious how it works. /s
Change-Id: I59a1e1c3631f426fbeabd0c752e0de44bcb5fd75
Reviewed-on: https://boringssl-review.googlesource.com/9031
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
A caller using EVP_Digest* which a priori knows tighter bounds on the
hash function used (perhaps because it is always a particular hash) can
assume the function will not write more bytes than the size of the hash.
The letter of the rules before vaguely[*] allowed for more than
EVP_MD_MAX_SIZE bytes written which made for some unreasonable code in
Chromium. Officially clarify this and add tests which, when paired with
valgrind and ASan prove it.
BUG=59
[*] Not really. I think it already promised the output length will be
both the number of bytes written and the size of the hash and the size
of the hash is given by what the function promises to compute. Meh.
Change-Id: I736d526e81cca30475c90897bca896293ff30278
Reviewed-on: https://boringssl-review.googlesource.com/9066
Reviewed-by: Eric Roman <ericroman@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
In TLS 1.2, this was allowed to be empty for the weird SHA-1 fallback
logic. In TLS 1.3, not only is the fallback logic gone, but omitting
them is a syntactic error.
struct {
opaque certificate_request_context<0..2^8-1>;
SignatureScheme
supported_signature_algorithms<2..2^16-2>;
DistinguishedName certificate_authorities<0..2^16-1>;
CertificateExtension certificate_extensions<0..2^16-1>;
} CertificateRequest;
Thanks to Eric Rescorla for pointing this out.
Change-Id: I4991e59bc4647bb665aaf920ed4836191cea3a5a
Reviewed-on: https://boringssl-review.googlesource.com/9062
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We were sending decode_error, but the spec explicitly says (RFC 5246):
unsupported_extension
sent by clients that receive an extended server hello containing
an extension that they did not put in the corresponding client
hello. This message is always fatal.
Also add a test for this when it's a known but unoffered extension. We
actually end up putting these in different codepaths now due to the
custom extensions stuff.
Thanks to Eric Rescorla for pointing this out.
Change-Id: If6c8033d4cfe69ef8af5678b873b25e0dbadfc4f
Reviewed-on: https://boringssl-review.googlesource.com/9061
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
As of a recent change, test_support always included the headers, which
causes Android's new build-system to be unhappy. It doesn't want to
include headers. Split them into test_support_headers and test_support
to match the other keys.
Then fix up references:
- Android's new build system only wants the sources. Fix this.
- Chromium's GN and GYP theoretically want the sources and headers, but
we've never supplied the headers because this isn't enforced at all.
Fix this. Headers are selected based on what target the header
"belongs to".
- Bazel has no change except to sort test_support_sources.
Change-Id: I85809e70a71236b5e91d87f87bb73bc2ea289251
Reviewed-on: https://boringssl-review.googlesource.com/9044
Reviewed-by: Adam Langley <agl@google.com>
To prevent configuration/established session confusion, the handshake
session state is separated into the configured session (ssl->session)
and the newly created session (ssl->s3->new_session). Upon conclusion of
the handshake, the finalized session is stored
in (ssl->s3->established_session). During the handshake, any requests
for the session (SSL_get_session) return a non-resumable session, to
prevent resumption of a partially filled session. Sessions should only
be cached upon the completion of the full handshake, using the resulting
established_session. The semantics of accessors on the session are
maintained mid-renego.
Change-Id: I4358aecb71fce4fe14a6746c5af1416a69935078
Reviewed-on: https://boringssl-review.googlesource.com/8612
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This finishes getting rid of ssl_read_bytes! Now we have separate
entry-points for the various cases. For now, I've kept TLS handshake
consuming records partially. When we do the BIO-less API, I expect that
will need to change, since we won't have the record buffer available.
(Instead, the ssl3_read_handshake_bytes and extend_handshake_buffer pair
will look more like the DTLS side or Go and pull the entire record into
init_buf.)
This change opts to make read_app_data drive the message to completion
in anticipation of DTLS 1.3. That hasn't been specified, but
NewSessionTicket certainly will exist. Knowing that DTLS necessarily has
interleave seems something better suited for the SSL_PROTOCOL_METHOD
internals to drive.
It needs refining, but SSL_PROTOCOL_METHOD is now actually a half-decent
abstraction boundary between the higher-level protocol logic and
DTLS/TLS-specific record-layer and message dispatchy bits.
BUG=83
Change-Id: I9b4626bb8a29d9cb30174d9e6912bb420ed45aff
Reviewed-on: https://boringssl-review.googlesource.com/9001
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Yo dawg I herd you like blinding so I put inversion blinding in your
RSA blinding so you can randomly mask your random mask.
This improves upon the current situation where we pretend that
|BN_mod_inverse_no_branch| is constant-time, and it avoids the need to
exert a lot of effort to make a actually-constant-time modular
inversion function just for RSA blinding.
Note that if the random number generator weren't working correctly then
the blinding of the inversion wouldn't be very effective, but in that
case the RSA blinding itself would probably be completely busted, so
we're not really losing anything by relying on blinding to blind the
blinding.
Change-Id: I771100f0ad8ed3c24e80dd859ec22463ef2a194f
Reviewed-on: https://boringssl-review.googlesource.com/8923
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We already forbid renego/app-data interleave. Forbid it within a
HelloRequest too because that's nonsense. No one would ever send:
[hs:HelloReq-] [app:Hello world] [hs:-uest]
Add tests for this case.
This is in preparation for our more complex TLS 1.3 post-handshake logic
which is going to go through the usual handshake reassembly logic and,
for sanity, will want to enforce this anyway.
BUG=83
Change-Id: I80eb9f3333da3d751f98f25d9469860d1993a97a
Reviewed-on: https://boringssl-review.googlesource.com/9000
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Per request from EKR. Also we have a lot of long test names, so this
seems generally a good idea.
Change-Id: Ie463f5367ec7d33005137534836005b571c8f424
Reviewed-on: https://boringssl-review.googlesource.com/9021
Reviewed-by: Adam Langley <agl@google.com>
With the previous DTLS change, the dispatch layer only cares about the
end of the handshake to know when to drop the current message. TLS 1.3
post-handshake messages will need a similar hook, so convert it to this
lower-level one.
BUG=83
Change-Id: I4c8c3ba55ba793afa065bf261a7bccac8816c348
Reviewed-on: https://boringssl-review.googlesource.com/8989
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for switching finish_handshake to a
release_current_message hook. finish_handshake in DTLS is also
responsible for releasing any memory associated with extra messages in
the handshake.
Except that's not right and we need to make it an error anyway. Given
that the rest of the DTLS dispatch layer already strongly assumes there
is only one message in epoch one, putting the check in the fragment
processing works fine enough. Add tests for this.
This will certainly need revising when DTLS 1.3 happens (perhaps just a
version check, perhaps bringing finish_handshake back as a function that
can fail... which means we need a state just before SSL_ST_OK), but DTLS
1.3 post-handshake messages haven't really been written down, so let's
do the easy thing for now and add a test for when it gets more
interesting.
This removes the sequence number reset in the DTLS code. That reset
never did anything becase we don't and never will renego. We should make
sure DTLS 1.3 does not bring the reset back for post-handshake stuff.
(It was wrong in 1.2 too. Penultimate-flight retransmits and renego
requests are ambiguous in DTLS.)
BUG=83
Change-Id: I33d645a8550f73e74606030b9815fdac0c9fb682
Reviewed-on: https://boringssl-review.googlesource.com/8988
Reviewed-by: Adam Langley <agl@google.com>
This was only used so we knew when we had a current message to discard
and when we didn't. With init_msg being tracked better, we can use that
instead.
As part of this, switch the V2ClientHello hack to not using
reuse_message. Otherwise we have to fill in init_msg and friends in two
places.
The next change will require that we have a better handle on the "is
there a current message" boolean.
BUG=83
Change-Id: I917efacbad10806d492bbe51eda74c0779084d60
Reviewed-on: https://boringssl-review.googlesource.com/8987
Reviewed-by: Adam Langley <agl@google.com>
For TLS 1.3, we will need to process more complex post-handshake
messages. It is simplest if we use the same mechanism. In preparation,
allow ssl3_get_message to be called at any point.
Note that this stops reserving SSL3_RT_MAX_PLAIN_LENGTH in init_buf
right off the bat. Instead it will grow as-needed to accomodate the
handshake. SSL3_RT_MAX_PLAIN_LENGTH is rather larger than we probably
need to receive, particularly as a server, so this seems a good plan.
BUG=83
Change-Id: Id7f4024afc4c8a713b46b0d1625432315594350e
Reviewed-on: https://boringssl-review.googlesource.com/8985
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Right now they're RSA PRIVATE KEY or EC PRIVATE KEY which requires a bit
more effort to parse. It means the PEM header is necessary to parse
these. OpenSSL and Go automagically convert the format, but other shims
(namely NSS) may not.
Change-Id: I9fa2767dcf1fe6ceeea546390759e1c364a8f16f
Reviewed-on: https://boringssl-review.googlesource.com/9020
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Fermat's Little Theorem is already used for the custom curve implementations.
Use it, for the same reasons, for the ec_montgomery-based implementations.
I tested the performance (only) on x86-64 Windows.
Change-Id: Ibf770fd3f2d3e2cfe69f06bc12c81171624ff557
Reviewed-on: https://boringssl-review.googlesource.com/8924
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This change allows the shim to return a magic error code (89) to
indicate that it doesn't implement some of the given flags for a test.
Unimplemented tests are, by default, an error. The --allow-unimplemented
flag to the test runner causes them to be ignored.
This is done in preparation for non-BoringSSL shims.
Change-Id: Iecfd545b9cf44df5e25b719bfd06275c8149311a
Reviewed-on: https://boringssl-review.googlesource.com/8970
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Besides reducing code duplication, also move the relative location of
the check of |count|. Previously, the code was generating a random
value and then terminating the loop without using it if |count| went
to zero. Now the wasted call to |BN_rand| is not made.
Also add a note about the applicability of the special case logic for
|range| of the form |0b100...| to RSA blinding.
Change-Id: Iaa33b9529f1665ac59aefcc8b371fa32445e7578
Reviewed-on: https://boringssl-review.googlesource.com/8960
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
tag2nbyte had -1 at 18th position, but underlying ASN1_mbstring_copy
supports NumericString. tag2nbyte is also used in do_print_ex which will
not be broken by setting 1 at 18th position of tag2nbyte
(Imported from upstream's bd598cc405e981de259a07558e600b5a9ef64bd6.)
Change-Id: Ie063afcaac8a7d5046cdb385059b991b92cd6659
Reviewed-on: https://boringssl-review.googlesource.com/8946
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The selector field could be omitted because it has a DEFAULT value.
In this case *sfld == NULL (sfld can never be NULL). This was not
noticed because this was never used in existing ASN.1 modules.
(Imported from upstream's c4210673313482edacede58d92e92c213d7a181a.)
svaldez and I stared at this for a while and we believe this change is
correct. It's also irrelevant because our only remaining ADB (ANY
DEFINED BY) table is POLICYQUALINFO which does not allow its selector to
be omitted. Also, if it did, it would be a slight change in behavior.
We'd switch from using POLICYQUALINFO's default_tt (filling in an
ASN1_ANY) to its null_tt (which doesn't exist, so error).
Change-Id: If6a929e3dafca18431775b01958d0dae1c09f3b4
Reviewed-on: https://boringssl-review.googlesource.com/8943
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This imports upstream's b62e9bf5cbbe278b7e0017c9234999dae68ee867 and
c3bc7f498815b355533d96b54b9a09e030d4130c. This is a no-op since we don't
use the XTS bits though keep the files in sync so long as we have them.
Comparing to master, we're now up-to-date on that file except for
a285992763f3961f69a8d86bf7dfff020a08cef9. (I've left that alone since
that touches lots of files and we should probably get better test
configuration before importing something scary like #undef __thumb2__.)
Change-Id: Ie0556757c954ef559e03a6d62c940d5901ca704a
Reviewed-on: https://boringssl-review.googlesource.com/8945
Reviewed-by: Adam Langley <agl@google.com>
This can be reverted, once the test runner on Android devices is
fixed.
BUG=chromium:631462
Change-Id: I1f9e38b4104f18c15bb942207c87e3b4b27472d2
Reviewed-on: https://boringssl-review.googlesource.com/8931
Reviewed-by: Andrii Shyshkalov <tandrii@google.com>
Ridiculous as it is, the protocol does not forbid packing HelloRequest
and Finished into the same record. Add a test for this case.
Change-Id: I8e1455b261f56169309070bf44d14d40a63eae50
Reviewed-on: https://boringssl-review.googlesource.com/8901
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This doesn't do anything since they're for DTLS, but we ought to satisfy
the API nonetheless. expect_flight is easy with
ssl_hs_flush_and_read_message. received_flight I think basically needs
to get sprinkled into the state machine.
Change-Id: I406c7f776ad8e5e3cbcafcac6b26a688c6d3caf1
Reviewed-on: https://boringssl-review.googlesource.com/8883
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Alas, we will need a version fallback for TLS 1.3 again.
This deprecates SSL_MODE_SEND_FALLBACK_SCSV. Rather than supplying a
boolean, have BoringSSL be aware of the real maximum version so we can
change the TLS 1.3 anti-downgrade logic to kick in, even when
max_version is set to 1.2.
The fallback version replaces the maximum version when it is set for
almost all purposes, except for downgrade protection purposes.
BUG=chromium:630165
Change-Id: I4c841dcbc6e55a282b223dfe169ac89c83c8a01f
Reviewed-on: https://boringssl-review.googlesource.com/8882
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
BN_mod_mul_montgomery has a problem where the modulus is much smaller
than one of the arguments. While bn_test.cc knows this and reduces the
inputs before testing |BN_mod_mul_montgomery|, none of the previous test
vectors actually failed without this. (Except those that passed negative
vaules.)
This change adds tests where M ≪ A and B.
Change-Id: I53b5188ea5fb5e48d0d197718ed33c644cde8477
Reviewed-on: https://boringssl-review.googlesource.com/8890
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Brian Smith <brian@briansmith.org>
Commit-Queue: David Benjamin <davidben@google.com>
The Android build system uses "blueprint" files now which are
represented by "Android.bp" instead of "Android.mk" files.
Some of the old sources.mk entries still exist, since they're still
being used by the Trusty build system.
Change-Id: I0b04100ace8599c8734bee77f656aab04c06cce9
Reviewed-on: https://boringssl-review.googlesource.com/8891
Reviewed-by: David Benjamin <davidben@google.com>
It seems risky in the context of cross-signed certificates when the
same certificate might have multiple potential issuers. Also rarely
used, since chains in OpenSSL typically only employ self-signed
trust-anchors, whose self-signatures are not checked, while untrusted
certificates are generally ephemeral.
(Imported from upstream's 0e76014e584ba78ef1d6ecb4572391ef61c4fb51.)
This is in master and not 1.0.2, but having a per-certificate signature
cache when this is a function of signature and issuer seems dubious at
best. Thanks to Viktor Dukhovni for pointing this change out to me.
(And for making the original change upstream, of course.)
Change-Id: Ie692d651726f14aeba6eaab03ac918fcaedb4eeb
Reviewed-on: https://boringssl-review.googlesource.com/8880
Reviewed-by: Adam Langley <agl@google.com>
Revert 3f3358ac15. Add documentation
clarifying the misunderstanding that lead to the mistake, and make use
of the recently-added |bn_set_words|.
Change-Id: I58814bace3db3b0b44e2dfe09c44918a4710c621
Reviewed-on: https://boringssl-review.googlesource.com/8831
Reviewed-by: Adam Langley <agl@google.com>
Our CBB patterns do not make it safe to use a CBB after any operation
failed. Suppose one does:
int add_to_cbb(CBB *cbb) {
CBB child;
return CBB_add_u8(cbb, 1) &&
CBB_add_u8_length_prefixed(cbb, &child) &&
CBB_add_u8(&child, 2) &&
/* Flush |cbb| before |child| goes out of scoped. */
CBB_flush(cbb);
}
If one of the earlier operations fails, any attempt to use |cbb| (except
CBB_cleanup) would hit a memory error. Doing this would be a bug anyway,
since the CBB would be in an undefined state anyway (wrote only half my
object), but the memory error is bad manners.
Officially document that using a CBB after failure is illegal and, to
avoid the memory error, set a poison bit on the cbb_buffer_st to prevent
all future operations. In theory we could make failure +
CBB_discard_child work, but this is not very useful and would require a
more complex CBB pattern.
Change-Id: I4303ee1c326785849ce12b5f7aa8bbde6b95d2ec
Reviewed-on: https://boringssl-review.googlesource.com/8840
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This adds three more formats to the SSLKEYLOGFILE format to support TLS
1.3:
EARLY_TRAFFIC_SECRET <client_random> <early_traffic_secret>
HANDSHAKE_TRAFFIC_SECRET <client_random> <handshake_traffic_secret>
TRAFFIC_SECRET_0 <client_random> <traffic_secret_0>
(We don't implement 0-RTT yet, so only the second two are implemented.)
Motivations:
1. If emitted the non-traffic secrets (early, handshake, and master) or
the IKMs, Wireshark needs to maintain a handshake hash. I don't
believe they need to do this today.
2. We don't store more than one non-traffic secret at a time and don't
keep traffic secrets for longer than needed. That suggests three
separate lines logged at different times rather than one line.
3. If 0-RTT isn't used, we probably won't even compute the early traffic
secret, so that further suggests three different lines.
4. If the handshake didn't get far enough to complete, we won't have an
TRAFFIC_SECRET_0 to log at all. That seems like exactly when
Wireshark would be handy, which means we want to log secrets as they
are computed.
MT from NSS has ACK'd over email that this format would be acceptable
for them, so let's go with it.
Change-Id: I4d685a1355dff4d4bd200310029d502bb6c511f9
Reviewed-on: https://boringssl-review.googlesource.com/8841
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Every flush but the last is always immediately followed by a read. Add a
combined wait mode to make things simpler. Unfortunately, both flights
we have (the state machine doesn't write the first ClientHello) are
followed immediately by a state change, which means we still need some
state in between because we must run code after write_message but before
read_message.
(This way to fix that is to get rid of the buffer BIO, change
write_message to write_flight, and allow things like init_message /
finish_message / init_message / finish_message / set_write_state /
init_message / finish_message / write_flight.)
Change-Id: Iebaa388ccbe7fcad48c1b2256e1c0d3a7c9c8a2a
Reviewed-on: https://boringssl-review.googlesource.com/8828
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is the equivalent of FragmentAcrossChangeCipherSuite for DTLS. It
is possible for us to, while receiving pre-CCS handshake messages, to
buffer up a message with sequence number meant for a post-CCS Finished.
When we then get to the new epoch and attempt to read the Finished, we
will process the buffered Finished although it was sent with the wrong
encryption.
Move ssl_set_{read,write}_state to SSL_PROTOCOL_METHOD hooks as this is
a property of the transport. Notably, read_state may fail. In DTLS
check the handshake buffer size. We could place this check in
read_change_cipher_spec, but TLS 1.3 has no ChangeCipherSpec message, so
we will need to implement this at the cipher change point anyway. (For
now, there is only an assert on the TLS side. This will be replaced with
a proper check in TLS 1.3.)
Change-Id: Ia52b0b81e7db53e9ed2d4f6d334a1cce13e93297
Reviewed-on: https://boringssl-review.googlesource.com/8790
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This allows us to implement custom RSA-PSS-based keys, so the async TLS
1.3 tests can proceed. For now, both sign and sign_digest exist, so
downstreams only need to manage a small change atomically. We'll remove
sign_digest separately.
In doing so, fold all the *_complete hooks into a single complete hook
as no one who implemented two operations ever used different function
pointers for them.
While I'm here, I've bumped BORINGSSL_API_VERSION. I do not believe we
have any SSL_PRIVATE_KEY_METHOD versions who cannot update atomically,
but save a round-trip in case we do. It's free.
Change-Id: I7f031aabfb3343805deee429b9e244aed5d76aed
Reviewed-on: https://boringssl-review.googlesource.com/8786
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This makes custom private keys and EVP_PKEYs symmetric again. There is
no longer a requirement that the caller pre-filter the configured
signing prefs.
Also switch EVP_PKEY_RSA to NID_rsaEncryption. These are identical, but
if some key types are to be NIDs, we should make them all NIDs.
Change-Id: I82ea41c27a3c57f4c4401ffe1ccad406783e4c64
Reviewed-on: https://boringssl-review.googlesource.com/8785
Reviewed-by: David Benjamin <davidben@google.com>
EVP_PKT_SIGN is redundant with the RSA/EC check which, in turn, is
redundant with sigalgs processing. The type need only be checked in the
pre-1.2 case which was indeed missing an else.
The client half was likewise missing an else, though it's unreachable
due to leaf cert checks.
Change-Id: Ib3550f71a2120b38eacdd671d4f1700876bcc485
Reviewed-on: https://boringssl-review.googlesource.com/8779
Reviewed-by: David Benjamin <davidben@google.com>
This is already duplicated between client and server and otherwise will
get duplicated yet again for TLS 1.3.
Change-Id: Ia8a352f9bc76fab0f88c1629d08a1da4c13d2510
Reviewed-on: https://boringssl-review.googlesource.com/8778
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.3 will go through very different code than everything else. Even
SSL 3.0 is somewhat special-cased now. Move the invalid signature tests
there and run at all versions.
Change-Id: Idd0ee9aac2939c0c8fd9af2ea7b4a22942121c60
Reviewed-on: https://boringssl-review.googlesource.com/8775
Reviewed-by: David Benjamin <davidben@google.com>
The TLS 1.3 CertificateRequest code advertised the signing set, not the
verify set. It also wasn't saving the peer's signature algorithm.
Change-Id: I62247d5703e30d8463c92f3d597dbeb403b355ae
Reviewed-on: https://boringssl-review.googlesource.com/8774
Reviewed-by: David Benjamin <davidben@google.com>
ServerKeyExchange and SigningHash are both very 1.2-specific names.
Replace with names that fit both 1.2 and 1.3 (and are a bit shorter).
Also fix a reference to ServerKeyExchange in sign.go.
Change-Id: I25d4ff135cc77cc545f0f9e94014244d56a9e96b
Reviewed-on: https://boringssl-review.googlesource.com/8773
Reviewed-by: David Benjamin <davidben@google.com>
deriveTrafficAEAD gets confused by the EnableAllCiphers bug. As a hack,
just return the nil cipher. We only need to progress far enough to read
the shim's error code.
Change-Id: I72d25ac463a03a0e99dd08c38a1a7daef1f94311
Reviewed-on: https://boringssl-review.googlesource.com/8763
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
RSASSA-PSS with SHA-512 is slightly too large for 1024-bit RSA. One
should not be using 1024-bit RSA, but it's common enough for tests
(including our own in runner before they were regenerated), that we
should probably do the size check and avoid unnecessary turbulence to
everyone else's test setups.
Change-Id: If0c7e401d7d05404755cba4cbff76de3bc65c138
Reviewed-on: https://boringssl-review.googlesource.com/8746
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Tested by having client and server talk to each other. This adds the
certificate_extensions field to CertificateRequest which I'd previously
missed. (We completely ignore the field, with the expectation that the C
code won't have anything useful to do with it either.)
Change-Id: I74f96acd36747d4b6a6f533535e36ea8e94d2be8
Reviewed-on: https://boringssl-review.googlesource.com/8710
Reviewed-by: David Benjamin <davidben@google.com>
For now, skip the 1.2 -> 1.1 signal since that will affect shipping
code. We may as well enable it too, but wait until things have settled
down. This implements the version in draft-14 since draft-13's isn't
backwards-compatible.
Change-Id: I46be43e6f4c5203eb4ae006d1c6a2fe7d7a949ec
Reviewed-on: https://boringssl-review.googlesource.com/8724
Reviewed-by: David Benjamin <davidben@google.com>
Now that the odd client/server split (a remnant from the original
crypto/tls code not handling signing-hash/PRF mismatches) is gone, it
can just be pulled from the config.
Change-Id: Idb46c026d6529a2afc2b43d4afedc0aa950614db
Reviewed-on: https://boringssl-review.googlesource.com/8723
Reviewed-by: David Benjamin <davidben@google.com>
This way we can test failing client auth without having to worry about
first getting through server auth.
Change-Id: Iaf996d87ac3df702a17e76c26006ca9b2a5bdd1f
Reviewed-on: https://boringssl-review.googlesource.com/8721
Reviewed-by: David Benjamin <davidben@google.com>
[Rebased and tests added by davidben.]
In doing so, regenerate the test RSA certificate to be 2048-bit RSA.
RSA-PSS with SHA-512 is actually too large for 1024-bit RSA. Also make
the sigalg test loop test versions that do and don't work which subsumes
the ecdsa_sha1 TLS 1.3 test.
For now, RSA-PKCS1 is still allowed because NSS has yet to implement
RSA-PSS and we'd like to avoid complicated interop testing.
Change-Id: I686b003ef7042ff757bdaab8d5838b7a4d6edd87
Reviewed-on: https://boringssl-review.googlesource.com/8613
Reviewed-by: David Benjamin <davidben@google.com>
Implement in both C and Go. To test this, route config into all the
sign.go functions so we can expose bugs to skip the check.
Unfortunately, custom private keys are going to be a little weird since
we can't check their curve type. We may need to muse on what to do here.
Perhaps the key type bit should return an enum that includes the curve?
It's weird because, going forward, hopefully all new key types have
exactly one kind of signature so key type == sig alg == sig alg prefs.
Change-Id: I1f487ec143512ead931e3392e8be2a3172abe3d2
Reviewed-on: https://boringssl-review.googlesource.com/8701
Reviewed-by: David Benjamin <davidben@google.com>
ssl_verify_* already ought to be checking this, so there's only a need
to check against the configured preferences.
Change-Id: I79bc771969c57f953278e622084641e6e20108e3
Reviewed-on: https://boringssl-review.googlesource.com/8698
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.3 also forbids signing SHA-1 digests, but this will be done as a
consequence of forbidding PKCS#1 in 1.3 altogether (rsa_sign_sha1) and
requiring a curve match in ECDSA (ecdsa_sha1).
Change-Id: I665971139ccef9e270fd5796c5e6a814a8f663b1
Reviewed-on: https://boringssl-review.googlesource.com/8696
Reviewed-by: David Benjamin <davidben@google.com>
This is a lot more verbose and looks the same between RSA and ECDSA for
now, but it gives us room to implement the various algorithm-specific
checks. ECDSA algorithms must match the curve, PKCS#1 is forbidden in
TLS 1.3, etc.
Change-Id: I348cfae664d7b08195a2ab1190820b410e74c5e9
Reviewed-on: https://boringssl-review.googlesource.com/8694
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This allows us to specify client-only and unused callbacks without
needing to include empty wrappers, and allows us to continue using the
default ext_*_parse_clienthello function for early parsing.
Change-Id: I4104e22a0a6dd6b02f9a5605e9866f6b3de6a097
Reviewed-on: https://boringssl-review.googlesource.com/8743
Reviewed-by: David Benjamin <davidben@google.com>
Upstream have added |EVP_PKEY_up_ref|, but their version returns an int.
Having this function with a different signature like that is dangerous
so this change aligns BoringSSL with upstream. Users of this function in
Chromium and internally should already have been updated.
Change-Id: I0a7aeaf1a1ca3b0f0c635e2ee3826aa100b18157
Reviewed-on: https://boringssl-review.googlesource.com/8736
Reviewed-by: David Benjamin <davidben@google.com>
Rather than blindly select SHA-1 if we can't find a matching one, act as
if the peer advertised rsa_pkcs1_sha1 and ecdsa_sha1. This means that we
will fail the handshake if no common algorithm may be found.
This is done in preparation for removing the SHA-1 default in TLS 1.3.
Change-Id: I3584947909d3d6988b940f9404044cace265b20d
Reviewed-on: https://boringssl-review.googlesource.com/8695
Reviewed-by: David Benjamin <davidben@google.com>
Instead, in SSL_set_private_key_digest_prefs, convert the NID list to a
sigalgs list. We'll need to add a new API later when custom key callers
are ready to start advertising RSA-PSS.
This removes all callers of tls12_get_hash except inside the signing and
verifying functions.
Change-Id: Ie534f3b736c6ac6ebeb0d7770d489f72e3321865
Reviewed-on: https://boringssl-review.googlesource.com/8693
Reviewed-by: David Benjamin <davidben@google.com>
Instead have ssl3_cert_verify_hash output the hash, since it already
knows it. Also add a missing EVP_PKEY_CTX_set_signature_md call on the
client half. (Although, the call isn't actually necessary.)
Also remove now unnecessary static assert. Since EVP_md5_sha1 is an
EVP_MD itself, EVP_MAX_MD_SIZE is required to fit it already.
Change-Id: Ief74fdbdf08e9f124679475bafba2f6f1d8fc687
Reviewed-on: https://boringssl-review.googlesource.com/8692
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Bazel sees BUILD files are markers for different packages. So by having
a file named “BUILD” in the source tree, Bazel thinks that there are
several packages involved.
This change renames it to BUILD.toplevel to avoid this.
Change-Id: Ia76167334cd52f72ff25ecb08533c30e5e423ab8
This reverts commits:
8d79ed674019fdcb52348d79ed6740
Because WebRTC (at least) includes our headers in an extern "C" block,
which precludes having any C++ in them.
Change-Id: Ia849f43795a40034cbd45b22ea680b51aab28b2d
MSVC doesn't define __cplusplus as 201103 to indicate C++11 support, so
just assume that the compiler supports C++11 if _MSC_VER is defined.
Change-Id: I27f6eeefe6e8dc522470f36fab76ab36d85eebac
Reviewed-on: https://boringssl-review.googlesource.com/8734
Reviewed-by: David Benjamin <davidben@google.com>
Last month's canary for loop did not die in the coal mine of decrepit
toolchains. Make a note of this in STYLE.md so we know to start breeding
more of them. We can indeed declare index variables like it's 1999.
I haven't bothered to convert all of our for loops because that will be
tedious, but we can do it as we touch the code. Or if someone feels
really really bored.
BUG=47
Change-Id: Ib76c0767c1b509e825eac66f8c2e3ee2134e2493
Reviewed-on: https://boringssl-review.googlesource.com/8740
Reviewed-by: Adam Langley <agl@google.com>
This change scatters the contents of the two scoped_types.h files into
the headers for each of the areas of the code. The types are now in the
|bssl| namespace.
Change-Id: I802b8de68fba4786b6a0ac1bacd11d81d5842423
Reviewed-on: https://boringssl-review.googlesource.com/8731
Reviewed-by: Adam Langley <agl@google.com>
We currently have the situation where the |tool| and |bssl_shim| code
includes scoped_types.h from crypto/test and ssl/test. That's weird and
shouldn't happen. Also, our C++ consumers might quite like to have
access to the scoped types.
Thus this change moves some of the template code to base.h and puts it
all in a |bssl| namespace to prepare for scattering these types into
their respective headers. In order that all the existing test code be
able to access these types, it's all moved into the same namespace.
Change-Id: I3207e29474dc5fcc344ace43119df26dae04eabb
Reviewed-on: https://boringssl-review.googlesource.com/8730
Reviewed-by: David Benjamin <davidben@google.com>
It still places the current message all over the place, but remove the
bizarre init_num/error/ok split. Now callers get the message length out
of init_num, which mirrors init_msg. Also fix some signedness.
Change-Id: Ic2e97b6b99e234926504ff217b8aedae85ba6596
Reviewed-on: https://boringssl-review.googlesource.com/8690
Reviewed-by: David Benjamin <davidben@google.com>
This machinery is so different between TLS and DTLS that there is no
sense in having them share structures. This switches us to maintaining
the full reassembled message in hm_fragment and get_message just lets
the caller read out of that when ready.
This removes the last direct handshake dependency on init_buf,
ssl3_hash_message.
Change-Id: I4eccfb6e6021116255daead5359a0aa3f4d5be7b
Reviewed-on: https://boringssl-review.googlesource.com/8667
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We usually put main at the end. There's now nothing interesting in the
function, so avoid having to declare every test at the top.
Change-Id: Iac469f41f0fb7d1f58d12dfbf651bf0d39f073d0
Reviewed-on: https://boringssl-review.googlesource.com/8712
Reviewed-by: David Benjamin <davidben@google.com>
../tool/transport_common.cc:429:14: error: ‘code_250’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
(I don't believe it can actually happen though.)
Change-Id: I78d19ad42ed4c05404f1d8d3e8f254ede3244b8d
This change adds support for doing an SMTP STARTTLS dance before a TLS
handshake when using the tool. This is useful for poking at SMTP
servers.
Change-Id: I04cd60d02d3377cce83e412d62e3257235a19116
Reviewed-on: https://boringssl-review.googlesource.com/8662
Reviewed-by: David Benjamin <davidben@google.com>
Since Bazel doesn't have a shared vs static concept that's exposed to
the build rules (as far as I know) symbol visibility would mean that the
symbols might be exposed when building a larger library. That could be
fixed with a linker script, but this change appears to be slightly more
useful for our consumers.
(Also, if we're going to set -fvisibility=hidden, we should also have set
the defines needed to include the visibility annotations.)
Change-Id: Ic7d64a553da48cfb9cf5460d26254de7e105fd65
Reviewed-on: https://boringssl-review.googlesource.com/8664
Reviewed-by: Adam Langley <agl@google.com>
Both DTLS and TLS still use it, but that will change in the following
commit. This also removes the handshake's knowledge of the
dtls_clear_incoming_messages function.
(It's possible we'll want to get rid of begin_handshake in favor of
allocating it lazily depending on how TLS 1.3 post-handshake messages
end up working out. But this should work for now.)
Change-Id: I0f512788bbc330ab2c947890939c73e0a1aca18b
Reviewed-on: https://boringssl-review.googlesource.com/8666
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
These are where the DTLS- and TLS-specific transport layer hooks will be
defined. Later we can probably move much of the implementations of these
hooks into these files so those functions can be static.
While I'm here, fix up the naming of some constants.
Change-Id: I1009dd9fdc3cc4fd49fbff0802f6289931abec3d
Reviewed-on: https://boringssl-review.googlesource.com/8665
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
[Originally written by nharper, revised by davidben.]
When we add this in the real code, this will want ample tests and hooks
for bugs, but get the core logic in to start with.
Change-Id: I86cf0b6416c9077dbb6471a1802ae984b8fa6c72
Reviewed-on: https://boringssl-review.googlesource.com/8598
Reviewed-by: David Benjamin <davidben@google.com>
V2ClientHello is going to be ugly wherever we do it, but this hides it
behind the transport method table. It removes a place where the
handshake state machine reaches into ssl3_get_message's internal state.
ssl3_get_message will now silently translate V2ClientHellos into true
ClientHellos and manage the handshake hash appropriately.
Now the only accesses of init_buf from the handshake state machines are
to create and destroy the buffer.
Change-Id: I81467a038f6ac472a465eec7486a443fe50a98e1
Reviewed-on: https://boringssl-review.googlesource.com/8641
Reviewed-by: Adam Langley <agl@google.com>
As with the client, the logic around extensions in 1.3 will want to be
tweaked. readClientHello will probably shrink a bit. (We could probably
stuff 1.3 into the existing parameter negotiation logic, but I expect
it'll get a bit unwieldy once HelloRetryRequest, PSK resumption, and
0-RTT get in there, so I think it's best we leave them separate.)
Change-Id: Id8c323a06a1def6857a59accd9f87fb0b088385a
Reviewed-on: https://boringssl-review.googlesource.com/8596
Reviewed-by: David Benjamin <davidben@google.com>
While the random connection property extensions like ALPN and SRTP
remain largely unchanged in TLS 1.3 (but for interaction with 0-RTT),
authentication-related extensions change significantly and need
dedicated logic.
Change-Id: I2588935c2563a22e9879fb81478b8df5168b43de
Reviewed-on: https://boringssl-review.googlesource.com/8602
Reviewed-by: David Benjamin <davidben@google.com>
Test with and without PackHandshakeFlight enabled to cover when the
early post-CCS fragment will get packed into one of the pre-CCS
handshake records. Also test the resumption cases too to cover more
state transitions.
The various CCS-related tests (since CCS is kind of a mess) are pulled
into their own group.
Change-Id: I6384f2fb28d9885cd2b06d59e765e080e3822d8a
Reviewed-on: https://boringssl-review.googlesource.com/8661
Reviewed-by: David Benjamin <davidben@google.com>
[Originally written by nharper and then revised by davidben.]
Most features are missing, but it works for a start. To avoid breaking
the fake TLS 1.3 tests while the C code is still not landed, all the
logic is gated on a global boolean. When the C code gets in, we'll
set it to true and remove this boolean.
Change-Id: I6b3a369890864c26203fc9cda37c8250024ce91b
Reviewed-on: https://boringssl-review.googlesource.com/8601
Reviewed-by: David Benjamin <davidben@google.com>
I'm surprised we'd never tested this. In addition to splitting handshake
records up, one may pack multiple handshakes into a single record, as
they fit. Generalize the DTLS handshake flush hook to do this in TLS as
well.
Change-Id: Ia546d18c7c56ba45e50f489c5b53e1fcd6404f51
Reviewed-on: https://boringssl-review.googlesource.com/8650
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.2 and 1.3 will process more-or-less the same server extensions,
but at slightly different points in the handshake. In preparation for
that, split this out into its own function.
Change-Id: I5494dee4724295794dfd13c5e9f9f83eade6b20a
Reviewed-on: https://boringssl-review.googlesource.com/8586
Reviewed-by: Adam Langley <agl@google.com>
[Originally written by nharper, tweaked by davidben.]
For now, ignore them completely.
Change-Id: I28602f219d210a857aa80d6e735557b8d2d1c590
Reviewed-on: https://boringssl-review.googlesource.com/8585
Reviewed-by: Adam Langley <agl@google.com>
Rather than have a separate codepath, just skip the message_complete
logic and parse what's in the buffer. This also cuts down on one input
to setting up a reuse_message; message_type is now only written to in
the get_message implementation.
Change-Id: I96689b5957a3f2548af9099ec4e53cabacdc395a
Reviewed-on: https://boringssl-review.googlesource.com/8640
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We now have a branch that contains the source code in a form suitable
for using with Bazel. This can be easier for projects that are already
using Bazel to deal with.
Change-Id: Iaaddb2ee50b367114ff2de88a526bf4bdb6ad7ab
Reviewed-on: https://boringssl-review.googlesource.com/8651
Reviewed-by: David Benjamin <davidben@google.com>
Amazingly, this function actually has (not crypto-related) callers, despite
being pretty much useless for cryptography.
BUG=31
Change-Id: I440827380995695c7a15bbf2220a05ffb28d9335
Reviewed-on: https://boringssl-review.googlesource.com/8594
Reviewed-by: David Benjamin <davidben@google.com>
These were generated by running test_mod_exp_mont5 10 times. The values with
Montgomery representation 1 were generated separately so the test file could
preserve the comment. (Though, at 10,000 lines, no one's going to find it...)
BUG=31
Change-Id: I8e9d4d6d7b5f7d283bd259df10a1dbdc90b888cf
Reviewed-on: https://boringssl-review.googlesource.com/8611
Reviewed-by: David Benjamin <davidben@google.com>
Honestly, with this size of number, they're pretty bad test vectors.
test_mod_exp_mont5 will be imported in the next commit which should help.
This was done by taking test_mod_exp's generation, running it a few times
(since otherwise the modulus is always the same). I also ran it a few times
with the odd constraint removed since BN_mod_exp is supposed to support it,
even if it's not actually useful.
BUG=31
Change-Id: Id53953f0544123a5ea71efac534946055dd5aabc
Reviewed-on: https://boringssl-review.googlesource.com/8610
Reviewed-by: David Benjamin <davidben@google.com>
That one needs reduced inputs and the other ought to be also tested against
unreduced ones is a bit annoying. But the previous commit made sure BN_nnmod
has tests, and test_mont could stand to inherit test_mod_mul's test data (it
only had five tests originally!), so I merged them.
BUG=31
Change-Id: I1eb585b14f85f0ea01ee81537a01e07ced9f5d9a
Reviewed-on: https://boringssl-review.googlesource.com/8608
Reviewed-by: David Benjamin <davidben@google.com>
In preparation for converting test_mont and test_mod_mul to test vectors, make
test_mont less silly. We can certainly get away with doing more than five
tests. Also generate |a| and |b| anew each time. Otherwise the first BN_nmod is
destructive.
Change-Id: I944007ed7b6013a16d972cb7290ab9992c9360ce
Reviewed-on: https://boringssl-review.googlesource.com/8605
Reviewed-by: David Benjamin <davidben@google.com>
When we have *-with-bazel branches this BUILD file will be copied to the
top-level for consumers that want to use Bazel.
From empirical testing, x86-64 on Linux is spelt “k8” and x86-64 on
macOS is spelt “darwin”. I've not tried to enable assembly for any other
cases yet.
Change-Id: Ic6cb739565f145db20756fb57c0d087227fd9e18
Reviewed-on: https://boringssl-review.googlesource.com/8571
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 will need to call it under different circumstances. We will also
wish to test TLS 1.3 post-handshake auth, so this function must work
without being passed handshake state.
In doing so, implement matching based on signature algorithms as 1.3
does away with the certificate type list.
Change-Id: Ibdee44bbbb589686fcbcd7412432100279bfac63
Reviewed-on: https://boringssl-review.googlesource.com/8589
Reviewed-by: David Benjamin <davidben@google.com>
[Originally written by nharper and then tweaked by davidben.]
TLS 1.3 tweaks them slightly, so being able to write them in one pass
rather than two will be somewhat more convenient.
Change-Id: Ib7e2d63e28cbae025c840bbb34e9e9c295b44dc6
Reviewed-on: https://boringssl-review.googlesource.com/8588
Reviewed-by: David Benjamin <davidben@google.com>
[Originally written by nharper, tweaked by davidben.]
In TLS 1.3, every extension the server previously sent gets moved to a
separate EncryptedExtensions message. To be able to share code between
the two, parse those extensions separately. For now, the handshake reads
from serverHello.extensions.foo, though later much of the extensions
logic will probably handle serverExtensions independent of whether it
resides in ServerHello or EncryptedExtensions.
Change-Id: I07aaae6df3ef6fbac49e64661d14078d0dbeafb0
Reviewed-on: https://boringssl-review.googlesource.com/8584
Reviewed-by: David Benjamin <davidben@google.com>
[Originally written by nharper and tweaked by davidben.]
This will end up being split in two with most of the ServerHello
extensions being serializable in both ServerHello and
EncryptedExtensions depending on version.
Change-Id: Ida5876d55fbafb982bc2e5fdaf82872e733d6536
Reviewed-on: https://boringssl-review.googlesource.com/8580
Reviewed-by: David Benjamin <davidben@google.com>
[Originally written by nharper and then slightly tweaked by davidben.]
Between the new deeply nested extension (KeyShare) and most of
ServerHello extensions moving to a separate message, this is probably
long overdue.
Change-Id: Ia86e30f56b597471bb7e27d726a9ec92687b4d10
Reviewed-on: https://boringssl-review.googlesource.com/8569
Reviewed-by: David Benjamin <davidben@google.com>
To match the Go side. That message will never be used for anything else,
so there's not much need to give it such a long name.
Change-Id: I3396c9d513d02d873e59cd8e81ee64005c5c706c
Reviewed-on: https://boringssl-review.googlesource.com/8620
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.3 defines its own EncryptedExtensions message. The existing one is
for Channel ID which probably should not have tried to generalize
itself.
Change-Id: I4f48bece98510eb54e64fbf3df6c2a7332bc0261
Reviewed-on: https://boringssl-review.googlesource.com/8566
Reviewed-by: David Benjamin <davidben@google.com>
Right now I believe we are testing against DTLS 1.3 ClientHellos. Fix
this in preparation for making VersionTLS13 go elsewhere in the Go code.
Unfortunately, I made the mistake of mapping DTLS 1.0 to TLS 1.0 rather
than 1.1 in Go. This does mean the names of the tests naturally work out
correctly, but we have to deal with this awkward DTLS-1.1-shaped hole in
our logic.
Change-Id: I8715582ed90acc1f08197831cae6de8d5442d028
Reviewed-on: https://boringssl-review.googlesource.com/8562
Reviewed-by: David Benjamin <davidben@google.com>
In preparation for TLS 1.3 using its actual handshake, switch most tests
to TLS 1.3 and add liberal TODOs for the tests which will need TLS 1.3
variants.
In doing so, move a few tests from basic tests into one of the groups.
Also rename BadECDSACurve to BadECDHECurve (it was never ECDSA) and add
a test to make sure FALLBACK_SCSV is correctly sensitive to the maximum
version.
Change-Id: Ifca6cf8f7a48d6f069483c0aab192ae691b1dd8e
Reviewed-on: https://boringssl-review.googlesource.com/8560
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.3 defines a new SignatureScheme uint16 enum that is backwards
compatible on the wire with TLS1.2's SignatureAndHashAlgorithm. This
change updates the go testing code to use a single signatureAlgorithm
enum (instead of 2 separate signature and hash enums) in preparation for
TLS 1.3. It also unifies all the signing around this new scheme,
effectively backporting the change to TLS 1.2.
For now, it does not distinguish signature algorithms between 1.2 and
1.3 (RSA-PSS instead of RSA-PKCS1, ECDSA must match curve types). When
the C code is ready make a similar change, the Go code will be updated
to match.
[Originally written by nharper, tweaked significantly by davidben.]
Change-Id: If9a315c4670755089ac061e4ec254ef3457a00de
Reviewed-on: https://boringssl-review.googlesource.com/8450
Reviewed-by: David Benjamin <davidben@google.com>
Also, update the documentation about aliasing for |BN_usub|. It might
be better to find a way to factor out the shared logic between the
tests of these functions and the tests of |BN_add| and |BN_usub|, but
doing so would end up up creating a lot of parameters due to the many
distinct strings used in the messages.
Change-Id: Ic9d714858212fc92aa6bbcc3959576fe6bbf58c3
Reviewed-on: https://boringssl-review.googlesource.com/8593
Reviewed-by: David Benjamin <davidben@google.com>
Upstream added this in a18a31e49d266. The various *_up_ref functions
return a variety of types, but this one returns int because upstream
appears to be trying to unify around that. (See upstream's c5ebfcab713.)
Change-Id: I7e1cfe78c3a32f5a85b1b3c14428bd91548aba6d
Reviewed-on: https://boringssl-review.googlesource.com/8581
Reviewed-by: Adam Langley <alangley@gmail.com>
|BN_mod_exp_mont| should be tested the same way as the other variants,
especially since it is exported.
Change-Id: I8c05725289c0ebcce7aba7e666915c4c1a841c2b
Reviewed-on: https://boringssl-review.googlesource.com/8590
Reviewed-by: David Benjamin <davidben@google.com>
In order to delay the digest of the handshake transcript and unify
around message-based signing callbacks, a copy of the transcript is kept
around until we are sure there is no certificate authentication.
This removes support for SSL_PRIVATE_KEY_METHOD as a client in SSL 3.0.
Change-Id: If8999a19ca021b4ff439319ab91e2cd2103caa64
Reviewed-on: https://boringssl-review.googlesource.com/8561
Reviewed-by: David Benjamin <davidben@google.com>
I'd meant to change the other -latest to -13 when I merged this, but we
may as well group the two together anyway. Also remove ticket_age as
that's likely to go away in PR#503.
Change-Id: Ibb2f447e344d0b13c937291de69ace37ac9a5e8d
Reviewed-on: https://boringssl-review.googlesource.com/8567
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This replaces the old key_exchange_info APIs and does not require the
caller be aware of the mess around SSL_SESSION management. They
currently have the same bugs around renegotiation as before, but later
work to fix up SSL_SESSION tracking will fix their internals.
For consistency with the existing functions, I've kept the public API at
'curve' rather than 'group' for now. I think it's probably better to
have only one name with a single explanation in the section header
rather than half and half. (I also wouldn't be surprised if the IETF
ends up renaming 'group' again to 'key exchange' at some point. We'll
see what happens.)
Change-Id: I8e90a503bc4045d12f30835c86de64ef9f2d07c8
Reviewed-on: https://boringssl-review.googlesource.com/8565
Reviewed-by: Adam Langley <agl@google.com>
We ended up switching this from a curve to a cipher suite, so the group
ID isn't used. This is in preparation for adding an API for the curve
ID, at which point leaving the protocol constants undefined seems
somewhat bad manners.
Change-Id: Icb8bf4594879dbbc24177551868ecfe89bc2f8c3
Reviewed-on: https://boringssl-review.googlesource.com/8563
Reviewed-by: Adam Langley <agl@google.com>
This isn't filled in on the client and Chromium no longer uses it for
plain RSA. It's redundant with existing APIs. This is part of removing
the need for callers to call SSL_get_session where possible.
SSL_get_session is ambiguous when it comes to renego. Some code wants
the current connection state which should not include the pending
handshake and some code wants the handshake scratch space which should.
Renego doesn't exist in TLS 1.3, but TLS 1.3 makes NewSessionTicket a
post-handshake message, so SSL_get_session is somewhat silly of an API
there too.
SSL_SESSION_get_key_exchange_info is a BoringSSL-only API, so we can
freely change it and replace it with APIs keyed on SSL. In doing so, I
think it is better to provide APIs like "SSL_get_dhe_group_size" and
"SSL_get_curve_id" rather than make the caller do the multi-step
SSL_get_current_cipher / SSL_CIPHER_is_ECDHE dance. To that end, RSA
key_exchange_info is pointless as it can already be determined from the
peer certificate.
Change-Id: Ie90523083d8649701c17934b7be0383502a0caa3
Reviewed-on: https://boringssl-review.googlesource.com/8564
Reviewed-by: Adam Langley <agl@google.com>
QUIC, in particular, will set min_version to TLS 1.3 and has no need to send
any legacy ciphers.
Note this requires changing some test expectations. Removing all of TLS 1.1 and
below's ciphers in TLS 1.3 has consequences for how a tripped minimum version
reads.
BUG=66
Change-Id: I695440ae78b95d9c7b5b921c3cb2eb43ea4cc50f
Reviewed-on: https://boringssl-review.googlesource.com/8514
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL's SSL_OP_NO_* flags allow discontinuous version ranges. This is a
nuisance for two reasons. First it makes it unnecessarily difficult to answer
"are any versions below TLS 1.3 enabled?". Second the protocol does not allow
discontinuous version ranges on the client anyway. OpenSSL instead picks the
first continous range of enabled versions on the client, but not the server.
This is bizarrely inconsistent. It also doesn't quite do this as the
ClientHello sending logic does this, but not the ServerHello processing logic.
So we actually break some invariants slightly. The logic is also cumbersome in
DTLS which kindly inverts the comparison logic.
First, switch min_version/max_version's storage to normalized versions. Next
replace all the ad-hoc version-related functions with a single
ssl_get_version_range function. Client and server now consistently pick a
contiguous range of versions. Note this is a slight behavior change for
servers. Version-range-sensitive logic is rewritten to use this new function.
BUG=66
Change-Id: Iad0d64f2b7a917603fc7da54c9fc6656c5fbdb24
Reviewed-on: https://boringssl-review.googlesource.com/8513
Reviewed-by: David Benjamin <davidben@google.com>
Bazel wants to know the header files of the targets that it builds too,
so output that in the generated BUILD files.
Change-Id: I5b90908342fc8819ae6bc7ff91eb6f5afc0ddf54
Reviewed-on: https://boringssl-review.googlesource.com/8570
Reviewed-by: David Benjamin <davidben@google.com>
The signing logic itself still depends on pre-hashed messages and will be fixed
in later commits.
Change-Id: I901b0d99917c311653d44efa34a044bbb9f11e57
Reviewed-on: https://boringssl-review.googlesource.com/8545
Reviewed-by: David Benjamin <davidben@google.com>
As part of the SignatureAlgorithm change in the TLS 1.3 specification,
the existing signature/hash combinations are replaced with a combined
signature algorithm identifier. This change maintains the existing APIs
while fixing the internal representations. The signing code currently
still treats the SignatureAlgorithm as a decomposed value, which will be
fixed as part of a separate CL.
Change-Id: I0cd1660d74ad9bcf55ce5da4449bf2922660be36
Reviewed-on: https://boringssl-review.googlesource.com/8480
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
These can all share one test type. Note test_div had a separate
division by zero test which had to be extracted.
BUG=31
Change-Id: I1de0220fba78cd7f82a5dc96adb34b79c07929e9
Reviewed-on: https://boringssl-review.googlesource.com/8527
Reviewed-by: Adam Langley <agl@google.com>
crypto/bn/bn_test.cc:404:44: error: ‘n’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
Change-Id: Id590dfee4b9ae1a4fbd0965e133310dac0d06ed3
Two of these were even regression tests for a past bug. These are also
moved to the file, now with the amazing innovation that we *actually
check the regression test gave the right answer*.
BUG=31
Change-Id: I8097336ad39a2bb5c0af07dd8e1e34723b68d182
Reviewed-on: https://boringssl-review.googlesource.com/8525
Reviewed-by: Adam Langley <agl@google.com>
This adds tests for:
for i = 0 to 199:
Sum: 2^i
A: 2^i - 1
B: 1
for i = 0 to 199:
Sum: 2^200
A: 2^200 - 2^i
B: 2^i
I don't believe any of the existing tests actually stressed this,
amazingly enough.
Change-Id: I5edab6327bad45fc21c62bd47f4169f8bb745ff7
Reviewed-on: https://boringssl-review.googlesource.com/8523
Reviewed-by: Adam Langley <agl@google.com>
This took some finesse. I merged the lshift1 and rshift1 test vectors as
one counted down and the other up. The rshift1 vectors were all rounded
to even numbers, with the test handling the odd case. Finally, each run
only tested positive or negative (it wasn't re-randomized), so I added
both positive and negative versions of each test vector.
BUG=31
Change-Id: Ic7de45ab797074547c44c2e4ff8089b1feec5d57
Reviewed-on: https://boringssl-review.googlesource.com/8522
Reviewed-by: Adam Langley <agl@google.com>
buffer buffer buffer buffer buffer. At some point, words lose their meaning if
they're used too many times. Notably, the DTLS code can't decide whether a
"buffered message" is an incoming message to be reassembled or an outgoing
message to be (re)transmitted.
Change-Id: Ibdde5c00abb062c603d21be97aff49e1c422c755
Reviewed-on: https://boringssl-review.googlesource.com/8500
Reviewed-by: Adam Langley <agl@google.com>
This allows us to use CBB for all handshake messages. Now, SSL_PROTOCOL_METHOD
is responsible for implementing a trio of CBB-related hooks to assemble
handshake messages.
Change-Id: I144d3cac4f05b6637bf45d3f838673fc5c854405
Reviewed-on: https://boringssl-review.googlesource.com/8440
Reviewed-by: Adam Langley <agl@google.com>
Test vectors taken from one run of bc_test with the -bc flag, along with
a handful of manual test vectors around numbers close to zero. (The
output was compared against bc to make sure it was correct.)
BUG=31
Change-Id: I9e9263ece64a877c8497716cd4713b4c3e44248c
Reviewed-on: https://boringssl-review.googlesource.com/8521
Reviewed-by: Adam Langley <agl@google.com>
Upstream added new instructions in
f4d456408d9d7bca31f34765d1a05fbd9fa55826 and
4e3d2866b6e8e7a700ea22e05840a093bfd7a4b1.
Change-Id: I835650426a0dffca2d8686d64aef99097a4bd186
Reviewed-on: https://boringssl-review.googlesource.com/8520
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 67b8bf4d849a7c40d0226de4ebe2590c4cc7c1f7.)
Verified a no-op in generate_build_files.py.
Change-Id: I09648893ab5c795f3934da0b2ecbc5fd7eb068d5
Reviewed-on: https://boringssl-review.googlesource.com/8519
Reviewed-by: Adam Langley <agl@google.com>
Depending on architecture, perlasm differed on which one or both of:
perl foo.pl flavor output.S
perl foo.pl flavor > output.S
Upstream has now unified on the first form after making a number of
changes to their files (the second does not even work for their x86
files anymore). Sync those portions of our perlasm scripts with upstream
and update CMakeLists.txt and generate_build_files.py per the new
convention.
This imports various commits like this one:
184bc45f683c76531d7e065b6553ca9086564576 (this was done by taking a
diff, so I don't have the full list)
Confirmed that generate_build_files.py sees no change.
BUG=14
Change-Id: Id2fb5b8bc2a7369d077221b5df9a6947d41f50d2
Reviewed-on: https://boringssl-review.googlesource.com/8518
Reviewed-by: Adam Langley <agl@google.com>
We're not using the masm output (and upstream does not even support it).
Reduce unnecessary diff from upstream.
Change-Id: Ic0b0f804bd7ec1429b3b1f40746297b57dcfcef6
Reviewed-on: https://boringssl-review.googlesource.com/8517
Reviewed-by: Adam Langley <agl@google.com>
OBJ_NAME in OpenSSL has an 'alias' field which some code consumes. We never
report anything OpenSSL considers an alias, so just leave it zero. It also has
a 'data' field which, confusingly, is a pointer to the EVP_CIPHER or EVP_MD
despite being a char pointer.
See calls to and implementation of OBJ_NAME_add in OpenSSL for comparison.
Change-Id: Ifc5c70424569db8783deb2fda7736c1954b5dd3a
Reviewed-on: https://boringssl-review.googlesource.com/8515
Reviewed-by: Adam Langley <agl@google.com>
It has size 7. There's no need for a priority queue structure, especially one
that's O(N^2) anyway.
Change-Id: I7609794aac1925c9bbf3015744cae266dcb79bff
Reviewed-on: https://boringssl-review.googlesource.com/8437
Reviewed-by: Adam Langley <agl@google.com>
The pair was a remnant of some weird statefulness and also ChangeCipherSpec
having a "sequence number" to make the pqueue turn into an array.
Change-Id: Iffd82594314df43934073bd141faee0fc167ed5f
Reviewed-on: https://boringssl-review.googlesource.com/8436
Reviewed-by: Adam Langley <agl@google.com>
Now that retransitting is a lot less stateful, a lot of surrounding code can
lose statefulness too. Rather than this overcomplicated pqueue structure,
hardcode that a handshake flight is capped at 7 messages (actually, DTLS can
only get up to 6 because we don't support NPN or Channel ID in DTLS) and used a
fixed size array.
This also resolves several TODOs.
Change-Id: I2b54c3441577a75ad5ca411d872b807d69aa08eb
Reviewed-on: https://boringssl-review.googlesource.com/8435
Reviewed-by: Adam Langley <agl@google.com>
Now dtls1_do_handshake_write takes in a serialized form of the full message and
writes it. It's a little weird to serialize and deserialize the header a bunch,
but msg_callback requires that we keep the full one around in memory anyway.
Between that and the handshake hash definition, DTLS really wants messages to
mean the assembled header, redundancies and all, so we'll just put together
messages that way.
This also fixes a bug where ssl_do_msg_callback would get passed in garbage
where the header was supposed to be. The buffered messages get sampled before
writing the fragment rather than after.
Change-Id: I4e3b8ce4aab4c4ab4502d5428dfb8f3f729c6ef9
Reviewed-on: https://boringssl-review.googlesource.com/8433
Reviewed-by: Adam Langley <agl@google.com>
It is an explicit copy of something, but it's a lot easier to reason about than
the init_buf/init_num gynmastics we were previously doing. This is along the
way to getting init_buf out of here.
Change-Id: Ia1819ba9db60ef6db09dd60d208dbc95fcfb4bd2
Reviewed-on: https://boringssl-review.googlesource.com/8432
Reviewed-by: Adam Langley <agl@google.com>
The only thing we've written before the signature is the hash. We can just
choose it anew. This is along the way to getting init_buf out of the handshake
output side. (init_buf is kind of a mess since it doesn't integrate nicely with
a top-level CBB. Some of the logic hasn't been converted to CBB because they're
interspersed with a BUF_MEM_grow.)
Change-Id: I693e834b5a03849bebb04f3f6b81f81fb04e2530
Reviewed-on: https://boringssl-review.googlesource.com/8431
Reviewed-by: Adam Langley <agl@google.com>
It doesn't really convey anything useful. Leave ssl_get_message alone for now
since it's called everywhere in the handshake and I'm about to tweak it
further.
Change-Id: I6f3a74c170e818f624be8fbe5cf6b796353406df
Reviewed-on: https://boringssl-review.googlesource.com/8430
Reviewed-by: Adam Langley <agl@google.com>
Saves us some mess if they're never zero. This also fixes a bug in
ssl3_get_max_client_version where it didn't account for all versions being
disabled properly.
Change-Id: I4c95ff57cf8953cb4a528263b252379f252f3e01
Reviewed-on: https://boringssl-review.googlesource.com/8512
Reviewed-by: David Benjamin <davidben@google.com>
Otherwise if the client's ClientHello logic is messed up and ServerHello is
fine, we won't notice.
Change-Id: I7f983cca45f7da1113ad4a72de1f991115e1b29a
Reviewed-on: https://boringssl-review.googlesource.com/8511
Reviewed-by: David Benjamin <davidben@google.com>
This also adds a missing check to the C half to ensure fake record types are
always correct, to keep implementations honest.
Change-Id: I1d65272e647ffa67018c721d52c639f8ba47d647
Reviewed-on: https://boringssl-review.googlesource.com/8510
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
As of 67cb49d045 and the corresponding upstream
change, BN_mod_word may fail, like BN_div_word. Handle this properly and
document in bn.h. Thanks to Brian Smith for pointing this out.
Change-Id: I6d4f32dc37bcabf70847c9a8b417d55d31b3a380
Reviewed-on: https://boringssl-review.googlesource.com/8491
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
While most of OpenSSL's assembly allows out < in too, some of it doesn't.
Upstream seems to not consider this a problem (or, at least, they're failing to
make a decision on whether it is a problem, so we should assume they'll stay
their course). Accordingly, require aliased buffers to exactly align so we
don't have to keep chasing this down.
Change-Id: I00eb3df3e195b249116c68f7272442918d7077eb
Reviewed-on: https://boringssl-review.googlesource.com/8231
Reviewed-by: Adam Langley <agl@google.com>
Decrypting is very easy to do in-place, but encrypting in-place is a hassle.
The rules actually were wrong due to record-splitting. The aliasing prefix and
the alignment prefix actually differ by 1. Take it out for now in preparation
for tightening the aliasing rules.
If we decide to do in-place encrypt later, probably it'd be more useful to
return header + in-place ciphertext + trailer. (That, in turn, needs a
scatter/gather thing on the AEAD thanks to TLS 1.3's padding and record type
construction.) We may also wish to rethink how record-splitting works here.
Change-Id: I0187d39c541e76ef933b7c2c193323164fd8a156
Reviewed-on: https://boringssl-review.googlesource.com/8230
Reviewed-by: Adam Langley <agl@google.com>
On systems where we do not have BN_ULLONG (notably Win64), BN_mod_word() can
return incorrect results if the supplied modulus is too big.
(Imported from upstream's e82fd1b4574c8908b2c3bb68e1237f057a981820 and
e4c4b2766bb97b34ea3479252276ab7c66311809.)
Change-Id: Icee8a7c5c67a8ee14c276097f43a7c491e68c2f9
Reviewed-on: https://boringssl-review.googlesource.com/8233
Reviewed-by: Adam Langley <agl@google.com>
Set ctx->error = X509_V_ERR_OUT_OF_MEM when verification cannot
continue due to malloc failure. Similarly for issuer lookup failures
and caller errors (bad parameters or invalid state).
Also, when X509_verify_cert() returns <= 0 make sure that the
verification status does not remain X509_V_OK, as a last resort set
it it to X509_V_ERR_UNSPECIFIED, just in case some code path returns
an error without setting an appropriate value of ctx->error.
Add new and some missing error codes to X509 error -> SSL alert switch.
(Imported from upstream's 5553a12735e11bc9aa28727afe721e7236788aab.)
Change-Id: I3231a6b2e72a3914cb9316b8e90ebaee009a1c5f
Reviewed-on: https://boringssl-review.googlesource.com/8170
Reviewed-by: David Benjamin <davidben@google.com>
This callback is used by BoringSSL tests in order to simulate the time,
so that the tests have repeatable results. This API will allow consumers
of BoringSSL to write the same sort of tests.
Change-Id: I79d72bce5510bbd83c307915cd2cc937579ce948
Reviewed-on: https://boringssl-review.googlesource.com/8200
Reviewed-by: David Benjamin <davidben@google.com>
It's useful, when combined with patching crypto/rand/deterministic.c in, for
debugging things. Also if we want to record fuzzer transcripts again, this
probably should be on.
Change-Id: I109cf27ebab64f01a13466f0d960def3257d8750
Reviewed-on: https://boringssl-review.googlesource.com/8192
Reviewed-by: David Benjamin <davidben@google.com>
x25519-x86_64.c, like the rest of crypto/curve25519, is descended from
SUPERCOP. Add the usual copyright header along with the SUPERCOP attribution.
BUG=64
Change-Id: I43f3de0731f33ab2aa48492c4b742e9f23c87fe1
Reviewed-on: https://boringssl-review.googlesource.com/8195
Reviewed-by: Adam Langley <agl@google.com>
Depending on bittedness of the runner, uint16 * uint16 can overflow an int.
There's other computations that can overflow a uint32 as well, so I just made
everything uint64 to avoid thinking about it too much.
Change-Id: Ia3c976987f39f78285c865a2d7688600d73c2514
Reviewed-on: https://boringssl-review.googlesource.com/8193
Reviewed-by: Adam Langley <agl@google.com>
The separation is purely historical (what happened to use an SSL_ctrl hook), so
put them all in one place. Make a vague attempt to match the order of the
header file, though we're still very far from matching.
Change-Id: Iba003ff4a06684a6be342e438d34bc92cab1cd14
Reviewed-on: https://boringssl-review.googlesource.com/8189
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reorder states and functions by where they appear in the handshake. Remove
unnecessary hooks on SSL_PROTOCOL_METHOD.
Change-Id: I78dae9cf70792170abed6f38510ce870707e82ff
Reviewed-on: https://boringssl-review.googlesource.com/8184
Reviewed-by: David Benjamin <davidben@google.com>
It's unreachable and wouldn't work anyway. We'd never bubble up to the caller
to retry. As a consequence, the TLS side doesn't actually need to pay attention
to init_off.
(For now anyway. We'll probably need state of this sort once the write half is
all reworked. All the craziness with wpend_buf ought to be limited to the
SSL_write bits.)
Change-Id: I951534f6bbeb547ce0492d5647aaf76be42108a3
Reviewed-on: https://boringssl-review.googlesource.com/8179
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
It can be folded into dtls1_read_app_data. This code, since it still takes an
output pointer, does not yet process records atomically. (Though, being DTLS,
it probably should...)
Change-Id: I57d60785c9c1dd13b5b2ed158a08a8f5a518db4f
Reviewed-on: https://boringssl-review.googlesource.com/8177
Reviewed-by: David Benjamin <davidben@google.com>
This was probably the worst offender of them all as read_bytes is the wrong
abstraction to begin with. Note this is a slight change in how processing a
record works. Rather than reading one fragment at a time, we process all
fragments in a record and return. The intent here is so that all records are
processed atomically since the connection eventually will not be able to retain
a buffer holding the record.
This loses a ton of (though not quite all yet) those a2b macros.
Change-Id: Ibe4bbcc33c496328de08d272457d2282c411b38b
Reviewed-on: https://boringssl-review.googlesource.com/8176
Reviewed-by: David Benjamin <davidben@google.com>
The business with ssl_record_prefix_len is rather a hassle. Instead, have
tls_open_record always decrypt in-place and give back a CBS to where the body
is.
This way the caller doesn't need to do an extra check all to avoid creating an
invalid pointer and underflow in subtraction.
Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee
Reviewed-on: https://boringssl-review.googlesource.com/8173
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Alert handling is more-or-less identical across all contexts. Push it down from
read_bytes into the low-level record functions. This also deduplicates the code
shared between TLS and DTLS.
Now the only type mismatch managed by read_bytes is if we get handshake data in
read_app_data.
Change-Id: Ia8331897b304566e66d901899cfbf31d2870194e
Reviewed-on: https://boringssl-review.googlesource.com/8124
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Move this logic out of dtls1_read_bytes and into dtls1_get_record. Only trigger
it when reading from the buffer fails. The other one shouldn't be necessary.
This exists to handle the blocking BIO case when the
BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT signal triggers, so we only need to do it when
timeouts actually trigger.
There also doesn't seem to be a need for most of the machinery. The
BIO_set_flags call seems to be working around a deficiency in the underlying
BIO. There also shouldn't be a need to check the handshake state as there
wouldn't be a timer to restart otherwise.
Change-Id: Ic901ccfb5b82aeb409d16a9d32c04741410ad6d7
Reviewed-on: https://boringssl-review.googlesource.com/8122
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The two modes are quite different. One of them requires the BIO honor an
extra BIO_ctrl. Also add an explanation at the top of
addDTLSRetransmitTests for how these tests work. The description is
scattered across many different places.
BUG=63
Change-Id: Iff4cdd1fbf4f4439ae0c293f565eb6780c7c84f9
Reviewed-on: https://boringssl-review.googlesource.com/8121
Reviewed-by: David Benjamin <davidben@google.com>
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>
The fake numbers collide with other numbers defined below. Also PUSH and POP
are actually used. DUP legitimately isn't though.
Change-Id: Iaa15a065d846b89b9b7958b78068393cfee2bd6f
Reviewed-on: https://boringssl-review.googlesource.com/8143
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>
In TLS 1.3, the actual record type is hidden within the encrypted data
and the record layer defaults to using a TLS 1.0 {3, 1} record version
for compatibility. Additionally the record layer no longer checks the
minor version of the record layer to maintain compatibility with the
TLS 1.3 spec.
Change-Id: If2c08e48baab170c1658e0715c33929d36c9be3a
Reviewed-on: https://boringssl-review.googlesource.com/8091
Reviewed-by: David Benjamin <davidben@google.com>
They match the new style not the old EAY style now. They're also not
likely to be reformatted. It's just the legacy ASN.1 stuff now and we're
intentionally not doing much with those. (The old text was written back
before the SSL stack had been reformatted.)
Change-Id: I4852761b013e8c2688ebc7eaf4970afbdc69e858
Reviewed-on: https://boringssl-review.googlesource.com/8129
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Assembly code for X25519 wasn't included on OS X when built with
build systems other than CMake, which lead to a SIGTRAP due to a
missing x25519_x86_64.
Reported by Gurgen Hrachyan.
Change-Id: Ib6026f31cce0405ec3e75d8a52bf0940e57c62c8
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/8111
Reviewed-by: David Benjamin <davidben@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>
OpenSSL was actually super-buggy here (though known bugs on our end have been
fixed), but pyOpenSSL was confused and incorrectly documented that callers call
SSL_read after SSL_shutdown to do bidi shutdown, so we should probably support
this. Add a test that it works.
Change-Id: I2b6d012161330aeb4cf894bae3a0b6a55d53c70d
Reviewed-on: https://boringssl-review.googlesource.com/8093
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 TLS 1.3 spec has an explicit nonce construction for AEADs that
requires xoring the IV and sequence number.
Change-Id: I77145e12f7946ffb35ebeeb9b2947aa51058cbe9
Reviewed-on: https://boringssl-review.googlesource.com/8042
Reviewed-by: Adam Langley <agl@google.com>
Constants representing TLS 1.3 are added to allow for future work to be
flagged on TLS1_3_VERSION. To prevent BoringSSL from negotiating the
non-existent TLS 1.3 version, it is explicitly disabled using
SSL_OP_NO_TLSv1_3.
Change-Id: Ie5258a916f4c19ef21646c4073d5b4a7974d6f3f
Reviewed-on: https://boringssl-review.googlesource.com/8041
Reviewed-by: Adam Langley <agl@google.com>
This renames the Channel ID EncryptedExtensions message to allow for
compatibility with TLS 1.3 EncryptedExtensions.
Change-Id: I5b67d00d548518045554becb1b7213fba86731f2
Reviewed-on: https://boringssl-review.googlesource.com/8040
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's bbio logic is kind of crazy. It would be good to eventually do the
buffering in a better way (notably, bbio is fragile, if not outright broken,
for DTLS). In the meantime, this fixes a number of bugs where the existence of
bbio was leaked in the public API and broke things.
- SSL_get_wbio returned the bbio during the handshake. It must always return
the BIO the consumer configured. In doing so, internal accesses of
SSL_get_wbio should be switched to ssl->wbio since those want to see bbio.
For consistency, do the same with rbio.
- The logic in SSL_set_rfd, etc. (which I doubt is quite right since
SSL_set_bio's lifetime is unclear) would get confused once wbio got wrapped.
Those want to compare to SSL_get_wbio.
- If SSL_set_bio was called mid-handshake, bbio would get disconnected and lose
state. It forgets to reattach the bbio afterwards. Unfortunately, Conscrypt
does this a lot. It just never ended up calling it at a point where the bbio
would cause problems.
- Make more explicit the invariant that any bbio's which exist are always
attached. Simplify a few things as part of that.
Change-Id: Ia02d6bdfb9aeb1e3021a8f82dcbd0629f5c7fb8d
Reviewed-on: https://boringssl-review.googlesource.com/8023
Reviewed-by: Kenny Root <kroot@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The 'elliptic_curves' extension is being renamed to 'supported_groups'
in the TLS 1.3 draft, and most of the curve-specific methods are
generalized to groups/group IDs.
Change-Id: Icd1a1cf7365c8a4a64ae601993dc4273802610fb
Reviewed-on: https://boringssl-review.googlesource.com/7955
Reviewed-by: David Benjamin <davidben@google.com>
open_iscsi assumes that it can get |OPENSSL_malloc| after including only
pem.h and err.h. Since pem.h already includes quite a lot, this change
adds crypto.h to that set so that open_iscsi is happy.
Change-Id: I6dc06c27088ce3ca46c1ab53bb29650033cba267
Reviewed-on: https://boringssl-review.googlesource.com/8031
Reviewed-by: David Benjamin <davidben@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>
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>
The recipes need to run with a funny GOROOT and we were clearing the
environment.
BUG=26
Change-Id: If233a16e060533ad3fa6f215ce596456c2d7afa5
Reviewed-on: https://boringssl-review.googlesource.com/7988
Reviewed-by: Adam Langley <agl@google.com>
We now have a copy of android-cmake. Also remove the mention of running cmake
twice. It seems to work fine once?
The API level also got specified twice somehow.
BUG=26
Change-Id: I1331b079a4d8531cd53f7de3605ac318c14b3e26
Reviewed-on: https://boringssl-review.googlesource.com/7985
Reviewed-by: Adam Langley <agl@google.com>
We don't use find_package/find_program on android to find go/perl
because the android toolchain reconfigure the $PATH. The pervious
way of solving this was to let ninja look for go/perl on the $PATH
but this approach prevented us from specifying explicit go/perl
executables what is needed for hermetic build using prebuilts. This
CL changes the Android specific discovery rule to only set
GO_EXECUTABLE and PERL_EXECUTABLE if they are not specified on the
command line or inside the toolchain file.
Change-Id: Ib6ef69707749073f2b79244ebb301502b2a5a34a
Reviewed-on: https://boringssl-review.googlesource.com/8000
Reviewed-by: David Benjamin <davidben@google.com>
Those checks contradict an assert up in read_app_data. This is part of
shrinking read_bytes further into get_record and its callers until it goes
away. Here, this kind of policy should be controlled by the callers.
Change-Id: If8f9a45b8b95093beab1b3d4abcd31da55c65322
Reviewed-on: https://boringssl-review.googlesource.com/7954
Reviewed-by: Adam Langley <agl@google.com>
There is no good reason why this needs to be this way. Later work should make
this all use a much more appropriate design. In the meantime, leave a note here
so this does not look accidental.
Change-Id: I7599dea7a474f54e26d9ab175b0e3cada99a974d
Reviewed-on: https://boringssl-review.googlesource.com/7951
Reviewed-by: Adam Langley <agl@google.com>
This was needed because ssl3_get_message would get confused if init_num were
not set back to zero when reading the next message. However, ssl3_get_message
now treats init_num only as an output, not an input. (The message sending logic
and the individual handshake states still use it, so we can't get rid of it
altogether yet.)
I've kept the init_num reset at the start and end of the handshake loop alone
for now since that's more about initialization and cleanup. Though I believe
they too do not do anything.
Change-Id: I64bbdd82122498de32364e7edb3b00b166059ecd
Reviewed-on: https://boringssl-review.googlesource.com/7950
Reviewed-by: Adam Langley <agl@google.com>
They're completely unused now. The handshake message reassembly logic should
not depend on the state machine. This should partially free it up (ugly as it
is) to be shared with a future TLS 1.3 implementation while, in parallel, it
and the layers below, get reworked. This also cuts down on the number of states
significantly.
Partially because I expect we'd want to get ssl_hash_message_t out of there
too. Having it in common code is fine, but it needs to be in the (supposed to
be) protocol-agnostic handshake state machine, not the protocol-specific
handshake message layer.
Change-Id: I12f9dc57bf433ceead0591106ab165d352ef6ee4
Reviewed-on: https://boringssl-review.googlesource.com/7949
Reviewed-by: Adam Langley <agl@google.com>
Rather than this confusing coordination with the handshake state machine and
init_num changing meaning partway through, use the length field already in
BUF_MEM. Like the new record layer parsing, is no need to keep track of whether
we are reading the header or the body. Simply keep extending the handshake
message until it's far enough along.
ssl3_get_message still needs tons of work, but this allows us to disentangle it
from the handshake state.
Change-Id: Ic2b3e7cfe6152a7e28a04980317d3c7c396d9b08
Reviewed-on: https://boringssl-review.googlesource.com/7948
Reviewed-by: Adam Langley <agl@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>
On Windows, if we write to our socket and then close it, the peer sometimes
doesn't get all the data. This was working for our shimShutsDown tests because
we send close_notify in parallel with the peer and sendAlert(alertCloseNotify)
did not internally return an error.
For convenience, sendAlert returns a local error for non-close_notify alerts.
Suppress that error to avoid the race condition. This makes it behave like the
other shimShutsDown tests.
Change-Id: Iad256e3ea5223285793991e2eba9c7d61f2e3ddf
Reviewed-on: https://boringssl-review.googlesource.com/7980
Reviewed-by: Adam Langley <agl@google.com>
Previously, SSL_ECDH_METHOD consisted of two methods: one to produce a
public key to be sent to the peer, and another to produce the shared key
upon receipt of the peer's message.
This API does not work for NEWHOPE, because the client-to-server message
cannot be produced until the server's message has been received by the
client.
Solve this by introducing a new method which consumes data from the
server key exchange message and produces data for the client key
exchange message.
Change-Id: I1ed5a2bf198ca2d2ddb6d577888c1fa2008ef99a
Reviewed-on: https://boringssl-review.googlesource.com/7961
Reviewed-by: David Benjamin <davidben@google.com>
There's an off-by-one when skipping blank lines. The initial logic also has an
off-by-one but since it starts lineNo 0-based and then switches to 1-based, it
cancels out.
The decl error line number also was not of where the decl began.
Change-Id: I58fd157dad3276cb9de52ac48ff8c7c73e40f337
Reviewed-on: https://boringssl-review.googlesource.com/7959
Reviewed-by: Adam Langley <agl@google.com>
This explicitly forbids an API pattern which formerly kind of worked, but was
extremely buggy (see preceding commits). Depending on how one interprets
close_notify and our API, one might wish to call SSL_shutdown only once
(morally shutdown(SHUT_WR)) and then SSL_read until EOF.
However, this exposes additional confusing states where we might try to send an
alert post-SHUT_WR, etc. Early commits made us more robust here (whether one is
allowed to touch the SSL* after an operattion failed because it read an alert
is... unclear), so we could support it if we wanted to, but this doesn't seem
worth the additional statespace. See if we can get away with not allowing it.
Change-Id: Ie7a7e5520b464360b1e6316c34ec9854b571782f
Reviewed-on: https://boringssl-review.googlesource.com/7433
Reviewed-by: David Benjamin <davidben@google.com>
The logic to drop records really should be in the caller. Unless
ssl3_read_bytes is broken apart, condition on the type field which is more
robust.
If we manage to call, say, SSL_read after SSL_shutdown completes at 0 (instead
of 1), this logic can incorrectly cause unknown record types to be dropped.
Change-Id: Iab90e5d9190fcccbf6ff55e17079a2704ed99901
Reviewed-on: https://boringssl-review.googlesource.com/7953
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The existing logic gets confused in a number of cases around close_notify vs.
fatal alert. SSL_shutdown, while still pushing to the error queue, will fail to
notice alerts. We also get confused if we try to send a fatal alert when we've
already sent something else.
Change-Id: I9b1d217fbf1ee8a9c59efbebba60165b7de9689e
Reviewed-on: https://boringssl-review.googlesource.com/7952
Reviewed-by: David Benjamin <davidben@google.com>
SSL_RECEIVED_SHUTDOWN checks in the record layer happen in two different
places. Some operations (but not all) check it, and so does read_bytes. Move it
to get_record.
This check should be at a low-level since it is otherwise duplicated in every
operation. It is also a signal which originates from around the peer's record
layer, so it makes sense to check it near the same code. (This one's in
get_record which is technically lower-level than read_bytes, but we're trying
to get rid of read_bytes. They're very coupled functions.)
Also, if we've seen a fatal alert, replay an error, not an EOF.
Change-Id: Idec35c5068ddabe5b1a9145016d8f945da2421cf
Reviewed-on: https://boringssl-review.googlesource.com/7436
Reviewed-by: David Benjamin <davidben@google.com>
In practice it seems that it's not clear that consumers of BoringSSL
generally check in the generated files.
Change-Id: Iaa03aa62139bbcf3e7e7f68662073854954b835f
Reviewed-on: https://boringssl-review.googlesource.com/7956
Reviewed-by: David Benjamin <davidben@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>
OpenSSL used to only forbid it on the server in plain PSK and allow it on the
client. Enforce it properly on both sides. My read of the rule in RFC 5246 ("A
non-anonymous server can optionally request a certificate") and in RFC 4279
("The Certificate and CertificateRequest payloads are omitted from the
response.") is that client auth happens iff we're certificate-based.
The line in RFC 4279 is under the plain PSK section, but that doesn't make a
whole lot of sense and there is only one diagram. PSK already authenticates
both sides. I think the most plausible interpretation is that this is for
certificate-based ciphers.
Change-Id: If195232c83f21e011e25318178bb45186de707e6
Reviewed-on: https://boringssl-review.googlesource.com/7942
Reviewed-by: David Benjamin <davidben@google.com>
A handshake message can go up to 2^24 bytes = 16MB which is a little large for
the peer to force us to buffer. Accordingly, we bound the size of a
handshake message.
Rather than have a global limit, the existing logic uses a different limit at
each state in the handshake state machine and, for certificates, allows
configuring the maximum certificate size. This is nice in that we engage larger
limits iff the relevant state is reachable from the handshake. Servers without
client auth get a tighter limit "for free".
However, this doesn't work for DTLS due to out-of-order messages and we use a
simpler scheme for DTLS. This scheme also is tricky on optional messages and
makes the handshake <-> message layer communication complex.
Apart from an ignored 20,000 byte limit on ServerHello, the largest
non-certificate limit is the common 16k limit on ClientHello. So this
complexity wasn't buying us anything. Unify everything on the DTLS scheme
except, so as not to regress bounds on client-auth-less servers, also correctly
check for whether client auth is configured. The value of 16k was chosen based
on this value.
(The 20,000 byte ServerHello limit makes no sense. We can easily bound the
ServerHello because servers may not send extensions we don't implement. But it
gets overshadowed by the certificate anyway.)
Change-Id: I00309b16d809a3c2a1543f99fd29c4163e3add81
Reviewed-on: https://boringssl-review.googlesource.com/7941
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>
Allow running only one of the test suites. The recipe expects these happen in
two separate steps (it wants only one JSON results file per "step"). Also add
an option to extract the results file.
BUG=26
Change-Id: I0cda19bd9643b66f40a30bc8410a357da33baacc
Reviewed-on: https://boringssl-review.googlesource.com/7945
Reviewed-by: Adam Langley <agl@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>
The specification, sadly, did not say that servers MUST NOT send it, only that
they are "not expected to" do anything with the client extension. Accordingly,
we decided to tolerate this. Add a test for this so that we check this
behavior.
This test also ensures that the original session's value for it carries over.
Change-Id: I38c738f218a09367c9d8d1b0c4d68ab5cbec730e
Reviewed-on: https://boringssl-review.googlesource.com/7860
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>
This allows an application to override the default of 1 second, which
is what's instructed in RFC 6347 but is not an absolute requirement.
Change-Id: I0bbb16e31990fbcab44a29325b6ec7757d5789e5
Reviewed-on: https://boringssl-review.googlesource.com/7930
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>
When setting a new SRTP connection profile using
SSL_CTX_set_tlsext_use_srtp() or SSL_set_tlsext_use_srtp() we should
free any existing profile first to avoid a memory leak.
(Imported from upstream's fbdf0299dc98bc611d854c0a62c6ab1810d856fc.)
Change-Id: I738e711f1c23ed4a8ac97486d94c08cc0db7aea7
Reviewed-on: https://boringssl-review.googlesource.com/7910
Reviewed-by: Steven Valdez <svaldez@google.com>
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>
The SSL tests are fairly different from most test suites. Add some high-level
documentation so people know where to start.
Change-Id: Ie5ea108883dca82675571a3025b3fbc4b9d66da9
Reviewed-on: https://boringssl-review.googlesource.com/7890
Reviewed-by: Adam Langley <agl@google.com>
This is particularly questionable with ClientHello encompassing several states.
ssl->shutdown is already initialized to zero and further reset in
SSL_set_{connect,accept}_state. At any other state, if it manages to not be a
no-op, it will erase a close_notify we have sent or received, neither of which
is okay. (I don't think this is possible, but I'm not positive.)
This dates to the initial commit in OpenSSL, so git is not enlightening. The
state machine logic historically reset many fields it had no reason to reset,
so this is likely more of that.
Change-Id: Ie872316701720cb8ef2cfcb67b7f07a9fea3620f
Reviewed-on: https://boringssl-review.googlesource.com/7874
Reviewed-by: Adam Langley <agl@google.com>
Having bbio be tri-state (not allocated, allocated but not active, and
allocated and active) is confusing.
The extra state is only used in the client handshake, where ClientHello is
special-cased to not go through the buffer while everything else is. This dates
to OpenSSL's initial commit and doesn't seem to do much. I do not believe it
can affect renego as the buffer only affects writes; although OpenSSL accepted
interleave on read (though this logic predates it slightly), it never sent
application data while it believed a handshake was active. The handshake would
always be driven to completion first.
My guess is this was to save a copy since the ClientHello is a one-message
flight so it wouldn't need to be buffered? This is probably not worth the extra
variation in the state. (Especially with the DTLS state machine going through
ClientHello twice and pushing the BIO in between the two. Though I suspect that
was a mistake in itself. If the optimization guess is correct, there was no
need to do that.)
Change-Id: I6726f866e16ee7213cab0c3e6abb133981444d47
Reviewed-on: https://boringssl-review.googlesource.com/7873
Reviewed-by: Adam Langley <agl@google.com>
That we're ignoring the return value is clearly wrong when
dtls1_retransmit_message has other code that doesn't ignore it, by way of
dtls1_do_handshake_write.
Change-Id: Ie3f8c0defdf1f5e709d67af4ca6fa4f0d83c76c9
Reviewed-on: https://boringssl-review.googlesource.com/7872
Reviewed-by: Adam Langley <agl@google.com>
The DTLS bbio logic is rather problematic, but this shouldn't make things
worse. In the in-handshake case, the new code merges the per-message
(unchecked) BIO_flush calls into one call at the end but otherwise the BIO is
treated as is. Otherwise any behavior around non-block writes should be
preserved.
In the post-handshake case, we now install the buffer when we didn't
previously. On write error, the buffer will have garbage in it, but it will be
discarded, so that will preserve any existing retry behavior. (Arguably the
existing retry behavior is a bug, but that's another matter.)
Add a test for all this, otherwise it is sure to regress. Testing for
record-packing is a little fuzzy, but we can assert ChangeCipherSpec always
shares a record with something.
BUG=57
Change-Id: I8603f20811d502c71ded2943b0e72a8bdc4e46f2
Reviewed-on: https://boringssl-review.googlesource.com/7871
Reviewed-by: Adam Langley <agl@google.com>
The BORINGSSL_YYYYMM #defines have served well to coordinate short-term skews
in BoringSSL's public API, but some consumers (notably wpa_supplicant in
Android) wish to build against multiple versions for an extended period of
time. Consumers should not do this unless there is no alternative, but to
accommodate this, start a BORINGSSL_API_VERSION counter. In future, instead of
BORINGSSL_YYYYMM #defines, we'll simply increment the number.
This is specifically called an "API version" rather than a plain "version" as
this number does not denote any particular point in development or stability.
It purely counts how many times we found it convenient to let the preprocessor
observe a public API change up to now.
Change-Id: I39f9740ae8e793cef4c2b5fb5707b9763b3e55ce
Reviewed-on: https://boringssl-review.googlesource.com/7870
Reviewed-by: Adam Langley <agl@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>
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>
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 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>
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>
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>
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>
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>
Split gn and gyp generators apart. Since we're pre-generating files, there's no
need to make BoringSSL's build depend on the gypi_to_gn.py script. Also emit
the tests and a list of fuzzers so we don't need to manually update BUILD.gn
each time.
The new gn generator is based on the bazel one since they're fairly similar.
BUG=chromium:429246
Change-Id: I5a819a964d6ac6e56e9251bb3fd3de1db08214a4
Reviewed-on: https://boringssl-review.googlesource.com/7726
Reviewed-by: Adam Langley <agl@google.com>
Opaquifying SSL_SESSION is less important than the other structs, but this will
cause less turbulence in wpa_supplicant if we add this API too. Semantics and
name taken from OpenSSL 1.1.0 to match.
BUG=6
Change-Id: Ic39f58d74640fa19a60aafb434dd2c4cb43cdea9
Reviewed-on: https://boringssl-review.googlesource.com/7725
Reviewed-by: Adam Langley <agl@google.com>
Probably better to keep it out of the way for someone just trying to figure out
how to use the library. Notably, we don't really want people to think they need
to use the directioned init function.
Change-Id: Icacc2061071581abf46e38eb1d7a52e7b1f8361b
Reviewed-on: https://boringssl-review.googlesource.com/7724
Reviewed-by: Adam Langley <agl@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>
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>
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>
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>
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>
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>
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>
Although the server_name extension was intended to be extensible to new name
types, OpenSSL 1.0.x had a bug which meant different name types will cause an
error. Further, RFC 4366 originally defined syntax inextensibly. RFC 6066
corrected this mistake, but adding new name types is no longer feasible.
Act as if the extensibility does not exist to simplify parsing. This also
aligns with OpenSSL 1.1.x's behavior. See upstream's
062178678f5374b09f00d70796f6e692e8775aca and
https://www.ietf.org/mail-archive/web/tls/current/msg19425.html
Change-Id: I5af26516e8f777ddc1dab5581ff552daf2ea59b5
Reviewed-on: https://boringssl-review.googlesource.com/7294
Reviewed-by: David Benjamin <davidben@google.com>
We reset it to SSL_NOTHING at the start of ever SSL_get_error-using operation.
Then we only set it to a non-NOTHING value in the rest of the stack on error
paths.
Currently, ssl->rwstate is set all over the place. Sometimes the pattern is:
ssl->rwstate = SSL_WRITING;
if (BIO_write(...) <= 0) {
goto err;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we only set it to the non-NOTHING value on error.
if (BIO_write(...) <= 0) {
ssl->rwstate = SSL_WRITING;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we just set it to SSL_NOTHING far from any callback in random places.
The third case is arbitrary and clearly should be removed.
But, in the second case, we sometimes forget to undo it afterwards. This is
largely harmless since an error in the error queue overrides rwstate, but we
don't always put something in the error queue (falling back to
SSL_ERROR_SYSCALL for "I'm not sure why it failed. Perhaps it was one of your
callbacks? Check your errno equivalent."), but in that case a stray rwstate
value will cause it to be wrong.
We could fix the cases where we fail to set SSL_NOTHING on success cases, but
this doesn't account for there being multiple SSL_get_error operations. The
consumer may have an SSL_read and an SSL_write running concurrently. Instead,
it seems the best option is to lift the SSL_NOTHING reset to the operations and
set SSL_WRITING and friends as in the second case.
(Someday hopefully we can fix this to just be an enum that is internally
returned. It can convert to something stateful at the API layer.)
Change-Id: I54665ec066a64eb0e48a06e2fcd0d2681a42df7f
Reviewed-on: https://boringssl-review.googlesource.com/7453
Reviewed-by: David Benjamin <davidben@google.com>
The concern is if the peer denies our renegotiation attempt, but we will never
initiate renegotiation. We only support server-initiated renegotiation when we
are acting as the client.
(Strictly speaking, only the client ever initiates renegotiation. The server
sends a HelloRequest to ask the client to initiate it. But we forbid
application data interleave as soon as we see the HelloRequest, so we treat it
as part of the handshake.)
Change-Id: I1a625130de32a7227e4471f2f889255aba962ce4
Reviewed-on: https://boringssl-review.googlesource.com/7452
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is just kind of a silly thing to do. NSS doesn't allow them either. Fatal
alerts would kill the connection regardless and warning alerts are useless. We
previously stopped accepting fragmented alerts but still allowed them doubled
up.
This is in preparation for pulling the shared alert processing code between TLS
and DTLS out of read_bytes into some common place.
Change-Id: Idbef04e39ad135f9601f5686d41f54531981e0cf
Reviewed-on: https://boringssl-review.googlesource.com/7451
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@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>
With SSL_PRIVATE_KEY_METHOD, decryption can happen outside of BoringSSL. Rather than crash the process, it would be nicer if BoringSSL handled the error gracefully.
Change-Id: I3f24d066f7a329d41420b208a7e13c82ec966710
Reviewed-on: https://boringssl-review.googlesource.com/7683
Reviewed-by: David Benjamin <davidben@google.com>
Both the header-level and section-level documentation define curve25519 which
is a little odd.
Change-Id: I81aa2b74e8028d3cfd5635e1d3cfda402ba1ae38
Reviewed-on: https://boringssl-review.googlesource.com/7680
Reviewed-by: Adam Langley <agl@google.com>
This is needed by trousers. As with the PSS function, the version that
assumes SHA-1 is put into decrepit.
Change-Id: I153e8ea0150e48061b978384b600a7b990d21d03
Reviewed-on: https://boringssl-review.googlesource.com/7670
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 is the one piece of functionality I miss from the openssl tool -
the ability to see some basic information about the server cert.
Sample output:
==========
$ bssl client -connect www.google.com
Connecting to [2607:f8b0:4006:80d::1010]:443
Connected.
Version: TLSv1.2
Resumed session: no
Cipher: ECDHE-RSA-AES128-GCM-SHA256
ECDHE curve: P-256
Secure renegotiation: yes
Next protocol negotiated:
ALPN protocol:
Cert subject: /C=US/ST=California/L=Mountain View/O=Google Inc/CN=www.google.com
Cert issuer: /C=US/O=Google Inc/CN=Google Internet Authority G2
==========
Change-Id: I758682784752a616628138e420f52586d5a1bb31
Reviewed-on: https://boringssl-review.googlesource.com/7620
Reviewed-by: David Benjamin <davidben@google.com>
2016-04-07 03:22:19 +00:00
2225 changed files with 101015 additions and 29744 deletions
@@ -28,7 +28,20 @@ From the `build/` directory, you can then run the fuzzers. For example:
The arguments to `jobs` and `workers` should be the number of cores that you wish to dedicate to fuzzing. By default, libFuzzer uses the largest test in the corpus (or 64 if empty) as the maximum test case length. The `max_len` argument overrides this.
The recommended values of `max_len` for each test may be found in `.options` files alongside the test source. These were determined by rounding up the length of the largest case in the corpus. When writing a new fuzzer, configure `max_len` in a similar file.
The recommended values of `max_len` for each test are:
| Test | `max_len` value |
|---------------|-----------------|
| `cert` | 3072 |
| `client` | 20000 |
| `pkcs8` | 2048 |
| `privkey` | 2048 |
| `server` | 4096 |
| `spki` | 1024 |
| `read_pem` | 512 |
| `ssl_ctx_api` | 256 |
These were determined by rounding up the length of the largest case in the corpus.
There are directories in `fuzz/` for each of the fuzzing tests which contain seed files for fuzzing. Some of the seed files were generated manually but many of them are “interesting” results generated by the fuzzing itself. (Where “interesting” means that it triggered a previously unknown path in the code.)
@@ -40,12 +53,31 @@ In order to minimise all the corpuses, build for fuzzing and run `./fuzz/minimis
## Fuzzer mode
When `-DFUZZ=1` is passed into CMake, BoringSSL builds with `BORINGSSL_UNSAFE_FUZZER_MODE` defined. This modifies the library, particularly the TLS stack, to be more friendly to fuzzers. It will:
When `-DFUZZ=1` is passed into CMake, BoringSSL builds with `BORINGSSL_UNSAFE_FUZZER_MODE` and `BORINGSSL_UNSAFE_DETERMINISTIC_MODE` defined. This modifies the library to be more friendly to fuzzers. If `BORINGSSL_UNSAFE_DETERMINISTIC_MODE` is set, BoringSSL will:
* Replace `RAND_bytes` with a deterministic PRNG. Call `RAND_reset_for_fuzzing()` at the start of fuzzers which use `RAND_bytes` to reset the PRNG state.
* Use a hard-coded time instead of the actual time.
Additionally, if `BORINGSSL_UNSAFE_FUZZER_MODE` is set, BoringSSL will:
* Modify the TLS stack to perform all signature checks (CertificateVerify and ServerKeyExchange) and the Finished check, but always act as if the check succeeded.
* Treat every cipher as the NULL cipher.
* Tickets are unencrypted and the MAC check is performed but ignored.
This is to prevent the fuzzer from getting stuck at a cryptographic invariant in the protocol.
## TLS transcripts
The `client` and `server` corpora are seeded from the test suite. The test suite has a `-fuzzer` flag which mirrors the fuzzer mode changes above and a `-deterministic` flag which removes all non-determinism on the Go side. Not all tests pass, so `ssl/test/runner/fuzzer_mode.json` contains the necessary suppressions. The `run_tests` target will pass appropriate command-line flags.
There are separate corpora, `client_corpus_no_fuzzer_mode` and `server_corpus_no_fuzzer_mode`. These are transcripts for fuzzers with only `BORINGSSL_UNSAFE_DETERMINISTIC_MODE` defined. To build in this mode, pass `-DNO_FUZZER_MODE=1` into CMake. This configuration is run in the same way but without `-fuzzer` and `-shim-path` flags.
If both sets of tests pass, refresh the fuzzer corpora with `refresh_ssl_corpora.sh`:
`SSL_CTRL_MODE` | `SSL_CTX_get_mode` or `SSL_CTX_set_mode`
`SSL_CTRL_NEED_TMP_RSA` | `SSL_CTX_need_tmp_RSA` is equivalent, but [*do not use this function*](https://freakattack.com/). (It is a no-op in BoringSSL.)
`SSL_CTRL_OPTIONS` | `SSL_CTX_get_options` or `SSL_CTX_set_options`
`SSL_CTRL_SET_TMP_RSA` | `SSL_CTX_set_tmp_rsa` is equivalent, but [*do not use this function*](https://freakattack.com/). (It is a no-op in BoringSSL.)
`SSL_CTRL_SET_TMP_RSA_CB` | `SSL_CTX_set_tmp_rsa_callback` is equivalent, but [*do not use this function*](https://freakattack.com/). (It is a no-op in BoringSSL.)
@@ -25,6 +25,8 @@ There are other files in this directory which might be helpful:
* [PORTING.md](/PORTING.md): how to port OpenSSL-using code to BoringSSL.
* [BUILDING.md](/BUILDING.md): how to build BoringSSL
* [INCORPORATING.md](/INCORPORATING.md): how to incorporate BoringSSL into a project.
* [API-CONVENTIONS.md](/API-CONVENTIONS.md): general API conventions for BoringSSL consumers and developers.
* [STYLE.md](/STYLE.md): rules and guidelines for coding style.
* include/openssl: public headers with API documentation in comments. Also [available online](https://commondatastorage.googleapis.com/chromium-boringssl-docs/headers.html).
* [FUZZING.md](/FUZZING.md): information about fuzzing BoringSSL.
@@ -294,3 +294,9 @@ int main(int argc, char **argv) {
returnFileTestMain(TestCipher,nullptr,argv[1]);
}
}// namespace bssl
intmain(intargc,char**argv){
returnbssl::Main(argc,argv);
}
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
David Benjamin
Aurelien Regat-Barrel
Steven Valdez
Steven Valdez
Jinguang Dong
Adam Langley
Matthew Braithwaite
Kenneth Geisshirt
Ladar Levison
Adam Langley
Brian Smith
Alessandro Ghedini
Nico Weber
Nick Harper
Daniel Bathgate
Mike Aizatsky
Martin Kreichgauer
EKR
Eric Roman
Andrii Shyshkalov
Radu Margarint
Dan Willemsen
Steven Valdez
Julien Schmidt
Taylor Brandstetter
Piotr Sikora
nmittler
Tamas Berghammer
Taylor Brandstetter
Max Moroz
Gabriel Redner