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>
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>
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>
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>
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>
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>
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>
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>
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>
Notably, drop all special cases around receiving a message in order and
receiving a full message. It makes things more complicated and was the source
of bugs (the MixCompleteMessageWithFragments tests added in this CL did not
pass before). Instead, every message goes through an hm_fragment, and
dtls1_get_message always checks buffered_messages to see if the next is
complete.
The downside is that we pay one more copy of the message data in the common
case. This is only during connection setup, so I think it's worth the
simplicity. (If we want to optimize later, we could either tighten
ssl3_get_message's interface to allow the handshake data being in the
hm_fragment's backing store rather than s->init_buf or swap out s->init_buf
with the hm_fragment's backing store when a mesasge completes.
This CL does not address ssl_read_bytes being an inappropriate API for DTLS.
Future work will revise the handshake/transport boundary to align better with
DTLS's needs. Also other problems that I've left as TODOs.
Change-Id: Ib4570d45634b5181ecf192894d735e8699b1c86b
Reviewed-on: https://boringssl-review.googlesource.com/3764
Reviewed-by: Adam Langley <agl@google.com>
The old test just sent an empty ServerKeyExchange which is sufficient as we
reject the message early. But be more thorough and implement the actual
ephemeral key logic in the test server.
Change-Id: I016658762e4502c928c051e14d69eea67b5a495f
Reviewed-on: https://boringssl-review.googlesource.com/3650
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 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>
We have test coverage for invalid alerts, but not for normal ones on the DTLS
side.
Change-Id: I359dce8d4dc80dfa99b5d8bacd73f48a8e4ac310
Reviewed-on: https://boringssl-review.googlesource.com/3291
Reviewed-by: Adam Langley <agl@google.com>
The check on the DTLS side was broken anyway. On the TLS side, the spec does
say to ignore them, but there should be no need for this in future-proofing and
NSS doesn't appear to be lenient here. See also
https://boringssl-review.googlesource.com/#/c/3233/
Change-Id: I0846222936c5e08acdcfd9d6f854a99df767e468
Reviewed-on: https://boringssl-review.googlesource.com/3290
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 minimum MTU (not consistently enforced) is just under 256, so it's
difficult to test everything, but this is a basic test. (E.g., without renego,
the only handshake message with encryption is Finished which fits in the MTU.)
It tests the server side because the Certificate message is large enough to
require fragmentation.
Change-Id: Ida11f1057cebae2b800ad13696f98bb3a7fbbc5e
Reviewed-on: https://boringssl-review.googlesource.com/2824
Reviewed-by: Adam Langley <agl@google.com>
The record-layer version of the ServerHello should match the final version. The
record-layer version of the ClientHello should be the advertised version, but
clamped at TLS 1.0. This is to ensure future rewrites do not regress this.
Change-Id: I96f1f0674944997ff38b562453a322ce61652635
Reviewed-on: https://boringssl-review.googlesource.com/2540
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>
Prior to this change, BoringSSL maintained a 2-byte buffer for alerts,
and would support reassembly of fragmented alerts.
NSS does not support fragmented alerts, nor would any reasonable
implementation produce them. Remove fragmented alert handling and
produce an error if a fragmented alert has ever been encountered.
Change-Id: I31530ac372e8a90b47cf89404630c1c207cfb048
Reviewed-on: https://boringssl-review.googlesource.com/2125
Reviewed-by: Adam Langley <agl@google.com>
These'll get removed once most of renego support is gone, but this is to prove
removing the warning alert from the previous commit still prevents legacy
renegotiations.
Change-Id: I7d9d95e1d4c5d23d3b6d170938a5499a65f2d5ea
Reviewed-on: https://boringssl-review.googlesource.com/2236
Reviewed-by: Adam Langley <agl@google.com>
Ensure that the client rejects it with UNEXPECTED_MESSAGE, not by attempting to
decode it.
Change-Id: Ifc5613cf1152e0f7dcbee73e05df1ef367dfbfd5
Reviewed-on: https://boringssl-review.googlesource.com/2232
Reviewed-by: Adam Langley <agl@google.com>
At the record layer, DTLS maintains a window of seen sequence numbers to detect
replays. Add tests to cover that case. Test both repeated sequence numbers
within the window and sequence numbers past the window's left edge. Also test
receiving sequence numbers far past the window's right edge.
Change-Id: If6a7a24869db37fdd8fb3c4b3521b730e31f8f86
Reviewed-on: https://boringssl-review.googlesource.com/2221
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>
