Commit Graph

127 Commits

Author SHA1 Message Date
David Benjamin
67d1fb59ad Test that client cipher preferences are enforced.
Change-Id: I6e760cfd785c0c5688da6f7d3d3092a8add40409
Reviewed-on: https://boringssl-review.googlesource.com/4070
Reviewed-by: Adam Langley <agl@google.com>
2015-03-19 22:44:49 +00:00
David Benjamin
3c9746a6d7 Regression test for CVE-2015-0291.
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>
2015-03-19 19:52:59 +00:00
David Benjamin
cdea40c3e2 Add tests for full handshakes under renegotiation.
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>
2015-03-19 19:51:16 +00:00
Adam Langley
524e717b87 Add a callback for DDoS protection.
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>
2015-03-18 19:53:29 +00:00
David Benjamin
dc3da93899 Process alerts between ChangeCipherSpec and Finished.
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>
2015-03-13 20:19:11 +00:00
David Benjamin
7538122ca6 Rework DTLS handshake message reassembly logic.
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>
2015-03-10 00:56:45 +00:00
David Benjamin
a4e6d48749 runner: Move Finished special-case into dtlsWriteRecord.
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>
2015-03-06 18:55:58 +00:00
David Benjamin
7eaab4cd57 Only retransmit on Finished if frag_off == 0.
If the peer fragments Finished into multiple pieces, there is no need to
retransmit multiple times.

Change-Id: Ibf708ad079e1633afd420ff1c9be88a80020cba9
Reviewed-on: https://boringssl-review.googlesource.com/3762
Reviewed-by: Adam Langley <agl@google.com>
2015-03-06 18:55:47 +00:00
David Benjamin
a3e894921e Test that we reject RSA ServerKeyExchange more thoroughly.
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>
2015-02-26 21:26:37 +00:00
David Benjamin
bcb2d91e10 Actually check that the message has the expected type in DTLS.
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>
2015-02-25 21:23:48 +00:00
David Benjamin
6f5c0f4471 Add tests for installing the certificate on the early callback.
Test both asynchronous and synchronous versions. This callback is somewhat
different from others. It's NOT called a second time when the handshake is
resumed. This appears to be intentional and not a mismerge from the internal
patch. The caller is expected to set up any state before resuming the handshake
state machine.

Also test the early callback returning an error.

Change-Id: If5e6eddd7007ea5cdd7533b4238e456106b95cbd
Reviewed-on: https://boringssl-review.googlesource.com/3590
Reviewed-by: Adam Langley <agl@google.com>
2015-02-25 21:22:35 +00:00
David Benjamin
87c8a643e1 Use TCP sockets rather than socketpairs in the SSL tests.
This involves more synchronization with child exits as the kernel no longer
closes the pre-created pipes for free, but it works on Windows. As long as
TCP_NODELAY is set, the performance seems comparable. Though it does involve
dealing with graceful socket shutdown. I couldn't get that to work on Windows
without draining the socket; not even SO_LINGER worked. Current (untested)
theory is that Windows refuses to gracefully shutdown a socket if the peer
sends data after we've stopped reading.

cmd.ExtraFiles doesn't work on Windows; it doesn't use fds natively, so you
can't pass fds 4 and 5. (stdin/stdout/stderr are special slots in
CreateProcess.) We can instead use the syscall module directly and mark handles
as inheritable (and then pass the numerical values out-of-band), but that
requires synchronizing all of our shim.Start() calls and assuming no other
thread is spawning a process.

PROC_THREAD_ATTRIBUTE_HANDLE_LIST fixes threading problems, but requires
wrapping more syscalls.  exec.Cmd also doesn't let us launch the process
ourselves. Plus it still requires every handle in the list be marked
inheritable, so it doesn't help if some other thread is launching a process
with bInheritHandles TRUE but NOT using PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
(Like Go, though we can take syscall.ForkLock there.)

http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx

The more natively Windows option seems to be named pipes, but that too requires
wrapping more system calls. (To be fair, that isn't too painful.) They also
involve a listening server, so we'd still have to synchronize with shim.Wait()
a la net.TCPListener.

Then there's DuplicateHandle, but then we need an out-of-band signal.

All in all, one cross-platform implementation with a TCP sockets seems
simplest.

Change-Id: I38233e309a0fa6814baf61e806732138902347c0
Reviewed-on: https://boringssl-review.googlesource.com/3563
Reviewed-by: Adam Langley <agl@google.com>
2015-02-23 19:59:06 +00:00
David Benjamin
195dc78c6e Allow False Start only for >= TLS 1.2 && AEAD && forward-secure && ALPN/NPN.
Tighten up the requirements for False Start. At this point, neither
AES-CBC or RC4 are something that we want to use unless we're sure that
the server wants to speak them.

Rebase of original CL at: https://boringssl-review.googlesource.com/#/c/1980/

BUG=427721

Change-Id: I9ef7a596edeb8df1ed070aac67c315b94f3cc77f
Reviewed-on: https://boringssl-review.googlesource.com/3501
Reviewed-by: Adam Langley <agl@google.com>
2015-02-19 18:32:39 +00:00
David Benjamin
5f237bc843 Add support for Chromium's JSON test result format.
Also adds a flag to runner.go to make it more suitable for printing to a pipe.

Change-Id: I26fae21f3e4910028f6b8bfc4821c8c595525504
Reviewed-on: https://boringssl-review.googlesource.com/3490
Reviewed-by: Adam Langley <agl@google.com>
2015-02-17 23:37:12 +00:00
David Benjamin
a54e2e85ee Remove server-side HelloVerifyRequest support.
I found no users of this. We can restore it if needbe, but I don't expect
anyone to find it useful in its current form. The API is suspect for the same
reasons DTLSv1_listen was. An SSL object is stateful and assumes you already
have the endpoint separated out.

If we ever need it, server-side HelloVerifyRequest and DTLSv1_listen should be
implemented by a separate stateless listener that statelessly handles
cookieless ClientHello + HelloVerifyRequest. Once a ClientHello with a valid
cookie comes in, it sets up a stateful SSL object and passes control along to
that.

Change-Id: I86adc1dfb6a81bebe987784c36ad6634a9a1b120
Reviewed-on: https://boringssl-review.googlesource.com/3480
Reviewed-by: Adam Langley <agl@google.com>
2015-02-17 20:50:08 +00:00
David Benjamin
9e128b06a1 Fix memory leak on malloc failure.
Found by running malloc tests with -valgrind. Unfortunately, the next one is
deep in crypto/asn1 itself, so I'm going to stop here for now.

Change-Id: I7a33971ee07c6b7b7a98715f2f18e0f29380c0a1
Reviewed-on: https://boringssl-review.googlesource.com/3350
Reviewed-by: Adam Langley <agl@google.com>
2015-02-10 01:23:34 +00:00
David Benjamin
95695c8d88 runner: Ignore dtlsFlushHandshake failures.
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>
2015-02-09 20:01:41 +00:00
David Benjamin
931ab3484f Fix handshake check when False Start is used with implicit read.
It may take up to two iterations of s->handshake_func before it is safe to
continue. Fortunately, even if anything was using False Start this way
(Chromium doesn't), we don't inherit NSS's security bug. The "redundant" check
in the type match case later on in this function saves us.

Amusingly, the success case still worked before this fix. Even though we fall
through to the post-handshake codepath and get a handshake record while
"expecting" app data, the handshake state machine is still pumped thanks to a
codepath meant for renego!

Change-Id: Ie129d83ac1451ad4947c4f86380879db8a3fd924
Reviewed-on: https://boringssl-review.googlesource.com/3335
Reviewed-by: Adam Langley <agl@google.com>
2015-02-09 19:52:08 +00:00
David Benjamin
e0e7d0da68 Initialize the record buffers after the handshake check.
The new V2ClientHello sniff asserts, for safety, that nothing else has
initialized the record layer before it runs. However, OpenSSL allows you to
avoid explicitly calling SSL_connect/SSL_accept and instead let
SSL_read/SSL_write implicitly handshake for you. This check happens at a fairly
low-level in the ssl3_read_bytes function, at which point the record layer has
already been initialized.

Add some tests to ensure this mode works.

(Later we'll lift the handshake check to a higher-level which is probably
simpler.)

Change-Id: Ibeb7fb78e5eb75af5411ba15799248d94f12820b
Reviewed-on: https://boringssl-review.googlesource.com/3334
Reviewed-by: Adam Langley <agl@google.com>
2015-02-09 19:49:45 +00:00
David Benjamin
b80168e1b8 Test that False Start fails if the server second leg is omitted.
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>
2015-02-09 19:43:48 +00:00
David Benjamin
3fd1fbd1c8 Add test coverage for normal alert parsing.
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>
2015-02-03 21:57:02 +00:00
David Benjamin
ddb9f15e18 Reject all invalid records.
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>
2015-02-03 21:55:53 +00:00
David Benjamin
afbc63fc2f Simplify DTLS epoch rewind.
SSL_AEAD_CTX ownership is currently too confusing. Instead, rely on the lack of
renego, so the previous epoch always uses the NULL cipher. (Were we to support
DTLS renego, we could keep track of s->d1->last_aead_write_ctx like
s->d1->last_write_sequence, but it isn't worth it.)

Buffered messages also tracked an old s->session, but this is unnecessary. The
s->session NULL check in tls1_enc dates to the OpenSSL initial commit and is
redundant with the aead NULL check.

Change-Id: I9a510468d95934c65bca4979094551c7536980ae
Reviewed-on: https://boringssl-review.googlesource.com/3234
Reviewed-by: Adam Langley <agl@google.com>
2015-02-03 20:34:06 +00:00
David Benjamin
0ea8dda93e Remove alert_fragment and handshake_fragment.
Nothing recognized through those codepaths is fragmentable in DTLS. Also remove
an unnecessary epoch check. It's not possible to process a record from the
wrong epoch.

Change-Id: I9d0f592860bb096563e2bdcd2c8e50a0d2b65f59
Reviewed-on: https://boringssl-review.googlesource.com/3232
Reviewed-by: Adam Langley <agl@google.com>
2015-02-03 19:10:08 +00:00
David Benjamin
b3774b9619 Add initial handshake reassembly tests.
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>
2015-02-03 19:05:30 +00:00
David Benjamin
d660b57208 runner: Refactor handshake fragmenting slightly.
No behavior change. This is in preparation for buffering a flight of handshake
messages to reorder vigorously on flush.

Change-Id: Ic348829b340bf58d28f332027646559cb11046ac
Reviewed-on: https://boringssl-review.googlesource.com/3215
Reviewed-by: Adam Langley <agl@google.com>
2015-02-03 00:43:13 +00:00
David Benjamin
83f9040339 Add DTLS timeout and retransmit tests.
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>
2015-02-03 00:40:58 +00:00
David Benjamin
d9b091b5e2 Revert "Drop retransmits in DTLS tests."
This reverts commit c67a3ae6ba. With a
deterministic clock, we can now go back to being strict about retransmits. Our
tests will now require that the shim only retransmit when we expect it to.

Change-Id: Iab1deb9665dcd294790c8253d920089e83a9140c
Reviewed-on: https://boringssl-review.googlesource.com/3211
Reviewed-by: Adam Langley <agl@google.com>
2015-02-03 00:39:57 +00:00
David Benjamin
6ae7f072e3 Only send sigalgs extension in 1.2-capable ClientHellos.
BUG=https://code.google.com/p/webrtc/issues/detail?id=4223

Change-Id: I88eb036fdc6da17bc6a5179df02f35486abe9add
Reviewed-on: https://boringssl-review.googlesource.com/3030
Reviewed-by: Adam Langley <agl@google.com>
2015-01-26 18:45:04 +00:00
David Benjamin
4189bd943c Test application data and Finished reordering.
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>
2015-01-26 18:43:02 +00:00
David Benjamin
5fa3eba03d Clear the error queue when dropping a bad DTLS packet.
This regressed in e95d20dcb8. EVP_AEAD will push
errors on the error queue (unlike the EVP_CIPHER codepath which checked
everything internally to ssl/ and didn't bother pushing anything). This meant
that a dropped packet would leave junk in the error queue.

Later, when SSL_read returns <= 0 (EOF or EWOULDBLOCK), the non-empty error
queue check in SSL_get_error kicks in and SSL_read looks to have failed.

BUG=https://code.google.com/p/webrtc/issues/detail?id=4214

Change-Id: I1e5e41c77a3e5b71e9eb0c72294abf0da677f840
Reviewed-on: https://boringssl-review.googlesource.com/2982
Reviewed-by: Adam Langley <agl@google.com>
2015-01-22 22:06:40 +00:00
David Benjamin
6095de8da2 Add tests for certificate mismatch.
Cover another mildly interesting error case.

Change-Id: Ice773af79f5e03f39f0cd2a9e158bae03e065392
Reviewed-on: https://boringssl-review.googlesource.com/2841
Reviewed-by: Adam Langley <agl@google.com>
2015-01-14 21:51:17 +00:00
David Benjamin
c67a3ae6ba Drop retransmits in DTLS tests.
BoringSSL currently retransmits non-deterministically on an internal timer
(rather than one supplied externally), so the tests currently fail flakily
depending on timing. Valgrind is a common source for this. We still assume an
in-order and reliable channel, but drop retransmits silently:

- Handshake messages may arrive with old sequence numbers.

- Retransmitted CCS records arrive from the previous epoch.

- We may receive a retransmitted Finished after we believe the handshake has
  completed. (Aside: even in a real implementation, only Finished is possible
  here. Even with out-of-order delivery, retransmitted or reordered messages
  earlier in the handshake come in under a different epoch.)

Note that because DTLS renego and a Finished retransmit are ambiguous at the
record layer[*], this precludes us writing tests for DTLS renego. But DTLS
renego should get removed anyway. As BoringSSL currently implements renego,
this ambiguity is also a source of complexity in the real implementation. (See
the SSL3_MT_FINISHED check in dtls1_read_bytes.)

[*] As a further fun aside, it's also complex if dispatching renego vs Finished
after handshake message reassembly. The spec doesn't directly say the sequence
number is reset across renegos, but it says "The first message each side
transmits in /each/ handshake always has message_seq = 0". This means that such
an implementation needs the handshake message reassembly logic be aware that a
Finished fragment with high sequence number is NOT an out-of-order fragment for
the next handshake.

Change-Id: I35d13560f82bcb5eeda62f4de1571d28c818cc36
Reviewed-on: https://boringssl-review.googlesource.com/2770
Reviewed-by: Adam Langley <agl@google.com>
2015-01-14 21:13:05 +00:00
David Benjamin
e95d20dcb8 Support EVP_AEAD in DTLS.
This CL removes the last of the EVP_CIPHER codepath in ssl/. The dead code is
intentionally not pruned for ease of review, except in DTLS-only code where
adding new logic to support both, only to remove half, would be cumbersome.

Fixes made:
- dtls1_retransmit_state is taught to retain aead_write_ctx rather than
  enc_write_ctx.
- d1_pkt.c reserves space for the variable-length nonce when echoed into the
  packet.
- dtls1_do_write sizes the MTU based on EVP_AEAD max overhead.
- tls1_change_cipher_state_cipher should not free AEAD write contexts in DTLS.
  This matches the (rather confused) ownership for the EVP_CIPHER contexts.
  I've added a TODO to resolve this craziness.

A follow-up CL will remove all the resultant dead code.

Change-Id: I644557f4db53bbfb182950823ab96d5e4c908866
Reviewed-on: https://boringssl-review.googlesource.com/2699
Reviewed-by: Adam Langley <agl@google.com>
2015-01-14 21:03:40 +00:00
David Benjamin
13be1de469 Add a basic MTU test.
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>
2015-01-12 22:37:25 +00:00
David Benjamin
780d6dd0fe Treat handshake_failure in response to ClientHello special.
Add a dedicated error code to the queue for a handshake_failure alert in
response to ClientHello. This matches NSS's client behavior and gives a better
error on a (probable) failure to negotiate initial parameters.

BUG=https://crbug.com/446505

Change-Id: I34368712085a6cbf0031902daf2c00393783d96d
Reviewed-on: https://boringssl-review.googlesource.com/2751
Reviewed-by: Adam Langley <agl@google.com>
2015-01-06 18:31:49 +00:00
David Benjamin
87909c0445 Add tests for version negotiation failure alerts.
Ensure that both the client and the server emit a protocol_version alert
(except in SSLv3 where it doesn't exist) with a record-layer version which the
peer will recognize.

Change-Id: I31650a64fe9b027ff3d51e303711910a00b43d6f
2014-12-13 15:23:28 -08:00
David Benjamin
82c9e90a58 Merge SSLv23_method and DTLS_ANY_VERSION.
This makes SSLv23_method go through DTLS_ANY_VERSION's version negotiation
logic. This allows us to get rid of duplicate ClientHello logic. For
compatibility, SSL_METHOD is now split into SSL_PROTOCOL_METHOD and a version.
The legacy version-locked methods set min_version and max_version based this
version field to emulate the original semantics.

As a bonus, we can now handle fragmented ClientHello versions now.

Because SSLv23_method is a silly name, deprecate that too and introduce
TLS_method.

Change-Id: I8b3df2b427ae34c44ecf972f466ad64dc3dbb171
2014-12-13 15:22:21 -08:00
David Benjamin
accb454e44 Add min_version tests.
These tests use both APIs. This also modifies the inline version negotiation's
error codes (currently only used for DTLS) to align with SSLv23's error codes.
Note: the peer should send a protocol_version alert which is currently untested
because it's broken.

Upstream would send such an alert if TLS 1.0 was supported but not otherwise,
which is somewhat bizarre. We've actually regressed and never send the alert in
SSLv23. When version negotiation is unified, we'll get the alerts back.

Change-Id: I4c77bcef3a3cd54a039a642f189785cd34387410
Reviewed-on: https://boringssl-review.googlesource.com/2584
Reviewed-by: Adam Langley <agl@google.com>
2014-12-13 23:00:02 +00:00
David Benjamin
1eb367c03e Add min_version and max_version APIs.
Amend the version negotiation tests to test this new spelling of max_version.
min_version will be tested in a follow-up.

Change-Id: Ic4bfcd43bc4e5f951140966f64bb5fd3e2472b01
Reviewed-on: https://boringssl-review.googlesource.com/2583
Reviewed-by: Adam Langley <agl@google.com>
2014-12-13 22:48:20 +00:00
David Benjamin
f080ecd86d Don't infinite loop on garbage server input.
else block got lost in a rewrite of this code.

Change-Id: I51f1655474ec8bbd4eccb4297124e8584329444e
Reviewed-on: https://boringssl-review.googlesource.com/2560
Reviewed-by: Adam Langley <agl@google.com>
2014-12-11 23:55:38 +00:00
David Benjamin
226a872d2f Don't set client_version to the ServerHello version.
The client_version needs to be preserved, both for the RSA key exchange and
(when this codepath is used for TLS) for the SChannel renego workaround. Fix
the tests to enforce this so the cipher suite version tests catch this.

Change-Id: I0c42dc3ec4830f3724026b400e5066e7a7f1ee97
Reviewed-on: https://boringssl-review.googlesource.com/2551
Reviewed-by: Adam Langley <agl@google.com>
2014-12-11 18:49:42 +00:00
David Benjamin
1e29a6b7c5 Add assertions on the initial record version number.
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>
2014-12-11 00:04:37 +00:00
Feng Lu
41aa325c6a ClientHello Padding for Fast Radio Opening in 3G.
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
2014-12-04 14:30:16 -08:00
David Benjamin
94d701b7e8 Left-pad a V2ClientHello's random, not right-pad.
The comment has it right, but the rewritten code was wrong.

Change-Id: I450193c39fb62eae32aae090a3834dd83db53421
Reviewed-on: https://boringssl-review.googlesource.com/2444
Reviewed-by: Adam Langley <agl@google.com>
2014-12-02 19:44:12 +00:00
David Benjamin
128dbc30f6 Factor out the client max-version logic into a helper function.
Replace the comment with a clearer one and reimplement it much more tidily. The
mask thing was more complicated than was needed.

This slightly changes behavior on the DTLS_ANY_VERSION side in that, if only
one method is enabled, we no longer short-circuit to the version-locked method
early. This "optimization" seems unnecessary.

Change-Id: I571c8b60ed16bd4357c67d65df0dd1ef9cc5eb57
Reviewed-on: https://boringssl-review.googlesource.com/2451
Reviewed-by: Adam Langley <agl@google.com>
2014-12-02 19:42:39 +00:00
David Benjamin
c44b1df459 Add test for renego client_version quirk.
In upstream's f4e1169341ad1217e670387db5b0c12d680f95f4, the client_version was
made constant across renegotiations, even if the server negotiated a lower
version. NSS has the same quirk, reportedly for SChannel:

https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/nss/ssl/ssl3con.c&sq=package:chromium&l=5103

Add a test to ensure we do not regress this.

Change-Id: I214e062463c203b86a9bab00f8503442e1bf74fe
Reviewed-on: https://boringssl-review.googlesource.com/2405
Reviewed-by: Adam Langley <agl@google.com>
2014-12-02 19:29:23 +00:00
David Benjamin
8b8c006564 Fix DTLS_ANY_VERSION and add tests.
This fixes bugs that kept the tests from working:

- Resolve DTLS version and cookie before the session.

- In DTLS_ANY_VERSION, ServerHello should be read with first_packet = 1. This
  is a regression from f2fedefdca. We'll want to
  do the same for TLS, but first let's change this to a boolean has_version in a
  follow-up.

Things not yet fixed:

- DTLS code is not EVP_AEAD-aware. Those ciphers are disabled for now.

- On the client, DTLS_ANY_VERSION creates SSL_SESSIONs with the wrong
  ssl_version. The tests pass because we no longer enforce the match as of
  e37216f56009fbf48c3a1e733b7a546ca6dfc2af. (In fact, we've gone from the server
  ignoring ssl_version and client enforcing to the client mostly ignoring
  ssl_version and the server enforcing.)

- ssl3_send_client_hello's ssl_version check checks for equality against
  s->version rather than >.

Change-Id: I5a0dde221b2009413df9b9443882b9bf3b29519c
Reviewed-on: https://boringssl-review.googlesource.com/2403
Reviewed-by: Adam Langley <agl@google.com>
2014-12-02 19:27:54 +00:00
David Benjamin
65ea8ff84c Debug resumption connections with -debug too.
Change-Id: Ib33cceed561698310f369d63de602123af146a45
Reviewed-on: https://boringssl-review.googlesource.com/2402
Reviewed-by: Adam Langley <agl@google.com>
2014-12-02 19:27:33 +00:00
David Benjamin
61f95277d4 Add tests for OCSP stapling and SCT lists.
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>
2014-12-02 19:26:01 +00:00