Commit Graph

104 Commits

Author SHA1 Message Date
David Benjamin
3baa6e153b Implement draft 16 HelloRetryRequest and cookie.
We'll never send cookies, but we'll echo them on request. Implement it
in runner as well and test.

BUG=98

Change-Id: Idd3799f1eaccd52ac42f5e2e5ae07c209318c270
Reviewed-on: https://boringssl-review.googlesource.com/11565
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-10-13 19:12:30 +00:00
Steven Valdez
c4aa727e73 Updating Key Schedule and KeyUpdate to draft 16.
This doesn't currently honor the required KeyUpdate response. That will
be done in a follow-up.

BUG=74

Change-Id: I750fc41278736cb24230303815e839c6f6967b6a
Reviewed-on: https://boringssl-review.googlesource.com/11412
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-10-13 19:12:23 +00:00
David Benjamin
52bf690ba4 Saved Finished messages are twelve bytes.
We only save them at TLS 1.0 through 1.2. This saves 104 bytes of
per-connection memory.

Change-Id: If397bdc10e40f0194cba01024e0e9857d6b812f0
Reviewed-on: https://boringssl-review.googlesource.com/11571
Reviewed-by: Adam Langley <agl@google.com>
2016-10-09 17:52:18 +00:00
David Benjamin
49ddf41557 Remove redundant copies of the Finished messages.
We only need one copy, not two. This trims 130 bytes of per-connection
memory.

Change-Id: I334aa7b1f8608e72426986bfa68534d416f3bda9
Reviewed-on: https://boringssl-review.googlesource.com/11569
Reviewed-by: Adam Langley <agl@google.com>
2016-10-09 17:43:03 +00:00
David Benjamin
a4c8ff0190 Move TLS 1.2 key exchange fields to SSL_HANDSHAKE.
SSL_HANDSHAKE is dropped after the handshake, so I've removed the logic
around smaller sizes. It's much simpler when we can use CBS_stow and
CBB_finish without extra bounds-checking.

Change-Id: Idafaa5d69e171aed9a8759f3d44e52cb01c40f39
Reviewed-on: https://boringssl-review.googlesource.com/11567
Reviewed-by: Adam Langley <agl@google.com>
2016-10-09 17:30:32 +00:00
David Benjamin
43612b6bc7 Move peer_supported_group_list to SSL_HANDSHAKE.
Now not only the pointers but also the list itself is released after the
handshake completes.

Change-Id: I8b568147d2d4949b3b0efe58a93905f77a5a4481
Reviewed-on: https://boringssl-review.googlesource.com/11528
Reviewed-by: Adam Langley <agl@google.com>
2016-10-09 17:20:33 +00:00
David Benjamin
a048678cd6 Move some fields from tmp to hs.
This releases memory associated with them after the handshake. Note this
changes the behavior of |SSL_get0_certificate_types| and
|SSL_get_client_CA_list| slightly. Both functions now return NULL
outside of the handshake. But they were already documented to return
something undefined when not called at the CertificateRequest.

A survey of callers finds none that would care. (Note
SSL_get_client_CA_list is used both as a getter for the corresponding
server config setter and to report client handshake properties. Only the
latter is affected.) It's also pretty difficult to imagine why a caller
would wish to query this stuff at any other time, and there are clear
benefits to dropping the CA list after the handshake (some servers send
ABSURDLY large lists).

Change-Id: I3ac3b601ff0cfa601881ce77ae33d99bb5327004
Reviewed-on: https://boringssl-review.googlesource.com/11521
Reviewed-by: Adam Langley <agl@google.com>
2016-10-09 16:47:31 +00:00
David Benjamin
c8b6b4fe4a Only predict X25519 in TLS 1.3.
We'd previously been assuming we'd want to predict P-256 and X25519 but,
on reflection, that's nonsense. Although, today, P-256 is widespread and
X25519 is less so, that's not the right question to ask. Those servers
are all 1.2.

The right question is whether we believe enough servers will get to TLS
1.3 before X25519 to justify wasting 64 bytes on all other connections.
Given that OpenSSL has already shipped X25519 and Microsoft was doing
interop testing on X25519 around when we were shipping it, I think the
answer is no.

Moreover, if we are wrong, it will be easier to go from predicting one
group to two rather than the inverse (provided we send a fake one with
GREASE). I anticipate prediction-miss HelloRetryRequest logic across the
TLS/TCP ecosystem will be largely untested (no one wants to pay an RTT),
so taking a group out of the predicted set will likely be a risky
operation.

Only predicting one group also makes things a bit simpler. I haven't
done this here, but we'll be able to fold the 1.2 and 1.3 ecdh_ctx's
together, even.

Change-Id: Ie7e42d3105aca48eb9d97e2e05a16c5379aa66a3
Reviewed-on: https://boringssl-review.googlesource.com/10960
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 21:18:34 +00:00
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
bac75b80cc Move peer_psk_identity_hint to SSL_HANDSHAKE.
One less field to reset on renego and save a pointer of post-handshake
memory.

Change-Id: Ifc0c3c73072af244ee3848d9a798988d2c8a7c38
Reviewed-on: https://boringssl-review.googlesource.com/11086
Reviewed-by: Adam Langley <agl@google.com>
2016-09-20 22:37:24 +00:00
David Benjamin
639846e5e4 Add tests for trailing data in handshake messages.
It's easy to forget to check those. Unfortunately, it's also easy to
forget to check inner structures, which is going to be harder to stress,
but do these to start with. In doing, so fix up and unify some
error-handling, and add a missing check when parsing TLS 1.2
CertificateRequest.

This was also inspired by the recent IETF posting.

Change-Id: I27fe3cd3506258389a75d486036388400f0a33ba
Reviewed-on: https://boringssl-review.googlesource.com/10963
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-12 21:00:50 +00:00
David Benjamin
0fc37ef082 Fix a number of sigalg scope issues.
peer_sigalgs should live on SSL_HANDSHAKE. This both releases a little
bit of memory after the handshake is over and also avoids the bug where
the sigalgs get dropped if SSL_set_SSL_CTX is called at a bad time. See
also upstream's 14e14bf6964965d02ce89805d9de867f000095aa.

This only affects consumers using the old SNI callback and not
select_certificate_cb.

Add a test that the SNI callback works as expected. In doing so, add an
SSL_CTX version of the signing preferences API. This is a property of
the cert/key pair (really just the key) and should be tied to that. This
makes it a bit easier to have the regression test work with TLS 1.2 too.

I thought we'd fixed this already, but apparently not... :-/

BUG=95

Change-Id: I75b02fad4059e6aa46c3b05183a07d72880711b3
Reviewed-on: https://boringssl-review.googlesource.com/10445
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-08-24 00:24:34 +00:00
David Benjamin
867bcba05d Move ssl_handshake_new, etc., into s3_both.c.
s3_both.c does a few too many things right now, but SSL_HANDSHAKE is not
only for TLS 1.3.

Change-Id: Ieac17c592a1271d4d5c9cee005eaf5642772b8f5
Reviewed-on: https://boringssl-review.googlesource.com/10443
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-08-18 19:42:41 +00:00
Steven Valdez
87eab4902d Splitting SSL session state.
To prevent configuration/established session confusion, the handshake
session state is separated into the configured session (ssl->session)
and the newly created session (ssl->s3->new_session). Upon conclusion of
the handshake, the finalized session is stored
in (ssl->s3->established_session). During the handshake, any requests
for the session (SSL_get_session) return a non-resumable session, to
prevent resumption of a partially filled session. Sessions should only
be cached upon the completion of the full handshake, using the resulting
established_session. The semantics of accessors on the session are
maintained mid-renego.

Change-Id: I4358aecb71fce4fe14a6746c5af1416a69935078
Reviewed-on: https://boringssl-review.googlesource.com/8612
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:22:46 +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
4497e58961 Switch finish_handshake to release_current_message.
With the previous DTLS change, the dispatch layer only cares about the
end of the handshake to know when to drop the current message. TLS 1.3
post-handshake messages will need a similar hook, so convert it to this
lower-level one.

BUG=83

Change-Id: I4c8c3ba55ba793afa065bf261a7bccac8816c348
Reviewed-on: https://boringssl-review.googlesource.com/8989
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-07-28 22:59:18 +00:00
David Benjamin
9fd9580137 Remove ssl->s3->message_complete in favor of ssl->init_msg.
This was only used so we knew when we had a current message to discard
and when we didn't. With init_msg being tracked better, we can use that
instead.

As part of this, switch the V2ClientHello hack to not using
reuse_message. Otherwise we have to fill in init_msg and friends in two
places.

The next change will require that we have a better handle on the "is
there a current message" boolean.

BUG=83

Change-Id: I917efacbad10806d492bbe51eda74c0779084d60
Reviewed-on: https://boringssl-review.googlesource.com/8987
Reviewed-by: Adam Langley <agl@google.com>
2016-07-28 22:52:47 +00:00
David Benjamin
a950948962 Use SSL3_HM_HEADER_LENGTH a bit more.
Somewhat clearer what it's for than just 4.

Change-Id: Ie7bb89ccdce188d61741da203acd624b49b69058
Reviewed-on: https://boringssl-review.googlesource.com/8986
Reviewed-by: Adam Langley <agl@google.com>
2016-07-28 22:49:57 +00:00
David Benjamin
481b9d2047 Remove begin_handshake and allocate init_buf lazily.
For TLS 1.3, we will need to process more complex post-handshake
messages. It is simplest if we use the same mechanism. In preparation,
allow ssl3_get_message to be called at any point.

Note that this stops reserving SSL3_RT_MAX_PLAIN_LENGTH in init_buf
right off the bat. Instead it will grow as-needed to accomodate the
handshake. SSL3_RT_MAX_PLAIN_LENGTH is rather larger than we probably
need to receive, particularly as a server, so this seems a good plan.

BUG=83

Change-Id: Id7f4024afc4c8a713b46b0d1625432315594350e
Reviewed-on: https://boringssl-review.googlesource.com/8985
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-07-28 22:07:28 +00:00
David Benjamin
bd4679d133 Tidy up ssl3_get_message slightly.
Change-Id: Iccd86440bf8721098050fac220dc9bb80bbfc670
Reviewed-on: https://boringssl-review.googlesource.com/8983
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-07-28 21:36:55 +00:00
David Benjamin
e776cc2956 Implement SSLKEYLOGFILE support for TLS 1.3.
This adds three more formats to the SSLKEYLOGFILE format to support TLS
1.3:

    EARLY_TRAFFIC_SECRET <client_random> <early_traffic_secret>
    HANDSHAKE_TRAFFIC_SECRET <client_random> <handshake_traffic_secret>
    TRAFFIC_SECRET_0 <client_random> <traffic_secret_0>

(We don't implement 0-RTT yet, so only the second two are implemented.)

Motivations:

1. If emitted the non-traffic secrets (early, handshake, and master) or
   the IKMs, Wireshark needs to maintain a handshake hash. I don't
   believe they need to do this today.

2. We don't store more than one non-traffic secret at a time and don't
   keep traffic secrets for longer than needed. That suggests three
   separate lines logged at different times rather than one line.

3. If 0-RTT isn't used, we probably won't even compute the early traffic
   secret, so that further suggests three different lines.

4. If the handshake didn't get far enough to complete, we won't have an
   TRAFFIC_SECRET_0 to log at all. That seems like exactly when
   Wireshark would be handy, which means we want to log secrets as they
   are computed.

MT from NSS has ACK'd over email that this format would be acceptable
for them, so let's go with it.

Change-Id: I4d685a1355dff4d4bd200310029d502bb6c511f9
Reviewed-on: https://boringssl-review.googlesource.com/8841
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-19 08:32:04 +00:00
David Benjamin
1f61f0d7c3 Implement TLS 1.3's downgrade signal.
For now, skip the 1.2 -> 1.1 signal since that will affect shipping
code. We may as well enable it too, but wait until things have settled
down. This implements the version in draft-14 since draft-13's isn't
backwards-compatible.

Change-Id: I46be43e6f4c5203eb4ae006d1c6a2fe7d7a949ec
Reviewed-on: https://boringssl-review.googlesource.com/8724
Reviewed-by: David Benjamin <davidben@google.com>
2016-07-12 19:17:43 +00:00
David Benjamin
09eb655e5c Simplify ssl_get_message somewhat.
It still places the current message all over the place, but remove the
bizarre init_num/error/ok split. Now callers get the message length out
of init_num, which mirrors init_msg. Also fix some signedness.

Change-Id: Ic2e97b6b99e234926504ff217b8aedae85ba6596
Reviewed-on: https://boringssl-review.googlesource.com/8690
Reviewed-by: David Benjamin <davidben@google.com>
2016-07-11 23:01:32 +00:00
David Benjamin
528bd26dd9 Don't use init_buf in DTLS.
This machinery is so different between TLS and DTLS that there is no
sense in having them share structures. This switches us to maintaining
the full reassembled message in hm_fragment and get_message just lets
the caller read out of that when ready.

This removes the last direct handshake dependency on init_buf,
ssl3_hash_message.

Change-Id: I4eccfb6e6021116255daead5359a0aa3f4d5be7b
Reviewed-on: https://boringssl-review.googlesource.com/8667
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-07-11 23:01:11 +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
4dbdf94c67 Push V2ClientHello handling into ssl3_get_message.
V2ClientHello is going to be ugly wherever we do it, but this hides it
behind the transport method table. It removes a place where the
handshake state machine reaches into ssl3_get_message's internal state.
ssl3_get_message will now silently translate V2ClientHellos into true
ClientHellos and manage the handshake hash appropriately.

Now the only accesses of init_buf from the handshake state machines are
to create and destroy the buffer.

Change-Id: I81467a038f6ac472a465eec7486a443fe50a98e1
Reviewed-on: https://boringssl-review.googlesource.com/8641
Reviewed-by: Adam Langley <agl@google.com>
2016-07-07 23:51:25 +00:00
David Benjamin
34a3c49875 Simplify TLS reuse_message implementation.
Rather than have a separate codepath, just skip the message_complete
logic and parse what's in the buffer. This also cuts down on one input
to setting up a reuse_message; message_type is now only written to in
the get_message implementation.

Change-Id: I96689b5957a3f2548af9099ec4e53cabacdc395a
Reviewed-on: https://boringssl-review.googlesource.com/8640
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-07-07 23:01:07 +00:00
Steven Valdez
2b8415e8ff Move the Digest/Sign split for SignatureAlgorithms to a lower level.
In order to delay the digest of the handshake transcript and unify
around message-based signing callbacks, a copy of the transcript is kept
around until we are sure there is no certificate authentication.

This removes support for SSL_PRIVATE_KEY_METHOD as a client in SSL 3.0.

Change-Id: If8999a19ca021b4ff439319ab91e2cd2103caa64
Reviewed-on: https://boringssl-review.googlesource.com/8561
Reviewed-by: David Benjamin <davidben@google.com>
2016-07-01 19:01:33 +00:00
Steven Valdez
f0451ca37d Cleaning up internal use of Signature Algorithms.
The signing logic itself still depends on pre-hashed messages and will be fixed
in later commits.

Change-Id: I901b0d99917c311653d44efa34a044bbb9f11e57
Reviewed-on: https://boringssl-review.googlesource.com/8545
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-29 21:22:25 +00:00
David Benjamin
352d0a9c6c Remove a/b parameters to send_change_cipher_spec.
They're not necessary.

Change-Id: Ifeb3fae73a8b22f88019e6ef9f9ba5e64ed3cfab
Reviewed-on: https://boringssl-review.googlesource.com/8543
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-29 18:50:47 +00:00
David Benjamin
7583643569 Disconnect handshake message creation from init_buf.
This allows us to use CBB for all handshake messages. Now, SSL_PROTOCOL_METHOD
is responsible for implementing a trio of CBB-related hooks to assemble
handshake messages.

Change-Id: I144d3cac4f05b6637bf45d3f838673fc5c854405
Reviewed-on: https://boringssl-review.googlesource.com/8440
Reviewed-by: Adam Langley <agl@google.com>
2016-06-27 22:15:01 +00:00
David Benjamin
171b5403ee Fix ssl3_do_write error handling.
The functions it calls all pass through <= 0 as error codes, not < 0.

Change-Id: I9d0d6b1df0065efc63f2d3a5e7f3497b2c28453a
Reviewed-on: https://boringssl-review.googlesource.com/8237
Reviewed-by: Adam Langley <agl@google.com>
2016-06-09 23:51:08 +00:00
David Benjamin
8f1e113a73 Ensure verify error is set when X509_verify_cert() fails.
Set ctx->error = X509_V_ERR_OUT_OF_MEM when verification cannot
continue due to malloc failure.  Similarly for issuer lookup failures
and caller errors (bad parameters or invalid state).

Also, when X509_verify_cert() returns <= 0 make sure that the
verification status does not remain X509_V_OK, as a last resort set
it it to X509_V_ERR_UNSPECIFIED, just in case some code path returns
an error without setting an appropriate value of ctx->error.

Add new and some missing error codes to X509 error -> SSL alert switch.

(Imported from upstream's 5553a12735e11bc9aa28727afe721e7236788aab.)

Change-Id: I3231a6b2e72a3914cb9316b8e90ebaee009a1c5f
Reviewed-on: https://boringssl-review.googlesource.com/8170
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-09 17:29:39 +00:00
David Benjamin
2a08c8d85d Remove ssl3_do_write's 0 case.
It's unreachable and wouldn't work anyway. We'd never bubble up to the caller
to retry. As a consequence, the TLS side doesn't actually need to pay attention
to init_off.

(For now anyway. We'll probably need state of this sort once the write half is
all reworked. All the craziness with wpend_buf ought to be limited to the
SSL_write bits.)

Change-Id: I951534f6bbeb547ce0492d5647aaf76be42108a3
Reviewed-on: https://boringssl-review.googlesource.com/8179
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-08 19:13:37 +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
1e6d6df943 Remove state parameters to ssl3_get_message.
They're completely unused now. The handshake message reassembly logic should
not depend on the state machine. This should partially free it up (ugly as it
is) to be shared with a future TLS 1.3 implementation while, in parallel, it
and the layers below, get reworked. This also cuts down on the number of states
significantly.

Partially because I expect we'd want to get ssl_hash_message_t out of there
too. Having it in common code is fine, but it needs to be in the (supposed to
be) protocol-agnostic handshake state machine, not the protocol-specific
handshake message layer.

Change-Id: I12f9dc57bf433ceead0591106ab165d352ef6ee4
Reviewed-on: https://boringssl-review.googlesource.com/7949
Reviewed-by: Adam Langley <agl@google.com>
2016-05-18 20:51:48 +00:00
David Benjamin
a6338be3fa Simplify ssl3_get_message.
Rather than this confusing coordination with the handshake state machine and
init_num changing meaning partway through, use the length field already in
BUF_MEM. Like the new record layer parsing, is no need to keep track of whether
we are reading the header or the body. Simply keep extending the handshake
message until it's far enough along.

ssl3_get_message still needs tons of work, but this allows us to disentangle it
from the handshake state.

Change-Id: Ic2b3e7cfe6152a7e28a04980317d3c7c396d9b08
Reviewed-on: https://boringssl-review.googlesource.com/7948
Reviewed-by: Adam Langley <agl@google.com>
2016-05-18 20:50:57 +00:00
David Benjamin
060cfb0911 Simplify handshake message size limits.
A handshake message can go up to 2^24 bytes = 16MB which is a little large for
the peer to force us to buffer. Accordingly, we bound the size of a
handshake message.

Rather than have a global limit, the existing logic uses a different limit at
each state in the handshake state machine and, for certificates, allows
configuring the maximum certificate size. This is nice in that we engage larger
limits iff the relevant state is reachable from the handshake. Servers without
client auth get a tighter limit "for free".

However, this doesn't work for DTLS due to out-of-order messages and we use a
simpler scheme for DTLS. This scheme also is tricky on optional messages and
makes the handshake <-> message layer communication complex.

Apart from an ignored 20,000 byte limit on ServerHello, the largest
non-certificate limit is the common 16k limit on ClientHello. So this
complexity wasn't buying us anything. Unify everything on the DTLS scheme
except, so as not to regress bounds on client-auth-less servers, also correctly
check for whether client auth is configured. The value of 16k was chosen based
on this value.

(The 20,000 byte ServerHello limit makes no sense. We can easily bound the
ServerHello because servers may not send extensions we don't implement. But it
gets overshadowed by the certificate anyway.)

Change-Id: I00309b16d809a3c2a1543f99fd29c4163e3add81
Reviewed-on: https://boringssl-review.googlesource.com/7941
Reviewed-by: David Benjamin <davidben@google.com>
2016-05-13 20:06:24 +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
981936791e Remove some easy obj.h dependencies.
A lot of consumers of obj.h only want the NID values. Others didn't need
it at all. This also removes some OBJ_nid2sn and OBJ_nid2ln calls in EVP
error paths which isn't worth pulling a large table in for.

BUG=chromium:499653

Change-Id: Id6dff578f993012e35b740a13b8e4f9c2edc0744
Reviewed-on: https://boringssl-review.googlesource.com/7563
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-31 20:50:33 +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
bf82aede67 Disable all TLS crypto in fuzzer mode.
Both sides' signature and Finished checks still occur, but the results
are ignored. Also, all ciphers behave like the NULL cipher.
Conveniently, this isn't that much code since all ciphers and their size
computations funnel into SSL_AEAD_CTX.

This does carry some risk that we'll mess up this code. Up until now, we've
tried to avoid test-only changes to the SSL stack.

There is little risk that anyone will ship a BORINGSSL_UNSAFE_FUZZER_MODE build
for anything since it doesn't interop anyway. There is some risk that we'll end
up messing up the disableable checks. However, both skipped checks have
negative tests in runner (see tests that set InvalidSKXSignature and
BadFinished). For good measure, I've added a server variant of the existing
BadFinished test to this CL, although they hit the same code.

Change-Id: I37f6b4d62b43bc08fab7411965589b423d86f4b8
Reviewed-on: https://boringssl-review.googlesource.com/7287
Reviewed-by: Adam Langley <agl@google.com>
2016-03-02 23:39:36 +00:00
David Benjamin
b83003ebc6 Don't initialize enc_method before version negotiation.
Move it into ssl->s3 so it automatically behaves correctly on SSL_clear.
ssl->version is still a mess though.

Change-Id: I17a692a04a845886ec4f8de229fa6cf99fa7e24a
Reviewed-on: https://boringssl-review.googlesource.com/6844
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-27 21:38:12 +00:00
David Benjamin
a1e9cabd8b Replace enc_flags with normalized version checks.
This removes the various non-PRF checks from SSL3_ENC_METHOD so that can
have a clearer purpose. It also makes TLS 1.0 through 1.2's
SSL3_ENC_METHOD tables identical and gives us an assert to ensure
nothing accesses the version bits before version negotiation.
Accordingly, ssl_needs_record_splitting was reordered slightly so we
don't rely on enc_method being initialized to TLS 1.2
pre-version-negotiation.

This leaves alert_value as the only part of SSL3_ENC_METHOD which may be
accessed before version negotiation.

Change-Id: If9e299e2ef5511b5fa442b2af654eed054c3e675
Reviewed-on: https://boringssl-review.googlesource.com/6842
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-27 21:17:55 +00:00
David Benjamin
baa1216ac0 Prune finished labels from SSL3_ENC_METHOD.
There's not much point in putting those in the interface as the
final_finished_mac implementation is itself different between SSL 3.0
and TLS.

Change-Id: I76528a88d255c451ae008f1a34e51c3cb57d3073
Reviewed-on: https://boringssl-review.googlesource.com/6838
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 22:04:53 +00:00
David Benjamin
4119d42e7c Tidy up keyblock and CCS logic slightly.
Move the actual SSL_AEAD_CTX swap into the record layer. Also revise the
intermediate state we store between setup_key_block and
change_cipher_state. With SSL_AEAD_CTX_new abstracted out, keeping the
EVP_AEAD around doesn't make much sense. Just store enough to partition
the key block.

Change-Id: I773fb46a2cb78fa570f00c0a89339c15bbb1d719
Reviewed-on: https://boringssl-review.googlesource.com/6832
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-01-15 20:40:44 +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
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
d28f59c27b Switch the keylog BIO to a callback.
The keylog BIO is internally synchronized by the SSL_CTX lock, but an
application may wish to log keys from multiple SSL_CTXs. This is in
preparation for switching Chromium to use a separate SSL_CTX per profile
to more naturally split up the session caches.

It will also be useful for routing up SSLKEYLOGFILE in WebRTC. There,
each log line must be converted to an IPC up from the renderer
processes.

This will require changes in Chromium when we roll BoringSSL.

BUG=458365,webrtc:4417

Change-Id: I2945bdb4def0a9c36e751eab3d5b06c330d66b54
Reviewed-on: https://boringssl-review.googlesource.com/6514
Reviewed-by: Adam Langley <agl@google.com>
2015-11-19 01:23:49 +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