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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
This mostly[*] doesn't matter for TLS since the message would have been
rejected anyway, but, in DTLS, if the peer rejects our Finished, it will send
an encrypted alert. This will then cause it to hang, which isn't very helpful.
I've made the change on both TLS and DTLS so the two protocols don't diverge on
this point. It is true that we're accepting nominally encrypted and
authenticated alerts before Finished, but, prior to ChangeCipherSpec, the
alerts are sent in the clear anyway so an attacker could already inject alerts.
A consumer could only be sensitive to it being post-CCS if it was watching
msg_callback. The only non-debug consumer of msg_callback I've found anywhere
is some hostapd code to detect Heartbeat.
See https://code.google.com/p/webrtc/issues/detail?id=4403 for an instance
where the equivalent behavior in OpenSSL masks an alert.
[*] This does change behavior slightly if the peer sends a warning alert
between CCS and Finished. I believe this is benign as warning alerts are
usually ignored apart from info_callback and msg_callback. The one exception is
a close_notify which is a slightly new state (accepting close_notify during a
handshake seems questionable...), but they're processed pre-CCS too.
Change-Id: Idd0d49b9f9aa9d35374a9f5e2f815cdb931f5254
Reviewed-on: https://boringssl-review.googlesource.com/3883
Reviewed-by: Adam Langley <agl@google.com>
We actually don't really care about this special-case since we only test client
full handshakes where the runner sends the second Finished not the shim
(otherwise the overlap logic and retransmitting on every fragment would
probably break us), but it should probably live next to the fragmentation
logic.
Change-Id: I54097d84ad8294bc6c42a84d6f22f496e63eb2a8
Reviewed-on: https://boringssl-review.googlesource.com/3763
Reviewed-by: Adam Langley <agl@google.com>
That might be a reasonable check to make, maybe.
DTLS handshake message reading has a ton of other bugs and needs a complete
rewrite. But let's fix this and get a test in now.
Change-Id: I4981fc302feb9125908bb6161ed1a18288c39e2b
Reviewed-on: https://boringssl-review.googlesource.com/3600
Reviewed-by: Adam Langley <agl@google.com>
This is consistent with ignoring writeRecord failures. Without doing this, the
DTLS MinimumVersion test now flakily fails with:
FAILED (MinimumVersion-Client-TLS12-TLS1-DTLS)
bad error (wanted ':UNSUPPORTED_PROTOCOL:' / 'remote error: protocol version not supported'): local error 'write unix @: broken pipe', child error 'exit status 2', stdout:
2092242157:error:1007b1a7:SSL routines:ssl3_get_server_hello:UNSUPPORTED_PROTOCOL:../ssl/s3_clnt.c:783:
This is because the MinimumVersion tests assert on /both/ expectedError and
expectedLocalError. The latter is valuable as it asserts on the alert the peer
returned. (I would like us to add more such assertions to our tests where
appropriate.) However, after we send ServerHello, we also send a few messages
following it. This races with the peer shutdown and we sometimes get EPIPE
before reading the alert.
Change-Id: I3fe37940a6a531379673a00976035f8e76e0f825
Reviewed-on: https://boringssl-review.googlesource.com/3337
Reviewed-by: Adam Langley <agl@google.com>
This works fine, but I believe NSS had a bug here a couple years ago. Also move
all the Skip* bug options next to each other in order.
Change-Id: I72dcb3babeee7ba73b3d7dc5ebef2e2298e37438
Reviewed-on: https://boringssl-review.googlesource.com/3333
Reviewed-by: Adam Langley <agl@google.com>
For now, only test reorderings when we always or never fragment messages.
There's a third untested case: when full messages and fragments are mixed. That
will be tested later after making it actually work.
Change-Id: Ic4efb3f5e87b1319baf2d4af31eafa40f6a50fa6
Reviewed-on: https://boringssl-review.googlesource.com/3216
Reviewed-by: Adam Langley <agl@google.com>
This extends the packet adaptor protocol to send three commands:
type command =
| Packet of []byte
| Timeout of time.Duration
| TimeoutAck
When the shim processes a Timeout in BIO_read, it sends TimeoutAck, fails the
BIO_read, returns out of the SSL stack, advances the clock, calls
DTLSv1_handle_timeout, and continues.
If the Go side sends Timeout right between sending handshake flight N and
reading flight N+1, the shim won't read the Timeout until it has sent flight
N+1 (it only processes packet commands in BIO_read), so the TimeoutAck comes
after N+1. Go then drops all packets before the TimeoutAck, thus dropping one
transmit of flight N+1 without having to actually process the packets to
determine the end of the flight. The shim then sees the updated clock, calls
DTLSv1_handle_timeout, and re-sends flight N+1 for Go to process for real.
When dropping packets, Go checks the epoch and increments sequence numbers so
that we can continue to be strict here. This requires tracking the initial
sequence number of the next epoch.
The final Finished message takes an additional special-case to test. DTLS
triggers retransmits on either a timeout or seeing a stale flight. OpenSSL only
implements the former which should be sufficient (and is necessary) EXCEPT for
the final Finished message. If the peer's final Finished message is lost, it
won't be waiting for a message from us, so it won't time out anything. That
retransmit must be triggered on stale message, so we retransmit the Finished
message in Go.
Change-Id: I3ffbdb1de525beb2ee831b304670a3387877634c
Reviewed-on: https://boringssl-review.googlesource.com/3212
Reviewed-by: Adam Langley <agl@google.com>
This is fatal for TLS but buffered in DTLS. The buffering isn't strictly
necessary (it would be just as valid to drop the record on the floor), but so
long as we want this behavior it should have a test.
Change-Id: I5846bb2fe80d78e25b6dfad51bcfcff2dc427c3f
Reviewed-on: https://boringssl-review.googlesource.com/3029
Reviewed-by: Adam Langley <agl@google.com>
The ClientHello record is padded to 1024 bytes when
fastradio_padding is enabled. As a result, the 3G cellular radio
is fast forwarded to DCH (high data rate) state. This mechanism
leads to a substantial redunction in terms of TLS handshake
latency, and benefits mobile apps that are running on top of TLS.
Change-Id: I3d55197b6d601761c94c0f22871774b5a3dad614
We forgot to add those when we implemented the features. (Also relevant because
they will provide test coverage later for configuring features when using the
generic method tables rather than *_client_method.)
Change-Id: Ie08b27de893095e01a05a7084775676616459807
Reviewed-on: https://boringssl-review.googlesource.com/2410
Reviewed-by: Adam Langley <agl@google.com>
This implements session IDs in client and server in runner.go.
Change-Id: I26655f996b7b44c7eb56340ef6a415d3f2ac3503
Reviewed-on: https://boringssl-review.googlesource.com/2350
Reviewed-by: Adam Langley <agl@google.com>
Just the negotiation portion as everything else is external. This feature is
used in WebRTC.
Change-Id: Iccc3983ea99e7d054b59010182f9a56a8099e116
Reviewed-on: https://boringssl-review.googlesource.com/2310
Reviewed-by: Adam Langley <agl@google.com>
Clients all consistently reject mismatches. If a different version was
negotiated, a server should ignore the resumption. This doesn't actually affect
current tests. We really want to be making this change in BoringSSL (and then
upstream), but get the Go half into shape first.
Change-Id: Ieee7e141331d9e08573592e661889bd756dccfa9
Reviewed-on: https://boringssl-review.googlesource.com/2243
Reviewed-by: Adam Langley <agl@google.com>
This change adds support to the Go code for renegotiation as a client,
meaning that we can test BoringSSL's renegotiation as a server.
Change-Id: Iaa9fb1a6022c51023bce36c47d4ef7abee74344b
Reviewed-on: https://boringssl-review.googlesource.com/2082
Reviewed-by: Adam Langley <agl@google.com>
Only the three plain PSK suites for now. ECDHE_PSK_WITH_AES_128_GCM_SHA256 will
be in a follow-up.
Change-Id: Iafc116a5b2798c61d90c139b461cf98897ae23b3
Reviewed-on: https://boringssl-review.googlesource.com/2051
Reviewed-by: Adam Langley <agl@google.com>
Notably, this would have caught ed8270a55c
(although, apart from staring at code coverage, knowing to set resumeSession on
the server test isn't exactly obvious). Perhaps we should systematically set it
on all extension server tests; ClientHello extension parsing happens after
resumption has been determined and is often sensitive to it.
Change-Id: Ie83f294a26881a6a41969e9dbd102d0a93cb68b5
Reviewed-on: https://boringssl-review.googlesource.com/1750
Reviewed-by: Adam Langley <agl@google.com>
