Commit Graph

117 Commits

Author SHA1 Message Date
David Benjamin
c027999c28 Take the version parameter out of ssl_do_msg_callback.
This will make it a little easier to store the normalized version rather
than the wire version. Also document the V2ClientHello behavior.

Change-Id: I5ce9ccce44ca48be2e60ddf293c0fab6bba1356e
Reviewed-on: https://boringssl-review.googlesource.com/11121
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-09-21 18:55:27 +00:00
David Benjamin
33dad1b7a1 Stop pretending to ssl_clear_bad_session.
We broke this to varying degrees ages ago.

This is the logic to implement the variations of rules in TLS to discard
sessions after a failed connection, where a failed connection could be
one of:

- A connection that was not cleanly shut down.

- A connection that received a fatal alert.

The first one is nonsense since close_notify does not actually work in
the real world. The second is a vaguely more plausible but...

- A stateless ticket-based server can't drop sessions anyway.

- In TLS 1.3, a client may receive many tickets over the lifetime of a
  single connection. With an external session cache like ours which may,
  in theory, but multithreaded, this will be a huge hassle to track.

- A client may well attempt to establish a connection and reuse the
  session before we receive the fatal alert, so any application state we
  hope to manage won't really work.

- An attacker can always close the connection before the fatal alert, so
  whatever security policy clearing the session gave is easily
  bypassable.

Implementation-wise, this has basically never worked. The
ssl_clear_bad_session logic called into SSL_CTX_remove_session which
relied on the internal session cache. (Sessions not in the internal
session cache don't get removed.) The internal session cache was only
useful for a server, where tickets prevent this mechanism from doing
anything. For a client, we since removed the internal session cache, so
nothing got removed. The API for a client also did not work as it gave
the SSL_SESSION, not the SSL, so a consumer would not know the key to
invalidate anyway.

The recent session state splitting change further broke this.

Moreover, calling into SSL_CTX_remove_session logic like that is
extremely dubious because it mutates the not_resumable flag on the
SSL_SESSION which isn't thread-safe.

Spec-wise, TLS 1.3 has downgraded the MUST to a SHOULD.

Given all that mess, just remove this code. It is no longer necessary to
call SSL_shutdown just to make session caching work.

Change-Id: Ib601937bfc5f6b40436941e1c86566906bb3165d
Reviewed-on: https://boringssl-review.googlesource.com/9091
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-08-03 21:07:36 +00:00
David Benjamin
163f29af07 Move post-handshake message handling out of read_app_data.
This finishes getting rid of ssl_read_bytes! Now we have separate
entry-points for the various cases. For now, I've kept TLS handshake
consuming records partially. When we do the BIO-less API, I expect that
will need to change, since we won't have the record buffer available.

(Instead, the ssl3_read_handshake_bytes and extend_handshake_buffer pair
will look more like the DTLS side or Go and pull the entire record into
init_buf.)

This change opts to make read_app_data drive the message to completion
in anticipation of DTLS 1.3. That hasn't been specified, but
NewSessionTicket certainly will exist. Knowing that DTLS necessarily has
interleave seems something better suited for the SSL_PROTOCOL_METHOD
internals to drive.

It needs refining, but SSL_PROTOCOL_METHOD is now actually a half-decent
abstraction boundary between the higher-level protocol logic and
DTLS/TLS-specific record-layer and message dispatchy bits.

BUG=83

Change-Id: I9b4626bb8a29d9cb30174d9e6912bb420ed45aff
Reviewed-on: https://boringssl-review.googlesource.com/9001
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-07-29 21:05:49 +00:00
David Benjamin
4792110b2b Forbid interleaving app data in a HelloRequest.
We already forbid renego/app-data interleave. Forbid it within a
HelloRequest too because that's nonsense. No one would ever send:

   [hs:HelloReq-] [app:Hello world] [hs:-uest]

Add tests for this case.

This is in preparation for our more complex TLS 1.3 post-handshake logic
which is going to go through the usual handshake reassembly logic and,
for sanity, will want to enforce this anyway.

BUG=83

Change-Id: I80eb9f3333da3d751f98f25d9469860d1993a97a
Reviewed-on: https://boringssl-review.googlesource.com/9000
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-07-29 15:44:42 +00:00
Steven Valdez
143e8b3fd9 Add TLS 1.3 1-RTT.
This adds the machinery for doing TLS 1.3 1RTT.

Change-Id: I736921ffe9dc6f6e64a08a836df6bb166d20f504
Reviewed-on: https://boringssl-review.googlesource.com/8720
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-07-18 09:54:46 +00:00
David Benjamin
397c8e6fb6 Forbid renegotiation in TLS 1.3.
Change-Id: I1b34acbbb5528e7e31595ee0cbce7618890f3955
Reviewed-on: https://boringssl-review.googlesource.com/8669
Reviewed-by: David Benjamin <davidben@google.com>
2016-07-11 18:26:27 +00:00
David Benjamin
f0ee907942 Remove the 'ssl_' prefix on most SSL_PROTOCOL_METHOD hooks.
It doesn't really convey anything useful. Leave ssl_get_message alone for now
since it's called everywhere in the handshake and I'm about to tweak it
further.

Change-Id: I6f3a74c170e818f624be8fbe5cf6b796353406df
Reviewed-on: https://boringssl-review.googlesource.com/8430
Reviewed-by: Adam Langley <agl@google.com>
2016-06-27 18:43:33 +00:00
David Benjamin
585320c9e9 Don't call read_bytes in read_change_cipher_spec.
Change-Id: If7d50e43c8ea28c5eed38209f31d481fb57bf225
Reviewed-on: https://boringssl-review.googlesource.com/8175
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-08 18:51:54 +00:00
David Benjamin
4aa4081e7f Don't use ssl3_read_bytes in ssl3_read_close_notify.
read_close_notify is a very straight-forward hook and doesn't need much.

Change-Id: I7407d842321ea1bcb47838424a0d8f7550ad71ca
Reviewed-on: https://boringssl-review.googlesource.com/8174
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-08 18:39:26 +00:00
David Benjamin
a7810c12e9 Make tls_open_record always in-place.
The business with ssl_record_prefix_len is rather a hassle. Instead, have
tls_open_record always decrypt in-place and give back a CBS to where the body
is.

This way the caller doesn't need to do an extra check all to avoid creating an
invalid pointer and underflow in subtraction.

Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee
Reviewed-on: https://boringssl-review.googlesource.com/8173
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-08 18:39:07 +00:00
David Benjamin
728f354a2b Push alert handling down into the record functions.
Alert handling is more-or-less identical across all contexts. Push it down from
read_bytes into the low-level record functions. This also deduplicates the code
shared between TLS and DTLS.

Now the only type mismatch managed by read_bytes is if we get handshake data in
read_app_data.

Change-Id: Ia8331897b304566e66d901899cfbf31d2870194e
Reviewed-on: https://boringssl-review.googlesource.com/8124
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-08 18:35:58 +00:00
David Benjamin
4e9cc71a27 Add helper functions for info_callback and msg_callback.
This is getting a little repetitive.

Change-Id: Ib0fa8ab10149557c2d728b88648381b9368221d9
Reviewed-on: https://boringssl-review.googlesource.com/8126
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-08 18:13:53 +00:00
David Benjamin
4bea8509da Lift an impossible check to an assert.
This cannot happen.

Change-Id: Ib1b473aa91d6479eeff43f7eaf94906d0b2c2a8f
Reviewed-on: https://boringssl-review.googlesource.com/8123
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-06 20:47:07 +00:00
Adam Langley
1cb405d96b Revert "Forbid calling SSL_read, SSL_peek, and SSL_do_handshake post-shutdown."
This reverts commit c7eae5a326. pyOpenSSL
expects to be able to call |SSL_read| after a shutdown and get EOF.

Change-Id: Icc5faa09d644ec29aac99b181dac0db197f283e3
Reviewed-on: https://boringssl-review.googlesource.com/8060
Reviewed-by: Adam Langley <agl@google.com>
2016-05-25 23:23:12 +00:00
David Benjamin
54092ffeaa Remove dead checks.
Those checks contradict an assert up in read_app_data. This is part of
shrinking read_bytes further into get_record and its callers until it goes
away. Here, this kind of policy should be controlled by the callers.

Change-Id: If8f9a45b8b95093beab1b3d4abcd31da55c65322
Reviewed-on: https://boringssl-review.googlesource.com/7954
Reviewed-by: Adam Langley <agl@google.com>
2016-05-18 20:52:38 +00:00
David Benjamin
c7eae5a326 Forbid calling SSL_read, SSL_peek, and SSL_do_handshake post-shutdown.
This explicitly forbids an API pattern which formerly kind of worked, but was
extremely buggy (see preceding commits). Depending on how one interprets
close_notify and our API, one might wish to call SSL_shutdown only once
(morally shutdown(SHUT_WR)) and then SSL_read until EOF.

However, this exposes additional confusing states where we might try to send an
alert post-SHUT_WR, etc. Early commits made us more robust here (whether one is
allowed to touch the SSL* after an operattion failed because it read an alert
is... unclear), so we could support it if we wanted to, but this doesn't seem
worth the additional statespace. See if we can get away with not allowing it.

Change-Id: Ie7a7e5520b464360b1e6316c34ec9854b571782f
Reviewed-on: https://boringssl-review.googlesource.com/7433
Reviewed-by: David Benjamin <davidben@google.com>
2016-05-17 21:28:40 +00:00
David Benjamin
ea65e100c7 Condition the read_close_notify check on type, not shutdown state.
The logic to drop records really should be in the caller. Unless
ssl3_read_bytes is broken apart, condition on the type field which is more
robust.

If we manage to call, say, SSL_read after SSL_shutdown completes at 0 (instead
of 1), this logic can incorrectly cause unknown record types to be dropped.

Change-Id: Iab90e5d9190fcccbf6ff55e17079a2704ed99901
Reviewed-on: https://boringssl-review.googlesource.com/7953
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-05-17 21:27:43 +00:00
David Benjamin
fa214e4a18 Tidy up shutdown state.
The existing logic gets confused in a number of cases around close_notify vs.
fatal alert. SSL_shutdown, while still pushing to the error queue, will fail to
notice alerts. We also get confused if we try to send a fatal alert when we've
already sent something else.

Change-Id: I9b1d217fbf1ee8a9c59efbebba60165b7de9689e
Reviewed-on: https://boringssl-review.googlesource.com/7952
Reviewed-by: David Benjamin <davidben@google.com>
2016-05-17 21:27:12 +00:00
David Benjamin
8f73135485 Consolidate SSL_RECEIVED_SHUTDOWN checks.
SSL_RECEIVED_SHUTDOWN checks in the record layer happen in two different
places. Some operations (but not all) check it, and so does read_bytes. Move it
to get_record.

This check should be at a low-level since it is otherwise duplicated in every
operation. It is also a signal which originates from around the peer's record
layer, so it makes sense to check it near the same code. (This one's in
get_record which is technically lower-level than read_bytes, but we're trying
to get rid of read_bytes. They're very coupled functions.)

Also, if we've seen a fatal alert, replay an error, not an EOF.

Change-Id: Idec35c5068ddabe5b1a9145016d8f945da2421cf
Reviewed-on: https://boringssl-review.googlesource.com/7436
Reviewed-by: David Benjamin <davidben@google.com>
2016-05-17 21:02:19 +00:00
David Benjamin
4c5ddb8047 Set rwstate consistently.
We reset it to SSL_NOTHING at the start of ever SSL_get_error-using operation.
Then we only set it to a non-NOTHING value in the rest of the stack on error
paths.

Currently, ssl->rwstate is set all over the place. Sometimes the pattern is:

  ssl->rwstate = SSL_WRITING;
  if (BIO_write(...) <= 0) {
    goto err;
  }
  ssl->rwstate = SSL_NOTHING;

Sometimes we only set it to the non-NOTHING value on error.

  if (BIO_write(...) <= 0) {
    ssl->rwstate = SSL_WRITING;
  }
  ssl->rwstate = SSL_NOTHING;

Sometimes we just set it to SSL_NOTHING far from any callback in random places.

The third case is arbitrary and clearly should be removed.

But, in the second case, we sometimes forget to undo it afterwards. This is
largely harmless since an error in the error queue overrides rwstate, but we
don't always put something in the error queue (falling back to
SSL_ERROR_SYSCALL for "I'm not sure why it failed. Perhaps it was one of your
callbacks? Check your errno equivalent."), but in that case a stray rwstate
value will cause it to be wrong.

We could fix the cases where we fail to set SSL_NOTHING on success cases, but
this doesn't account for there being multiple SSL_get_error operations. The
consumer may have an SSL_read and an SSL_write running concurrently. Instead,
it seems the best option is to lift the SSL_NOTHING reset to the operations and
set SSL_WRITING and friends as in the second case.

(Someday hopefully we can fix this to just be an enum that is internally
returned. It can convert to something stateful at the API layer.)

Change-Id: I54665ec066a64eb0e48a06e2fcd0d2681a42df7f
Reviewed-on: https://boringssl-review.googlesource.com/7453
Reviewed-by: David Benjamin <davidben@google.com>
2016-04-18 20:30:32 +00:00
David Benjamin
c6972eb1f0 Remove the no_renegotiation special case.
The concern is if the peer denies our renegotiation attempt, but we will never
initiate renegotiation. We only support server-initiated renegotiation when we
are acting as the client.

(Strictly speaking, only the client ever initiates renegotiation. The server
sends a HelloRequest to ask the client to initiate it. But we forbid
application data interleave as soon as we see the HelloRequest, so we treat it
as part of the handshake.)

Change-Id: I1a625130de32a7227e4471f2f889255aba962ce4
Reviewed-on: https://boringssl-review.googlesource.com/7452
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-04-18 20:29:30 +00:00
David Benjamin
0d3a8c6ac0 Don't allow alert records with multiple alerts.
This is just kind of a silly thing to do. NSS doesn't allow them either. Fatal
alerts would kill the connection regardless and warning alerts are useless. We
previously stopped accepting fragmented alerts but still allowed them doubled
up.

This is in preparation for pulling the shared alert processing code between TLS
and DTLS out of read_bytes into some common place.

Change-Id: Idbef04e39ad135f9601f5686d41f54531981e0cf
Reviewed-on: https://boringssl-review.googlesource.com/7451
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-04-18 20:29:02 +00:00
David Benjamin
c79845c2a8 Move implicit handshake driving out of read_bytes.
This removes the final use of in_handshake. Note that there is still a
rentrant call of read_bytes -> handshake_func when we see a
HelloRequest. That will need to be signaled up to ssl_read_impl
separately out of read_app_data.

Change-Id: I823de243f75e6b73eb40c6cf44157b4fc21eb8fb
Reviewed-on: https://boringssl-review.googlesource.com/7439
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-26 20:23:25 +00:00
David Benjamin
d7ac143814 Lift the handshake driving in write_bytes up to SSL_write.
This removes one use of in_handshake and consolidates some DTLS and TLS
code.

Change-Id: Ibbdd38360a983dabfb7b18c7bd59cb5e316b2adb
Reviewed-on: https://boringssl-review.googlesource.com/7435
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-26 20:09:37 +00:00
David Benjamin
282511d7eb Consolidate shutdown state.
fatal_alert isn't read at all right now, and warn_alert is only checked
for close_notify. We only need three states:

 - Not shutdown.
 - Got a fatal alert (don't care which).
 - Got a warning close_notify.

Leave ssl->shutdown alone for now as it's tied up with SSL_set_shutdown
and friends. To distinguish the remaining two, we only need a boolean.

Change-Id: I5877723af82b76965c75cefd67ec1f981242281b
Reviewed-on: https://boringssl-review.googlesource.com/7434
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-26 20:04:34 +00:00
David Benjamin
51545ceac6 Remove a number of unnecessary stdio.h includes.
Change-Id: I6267c9bfb66940d0b6fe5368514210a058ebd3cc
Reviewed-on: https://boringssl-review.googlesource.com/7494
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-17 18:22:28 +00:00
David Benjamin
a857159dd6 Clean up some silly variable names.
Change-Id: I5b38e2938811520f52ece6055245248c80308b4d
Reviewed-on: https://boringssl-review.googlesource.com/7416
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-10 19:21:20 +00:00
David Benjamin
a565d29ce6 Remove alert mapping machinery.
For TLS, this machinery only exists to swallow no_certificate alerts
which only get sent in an SSL 3.0 codepath anyway. It's much less a
no-op for SSL 3.0 which, strictly speaking, has only a subset of TLS's
alerts.

This gets messy around version negotiation because of the complex
relationship between enc_method, have_version, and version which all get
set at different times. Given that SSL 3.0 is nearly dead and all these
alerts are fatal to the connection anyway, this doesn't seem worth
carrying around. (It doesn't work very well anyway. An SSLv3-only server
may still send a record_overflow alert before version negotiation.)

This removes the last place enc_method is accessed prior to version
negotiation.

Change-Id: I79a704259fca69e4df76bd5a6846c9373f46f5a9
Reviewed-on: https://boringssl-review.googlesource.com/6843
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-27 21:28:48 +00:00
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