(Of course, it's still signing ServerKeyExchange messages since the
handshake's the old one.)
Change-Id: I35844a329d983f61ed0b5be20b333487406fe7e4
Reviewed-on: https://boringssl-review.googlesource.com/8614
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>
That instead happens via signature algorithms, which will be done in a
follow-up commit.
Change-Id: I97bc4646319dddbff62552244b0dd7e9bb2650ef
Reviewed-on: https://boringssl-review.googlesource.com/8700
Reviewed-by: David Benjamin <davidben@google.com>
This is in preparation for TLS 1.3 enforcing curve matches in signature
algorithms.
Change-Id: I82c3a1862703a15e4e36ceb7ec40e27235b620c3
Reviewed-on: https://boringssl-review.googlesource.com/8699
Reviewed-by: David Benjamin <davidben@google.com>
{sha256,ecdsa} should not be silently accepted for an RSA key.
Change-Id: I0c0eea5071f7a59f2707ca0ea023a16cc4126d6a
Reviewed-on: https://boringssl-review.googlesource.com/8697
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>
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>
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
For when the PackHandshakeFlight tests get enabled.
Change-Id: Iee20fd27d88ed58f59af3b7e2dd92235d35af9ce
Reviewed-on: https://boringssl-review.googlesource.com/8663
Reviewed-by: David Benjamin <davidben@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>
Since they include an ECDHE exchange in them, they are equally-well
suited to False Start.
Change-Id: I75d31493a614a78ccbf337574c359271831d654d
Reviewed-on: https://boringssl-review.googlesource.com/8732
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>
TLS 1.3 will use a different function from processClientHello.
Change-Id: I8b26a601cf553834b508feab051927d5986091ca
Reviewed-on: https://boringssl-review.googlesource.com/8597
Reviewed-by: David Benjamin <davidben@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>
Also move them with the other version negotiation tests.
Change-Id: I8ea5777c131f8ab618de3c6d02038e802bd34dd0
Reviewed-on: https://boringssl-review.googlesource.com/8550
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.2 and 1.3 will both need to call it at different points.
Change-Id: Id62ec289213aa6c06ebe5fe65a57ca6c2b53d538
Reviewed-on: https://boringssl-review.googlesource.com/8600
Reviewed-by: David Benjamin <davidben@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. Test added by davidben.]
Test vectors taken from hkdf_test.c.
Change-Id: I214bcae325e9c7c242632a169ab5cf80a3178989
Reviewed-on: https://boringssl-review.googlesource.com/8587
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>
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>
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>
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>
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>
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>
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>
This unifies a bunch of tests and also adds a few missing ones.
Change-Id: I91652bd010da6cdb62168ce0a3415737127e1577
Reviewed-on: https://boringssl-review.googlesource.com/8360
Reviewed-by: Nick Harper <nharper@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
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>
This is not very satisfactory.
Change-Id: I7e7a86f921e66f8f830c72eac084e9fea5ffd4d9
Reviewed-on: https://boringssl-review.googlesource.com/8270
Reviewed-by: David Benjamin <davidben@google.com>
By corrupting the X25519 and Newhope parts separately, the test shows
that both are in use. Possibly excessive?
Change-Id: Ieb10f46f8ba876faacdafe70c5561c50a5863153
Reviewed-on: https://boringssl-review.googlesource.com/8250
Reviewed-by: Adam Langley <agl@google.com>
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>
Change-Id: I0aaf9d926a81c3a10e70ae3ae6605d4643419f89
Reviewed-on: https://boringssl-review.googlesource.com/8210
Reviewed-by: Taylor Brandstetter <deadbeef@google.com>
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>
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>
-timeout collides with go test's flags.
Change-Id: Icfc954915a61f1bb4d0acc8f02ec8a482ea10158
Reviewed-on: https://boringssl-review.googlesource.com/8188
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 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>
This is easier to deploy, and more obvious. This commit reverts a few
pieces of e25775bc, but keeps most of it.
Change-Id: If8d657a4221c665349c06041bb12fffca1527a2c
Reviewed-on: https://boringssl-review.googlesource.com/8061
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>
GetConfigPtr was a silly name. GetTestConfig matches the type and GetTestState.
Change-Id: I9998437a7be35dbdaab6e460954acf1b95375de0
Reviewed-on: https://boringssl-review.googlesource.com/8024
Reviewed-by: Adam Langley <agl@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>
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>
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>
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>
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>
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>
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>
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>
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>
A lot of consumers of obj.h only want the NID values. Others didn't need
it at all. This also removes some OBJ_nid2sn and OBJ_nid2ln calls in EVP
error paths which isn't worth pulling a large table in for.
BUG=chromium:499653
Change-Id: Id6dff578f993012e35b740a13b8e4f9c2edc0744
Reviewed-on: https://boringssl-review.googlesource.com/7563
Reviewed-by: David Benjamin <davidben@google.com>
These only affect the tests.
Change-Id: If22d047dc98023501c771787b485276ece92d4a2
Reviewed-on: https://boringssl-review.googlesource.com/7573
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Otherwise it's confusing if you mistype the test name.
Change-Id: Idf32081958f85f3b5aeb8993a07f6975c27644f8
Reviewed-on: https://boringssl-review.googlesource.com/7500
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Align all unexpected messages on SSL_R_UNEXPECTED_MESSAGE. Make the SSL 3.0
case the exceptional case. In doing so, make sure the SSL 3.0
SSL_VERIFY_FAIL_IF_NO_PEER_CERT case has its own test as that's a different
handshake shape.
Change-Id: I1a539165093fbdf33e2c1b25142f058aa1a71d83
Reviewed-on: https://boringssl-review.googlesource.com/7421
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If we're doing substring matching, we should at least include the delimiter.
Change-Id: I98bee568140d0304bbb6a2788333dbfca044114c
Reviewed-on: https://boringssl-review.googlesource.com/7420
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In TLS, you never skip the Certificate message. It may be empty, but its
presence is determined by CertificateRequest. (This is sensible.)
In SSL 3.0, the client omits the Certificate message. This means you need to
probe and may receive either Certificate or ClientKeyExchange (thankfully,
ClientKeyExchange is not optional, or we'd have to probe at ChangeCipherSpec).
We didn't have test coverage for this, despite some of this logic being a
little subtle asynchronously. Fix this.
Change-Id: I149490ae5506f02fa0136cb41f8fea381637bf45
Reviewed-on: https://boringssl-review.googlesource.com/7419
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also add no-certificate cases to the state machine coverage tests.
Change-Id: I88a80df6f3ea69aabc978dd356abcb9e309e156f
Reviewed-on: https://boringssl-review.googlesource.com/7417
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If a Read or Write blocks for too long, time out the operation. Otherwise, some
kinds of test failures result in hangs, which prevent the test harness from
progressing. (Notably, OpenSSL currently has a lot of those failure modes and
upstream expressed interest in being able to run the tests to completion.)
Go's APIs want you to send an absolute timeout, to avoid problems when a Read
is split into lots of little Reads. But we actively want the timer to reset in
that case, so this needs a trivial adapter.
The default timeout is set at 15 seconds for now. If this becomes a problem, we
can extend it or build a more robust deadlock detector given an out-of-band
channel (shim tells runner when it's waiting on data, abort if we're also
waiting on data at the same time). But I don't think we'll need that
complexity. 15 seconds appears fine for both valgrind and running tests on a
Nexus 4.
BUG=460189
Change-Id: I6463fd36058427d883b526044da1bbefba851785
Reviewed-on: https://boringssl-review.googlesource.com/7380
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
I went with NID_x25519 to match NID_sha1 and friends in being lowercase.
However, upstream seems to have since chosen NID_X25519. Match their
name.
Change-Id: Icc7b183a2e2dfbe42c88e08e538fcbd242478ac3
Reviewed-on: https://boringssl-review.googlesource.com/7331
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I9540c931b6cdd4d65fa9ebfc52e1770d2174abd2
Reviewed-on: https://boringssl-review.googlesource.com/7330
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This can be used to get some initial corpus for fuzzing.
Change-Id: Ifcd365995b54d202c4a2674f49e7b28515f36025
Reviewed-on: https://boringssl-review.googlesource.com/7289
Reviewed-by: Adam Langley <agl@google.com>
It's useful to make sure our fuzzer mode works. Not all tests pass, but most
do. (Notably the negative tests for everything we've disabled don't work.) We
can also use then use runner to record fuzzer-mode transcripts with the ciphers
correctly nulled.
Change-Id: Ie41230d654970ce6cf612c0a9d3adf01005522c6
Reviewed-on: https://boringssl-review.googlesource.com/7288
Reviewed-by: Adam Langley <agl@google.com>
Both sides' signature and Finished checks still occur, but the results
are ignored. Also, all ciphers behave like the NULL cipher.
Conveniently, this isn't that much code since all ciphers and their size
computations funnel into SSL_AEAD_CTX.
This does carry some risk that we'll mess up this code. Up until now, we've
tried to avoid test-only changes to the SSL stack.
There is little risk that anyone will ship a BORINGSSL_UNSAFE_FUZZER_MODE build
for anything since it doesn't interop anyway. There is some risk that we'll end
up messing up the disableable checks. However, both skipped checks have
negative tests in runner (see tests that set InvalidSKXSignature and
BadFinished). For good measure, I've added a server variant of the existing
BadFinished test to this CL, although they hit the same code.
Change-Id: I37f6b4d62b43bc08fab7411965589b423d86f4b8
Reviewed-on: https://boringssl-review.googlesource.com/7287
Reviewed-by: Adam Langley <agl@google.com>
Found by libFuzzer combined with some experimental unsafe-fuzzer-mode patches
(to be uploaded once I've cleaned them up a bit) to disable all those pesky
cryptographic checks in the protocol.
Change-Id: I9153164fa56a0c2262c4740a3236c2b49a596b1b
Reviewed-on: https://boringssl-review.googlesource.com/7282
Reviewed-by: Adam Langley <agl@google.com>
If LeakSanitizer fires something on a test that's expected to fail, runner will
swallow it. Have stderr output always end in a "--- DONE ---" marker and treat
all output following that as a test failure.
Change-Id: Ia8fd9dfcaf48dd23972ab8f906d240bcb6badfe2
Reviewed-on: https://boringssl-review.googlesource.com/7281
Reviewed-by: Adam Langley <agl@google.com>
Otherwise it still thinks this is an RFC 5114 prime and kicks in the (now
incorrect) validity check.
Change-Id: Ie78514211927f1f2d2549958621cb7896f68b5ce
Reviewed-on: https://boringssl-review.googlesource.com/7050
Reviewed-by: Adam Langley <agl@google.com>
Take the mappings for MD5 and SHA-224 values out of the code altogether. This
aligns with the current TLS 1.3 draft.
For MD5, this is a no-op. It is not currently possible to configure accepted
signature algorithms, MD5 wasn't in the hardcoded list, and we already had a
test ensuring we enforced our preferences correctly. MD5 also wasn't in the
default list of hashes our keys could sign and no one overrides it with a
different hash.
For SHA-224, this is not quite a no-op. The hardcoded accepted signature
algorithms list included SHA-224, so this will break servers relying on that.
However, Chrome's metrics have zero data points of servers picking SHA-224 and
no other major browser includes it. Thus that should be safe.
SHA-224 was also in the default list of hashes we are willing to sign. For
client certificates, Chromium's abstractions already did not allow signing
SHA-224, so this is a no-op there. For servers, this will break any clients
which only accept SHA-224. But no major browsers do this and I am not aware of
any client implementation which does such ridiculous thing.
(SHA-1's still in there. Getting rid of that one is going to take more effort.)
Change-Id: I6a765fdeea9e19348e409d58a0eac770b318e599
Reviewed-on: https://boringssl-review.googlesource.com/7020
Reviewed-by: Adam Langley <agl@google.com>
For TLS, this machinery only exists to swallow no_certificate alerts
which only get sent in an SSL 3.0 codepath anyway. It's much less a
no-op for SSL 3.0 which, strictly speaking, has only a subset of TLS's
alerts.
This gets messy around version negotiation because of the complex
relationship between enc_method, have_version, and version which all get
set at different times. Given that SSL 3.0 is nearly dead and all these
alerts are fatal to the connection anyway, this doesn't seem worth
carrying around. (It doesn't work very well anyway. An SSLv3-only server
may still send a record_overflow alert before version negotiation.)
This removes the last place enc_method is accessed prior to version
negotiation.
Change-Id: I79a704259fca69e4df76bd5a6846c9373f46f5a9
Reviewed-on: https://boringssl-review.googlesource.com/6843
Reviewed-by: Adam Langley <alangley@gmail.com>
We haven't had problems with this, but make sure it stays that way.
Bogus signature algorithms are already covered.
Change-Id: I085350d89d79741dba3f30fc7c9f92de16bf242a
Reviewed-on: https://boringssl-review.googlesource.com/6910
Reviewed-by: David Benjamin <davidben@google.com>
This is a minor regression from
https://boringssl-review.googlesource.com/5235.
If the client, for whatever reason, had an ID-based session but also
supports tickets, it will send non-empty ID + empty ticket extension.
If the ticket extension is non-empty, then the ID is not an ID but a
dummy signaling value, so 5235 avoided looking it up. But if it is
present and empty, the ID is still an ID and should be looked up.
This shouldn't have any practical consequences, except if a server
switched from not supporting tickets and then started supporting it,
while keeping the session cache fixed.
Add a test for this case, and tighten up existing ID vs ticket tests so
they fail if we resume with the wrong type.
Change-Id: Id4d08cd809af00af30a2b67fe3a971078e404c75
Reviewed-on: https://boringssl-review.googlesource.com/6554
Reviewed-by: Adam Langley <alangley@gmail.com>
We don't actually have an API to let you know if the value is legal to
interpret as a curve ID. (This was kind of a poor API. Oh well.) Also add tests
for key_exchange_info. I've intentionally left server-side plain RSA missing
for now because the SSL_PRIVATE_KEY_METHOD abstraction only gives you bytes and
it's probably better to tweak this API instead.
(key_exchange_info also wasn't populated on the server, though due to a
rebasing error, that fix ended up in the parent CL. Oh well.)
Change-Id: I74a322c8ad03f25b02059da7568c9e1a78419069
Reviewed-on: https://boringssl-review.googlesource.com/6783
Reviewed-by: Adam Langley <agl@google.com>
The new curve is not enabled by default.
As EC_GROUP/EC_POINT is a bit too complex for X25519, this introduces an
SSL_ECDH_METHOD abstraction which wraps just the raw ECDH operation. It
also tidies up some of the curve code which kept converting back and
force between NIDs and curve IDs. Now everything transits as curve IDs
except for API entry points (SSL_set1_curves) which take NIDs. Those
convert immediately and act on curve IDs from then on.
Note that, like the Go implementation, this slightly tweaks the order of
operations. The client sees the server public key before sending its
own. To keep the abstraction simple, SSL_ECDH_METHOD expects to
generate a keypair before consuming the peer's public key. Instead, the
client handshake stashes the serialized peer public value and defers
parsing it until it comes time to send ClientKeyExchange. (This is
analogous to what it was doing before where it stashed the parsed peer
public value instead.)
It still uses TLS 1.2 terminology everywhere, but this abstraction should also
be compatible with TLS 1.3 which unifies (EC)DH-style key exchanges.
(Accordingly, this abstraction intentionally does not handle parsing the
ClientKeyExchange/ServerKeyExchange framing or attempt to handle asynchronous
plain RSA or the authentication bits.)
BUG=571231
Change-Id: Iba09dddee5bcdfeb2b70185308e8ab0632717932
Reviewed-on: https://boringssl-review.googlesource.com/6780
Reviewed-by: Adam Langley <agl@google.com>
This injects an interface to abstract between elliptic.Curve and a
byte-oriented curve25519. The C implementation will follow a similar
strategy.
Note that this slightly tweaks the order of operations. The client sees
the server public key before sending its own. To keep the abstraction
simple, ecdhCurve expects to generate a keypair before consuming the
peer's public key. Instead, the client handshake stashes the serialized
peer public value and defers parsing it until it comes time to send
ClientKeyExchange. (This is analogous to what it was doing before where
it stashed the parsed peer public value instead.)
BUG=571231
Change-Id: I771bb9aee0dd6903d395c84ec4f2dd7b3e366c75
Reviewed-on: https://boringssl-review.googlesource.com/6777
Reviewed-by: Adam Langley <agl@google.com>
Hopefully this can be replaced with a standard library version later.
BUG=571231
Change-Id: I61ae1d9d057c6d9e1b92128042109758beccc7ff
Reviewed-on: https://boringssl-review.googlesource.com/6776
Reviewed-by: Adam Langley <agl@google.com>
We don't live in a workspace, but relative import paths exist, so we
don't have to modify the modules we bundle to avoid naming collisions.
Change-Id: Ie7c70dbc4bb0485421814d40b6a6bd5f140e1d29
Reviewed-on: https://boringssl-review.googlesource.com/6781
Reviewed-by: Adam Langley <agl@google.com>
It already wasn't in the default list and no one enables it. Remove it
altogether. (It's also gone from the current TLS 1.3 draft.)
Change-Id: I143d07d390d186252204df6bdb8ffd22649f80e3
Reviewed-on: https://boringssl-review.googlesource.com/6775
Reviewed-by: Adam Langley <agl@google.com>
Apple these days ships lldb without gdb. Teach runner how to launch it
too.
Change-Id: I25f845f84f1c87872a9e3bc4b7fe3e7344e8c1f7
Reviewed-on: https://boringssl-review.googlesource.com/6769
Reviewed-by: Adam Langley <agl@google.com>
Only ECDHE-based ciphers are implemented. To ease the transition, the
pre-standard cipher shares a name with the standard one. The cipher rule parser
is hacked up to match the name to both ciphers. From the perspective of the
cipher suite configuration language, there is only one cipher.
This does mean it is impossible to disable the old variant without a code
change, but this situation will be very short-lived, so this is fine.
Also take this opportunity to make the CK and TXT names align with convention.
Change-Id: Ie819819c55bce8ff58e533f1dbc8bef5af955c21
Reviewed-on: https://boringssl-review.googlesource.com/6686
Reviewed-by: Adam Langley <agl@google.com>
This will be used to test the C implementation against.
Change-Id: I2d396d27630937ea610144e381518eae76f78dab
Reviewed-on: https://boringssl-review.googlesource.com/6685
Reviewed-by: Adam Langley <agl@google.com>
In preparation for a Go implementation of the new TLS ciphers to test
against, implement the AEAD primitive.
Change-Id: I69b5b51257c3de16bdd36912ed2bc9d91ac853c8
Reviewed-on: https://boringssl-review.googlesource.com/6684
Reviewed-by: Adam Langley <agl@google.com>
In preparation for implementing the RFC 7539 variant to test against.
Change-Id: I0ce5e856906e00925ad1d849017f9e7fda087a8e
Reviewed-on: https://boringssl-review.googlesource.com/6683
Reviewed-by: Adam Langley <agl@google.com>
This uses ssl3_read_bytes for now. We still need to dismantle that
function and then invert the handshake state machine, but this gets
things closer to the right shape as an intermediate step and is a large
chunk in itself. It simplifies a lot of the CCS/handshake
synchronization as a lot of the invariants much more clearly follow from
the handshake itself.
Tests need to be adjusted since this changes some error codes. Now all
the CCS/Handshake checks fall through to the usual
SSL_R_UNEXPECTED_RECORD codepath. Most of what used to be a special-case
falls out naturally. (If half of Finished was in the same record as the
pre-CCS message, that part of the handshake record would have been left
unconsumed, so read_change_cipher_spec would have noticed, just like
read_app_data would have noticed.)
Change-Id: I15c7501afe523d5062f0e24a3b65f053008d87be
Reviewed-on: https://boringssl-review.googlesource.com/6642
Reviewed-by: Adam Langley <agl@google.com>
Sometimes BadRSAClientKeyExchange-1 fails with DATA_TOO_LARGE_FOR_MODULUS if
the corruption brings the ciphertext above the RSA modulus. Ensure this does
not happen.
Change-Id: I0d8ea6887dfcab946fdf5d38f5b196f5a927c4a9
Reviewed-on: https://boringssl-review.googlesource.com/6731
Reviewed-by: Adam Langley <agl@google.com>
Cover not just the wrong version, but also other mistakes.
Change-Id: I46f05a9a37b7e325adc19084d315a415777d3a46
Reviewed-on: https://boringssl-review.googlesource.com/6610
Reviewed-by: Adam Langley <agl@google.com>
I don't think we're ever going to manage to enforce this, and it doesn't
seem worth the trouble. We don't support application protocols which use
renegotiation outside of the HTTP/1.1 mid-stream client auth hack.
There, it's on the server to reject legacy renegotiations.
This removes the last of SSL_OP_ALL.
Change-Id: I996fdeaabf175b6facb4f687436549c0d3bb0042
Reviewed-on: https://boringssl-review.googlesource.com/6580
Reviewed-by: Adam Langley <agl@google.com>
RFC 5746 forbids a server from downgrading or upgrading
renegotiation_info support. Even with SSL_OP_LEGACY_SERVER_CONNECT set
(the default), we can still enforce a few things.
I do not believe this has practical consequences. The attack variant
where the server half is prefixed does not involve a renegotiation on
the client. The converse where the client sees the renegotiation and
prefix does, but we only support renego for the mid-stream HTTP/1.1
client auth hack, which doesn't do this. (And with triple-handshake,
HTTPS clients should be requiring the certificate be unchanged across
renego which makes this moot.)
Ultimately, an application which makes the mistake of using
renegotiation needs to be aware of what exactly that means and how to
handle connection state changing mid-stream. We make renego opt-in now,
so this is a tenable requirement.
(Also the legacy -> secure direction would have been caught by the
server anyway since we send a non-empty RI extension.)
Change-Id: I915965c342f8a9cf3a4b6b32f0a87a00c3df3559
Reviewed-on: https://boringssl-review.googlesource.com/6559
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.8.0 (or earlier). The use counter sees virtually
no hits.
Change-Id: Iff4c8899d5cb0ba4afca113c66d15f1d980ffe41
Reviewed-on: https://boringssl-review.googlesource.com/6558
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.9.0. The Internet seems to have completely
forgotten what "D5" is. (I can't find reference to it beyond
documentation of this quirk.) The use counter we added sees virtually no
hits.
Change-Id: I9781d401acb98ce3790b1b165fc257a6f5e9b155
Reviewed-on: https://boringssl-review.googlesource.com/6557
Reviewed-by: Adam Langley <agl@google.com>
At least for newlib (Native Client) including sys/types.h
is not enough to get a timeval declaration.
Change-Id: I4971a1aacc80b6fdc12c0e81c5d8007ed13eb8b7
Reviewed-on: https://boringssl-review.googlesource.com/6722
Reviewed-by: Adam Langley <agl@google.com>
yaSSL has a couple of bugs in their DH client implementation. This
change works around the worst of the two.
Firstly, they expect the the DH public value to be the same length as
the prime. This change pads the public value as needed to ensure this.
Secondly, although they handle the first byte of the shared key being
zero, they don't handle the case of the second, third, etc bytes being
zero. So whenever that happens the handshake fails. I don't think that
there's anything that we can do about that one.
Change-Id: I789c9e5739f19449473305d59fe5c3fb9b4a6167
Reviewed-on: https://boringssl-review.googlesource.com/6578
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Right now your options are:
- Bounce on a reference and deal with cleanup needlessly.
- Manually check the type tag and peek into the union.
We probably have no hope of opaquifying this struct, but for new code, let's
recommend using this function rather than the more error-prone thing.
Change-Id: I9b39ff95fe4264a3f7d1e0d2894db337aa968f6c
Reviewed-on: https://boringssl-review.googlesource.com/6551
Reviewed-by: Adam Langley <agl@google.com>
Chromium's toolchains may now assume C++11 library support, so we may freely
use C++11 features. (Chromium's still in the process of deciding what to allow,
but we use Google's style guide directly, toolchain limitations aside.)
Change-Id: I1c7feb92b7f5f51d9091a4c686649fb574ac138d
Reviewed-on: https://boringssl-review.googlesource.com/6465
Reviewed-by: Adam Langley <agl@google.com>
dh.c had a 10k-bit limit but it wasn't quite correctly enforced. However,
that's still 1.12s of jank on the IO thread, which is too long. Since the SSL
code consumes DHE groups from the network, it should be responsible for
enforcing what sanity it needs on them.
Costs of various bit lengths on 2013 Macbook Air:
1024 - 1.4ms
2048 - 14ms
3072 - 24ms
4096 - 55ms
5000 - 160ms
10000 - 1.12s
UMA says that DHE groups are 0.2% 4096-bit and otherwise are 5.5% 2048-bit and
94% 1024-bit and some noise. Set the limit to 4096-bit to be conservative,
although that's already quite a lot of jank.
BUG=554295
Change-Id: I8e167748a67e4e1adfb62d73dfff094abfa7d215
Reviewed-on: https://boringssl-review.googlesource.com/6464
Reviewed-by: Adam Langley <agl@google.com>
The current check has two problems:
- It only runs on the server, where there isn't a curve list at all. This was a
mistake in https://boringssl-review.googlesource.com/1843 which flipped it
from client-only to server-only.
- It only runs in TLS 1.2, so one could bypass it by just negotiating TLS 1.1.
Upstream added it as part of their Suite B mode, which requires 1.2.
Move it elsewhere. Though we do not check the entire chain, leaving that to the
certificate verifier, signatures made by the leaf certificate are made by the
SSL/TLS stack, so it's reasonable to check the curve as part of checking
suitability of a leaf.
Change-Id: I7c12f2a32ba946a20e9ba6c70eff23bebcb60bb2
Reviewed-on: https://boringssl-review.googlesource.com/6414
Reviewed-by: Adam Langley <agl@google.com>
This exposes the ServerKeyExchange signature hash type used in the most recent
handshake, for histogramming on the client.
BUG=549662
Change-Id: I8a4e00ac735b1ecd2c2df824112c3a0bc62332a7
Reviewed-on: https://boringssl-review.googlesource.com/6413
Reviewed-by: Adam Langley <agl@google.com>
This fixes a number of bugs with the original logic:
- If handshake messages are fragmented and writes need to be retried, frag_off
gets completely confused.
- The BIO_flush call didn't set rwstate, so it wasn't resumable at that point.
- The msg_callback call gets garbage because the fragment header would get
scribbled over the handshake buffer.
The original logic was also extremely confusing with how it handles init_off.
(init_off gets rewound to make room for the fragment header. Depending on
where you pause, resuming may or may not have already been rewound.)
For simplicity, just allocate a new buffer to assemble the fragment in and
avoid clobbering the old one. I don't think it's worth the complexity to
optimize that. If we want to optimize this sort of thing, not clobbering seems
better anyway because the message may need to be retransmitted. We could avoid
doing a copy when buffering the outgoing message for retransmission later.
We do still need to track how far we are in sending the current message via
init_off, so I haven't opted to disconnect this function from
init_{buf,off,num} yet.
Test the fix to the retry + fragment case by having the splitHandshake option
to the state machine tests, in DTLS, also clamp the MTU to force handshake
fragmentation.
Change-Id: I66f634d6c752ea63649db8ed2f898f9cc2b13908
Reviewed-on: https://boringssl-review.googlesource.com/6421
Reviewed-by: Adam Langley <agl@google.com>
This option causes clients to ignore HelloRequest messages completely.
This can be suitable in cases where a server tries to perform concurrent
application data and handshake flow, e.g. because they are trying to
“renew” symmetric keys.
Change-Id: I2779f7eff30d82163f2c34a625ec91dc34fab548
Reviewed-on: https://boringssl-review.googlesource.com/6431
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Although the DTLS transport layer logic drops failed writes on the floor, it is
actually set up to work correctly. If an SSL_write fails at the transport,
dropping the buffer is fine. Arguably it works better than in TLS because we
don't have the weird "half-committed to data" behavior. Likewise, the handshake
keeps track of how far its gotten and resumes the message at the right point.
This broke when the buffering logic was rewritten because I didn't understand
what the DTLS code was doing. The one thing that doesn't work as one might
expect is non-fatal write errors during rexmit are not recoverable. The next
timeout must fire before we try again.
This code is quite badly sprinkled in here, so add tests to guard it against
future turbulence. Because of the rexmit issues, the tests need some hacks
around calls which may trigger them. It also changes the Go DTLS implementation
from being completely strict about sequence numbers to only requiring they be
monotonic.
The tests also revealed another bug. This one seems to be upstream's fault, not
mine. The logic to reset the handshake hash on the second ClientHello (in the
HelloVerifyRequest case) was a little overenthusiastic and breaks if the
ClientHello took multiple tries to send.
Change-Id: I9b38b93fff7ae62faf8e36c4beaf848850b3f4b9
Reviewed-on: https://boringssl-review.googlesource.com/6417
Reviewed-by: Adam Langley <agl@google.com>
It's somewhat annoying to have to parse out the packetAdaptor mini-language.
Actually seeing those is only useful when debugging the adaptor itself, rather
than DTLS. Switch the order of the two middleware bits and add an escape hatch
to log the funny opcodes.
Change-Id: I249c45928a76b747d69f3ab972ea4d31e0680a62
Reviewed-on: https://boringssl-review.googlesource.com/6416
Reviewed-by: Adam Langley <agl@google.com>
Right whether NPN is advertised can only be configured globally on the SSL_CTX.
Rather than adding two pointers to each SSL*, add an options bit to disable it
so we may plumb in a field trial to disable NPN.
Chromium wants to be able to route a bit in to disable NPN, but it uses SSL_CTX
incorrectly and has a global one, so it can't disconnect the callback. (That
really needs to get fixed. Although it's not clear this necessarily wants to be
lifted up to SSL_CTX as far as Chromium's SSLClientSocket is concerned since
NPN doesn't interact with the session cache.)
BUG=526713
Change-Id: I49c86828b963eb341c6ea6a442557b7dfa190ed3
Reviewed-on: https://boringssl-review.googlesource.com/6351
Reviewed-by: Adam Langley <alangley@gmail.com>
It just calls CRYPTO_library_init and doesn't do anything else. If
anything, I'd like to make CRYPTO_library_init completely go away too.
We have CRYPTO_once now, so I think it's safe to assume that, if ssl/
ever grows initialization needs beyond that of crypto/, we can hide it
behind a CRYPTO_once and not burden callers.
Change-Id: I63dc362e0e9e98deec5516f4620d1672151a91b6
Reviewed-on: https://boringssl-review.googlesource.com/6311
Reviewed-by: Adam Langley <alangley@gmail.com>
RFC 5077 explicitly allows the server to change its mind and send no
ticket by sending an empty NewSessionTicket. See also upstream's
21b538d616b388fa0ce64ef54da3504253895cf8.
CBS_stow handles this case somewhat, so we won't get confused about
malloc(0) as upstream did. But we'll still fill in a bogus SHA-256
session ID, cache the session, and send a ClientHello with bogus session
ID but empty ticket extension. (The session ID field changes meaning
significantly when the ticket is or isn't empty. Non-empty means "ignore
the session ID, but echo if it resuming" while empty means "I support
tickets, but am offering this session ID".
The other behavior change is that a server which changes its mind on a
resumption handshake will no longer override the client's session cache
with a ticket-less session.
(This is kind of silly. Given that we don't get completely confused due
to CBS_stow, it might not be worth bothering with the rest. Mostly it
bugged me that we send an indicator session ID with no ticket.)
Change-Id: Id6b5bde1fe51aa3e1f453a948e59bfd1e2502db6
Reviewed-on: https://boringssl-review.googlesource.com/6340
Reviewed-by: Adam Langley <alangley@gmail.com>
Also added a SSL_CTX_set_select_certificate_cb setter for
select_certificate_cb so code needn't access SSL_CTX directly. Plus it
serves as a convenient anchor for the documentation.
Change-Id: I23755b910e1d77d4bea7bb9103961181dd3c5efe
Reviewed-on: https://boringssl-review.googlesource.com/6291
Reviewed-by: Adam Langley <alangley@gmail.com>
Add a slightly richer API. Notably, one can configure ssl_renegotiate_once to
only accept the first renego.
Also, this API doesn't repeat the mistake I made with
SSL_set_reject_peer_renegotiations which is super-confusing with the negation.
Change-Id: I7eb5d534e3e6c553b641793f4677fe5a56451c71
Reviewed-on: https://boringssl-review.googlesource.com/6221
Reviewed-by: Adam Langley <agl@google.com>
SSL 3.0 used to have a nice and simple rule around extensions. They don't
exist. And then RFC 5746 came along and made this all extremely confusing.
In an SSL 3.0 server, rather than blocking ServerHello extension
emission when renegotiation_info is missing, ignore all ClientHello
extensions but renegotiation_info. This avoids a mismatch between local
state and the extensions with emit.
Notably if, for some reason, a ClientHello includes the session_ticket
extension, does NOT include renegotiation_info or the SCSV, and yet the
client or server are decrepit enough to negotiate SSL 3.0, the
connection will fail due to unexpected NewSessionTicket message.
See https://crbug.com/425979#c9 for a discussion of something similar
that came up in diagnosing https://poodle.io/'s buggy POODLE check.
This is analogous to upstream's
5a3d8eebb7667b32af0ccc3f12f314df6809d32d.
(Not supporting renego as a server in any form anyway, we may as well
completely ignore extensions, but then our extensions callbacks can't
assume the parse hooks are always called. This way the various NULL
handlers still function.)
Change-Id: Ie689a0e9ffb0369ef7a20ab4231005e87f32d5f8
Reviewed-on: https://boringssl-review.googlesource.com/6180
Reviewed-by: Adam Langley <agl@google.com>
This change makes the runner tests (in ssl/test/runner) act like a
normal Go test rather than being a Go binary. This better aligns with
some internal tools.
Thus, from this point onwards, one has to run the runner tests with `go
test` rather than `go run` or `go build && ./runner`.
This will break the bots.
Change-Id: Idd72c31e8e0c2b7ed9939dacd3b801dbd31710dd
Reviewed-on: https://boringssl-review.googlesource.com/6009
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Allow configuring digest preferences for the private key. Some
smartcards have limited support for signing digests, notably Windows
CAPI keys and old Estonian smartcards. Chromium used the supports_digest
hook in SSL_PRIVATE_KEY_METHOD to limit such keys to SHA1. However,
detecting those keys was a heuristic, so some SHA256-capable keys
authenticating to SHA256-only servers regressed in the switch to
BoringSSL. Replace this mechanism with an API to configure digest
preference order. This way heuristically-detected SHA1-only keys may be
configured by Chromium as SHA1-preferring rather than SHA1-requiring.
In doing so, clean up the shared_sigalgs machinery somewhat.
BUG=468076
Change-Id: I996a2df213ae4d8b4062f0ab85b15262ca26f3c6
Reviewed-on: https://boringssl-review.googlesource.com/5755
Reviewed-by: Adam Langley <agl@google.com>
SCT and OCSP are part of the session data and as such shouldn't be sent
again to the client when resuming.
Change-Id: Iaee3a3c4c167ea34b91504929e38aadee37da572
Reviewed-on: https://boringssl-review.googlesource.com/5900
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Last set of changes didn't do that.
Change-Id: Iae24e75103529ce4d50099c5cbfbcef0e10ba663
Reviewed-on: https://boringssl-review.googlesource.com/5871
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifa44fef160fc9d67771eed165f8fc277f28a0222
Reviewed-on: https://boringssl-review.googlesource.com/5840
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The size of the stack caused by this object is problematic for systems
that have smaller stacks because they expect many threads.
Change-Id: Ib8f03741f9dd96bf474126f001947f879e50a781
Reviewed-on: https://boringssl-review.googlesource.com/5831
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Applications may require the stapled OCSP response in order to verify
the certificate within the verification callback.
Change-Id: I8002e527f90c3ce7b6a66e3203c0a68371aac5ec
Reviewed-on: https://boringssl-review.googlesource.com/5730
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This change adds the ability to configure ciphers specifically for
TLS ≥ 1.0. This compliments the existing ability to specify ciphers
for TLS ≥ 1.1.
This is useful because TLS 1.0 is the first version not to suffer from
POODLE. (Assuming that it's implemented correctly[1].) Thus one might
wish to reserve RC4 solely for SSLv3.
[1] https://www.imperialviolet.org/2014/12/08/poodleagain.html
Change-Id: I774d5336fead48f03d8a0a3cf80c369692ee60df
Reviewed-on: https://boringssl-review.googlesource.com/5793
Reviewed-by: Adam Langley <agl@google.com>
setup_key_block is called when the first CCS resolves, but for resumptions this
is the incoming CCS (see ssl3_do_change_cipher_spec). Rather than set
need_record_splitting there, it should be set in the write case of
tls1_change_cipher_state.
This fixes a crash from the new record layer code in resumption when
record-splitting is enabled. Tweak the record-splitting tests to cover this
case.
This also fixes a bug where renego from a cipher which does require record
splitting to one which doesn't continues splitting. Since version switches are
not allowed, this can only happen after a renego from CBC to RC4.
Change-Id: Ie4e1b91282b10f13887b51d1199f76be4fbf09ad
Reviewed-on: https://boringssl-review.googlesource.com/5787
Reviewed-by: Adam Langley <agl@google.com>
Note that DTLS treats oversized ciphertexts different from everything else.
Change-Id: I71cba69ebce0debdfc96a7fdeb2666252e8d28ed
Reviewed-on: https://boringssl-review.googlesource.com/5786
Reviewed-by: Adam Langley <agl@google.com>
If the two extensions select different next protocols (quite possible since one
is server-selected and the other is client-selected), things will break. This
matches the behavior of NSS (Firefox) and Go.
Change-Id: Ie1da97bf062b91a370c85c12bc61423220a22f36
Reviewed-on: https://boringssl-review.googlesource.com/5780
Reviewed-by: Adam Langley <agl@google.com>
The maximum buffer size computation wasn't quite done right in
ssl_buffer.c, so we were failing with BUFFER_TOO_SMALL for sufficiently
large records. Fix this and, as penance, add 103 tests.
(Test that we can receive maximum-size records in all cipher suites.
Also test SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER while I'm here.)
BUG=526998
Change-Id: I714c16dda2ed13f49d8e6cd1b48adc5a8491f43c
Reviewed-on: https://boringssl-review.googlesource.com/5785
Reviewed-by: Adam Langley <agl@google.com>
Now that it even works at all (type = 0 bug aside), add tests for it.
Test both close_notify being received before and after SSL_shutdown is
called. In the latter case, have the peer send some junk to be ignored
to test that works.
Also test that SSL_shutdown fails on unclean shutdown and that quiet
shutdowns ignore it.
BUG=526437
Change-Id: Iff13b08feb03e82f21ecab0c66d5f85aec256137
Reviewed-on: https://boringssl-review.googlesource.com/5769
Reviewed-by: Adam Langley <agl@google.com>
Our tests shouldn't panic if the program misbehaves.
Change-Id: I113e050222bcf48e5f25883f860dbc1c5c77e77e
Reviewed-on: https://boringssl-review.googlesource.com/5764
Reviewed-by: Adam Langley <agl@google.com>
The old empty record logic discarded the records at a very low-level.
Let the error bubble up to ssl3_read_bytes so the type mismatch logic
may kick in before the empty record is skipped.
Add tests for when the record in question is application data, before
before the handshake and post ChangeCipherSpec.
BUG=521840
Change-Id: I47dff389cda65d6672b9be39d7d89490331063fa
Reviewed-on: https://boringssl-review.googlesource.com/5754
Reviewed-by: Adam Langley <agl@google.com>
This made sense when the cipher might have been standardized as-is, so a
DHE_RSA variant could appease the IETF. Since the standardized variant is going
to have some nonce tweaks anyway, there's no sense in keeping this around. Get
rid of one non-standard cipher suite value early. (Even if they were to be
standardized as-is, it's not clear we should implement new DHE cipher suites at
this point.)
Chrome UMA, unsurprisingly, shows that it's unused.
Change-Id: Id83d73a4294b470ec2e94d5308fba135d6eeb228
Reviewed-on: https://boringssl-review.googlesource.com/5750
Reviewed-by: Adam Langley <agl@google.com>
This is a simpler implementation than OpenSSL's, lacking responder IDs
and request extensions support. This mirrors the client implementation
already present.
Change-Id: I54592b60e0a708bfb003d491c9250401403c9e69
Reviewed-on: https://boringssl-review.googlesource.com/5700
Reviewed-by: Adam Langley <agl@google.com>
Rather than sniff for ClientHello, just fall through to standard logic
once weird cases are resolved.
This means that garbage will now read as WRONG_VERSION rather than
UNKNOWN_PROTOCOL, but the rules here were slightly odd anyway. This also
means we'll now accept empty records before the ClientHello (up to the
empty record limit), and process records of the wrong type with the
usual codepath during the handshake.
This shouldn't be any more risk as it just makes the ClientHello more
consistent with the rest of the protocol. A TLS implementation that
doesn't parse V2ClientHello would do the same unless it still
special-cased the first record. All newly-exposed states are reachable
by fragmenting ClientHello by one byte and then sending the record in
question.
BUG=468889
Change-Id: Ib701ae5d8adb663e158c391639b232a9d9cd1c6e
Reviewed-on: https://boringssl-review.googlesource.com/5712
Reviewed-by: Adam Langley <agl@google.com>
Due to a typo, when a server sent an unknown extension, the extension
number would be taken from a NULL structure rather than from the
variable of the same name that's in the local scope.
BUG=517935
Change-Id: I29d5eb3c56cded40f6155a81556199f12439ae06
Reviewed-on: https://boringssl-review.googlesource.com/5650
Reviewed-by: Adam Langley <agl@google.com>
The existing tests only went monotonic. Allow an arbitrary mapping
function. Also test by sending more app data. The handshake is fairly
resilient to replayed packets, whereas our test code intentionally
isn't.
Change-Id: I0fb74bbacc260c65ec5f6a1ca8f3cb23b4192855
Reviewed-on: https://boringssl-review.googlesource.com/5556
Reviewed-by: Adam Langley <agl@google.com>
I don't think we had coverage for this check.
Change-Id: I5e454e69c1ee9f1b9760d2ef1431170d76f78d63
Reviewed-on: https://boringssl-review.googlesource.com/5544
Reviewed-by: Adam Langley <agl@google.com>
That got out of sync at some point.
Change-Id: I5a45f50f330ceb65053181afc916053a80aa2c5d
Reviewed-on: https://boringssl-review.googlesource.com/5541
Reviewed-by: Adam Langley <agl@google.com>
The RSA key exchange needs decryption and is still unsupported.
Change-Id: I8c13b74e25a5424356afbe6e97b5f700a56de41f
Reviewed-on: https://boringssl-review.googlesource.com/5467
Reviewed-by: Adam Langley <agl@google.com>
This change mirrors upstream's custom extension API because we have some
internal users that depend on it.
Change-Id: I408e442de0a55df7b05c872c953ff048cd406513
Reviewed-on: https://boringssl-review.googlesource.com/5471
Reviewed-by: Adam Langley <agl@google.com>
These were used in the upstream Go code to fuzz-test the handshake
marshal/unmarshal functions. But we don't do that there so best to
remove them.
(The ClientHello equals function is still used, however, to test DTLS
retransmission.)
Change-Id: I950bdf4f7eefa2bca13c10f5328d2e6c586604e2
Reviewed-on: https://boringssl-review.googlesource.com/5470
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Fastradio was a trick where the ClientHello was padding to at least 1024
bytes in order to trick some mobile radios into entering high-power mode
immediately. After experimentation, the feature is being dropped.
This change also tidies up a bit of the extensions code now that
everything is using the new system.
Change-Id: Icf7892e0ac1fbe5d66a5d7b405ec455c6850a41c
Reviewed-on: https://boringssl-review.googlesource.com/5466
Reviewed-by: Adam Langley <agl@google.com>
This also removes support for the “old” Channel ID extension.
Change-Id: I1168efb9365c274db6b9d7e32013336e4404ff54
Reviewed-on: https://boringssl-review.googlesource.com/5462
Reviewed-by: Adam Langley <agl@google.com>
Historically we had a bug around this and this change implements a test
that we were previously carrying internally.
Change-Id: Id181fedf66b2b385b54131ac91d74a31f86f0205
Reviewed-on: https://boringssl-review.googlesource.com/5380
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This change also switches the behaviour of the client. Previously the
client would send the SCSV rather than the extension, but now it'll only
do that for SSLv3 connections.
Change-Id: I67a04b8abbef2234747c0dac450458deb6b0cd0a
Reviewed-on: https://boringssl-review.googlesource.com/5143
Reviewed-by: Adam Langley <agl@google.com>
This test shouldn't trigger a renegotiation: the test is trying to
assert that without the legacy-server flag set, a server that doesn't
echo the renegotiation extension can't be connected to.
Change-Id: I1368d15ebc8f296f3ff07040c0e6c48fdb49e56f
Reviewed-on: https://boringssl-review.googlesource.com/5141
Reviewed-by: Adam Langley <agl@google.com>
Rather than rely on Chromium to query SSL_initial_handshake_complete in the
callback (which didn't work anyway because the callback is called afterwards),
move the logic into BoringSSL. BoringSSL already enforces that clients never
offer resumptions on renegotiation (it wouldn't work well anyway as client
session cache lookup is external), so it's reasonable to also implement
in-library that sessions established on a renegotiation are not cached.
Add a bunch of tests that new_session_cb is called when expected.
BUG=501418
Change-Id: I42d44c82b043af72b60a0f8fdb57799e20f13ed5
Reviewed-on: https://boringssl-review.googlesource.com/5171
Reviewed-by: Adam Langley <agl@google.com>
b4d65fda70 was written concurrently with
my updating runner to handle -resource-dir (in
7c803a65d5) and thus it didn't include the
needed change for the test that it added to handle it.
This change fixes that added test so that it can run with -resource-dir.
Change-Id: I06b0adfb3fcf3f11c061fe1c8332a45cd7cd2dbc
This adds a new API, SSL_set_private_key_method, which allows the consumer to
customize private key operations. For simplicity, it is incompatible with the
multiple slots feature (which will hopefully go away) but does not, for now,
break it.
The new method is only routed up for the client for now. The server will
require a decrypt hook as well for the plain RSA key exchange.
BUG=347404
Change-Id: I35d69095c29134c34c2af88c613ad557d6957614
Reviewed-on: https://boringssl-review.googlesource.com/5049
Reviewed-by: Adam Langley <agl@google.com>
Turns out the safer/simpler method still wasn't quite right. :-)
session->sess_cert isn't serialized and deserialized, which is poor. Duplicate
it manually for now. Leave a TODO to get rid of that field altogether as it's
not especially helpful. The certificate-related fields should be in the
session. The others probably have no reason to be preserved on resumptions at
all.
Test by making bssl_shim.cc assert the peer cert chain is there or not as
expected.
BUG=501220
Change-Id: I44034167629720d6e2b7b0b938d58bcab3ab0abe
Reviewed-on: https://boringssl-review.googlesource.com/5170
Reviewed-by: Adam Langley <agl@google.com>
If gdb is attached, it's convenient to be able to continue running.
Change-Id: I3bbb2634d05a08f6bad5425f71da2210dbb80cfe
Reviewed-on: https://boringssl-review.googlesource.com/5125
Reviewed-by: Adam Langley <agl@google.com>
This change adds flags to runner to allow it to be sufficiently
configured that it can run from any directory.
Change-Id: I82c08da4ffd26c5b11637480b0a79eaba0904d38
Reviewed-on: https://boringssl-review.googlesource.com/5130
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 9dcab127e14467733523ff7626da8906e67eedd6. The root problem
is dtls1_read_bytes is wrong, but we can get the right behavior now and add a
regression test for it before cleaning it up.
Change-Id: I4e5c39ab254a872d9f64242c9b77b020bdded6e6
Reviewed-on: https://boringssl-review.googlesource.com/5123
Reviewed-by: Adam Langley <agl@google.com>
If we're going to have PSK and use standard cipher suites, this might be
the best that we can do for the moment.
Change-Id: I35d9831b2991dc5b23c9e24d98cdc0db95919d39
Reviewed-on: https://boringssl-review.googlesource.com/5052
Reviewed-by: Adam Langley <agl@google.com>
This is the best PSK cipher suite, but it's non-standard and nobody is
using it. Trivial to bring back in the future if we have need of it.
Change-Id: Ie78790f102027c67d1c9b19994bfb10a2095ba92
Reviewed-on: https://boringssl-review.googlesource.com/5051
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The NULL checks later on notice, but failing with
SSL_R_UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS on accident is confusing.
Require that the message be non-empty.
Change-Id: Iddfac6a3ae6e6dc66c3de41d3bb26e133c0c6e1d
Reviewed-on: https://boringssl-review.googlesource.com/5046
Reviewed-by: Adam Langley <agl@google.com>
We shouldn't have protocol constraints that are sensitive to whether
data is returned synchronously or not.
Per https://boringssl-review.googlesource.com/#/c/4112/, the original
limitation was to avoid OpenSSL ABI changes. This is no longer a
concern.
Add tests for the sync and async case. Send the empty records in two
batches to ensure the count is reset correctly.
Change-Id: I3fee839438527e71adb83d437879bb0d49ca5c07
Reviewed-on: https://boringssl-review.googlesource.com/5040
Reviewed-by: Adam Langley <agl@google.com>
The client and server both have to decide on behaviour when resuming a
session where the EMS state of the session doesn't match the EMS state
as exchanged in the handshake.
Original handshake
| No Yes
------+--------------------------------------------------------------
|
R | Server: ok [1] Server: abort [3]
e No | Client: ok [2] Client: abort [4]
s |
u |
m |
e |
Yes | Server: don't resume No problem
| Client: abort; server
| shouldn't have resumed
[1] Servers want to accept legacy clients. The draft[5] says that
resumptions SHOULD be rejected so that Triple-Handshake can't be done,
but we'll rather enforce that EMS was used when using tls-unique etc.
[2] The draft[5] says that even the initial handshake should be aborted
if the server doesn't support EMS, but we need to be able to talk to the
world.
[3] This is a very weird case where a client has regressed without
flushing the session cache. Hopefully we can be strict and reject these.
[4] This can happen when a server-farm shares a session cache but
frontends are not all updated at once. If Chrome is strict here then
hopefully we can prevent any servers from existing that will try to
resume an EMS session that they don't understand. OpenSSL appears to be
ok here: https://www.ietf.org/mail-archive/web/tls/current/msg16570.html
[5] https://tools.ietf.org/html/draft-ietf-tls-session-hash-05#section-5.2
BUG=492200
Change-Id: Ie1225a3960d49117b05eefa5a36263d8e556e467
Reviewed-on: https://boringssl-review.googlesource.com/4981
Reviewed-by: Adam Langley <agl@google.com>
Add expectResumeRejected to note cases where we expect a resumption
handshake to be rejected. (This was previously done by adding a flag,
which is a little less clear.)
Also, save the result of crypto/tls.Conn.ConnectionState() rather than
repeat that a lot.
Change-Id: I963945eda5ce1f3040b655e2441174b918b216b3
Reviewed-on: https://boringssl-review.googlesource.com/4980
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Make sure we don't break that on accident.
Change-Id: I22d58d35170d43375622fe61e4a588d1d626a054
Reviewed-on: https://boringssl-review.googlesource.com/4960
Reviewed-by: Adam Langley <agl@google.com>
9a41d1b946 broke handling of multiple records in
a single packet. If |extend| is true, not all of the previous packet should be
consumed, only up to the record length.
Add a test which stresses the DTLS stack's handling of multiple handshake
fragments in a handshake record and multiple handshake records in a packet.
Change-Id: I96571098ad9001e96440501c4730325227b155b8
Reviewed-on: https://boringssl-review.googlesource.com/4950
Reviewed-by: Adam Langley <agl@google.com>
The only case where renego is supported is if we are a client and the
server sends a HelloRequest. That is still needed to support the renego
+ client auth hack in Chrome. Beyond that, no other forms of renego will
work.
The messy logic where the handshake loop is repurposed to send
HelloRequest and the extremely confusing tri-state s->renegotiate (which
makes SSL_renegotiate_pending a lie during the initial handshake as a
server) are now gone. The next change will further simplify things by
removing ssl->s3->renegotiate and the renego deferral logic. There's
also some server-only renegotiation checks that can go now.
Also clean up ssl3_read_bytes' HelloRequest handling. The old logic relied on
the handshake state machine to reject bad HelloRequests which... actually that
code probably lets you initiate renego by sending the first four bytes of a
ServerHello and expecting the peer to read it later.
BUG=429450
Change-Id: Ie0f87d0c2b94e13811fe8e22e810ab2ffc8efa6c
Reviewed-on: https://boringssl-review.googlesource.com/4824
Reviewed-by: Adam Langley <agl@google.com>
Now that WebRTC honors packet boundaries (https://crbug.com/447431), we
can start enforcing them correctly. Configuring read-ahead now does
nothing. Instead DTLS will always set "read-ahead" and also correctly
enforce packet boundaries when reading records. Add tests to ensure that
badly fragmented packets are ignored. Because such packets don't fail
the handshake, the tests work by injecting an alert in the front of the
handshake stream and ensuring the DTLS implementation ignores them.
ssl3_read_n can be be considerably unraveled now, but leave that for
future cleanup. For now, make it correct.
BUG=468889
Change-Id: I800cfabe06615af31c2ccece436ca52aed9fe899
Reviewed-on: https://boringssl-review.googlesource.com/4820
Reviewed-by: Adam Langley <agl@google.com>
Currently far from passing and I haven't even tried with a leak checker yet.
Also bn_test is slow.
Change-Id: I4fe2783aa5f7897839ca846062ae7e4a367d2469
Reviewed-on: https://boringssl-review.googlesource.com/4794
Reviewed-by: Adam Langley <agl@google.com>
Rather than duplicate all the various modifiers, which is quite
error-prone, write all the tests to a temporary array and then apply
modifiers afterwards.
Change-Id: I19bfeb83b722ed34e973f17906c5e071471a926a
Reviewed-on: https://boringssl-review.googlesource.com/4782
Reviewed-by: Adam Langley <agl@google.com>
We should be testing asynchronous renego.
BUG=429450
Change-Id: Ib7a5d42f2ac728f9ea0d80158eef63ad77cd77a4
Reviewed-on: https://boringssl-review.googlesource.com/4781
Reviewed-by: Adam Langley <agl@google.com>
Since we hope to eventually lose server-side renegotiation support
altogether, get the client-side version of those tests. We should have
had those anyway to test that the default is to allow it.
BUG=429450
Change-Id: I4a18f339b55f3f07d77e22e823141e10a12bc9ff
Reviewed-on: https://boringssl-review.googlesource.com/4780
Reviewed-by: Adam Langley <agl@google.com>
DH groups less than 1024 bits are clearly not very safe. Ideally servers
would switch to ECDHE because 1024 isn't great either, but this will
serve for the short term.
BUG=490240
Change-Id: Ic9aac714cdcdcbfae319b5eb1410675d3b903a69
Reviewed-on: https://boringssl-review.googlesource.com/4813
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
We can't actually catch this with MSan because it requires all code be
instrumented, so it needs a NO_ASM build which no disables that code. valgrind
doesn't notice either, possibly because there's some computation being done on
it. Still, we shouldn't use uninitialized memory.
Also get us closer to being instrumentable by MSan, but the runner tests will
need to build against an instrumented STL and I haven't tried that yet.
Change-Id: I2d65697a3269b5b022899f361730a85c51ecaa12
Reviewed-on: https://boringssl-review.googlesource.com/4760
Reviewed-by: Adam Langley <agl@google.com>
SSL_get_current_cipher is documented by upstream to return the cipher actually
being used. However, because it reads s->session, it returns information
pertaining to the session to be offered if queried before ServerHello or early
in an abbreviated handshake.
Logic around s->session needs more comprehensive cleanup but for just this
function, defining it to be the current outgoing cipher is close to the current
semantics but for fixing the initial state (s->session->cipher is populated
when sending CCS). Store it in the SSL_AEAD_CTX which seems a natural place to
associate state pertaining to a connection half.
BUG=484744
Change-Id: Ife8db27a16615d0dbb2aec65359537243e08af7c
Reviewed-on: https://boringssl-review.googlesource.com/4733
Reviewed-by: Adam Langley <agl@google.com>
This cuts down on one config knob as well as one case in the renego
combinatorial explosion. Since the only case we care about with renego
is the client auth hack, there's no reason to ever do resumption.
Especially since, no matter what's in the session cache:
- OpenSSL will only ever offer the session it just established,
whether or not a newer one with client auth was since established.
- Chrome will never cache sessions created on a renegotiation, so
such a session would never make it to the session cache.
- The new_session + SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
logic had a bug where it would unconditionally never offer tickets
(but would advertise support) on renego, so any server doing renego
resumption against an OpenSSL-derived client must not support
session tickets.
This also gets rid of s->new_session which is now pointless.
BUG=429450
Change-Id: I884bdcdc80bff45935b2c429b4bbc9c16b2288f8
Reviewed-on: https://boringssl-review.googlesource.com/4732
Reviewed-by: Adam Langley <agl@google.com>
As of crbug.com/484543, Chromium's SSLClientSocket is not sensitive to whether
renegotiation is enabled or not. Disable it by default and require consumers to
opt into enabling this protocol mistake.
BUG=429450
Change-Id: I2329068284dbb851da010ff1fd398df3d663bcc3
Reviewed-on: https://boringssl-review.googlesource.com/4723
Reviewed-by: Adam Langley <agl@google.com>
With DTLSv1_get_timeout de-ctrl-ified, the type checker complains about
OPENSSL_timeval. Existing callers all use the real timeval.
Now that OPENSSL_timeval is not included in any public structs, simply
forward-declare timeval itself in ssl.h and pull in winsock2.h in internal
headers.
Change-Id: Ieaf110e141578488048c28cdadb14881301a2ce1
Reviewed-on: https://boringssl-review.googlesource.com/4682
Reviewed-by: Adam Langley <agl@google.com>
“ECDHE-PSK-WITH-AES-128-GCM-SHA256” doesn't follow the standard naming
for OpenSSL: it was “-WITH-” in it and has a hyphen between “AES” and
“128”. This change fixes that.
Change-Id: I7465b1ec83e7d5b9a60d8ca589808aeee10c174e
Reviewed-on: https://boringssl-review.googlesource.com/4601
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This will be reverted in a minute. The bots should run both suites of tests and
report the names of all failing tests in the summary.
Change-Id: Ibe351017dfa8ccfd182b3c88eee413cd2cbdeaf0
This is a really dumb API wart. Now that we have a limited set of curves that
are all reasonable, the automatic logic should just always kick in. This makes
set_ecdh_auto a no-op and, instead of making it the first choice, uses it as
the fallback behavior should none of the older curve selection APIs be used.
Currently, by default, server sockets can only use the plain RSA key exchange.
BUG=481139
Change-Id: Iaabc82de766cd00968844a71aaac29bd59841cd4
Reviewed-on: https://boringssl-review.googlesource.com/4531
Reviewed-by: Adam Langley <agl@google.com>
See upstream's bd891f098bdfcaa285c073ce556d0f5e27ec3a10. It honestly seems
kinda dumb for a client to do this, but apparently the spec allows this.
Judging by code inspection, OpenSSL 1.0.1 also allowed this, so this avoids a
behavior change when switching from 1.0.1 to BoringSSL.
Add a test for this, which revealed that, unlike upstream's version, this
actually works with ecdh_auto since tls1_get_shared_curve also needs updating.
(To be mentioned in newsletter.)
Change-Id: Ie622700f17835965457034393b90f346740cfca8
Reviewed-on: https://boringssl-review.googlesource.com/4464
Reviewed-by: Adam Langley <agl@google.com>
Because RFC 6066 is obnoxious like that and IIS servers actually do this
when OCSP-stapling is configured, but the OCSP server cannot be reached.
BUG=478947
Change-Id: I3d34c1497e0b6b02d706278dcea5ceb684ff60ae
Reviewed-on: https://boringssl-review.googlesource.com/4461
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I489d00bc4ee22a5ecad75dc1eb84776f044566e5
Reviewed-on: https://boringssl-review.googlesource.com/4391
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This causes any unexpected handshake records to be met with a fatal
no_renegotiation alert.
In addition, restore the redundant version sanity-checks in the handshake state
machines. Some code would zero the version field as a hacky way to break the
handshake on renego. Those will be removed when switching to this API.
The spec allows for a non-fatal no_renegotiation alert, but ssl3_read_bytes
makes it difficult to find the end of a ClientHello and skip it entirely. Given
that OpenSSL goes out of its way to map non-fatal no_renegotiation alerts to
fatal ones, this seems probably fine. This avoids needing to account for
another source of the library consuming an unbounded number of bytes without
returning data up.
Change-Id: Ie5050d9c9350c29cfe32d03a3c991bdc1da9e0e4
Reviewed-on: https://boringssl-review.googlesource.com/4300
Reviewed-by: Adam Langley <agl@google.com>
A previous change in BoringSSL renamed ERR_print_errors_fp to
BIO_print_errors_fp as part of refactoring the code to improve the
layering of modules within BoringSSL. Rename it back for better
compatibility with code that was using the function under the original
name. Move its definition back to crypto/err using an implementation
that avoids depending on crypto/bio.
Change-Id: Iee7703bb1eb4a3d640aff6485712bea71d7c1052
Reviewed-on: https://boringssl-review.googlesource.com/4310
Reviewed-by: Adam Langley <agl@google.com>
The rest of ssl/ still includes things everywhere, but this at least fixes the
includes that were implicit from ssl/internal.h.
Change-Id: I7ed22590aca0fe78af84fd99a3e557f4b05f6782
Reviewed-on: https://boringssl-review.googlesource.com/4281
Reviewed-by: Adam Langley <agl@google.com>
This drops in a copy of a subset of golang.org/x/crypto/poly1305 to implement
Poly1305. Hopefully this will keep them from regression as we rework the record
layer.
Change-Id: Ic1e0d941a0a9e5ec260151ced8acdf9215c4b887
Reviewed-on: https://boringssl-review.googlesource.com/4257
Reviewed-by: Adam Langley <agl@google.com>
After sharding the session cache for fallbacks, the numbers have been pretty
good; 0.03% on dev and 0.02% on canary. Stable is at 0.06% but does not have
the sharded session cache. Before sharding, stable, beta, and dev had been
fairly closely aligned. Between 0.03% being low and the fallback saving us in
all but extremely contrived cases, I think this should be fairly safe.
Add tests for both the cipher suite and protocol version mismatch checks.
BUG=441456
Change-Id: I2374bf64d0aee0119f293d207d45319c274d89ab
Reviewed-on: https://boringssl-review.googlesource.com/3972
Reviewed-by: Adam Langley <agl@google.com>
This conceivably has a use, but NSS doesn't do this buffer either and it still
suffers from the same problems as the other uses of record_pqueue. This removes
the last use of record_pqueue. It also opens the door to removing pqueue
altogether as it isn't the right data structure for either of the remaining
uses either. (It's not clear it was right for record_pqueue either, but I don't
feel like digging into this code.)
Change-Id: If8a43e7332b3cd11a78a516f3e8ebf828052316f
Reviewed-on: https://boringssl-review.googlesource.com/4239
Reviewed-by: Adam Langley <agl@google.com>
If the key is missing, it seems the failure is assumed to be expected.
BUG=473924
Change-Id: I62edd9110fa74bee5e6425fd6786badf5398728c
Reviewed-on: https://boringssl-review.googlesource.com/4231
Reviewed-by: Adam Langley <agl@google.com>
Move the state to TestState rather than passing pointers to them everywhere.
Also move SSL_read and SSL_write retry loops into helper functions so they
aren't repeated everywhere. This also makes the SSL_write calls all
consistently account for partial writes.
Change-Id: I9bc083a03da6a77ab2fc03c29d4028435fc02620
Reviewed-on: https://boringssl-review.googlesource.com/4214
Reviewed-by: Adam Langley <agl@google.com>
Extend the False Start tests to optionally send an alert (thus avoiding
deadlock) before waiting for the out-of-order app data. Based on whether the
peer shuts off the connection before or after sending app data, we can
determine whether the peer False Started by observing purely external effects.
Change-Id: I8b9fecc29668e0b0c34b5fd19d0f239545011bae
Reviewed-on: https://boringssl-review.googlesource.com/4213
Reviewed-by: Adam Langley <agl@google.com>
Based on whether -false-start is passed, we expect SSL_CB_HANDSHAKE_DONE to or
not to fire. Also add a flag that asserts SSL_CB_HANDSHAKE_DONE does *not* fire
in any False Start test where the handshake fails after SSL_connect returns.
Change-Id: I6c5b960fff15e297531e15b16abe0b98be95bec8
Reviewed-on: https://boringssl-review.googlesource.com/4212
Reviewed-by: Adam Langley <agl@google.com>
That's a pretty obvious thing to test. I'm not sure how we forgot that one.
Change-Id: I7e1a7df6c6abbdd587e0f7723117f50d09faa5c4
Reviewed-on: https://boringssl-review.googlesource.com/4211
Reviewed-by: Adam Langley <agl@google.com>
ASan's own malloc interceptor isn't compatible with this mechanism; it doesn't
see calls to __libc_malloc.
Change-Id: Ibac5aa05c6e40f1c72dcee3a2597e96deffca62c
Reviewed-on: https://boringssl-review.googlesource.com/4191
Reviewed-by: Adam Langley <agl@google.com>
This is in preparation for using RAII in the unit tests. Those tests are built
in Chromium as well, but Chromium does not have C++11 library support across
all its toolchains. Compiler support is available, so add a partial
reimplementation of std::unique_ptr and std::move under crypto/test/. The
scopers for the crypto/ library are also moved there while the ones for ssl/
stay in ssl/test/.
Change-Id: I38f769acbc16a870db34649928575c7314b6e9f6
Reviewed-on: https://boringssl-review.googlesource.com/4120
Reviewed-by: Adam Langley <agl@google.com>
MinGW on Linux needs lowercase include files. On Windows this doesn't
matter since the filesystems are case-insensitive, but building
BoringSSL on Linux with MinGW has case-sensitive filesystems.
Change-Id: Id9c120d819071b041341fbb978352812d6d073bc
Reviewed-on: https://boringssl-review.googlesource.com/4090
Reviewed-by: Adam Langley <agl@google.com>
Partly inspired by the new state exposed in
dc3da93899, stress this codepath by spamming our
poor shim with warning alerts.
Change-Id: I876c6e52911b6eb57493cf3e1782b37ea96d01f8
Reviewed-on: https://boringssl-review.googlesource.com/4112
Reviewed-by: Adam Langley <agl@google.com>
Google C++ style allows these. It's also considerably less tedious and
error-prone than defining an out-of-line constructor.
Change-Id: Ib76ccf6079be383722433046ac5c5d796dd1f525
Reviewed-on: https://boringssl-review.googlesource.com/4111
Reviewed-by: Adam Langley <agl@google.com>
It was failing only on 32-bit for some reason. Part of TestConfig wasn't
getting initialized.
Change-Id: I2a3747a347a47b47e2357f34d32f8db86d6cc629
Reviewed-on: https://boringssl-review.googlesource.com/4110
Reviewed-by: Adam Langley <agl@google.com>
I'm guessing a previous iteration used dlsym to look up the real malloc.
Change-Id: I18be9ef4db4ed059400074c8507d4e2fea882fbc
Reviewed-on: https://boringssl-review.googlesource.com/4100
Reviewed-by: Adam Langley <agl@google.com>
Both on the client and the server.
Change-Id: I9892c6dbbb29938154aba4f53b10e8b5231f9c47
Reviewed-on: https://boringssl-review.googlesource.com/4071
Reviewed-by: Adam Langley <agl@google.com>
This is really just scar tissue with https://crbug.com/468889 being the real
underlying problem. But the test is pretty easy.
Change-Id: I5eca18fdcbde8665c0e6c3ac419a28152647d66f
Reviewed-on: https://boringssl-review.googlesource.com/4052
Reviewed-by: Adam Langley <agl@google.com>
In verifying the fix for CVE-2015-0291, I noticed we don't actually have any
test coverage for full handshakes on renegotiation. All our tests always do
resumptions.
Change-Id: Ia9b701e8a50ba9353fefb8cc4fb86e78065d0b40
Reviewed-on: https://boringssl-review.googlesource.com/4050
Reviewed-by: Adam Langley <agl@google.com>
Noticed these as I was poking around.
Change-Id: I93833a152583feced374c9febf7485bec7abc1c7
Reviewed-on: https://boringssl-review.googlesource.com/3973
Reviewed-by: Adam Langley <agl@google.com>
This fixes test flakiness on Windows.
BUG=467767
Change-Id: Ie69b5b43ddd524aadb15c53705f6ec860e928786
Reviewed-on: https://boringssl-review.googlesource.com/4001
Reviewed-by: Adam Langley <agl@google.com>
This callback receives information about the ClientHello and can decide
whether or not to allow the handshake to continue.
Change-Id: I21be28335fa74fedb5b73a310ee24310670fc923
Reviewed-on: https://boringssl-review.googlesource.com/3721
Reviewed-by: Adam Langley <agl@google.com>
