Commit Graph

89 Commits

Author SHA1 Message Date
David Benjamin
79978df4ec Move aead_{read,write}_ctx and next_proto_negotiated into ssl->s3.
Both are connection state rather than configuration state. Notably this
cuts down more of SSL_clear that can't just use ssl_free + ssl_new.

Change-Id: I3c05b3ae86d4db8bd75f1cd21656f57fc5b55ca9
Reviewed-on: https://boringssl-review.googlesource.com/6835
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 21:40:25 +00:00
David Benjamin
57997da8ee Simplify the ChangeCipherSpec logic.
It's the same between TLS and SSL 3.0. There's also no need for the
do_change_cipher_spec wrapper (it no longer needs checks to ensure it
isn't called at a bad place). Finally fold the setup_key_block call into
change_cipher_spec.

Change-Id: I7917f48e1a322f5fbafcf1dfb8ad53f66565c314
Reviewed-on: https://boringssl-review.googlesource.com/6834
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 21:33:57 +00:00
David Benjamin
0623bceb25 Fill in ssl->session->cipher when resumption is resolved.
Doing it at ChangeCipherSpec makes it be set twice and, more
importantly, causes us to touch SSL_SESSION objects on resumption. (With
a no-op change, but this still isn't a good idea.)

This should actually let us get rid of ssl->s3->tmp.new_cipher but some
of external code accesses that field directly.

Change-Id: Ia6b7e0964c1b430f963ad0b1a5417b339b7b19d3
Reviewed-on: https://boringssl-review.googlesource.com/6833
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 20:46:45 +00:00
David Benjamin
0d56f888c3 Switch s to ssl everywhere.
That we're half and half is really confusing.

Change-Id: I1c2632682e8a3e63d01dada8e0eb3b735ff709ce
Reviewed-on: https://boringssl-review.googlesource.com/6785
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 23:28:22 +00:00
David Benjamin
7fc010014c Slightly simplify SSL3_RECORD.
There's no need to track consumed bytes, so rr->data and rr->off may be
merged together.

Change-Id: I8842d005665ea8b4d4a0cced941f3373872cdac4
Reviewed-on: https://boringssl-review.googlesource.com/6644
Reviewed-by: Adam Langley <agl@google.com>
2015-12-16 18:41:59 +00:00
David Benjamin
a41280d8cb Pull ChangeCipherSpec into the handshake state machine.
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>
2015-12-16 18:36:57 +00:00
David Benjamin
8fd5c23218 Simplify fragmented HelloRequest state.
With server-side renegotiation gone, handshake_fragment's only purpose
in life is to handle a fragmented HelloRequest (we probably do need to
support those if some server does 1/n-1 record-splitting on handshake
records). The logic to route the data into
ssl3_read_bytes(SSL3_RT_HANDSHAKE) never happens, and the contents are
always a HelloRequest prefix.

This also trims a tiny bit of per-connection state.

Change-Id: Ia1b0dda5b7e79d817c28da1478640977891ebc97
Reviewed-on: https://boringssl-review.googlesource.com/6641
Reviewed-by: Adam Langley <agl@google.com>
2015-12-16 17:45:31 +00:00
Adam Langley
27a0d086f7 Add ssl_renegotiate_ignore.
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>
2015-11-03 21:58:13 +00:00
David Benjamin
2e0901b75f Don't use ssl3_write_pending in DTLS.
That function doesn't do anything useful for DTLS. It's meant for tracking the
rest of the record we've already committed to by writing half of one. But one
cannot write half a datagram, so DTLS never tracks this. Just call
ssl_write_buffer_flush straight and don't touch wpend_*.

Change-Id: Ibe191907d64c955c7cfeefba26f5c11ad5e4b939
Reviewed-on: https://boringssl-review.googlesource.com/6418
Reviewed-by: Adam Langley <agl@google.com>
2015-11-02 23:17:24 +00:00
David Benjamin
82170248e7 Document the info callback.
This callback is some combination of arguably useful stuff (bracket
handshakes, alerts) and completely insane things (find out when the
state machine advances). Deprecate the latter.

Change-Id: Ibea5b32cb360b767b0f45b302fd5f1fe17850593
Reviewed-on: https://boringssl-review.googlesource.com/6305
Reviewed-by: Adam Langley <alangley@gmail.com>
2015-10-26 18:12:22 +00:00
David Benjamin
1d5ef3bb1e Add SSL_set_renegotiate_mode.
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>
2015-10-13 18:02:28 +00:00
David Benjamin
324dce4fd7 Unbreak SSL_total_renegotiations.
The logic to update that got removed in
https://boringssl-review.googlesource.com/4825. Add tests.

Change-Id: Idc550e8fa3ce6f69a76fa65d7651adde281edba6
Reviewed-on: https://boringssl-review.googlesource.com/6220
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
Reviewed-by: Adam Langley <agl@google.com>
2015-10-13 17:53:30 +00:00
David Benjamin
9e4e01ee14 Align the SSL stack on #include style.
ssl.h should be first. Also two lines after includes and the rest of the
file.

Change-Id: Icb7586e00a3e64170082c96cf3f8bfbb2b7e1611
Reviewed-on: https://boringssl-review.googlesource.com/5892
Reviewed-by: Adam Langley <agl@google.com>
2015-09-15 23:32:07 +00:00
David Benjamin
ee0b02a10d Don't confuse TLS bidirectional shutdown on record type zero.
The bidi shutdown code uses type = 0 as a special signal value, but code
elsewhere doesn't account for this.

BUG=526437

Change-Id: I090cee421633d70ef3b84f4daa811608031b9ed9
Reviewed-on: https://boringssl-review.googlesource.com/5771
Reviewed-by: Adam Langley <agl@google.com>
2015-08-31 19:08:24 +00:00
David Benjamin
aa9361bc66 Fix discarding records in bidirectional shutdown.
When discarding a record, it's important to start reading the next one,
or the state machine retry signaling doesn't work.

BUG=526437

Change-Id: I5e4a5155310d097c0033cdf5d06712410a01ee08
Reviewed-on: https://boringssl-review.googlesource.com/5768
Reviewed-by: Adam Langley <agl@google.com>
2015-08-31 18:55:49 +00:00
David Benjamin
4cf369b920 Reject empty records of unexpected type.
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>
2015-08-28 22:03:00 +00:00
David Benjamin
b8d28cf532 Factor out the buffering and low-level record code.
This begins decoupling the transport from the SSL state machine. The buffering
logic is hidden behind an opaque API. Fields like ssl->packet and
ssl->packet_length are gone.

ssl3_get_record and dtls1_get_record now call low-level tls_open_record and
dtls_open_record functions that unpack a single record independent of who owns
the buffer. Both may be called in-place. This removes ssl->rstate which was
redundant with the buffer length.

Future work will push the buffer up the stack until it is above the handshake.
Then we can expose SSL_open and SSL_seal APIs which act like *_open_record but
return a slightly larger enum due to other events being possible. Likewise the
handshake state machine will be detached from its buffer. The existing
SSL_read, SSL_write, etc., APIs will be implemented on top of SSL_open, etc.,
combined with ssl_read_buffer_* and ssl_write_buffer_*. (Which is why
ssl_read_buffer_extend still tries to abstract between TLS's and DTLS's fairly
different needs.)

The new buffering logic does not support read-ahead (removed previously) since
it lacks a memmove on ssl_read_buffer_discard for TLS, but this could be added
if desired. The old buffering logic wasn't quite right anyway; it tried to
avoid the memmove in some cases and could get stuck too far into the buffer and
not accept records. (The only time the memmove is optional is in DTLS or if
enough of the record header is available to know that the entire next record
would fit in the buffer.)

The new logic also now actually decrypts the ciphertext in-place again, rather
than almost in-place when there's an explicit nonce/IV. (That accidentally
switched in https://boringssl-review.googlesource.com/#/c/4792/; see
3d59e04bce96474099ba76786a2337e99ae14505.)

BUG=468889

Change-Id: I403c1626253c46897f47c7ae93aeab1064b767b2
Reviewed-on: https://boringssl-review.googlesource.com/5715
Reviewed-by: Adam Langley <agl@google.com>
2015-08-28 22:01:02 +00:00
David Benjamin
3570d73bf1 Remove the func parameter to OPENSSL_PUT_ERROR.
Much of this was done automatically with
  find . -name '*.c' | xargs sed -E -i '' -e 's/(OPENSSL_PUT_ERROR\([a-zA-Z_0-9]+, )[a-zA-Z_0-9]+, ([a-zA-Z_0-9]+\);)/\1\2/'
  find . -name '*.c' | xargs sed -E -i '' -e 's/(OPENSSL_PUT_ERROR\([a-zA-Z_0-9]+, )[a-zA-Z_0-9]+,  ([a-zA-Z_0-9]+\);)/\1\2/'

BUG=468039

Change-Id: I4c75fd95dff85ab1d4a546b05e6aed1aeeb499d8
Reviewed-on: https://boringssl-review.googlesource.com/5276
Reviewed-by: Adam Langley <agl@google.com>
2015-07-16 02:02:37 +00:00
David Benjamin
24f346d77b Limit the number of warning alerts silently consumed.
Per review comments on
https://boringssl-review.googlesource.com/#/c/4112/.

Change-Id: I82cacf67c6882e64f6637015ac41945522699797
Reviewed-on: https://boringssl-review.googlesource.com/5041
Reviewed-by: Adam Langley <agl@google.com>
2015-06-08 22:16:14 +00:00
David Benjamin
a8ebe2261f Add tests for empty record limit and make it work in the async case.
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>
2015-06-08 21:45:21 +00:00
David Benjamin
a6022771b3 Split ssl_read_bytes hook into app_data and close_notify hooks.
This still needs significant work, especially the close_notify half, but
clarify the interface and get *_read_bytes out of SSL_PROTOCOL_METHOD.
read_bytes is an implementation detail of those two and get_message
rather than both an implementation detail of get_message for handshake
and a (wholly inappropriate) exposed interface for the other two.

BUG=468889

Change-Id: I7dd23869e0b7c3532ceb2e9dd31ca25ea31128e7
Reviewed-on: https://boringssl-review.googlesource.com/4956
Reviewed-by: Adam Langley <agl@google.com>
2015-06-01 22:27:55 +00:00
David Benjamin
c933a47e6f Switch the ssl_write_bytes hook to ssl_write_app_data.
The SSL_PROTOCOL_METHOD table needs work, but this makes it clearer
exactly what the shared interface between the upper later and TLS/DTLS
is.

BUG=468889

Change-Id: I38931c484aa4ab3f77964d708d38bfd349fac293
Reviewed-on: https://boringssl-review.googlesource.com/4955
Reviewed-by: Adam Langley <agl@google.com>
2015-06-01 22:18:06 +00:00
David Benjamin
bd15a8e748 Fix DTLS handling of multiple records in a packet.
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>
2015-05-29 22:59:38 +00:00
David Benjamin
cd90f3a241 Remove renegotiation deferral logic.
When the peer or caller requests a renegotiation, OpenSSL doesn't
renegotiate immediately. It sets a flag to begin a renegotiation as soon
as record-layer read and write buffers are clear. One reason is that
OpenSSL's record layer cannot write a handshake record while an
application data record is being written. The buffer consistency checks
around partial writes will break.

None of these cases are relevant for the client auth hack. We already
require that renego come in at a quiescent part of the application
protocol by forbidding handshake/app_data interleave.

The new behavior is now: when a HelloRequest comes in, if the record
layer is not idle, the renegotiation is rejected as if
SSL_set_reject_peer_renegotiations were set. Otherwise we immediately
begin the new handshake. The server may not send any application data
between HelloRequest and completing the handshake. The HelloRequest may
not be consumed if an SSL_write is pending.

Note this does require that Chromium's HTTP stack not attempt to read
the HTTP response until the request has been written, but the
renegotiation logic already assumes it. Were Chromium to drive the
SSL_read state machine early and the server, say, sent a HelloRequest
after reading the request headers but before we've sent the whole POST
body, the SSL state machine may racily enter renegotiate early, block
writing the POST body on the new handshake, which would break Chromium's
ERR_SSL_CLIENT_AUTH_CERT_NEEDED plumbing.

BUG=429450

Change-Id: I6278240c3bceb5d2e1a2195bdb62dd9e0f4df718
Reviewed-on: https://boringssl-review.googlesource.com/4825
Reviewed-by: Adam Langley <agl@google.com>
2015-05-21 20:50:43 +00:00
David Benjamin
44d3eed2bb Forbid caller-initiated renegotiations and all renego as a servers.
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>
2015-05-21 20:43:56 +00:00
David Benjamin
9a41d1b946 Deprecate SSL_*_read_ahead and enforce DTLS packet boundaries.
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>
2015-05-21 18:29:34 +00:00
David Benjamin
31a07798a5 Factor SSL_AEAD_CTX into a dedicated type.
tls1_enc is now SSL_AEAD_CTX_{open,seal}. This starts tidying up a bit
of the record-layer logic. This removes rr->input, as encrypting and
decrypting records no longer refers to various globals. It also removes
wrec altogether. SSL3_RECORD is now only used to maintain state about
the current incoming record. Outgoing records go straight to the write
buffer.

This also removes the outgoing alignment memcpy and simply calls
SSL_AEAD_CTX_seal with the parameters as appropriate. From bssl speed
tests, this seems to be faster on non-ARM and a bit of a wash on ARM.

Later it may be worth recasting these open/seal functions to write into
a CBB (tweaked so it can be malloc-averse), but for now they take an
out/out_len/max_out trio like their EVP_AEAD counterparts.

BUG=468889

Change-Id: Ie9266a818cc053f695d35ef611fd74c5d4def6c3
Reviewed-on: https://boringssl-review.googlesource.com/4792
Reviewed-by: Adam Langley <agl@google.com>
2015-05-21 17:59:15 +00:00
David Benjamin
4b27d9f8bd Never resume sessions on renegotiations.
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>
2015-05-14 22:53:21 +00:00
David Benjamin
897e5e0013 Default renegotiations to off.
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>
2015-05-13 17:02:14 +00:00
David Benjamin
6a08da2cf8 Remove redundant setup buffer calls.
Nothing should call ssl3_setup_read_buffer or ssl3_setup_write_buffer unless it
intends to write into the buffer. This way buffer management can later be an
implementation detail of the record layer.

Change-Id: Idb0effba00e77c6169764843793f40ec37868b61
Reviewed-on: https://boringssl-review.googlesource.com/4687
Reviewed-by: Adam Langley <agl@google.com>
2015-05-11 21:31:59 +00:00
David Benjamin
b1f5bca538 Remove max parameter to ssl3_read_n.
It's completely redundant with the extend bit. If extend is 0, we're reading a
new record, and rbuf.len is passed. Then it needs to get clamped by ssl3_read_n
post alignment anyway. If extend is 1, we're reading the rest of the current
record and max is always n. (For TLS, we actually could just read more, but not
for DTLS. Basically no one sets it on the TLS side of things, so instead, after
WebRTC's broken DTLS handling is fixed, read_ahead can go away altogether and
DTLS/TLS record layers can be separated.)

This removes ssl3_read_n's callers' dependency on ssl3_setup_read_buffer
setting up rbuf.len.

Change-Id: Iaf11535d01017507a52a33b19240f42984d6cf52
Reviewed-on: https://boringssl-review.googlesource.com/4686
Reviewed-by: Adam Langley <agl@google.com>
2015-05-11 18:41:41 +00:00
David Benjamin
9417b7649f Remove DTLS special-cases in buffer releasing.
They date to https://rt.openssl.org/Ticket/Display.html?id=2533, but no
particularly good justification was given for them. It seems it was just a
bandaid because d1_pkt.c forgot to initialize the buffer. I went through
codesearch for all accesses to SSL3_BUFFER::buf and SSL::packet. They seem
appropriately guarded but for this one.

Change-Id: Ife4e7afdb7a7c137d6be4791542eb5de6dd5b1b6
Reviewed-on: https://boringssl-review.googlesource.com/4685
Reviewed-by: Adam Langley <agl@google.com>
2015-05-11 18:40:04 +00:00
David Benjamin
aebefed905 Always enable SSL_MODE_RELEASE_BUFFERS.
There's no real need to ever disable it, so this is one fewer configuration to
test. It's still disabled for DTLS, but a follow-up will resolve that.

Change-Id: Ia95ad8c17ae8236ada516b3968a81c684bf37fd9
Reviewed-on: https://boringssl-review.googlesource.com/4683
Reviewed-by: Adam Langley <agl@google.com>
2015-05-11 18:39:09 +00:00
David Benjamin
b16346b0ad Add SSL_set_reject_peer_renegotiations.
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>
2015-04-13 22:38:58 +00:00
David Benjamin
f0ae170021 Include-what-you-use ssl/internal.h.
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>
2015-04-10 22:15:02 +00:00
David Benjamin
2ee94aabf5 Rename ssl_locl.h to internal.h
Match the other internal headers.

Change-Id: Iff7e2dd06a1a7bf993053d0464cc15638ace3aaa
Reviewed-on: https://boringssl-review.googlesource.com/4280
Reviewed-by: Adam Langley <agl@google.com>
2015-04-10 22:14:09 +00:00
David Benjamin
ef4962f5a3 Shush warning in alignment code.
MSVC doesn't like unary - on unsigned numbers. Also switch ssl3_read_n's
version to uintptr_t to match the write half. This gets us closer to clearing
through C4311 violations. (The remaining one is in asn1_add_error which can go
after verifying that most of asn1_mac.h is safe to drop.)

Change-Id: Idb33dda8863bf1a3408b14d5513a667338311b6b
Reviewed-on: https://boringssl-review.googlesource.com/4255
Reviewed-by: Adam Langley <agl@google.com>
2015-04-07 00:40:50 +00:00
David Benjamin
883e49fdd8 Remove dead code in do_dtls1_write and document another bug.
Change-Id: I250d3cf5b8124f205d67268958a01cb02a6d05ac
Reviewed-on: https://boringssl-review.googlesource.com/4240
Reviewed-by: Adam Langley <agl@google.com>
2015-04-06 21:39:58 +00:00
David Benjamin
bc746e3e9c Don't switch s->version on record-layer version mismatch.
At this point, has_version has been set and we may even have a non-null cipher.
Trying to assign meaning to the record-layer version number is not worth making
s->version's semantics even more complicated.

Change-Id: Ia1cf341cf7306eb48d2d11241316dc2116306968
Reviewed-on: https://boringssl-review.googlesource.com/4237
Reviewed-by: Adam Langley <agl@google.com>
2015-04-06 20:50:54 +00:00
David Benjamin
4a3f0732fd Tidy record length check.
Compression is gone, so don't allow for compression overhead. With that fixed,
the second rr->length check in ssl3_get_record matches the length computation
which sizes the read buffer. The first is wrong and doesn't account for the
alignment padding. Move the second to the first.

Change-Id: I3f4f05de9fdf5c645ff24493bbfdf303dcc1aa90
Reviewed-on: https://boringssl-review.googlesource.com/4236
Reviewed-by: Adam Langley <agl@google.com>
2015-04-06 20:50:45 +00:00
David Benjamin
9faafdaeb8 Clean up do_ssl3_write fragment handling.
Separate actually writing the fragment to the network from assembling it so
there is no need for is_fragment. record_split_done also needn't be a global;
as of 7fdeaf1101, it is always reset to 0 whether
or not SSL3_WANT_WRITE occurred, despite the comment.

I believe this is sound, but the pre-7fdeaf1 logic wasn't quiiite right;
ssl3_write_pending allows a retry to supply *additional* data, so not all
plaintext had been commited to before the IV was randomized. We could fix this
by tracking how many bytes were committed to the last time we fragmented, but
this is purely an optimization and doesn't seem worth the complexity.

This also fixes the alignment computation in the record-splitting case. The
extra byte was wrong, as demonstrated by the assert.

Change-Id: Ia087a45a6622f4faad32e501942cc910eca1237b
Reviewed-on: https://boringssl-review.googlesource.com/4234
Reviewed-by: Adam Langley <agl@google.com>
2015-04-06 18:53:15 +00:00
David Benjamin
a58c57822e Simplify the pointer management around do_ssl3_write.
It's still rather a mess, but this is at least somewhat clearer. The old one
had a lot of remnants of compression, etc.

Change-Id: Iffcb4dd4e8c4ab14f60abf917d22b7af960c93ba
Reviewed-on: https://boringssl-review.googlesource.com/4233
Reviewed-by: Adam Langley <agl@google.com>
2015-04-06 18:17:55 +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
Adam Langley
c4482d65dc Switch an assert back to a check.
The assert was supposed to be *added* in fcf25833 but instead replaced
the check.

BUG=465557

Change-Id: I0d3db5038515021e5bdd1ccb9ff08d4f78552621
Reviewed-on: https://boringssl-review.googlesource.com/3850
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
2015-03-10 02:15:50 +00:00
David Benjamin
86058a256b Tidy up the alert-parsing code.
Align the DTLS and TLS implementations more. s3_pkt.c's version still has
remnants of fragmentable alerts and only one side marks some variables as
const. Also use warning/fatal constants rather than the numbers with comments.

Change-Id: Ie62d3af1747b6fe4336496c047dfccc9d71fde3f
Reviewed-on: https://boringssl-review.googlesource.com/3562
Reviewed-by: Adam Langley <agl@google.com>
2015-02-23 19:34:52 +00:00
David Benjamin
ed7c475154 Rename cutthrough to False Start.
False Start is the name it's known by now. Deprecate the old API and expose new
ones with the new name.

Change-Id: I32d307027e178fd7d9c0069686cc046f75fdbf6f
Reviewed-on: https://boringssl-review.googlesource.com/3481
Reviewed-by: Adam Langley <agl@google.com>
2015-02-17 20:51:22 +00:00
David Benjamin
ccf74f8085 Revise SSL_cutthrough_complete and SSL_in_init.
This makes the following changes:

- SSL_cutthrough_complete no longer rederives whether cutthrough happened and
  just maintains a handshake bit.

- SSL_in_init no longer returns true if we are False Starting but haven't
  completed the handshake. That logic was awkward as it depended on querying
  in_read_app_data to force SSL_read to flush the entire handshake. Defaulting
  SSL_in_init to continue querying the full handshake and special-casing
  SSL_write is better. E.g. the check in bidirectional SSL_shutdown wants to know
  if we're in a handshake. No internal consumer of
  SSL_MODE_HANDSHAKE_CUTTHROUGH ever queries SSL_in_init directly.

- in_read_app_data is gone now that the final use is dead.

Change-Id: I05211a116d684054dfef53075cd277b1b30623b5
Reviewed-on: https://boringssl-review.googlesource.com/3336
Reviewed-by: Adam Langley <agl@google.com>
2015-02-09 20:00:43 +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
e820df9371 Forbid interleaving between application data and handshake protocols.
This is the source of much of renegotiation's complexity, and of OpenSSL's
implementation of it. In practice, we only care about renegotiation because of
the client auth hack. There, we can safely assume that no server will send
application data between sending the HelloRequest and completing the handshake.

BUG=429450

Change-Id: I37f5abea5fdedb1d53e24ceb11f71287c74bb777
Reviewed-on: https://boringssl-review.googlesource.com/3332
Reviewed-by: Adam Langley <agl@google.com>
2015-02-09 19:42:44 +00:00