Commit Graph

100 Commits

Author SHA1 Message Date
David Benjamin
650aa1c80a Clean up certificate auto-chaining.
Rather than doing it right before outputing, treat this as a part of the
pipeline to finalize the certificate chain, and run it right after
cert_cb to modify the certificate configuration itself. This means
nothing else in the stack needs to worry about this case existing.

It also makes it easy to support in both TLS 1.2 and TLS 1.3.

Change-Id: I6a088297a54449f1f5f5bb8b5385caa4e8665eb6
Reviewed-on: https://boringssl-review.googlesource.com/12966
Reviewed-by: Adam Langley <agl@google.com>
2017-01-04 01:36:26 +00:00
David Benjamin
17cf2cb1d2 Work around language and compiler bug in memcpy, etc.
Most C standard library functions are undefined if passed NULL, even
when the corresponding length is zero. This gives them (and, in turn,
all functions which call them) surprising behavior on empty arrays.
Some compilers will miscompile code due to this rule. See also
https://www.imperialviolet.org/2016/06/26/nonnull.html

Add OPENSSL_memcpy, etc., wrappers which avoid this problem.

BUG=23

Change-Id: I95f42b23e92945af0e681264fffaf578e7f8465e
Reviewed-on: https://boringssl-review.googlesource.com/12928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-12-21 20:34:47 +00:00
Adam Langley
0567220b8b Don't use X.509 functions to check ECDSA keyUsage.
This removes another dependency on the crypto/x509 code.

Change-Id: Ia72da4d47192954c2b9a32cf4bcfd7498213c0c7
Reviewed-on: https://boringssl-review.googlesource.com/12709
Reviewed-by: Adam Langley <agl@google.com>
2016-12-14 17:51:03 +00:00
David Benjamin
f1050fd79a Preserve the peer signature algorithm across resumes.
So we can report it cleanly out of DevTools, it should behave like
SSL_get_curve_id and be reported on resumption too.

BUG=chromium:658905

Change-Id: I0402e540a1e722e09eaebadf7fb4785d8880c389
Reviewed-on: https://boringssl-review.googlesource.com/12694
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-12-14 01:12:40 +00:00
David Benjamin
4882a6c50b Replace key_exchange_info with group_id.
The only accessor for this field is the group/curve ID. Switch to only
storing that so no cipher checks are needed to interpret it. Instead,
ignore older values at parse time.

Change-Id: Id0946d4ac9e7482c69e64cc368a9d0cddf328bd3
Reviewed-on: https://boringssl-review.googlesource.com/12693
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-12-14 01:07:20 +00:00
Adam Langley
d515722d22 Don't depend on the X509 code for getting public keys.
This change removes the use of |X509_get_pubkey| from the TLS <= 1.2
code. That function is replaced with a shallow parse of the certificate
to extract the public key instead.

Change-Id: I8938c6c5a01b32038c6b6fa58eb065e5b44ca6d2
Reviewed-on: https://boringssl-review.googlesource.com/12707
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-12-13 21:27:31 +00:00
Adam Langley
d519bf6be0 Add |SSL_CTX_set0_buffer_pool|.
This currently only works for certificates parsed from the network, but
if making several connections that share certificates, some KB of memory
might be saved.

BUG=chromium:671420

Change-Id: I1c7a71d84e1976138641f71830aafff87f795f9d
Reviewed-on: https://boringssl-review.googlesource.com/12706
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-12-13 18:54:22 +00:00
Adam Langley
68e7124ddf Hold certificates in an SSL_SESSION as CRYPTO_BUFFERSs as well.
This change adds a STACK_OF(CRYPTO_BUFFER) to an SSL_SESSION which
contains the raw form of the received certificates. The X509-based
members still exist, but their |enc| buffer will alias the
CRYPTO_BUFFERs.

(This is a second attempt at
https://boringssl-review.googlesource.com/#/c/12163/.)

BUG=chromium:671420

Change-Id: I508a8a46cab89a5a3fcc0c1224185d63e3d59cb8
Reviewed-on: https://boringssl-review.googlesource.com/12705
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-12-13 17:28:25 +00:00
Adam Langley
364f7a6d21 Push the difference in chain semantics to the edge.
OpenSSL includes a leaf certificate in a certificate chain when it's a
client, but doesn't when it's a server. This is also reflected in the
serialisation of sessions.

This change makes the internal semantics consistent: the leaf is always
included in the chain in memory, and never duplicated when serialised.
To maintain the same API, SSL_get_peer_cert_chain will construct a copy
of the chain without the leaf if needed.

Since the serialised format of a client session has changed, an
|is_server| boolean is added to the ASN.1 that defaults to true. Thus
any old client sessions will be parsed as server sessions and (silently)
discarded by a client.

Change-Id: Ibcf72bc8a130cedb423bc0fd3417868e0af3ca3e
Reviewed-on: https://boringssl-review.googlesource.com/12704
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-12-13 17:18:53 +00:00
David Benjamin
cb0c29ff75 Move state and next_state to SSL_HANDSHAKE.
state is now initialized to SSL_ST_INIT in SSL_HANDSHAKE. If there is no
handshake present, we report SSL_ST_OK. This saves 8 bytes of
per-connection post-handshake memory.

Change-Id: Idb3f7031045caed005bd7712bc8c4b42c81a1d04
Reviewed-on: https://boringssl-review.googlesource.com/12697
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-12-12 22:09:01 +00:00
David Benjamin
5edfc8cc17 Emulate the client_cert_cb with cert_cb.
This avoids needing a extra state around client certificates to avoid
calling the callbacks twice. This does, however, come with a behavior
change: configuring both callbacks won't work. No consumer does this.

(Except bssl_shim which needed slight tweaks.)

Change-Id: Ia5426ed2620e40eecdcf352216c4a46764e31a9a
Reviewed-on: https://boringssl-review.googlesource.com/12690
Reviewed-by: Adam Langley <agl@google.com>
2016-12-12 21:58:24 +00:00
Adam Langley
c0fc7a1385 Revert "Add |SSL_CTX_set0_buffer_pool|." and "Hold certificates in an SSL_SESSION as CRYPTO_BUFFERSs as well."
This reverts commits 5a6e616961 and
e8509090cf. I'm going to unify how the
chains are kept in memory between client and server first otherwise the
mess just keeps growing.

Change-Id: I76df0d94c9053b2454821d22a3c97951b6419831
Reviewed-on: https://boringssl-review.googlesource.com/12701
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-12-09 23:31:12 +00:00
Matthew Braithwaite
651aaefb44 Remove CECPQ1 (experimental post-quantum key agreement).
Change-Id: Ie947ab176d10feb709c6e135d5241c6cf605b8e8
Reviewed-on: https://boringssl-review.googlesource.com/12700
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-12-09 19:16:56 +00:00
Adam Langley
e8509090cf Hold certificates in an SSL_SESSION as CRYPTO_BUFFERSs as well.
This change adds a STACK_OF(CRYPTO_BUFFER) to an SSL_SESSION which
contains the raw form of the received certificates. The X509-based
members still exist, but their |enc| buffer will alias the
CRYPTO_BUFFERs.

The serialisation format of SSL_SESSIONs is also changed, in a backwards
compatible way. Previously, some sessions would duplicate the leaf
certificate in the certificate chain. These sessions can still be read,
but will be written in a way incompatible with older versions of the
code. This should be fine because the situation where multiple versions
exchange serialised sessions is at the server, and the server doesn't
duplicate the leaf certifiate in the chain anyway.

Change-Id: Id3b75d24f1745795315cb7f8089a4ee4263fa938
Reviewed-on: https://boringssl-review.googlesource.com/12163
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-12-09 18:12:40 +00:00
David Benjamin
f04c2e9878 Move client_version into SSL_HANDSHAKE.
There is no need to retain it beyond this point.

Change-Id: Ib5722ab30fc013380198b1582d1240f0fe0aa770
Reviewed-on: https://boringssl-review.googlesource.com/12620
Reviewed-by: Adam Langley <agl@google.com>
2016-12-08 16:39:52 +00:00
David Benjamin
f3c8f8d19d Pass explicit parameters elsewhere.
The remaining direct accesses are in functions which expect to be called
in and out of the handshake. Accordingly, they are NULL-checked.

Change-Id: I07a7de6bdca7b6f8d09e22da11b8863ebf41389a
Reviewed-on: https://boringssl-review.googlesource.com/12343
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-12-06 19:54:58 +00:00
David Benjamin
6773972ff6 Pass explicit hs parameters into t1_enc.c.
Change-Id: I5ef0fe5cc3ae0d5029ae41db36e66d22d76f6158
Reviewed-on: https://boringssl-review.googlesource.com/12341
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-12-06 19:49:46 +00:00
David Benjamin
6e4fc336c4 Pass explicit hs parameters to tls13_*.c.
This removes all explicit ssl->s3->hs access in those files.

Change-Id: I801ca1c894936aecef21e56ec7e7acb9d1b99688
Reviewed-on: https://boringssl-review.googlesource.com/12318
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-12-06 19:49:24 +00:00
David Benjamin
8c880a2b95 Pass explicit hs parameters to kExtensions callbacks.
This takes care of many of the explicit ssl->s3->hs accesses.

Change-Id: I380fae959f3a7021d6de9d19a4ca451b9a0aefe5
Reviewed-on: https://boringssl-review.googlesource.com/12317
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-12-06 19:48:37 +00:00
David Benjamin
c3c8882918 Match state machine functions with new calling convention.
This cuts down on a lot of unchecked ssl->s3->hs accesses. Next is
probably the mass of extensions callbacks, and then we can play
whack-a-mole with git grep.

Change-Id: I81c506ea25c2569a51ceda903853465b8b567b0f
Reviewed-on: https://boringssl-review.googlesource.com/12237
Reviewed-by: Steven Valdez <svaldez@google.com>
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-12-06 19:36:45 +00:00
David Benjamin
ce8c9d2b41 Maintain SSL_HANDSHAKE lifetime outside of handshake_func.
We currently look up SSL_HANDSHAKE off of ssl->s3->hs everywhere, but
this is a little dangerous. Unlike ssl->s3->tmp, ssl->s3->hs may not be
present. Right now we just know not to call some functions outside the
handshake.

Instead, code which expects to only be called during a handshake should
take an explicit SSL_HANDSHAKE * parameter and can assume it non-NULL.
This replaces the SSL * parameter. Instead, that is looked up from
hs->ssl.

Code which is called in both cases, reads from ssl->s3->hs. Ultimately,
we should get to the point that all direct access of ssl->s3->hs needs
to be NULL-checked.

As a start, manage the lifetime of the ssl->s3->hs in SSL_do_handshake.
This allows the top-level handshake_func hooks to be passed in the
SSL_HANDSHAKE *. Later work will route it through the stack. False Start
is a little wonky, but I think this is cleaner overall.

Change-Id: I26dfeb95f1bc5a0a630b5c442c90c26a6b9e2efe
Reviewed-on: https://boringssl-review.googlesource.com/12236
Reviewed-by: Steven Valdez <svaldez@google.com>
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-12-06 19:36:27 +00:00
David Benjamin
b8d74f5b6a Add tests for failing cert_cb.
We missed that the TLS 1.3 code was inconsistent with the TLS 1.2 code.
Only on the server did we push an error code. But consistency between
client and server is probably worthwhile so, fix the 1.2 code to match
for now.

Change-Id: I17952c72048697dc66eacf0f144a66ced9cb3be8
Reviewed-on: https://boringssl-review.googlesource.com/12260
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-11-15 07:15:54 +00:00
Steven Valdez
a833c357ed Update to TLS 1.3 draft 18.
This is the squash of the following CLs:
https://boringssl-review.googlesource.com/c/12021/9
https://boringssl-review.googlesource.com/c/12022/9
https://boringssl-review.googlesource.com/c/12107/19
https://boringssl-review.googlesource.com/c/12141/22
https://boringssl-review.googlesource.com/c/12181/33

The Go portions were written by Nick Harper

BUG=112

Change-Id: I375a1fcead493ec3e0282e231ccc8d7c4dde5063
Reviewed-on: https://boringssl-review.googlesource.com/12300
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2016-11-15 06:57:21 +00:00
David Benjamin
78476f6065 Move tlsext_ticket_expected to SSL_HANDSHAKE.
It's all of one bit, but having it on the SSL object means we need
manually to reset it on renego.

Change-Id: I989dacd430fe0fa63d76451b95f036a942aefcfe
Reviewed-on: https://boringssl-review.googlesource.com/12229
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-11-12 07:32:42 +00:00
Steven Valdez
5eead165fc Splitting finish_message to finish_message/queue_message.
This is to allow for PSK binders to be munged into the ClientHello as part of
draft 18.

BUG=112

Change-Id: Ic4fd3b70fa45669389b6aaf55e61d5839f296748
Reviewed-on: https://boringssl-review.googlesource.com/12228
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-11-12 05:01:20 +00:00
Adam Langley
c5ac2b6c78 Rename X.509 members in |SSL_SESSION| and |CERT|.
This change renames |peer| to |x509_peer| and |cert_chain| to
|x509_chain| in |SSL_SESSION|. It also renames |x509| to |x509_leaf| and
|chain| to |x509_chain| in |CERT|. (All with an eye to maybe making
them lazily initialised in the future).

This a) catches anyone who might be accessing these members directly and
b) makes space for |CRYPTO_BUFFER|-based values to take the unprefixed
names.

Change-Id: I10573304fb7d6f1ea03f9e645f7fc0acdaf71ac2
Reviewed-on: https://boringssl-review.googlesource.com/12162
Reviewed-by: David Benjamin <davidben@google.com>
2016-11-09 20:07:57 +00:00
David Benjamin
4e41926774 Move key_block into SSL_HANDSHAKE.
This is already manually released at the end of the handshake. With this
change, it can happen implicitly, and SSL3_STATE shrinks further by
another pointer.

Change-Id: I94b9f2e4df55e8f2aa0b3a8799baa3b9a34d7ac1
Reviewed-on: https://boringssl-review.googlesource.com/12121
Reviewed-by: Adam Langley <agl@google.com>
2016-11-09 17:02:33 +00:00
David Benjamin
123db57009 Measure session->timeout from ticket issuance.
The distinction for full handshakes is not meaningful (the timestamp is
currently the start of the handshake), but for renewed sessions, we
currently retain the timestamp of the original issuance.

Instead, when minting or receiving tickets, adjust session->time and
session->timeout so that session->time is the ticket issuance time.

This is still not our final TLS 1.3 behavior (which will need a both
renewable and non-renewable times to honor the server ticket lifetime),
but it gets us closer and unblocks handling ticket_age_add from TLS 1.3
draft 18 and sends the correct NewSessionTicket lifetime.

This fixes the ticket lifetime hint which we emit on the server to
mirror the true ticket lifetime. It also fixes the TLS 1.3 server code
to not set the ticket lifetime hint. There is no need to waste ticket
size with it, it is no longer a "hint" in TLS 1.3, and even in the TLS
1.3 code we didn't fill it in on the server.

Change-Id: I140541f1005a24e53e1b1eaa90996d6dada1c3a1
Reviewed-on: https://boringssl-review.googlesource.com/12105
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-11-08 23:51:10 +00:00
David Benjamin
d2cb1c19e2 Remove cipher_list_by_id.
This is only used in one place where we don't take advantage of it being
sorted anyway.

Change-Id: If6f0d04e975db903e8a93c57c869ea4964c0be37
Reviewed-on: https://boringssl-review.googlesource.com/12062
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-11-03 16:43:56 +00:00
David Benjamin
abbbee10ad Detach TLS 1.3 cipher configuration from the cipher language.
TLS 1.3 ciphers are now always enabled and come with a hard-coded
preference order.

BUG=110

Change-Id: Idd9cb0d75fb6bf2676ecdee27d88893ff974c4a3
Reviewed-on: https://boringssl-review.googlesource.com/12025
Reviewed-by: Adam Langley <agl@google.com>
2016-11-02 20:47:55 +00:00
Nick Harper
60a85cb5e4 Implement ChannelID for TLS 1.3.
Channel ID for TLS 1.3 uses the same digest construction as
CertificateVerify. This message is signed with the Channel ID key and
put in the same handshake message (with the same format) as in TLS 1.2.

BUG=103

Change-Id: Ia5b2dffe5a39c39db0cecb0aa6bdc328e53accc2
Reviewed-on: https://boringssl-review.googlesource.com/11420
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-10-20 20:57:10 +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
b74b08144e Move next_proto_neg_seen into SSL_HANDSHAKE.
Change-Id: I7f1d546f735ca854ac58c65b529218afda164ec0
Reviewed-on: https://boringssl-review.googlesource.com/11523
Reviewed-by: Adam Langley <agl@google.com>
2016-10-09 16:50:13 +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
Steven Valdez
803c77a681 Update crypto negotation to draft 15.
BUG=77

Change-Id: If568412655aae240b072c29d763a5b17bb5ca3f7
Reviewed-on: https://boringssl-review.googlesource.com/10840
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
2016-10-06 14:37:09 +00:00
Steven Valdez
fdd10998e1 Moving TLS 1.3 version negotiation into extension.
Change-Id: I73f9fd64b46f26978b897409d817b34ec9d93afd
Reviewed-on: https://boringssl-review.googlesource.com/11080
Reviewed-by: Steven Valdez <svaldez@google.com>
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-27 20:12:22 +00:00
David Benjamin
65ac997f20 Implement draft-davidben-tls-grease-01.
This GREASEs cipher suites, groups, and extensions. For now, we'll
always place them in a hard-coded position. We can experiment with more
interesting strategies later.

If we add new ciphers and curves, presumably we prefer them over current
ones, so place GREASE values at the front. This prevents implementations
from parsing only the first value and ignoring the rest.

Add two new extensions, one empty and one non-empty. Place the empty one
in front (IBM WebSphere can't handle trailing empty extensions) and the
non-empty one at the end.

Change-Id: If2e009936bc298cedf2a7a593ce7d5d5ddbb841a
Reviewed-on: https://boringssl-review.googlesource.com/11241
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-09-23 21:11:15 +00:00
David Benjamin
2dc0204603 Don't return invalid versions in version_from_wire.
This is in preparation for using the supported_versions extension to
experiment with draft TLS 1.3 versions, since we don't wish to restore
the fallback. With versions begin opaque values, we will want
version_from_wire to reject unknown values, not attempt to preserve
order in some way.

This means ClientHello.version processing needs to be separate code.
That's just written out fully in negotiate_version now. It also means
SSL_set_{min,max}_version will notice invalid inputs which aligns us
better with upstream's versions of those APIs.

This CL doesn't replace ssl->version with an internal-representation
version, though follow work should do it once a couple of changes land
in consumers.

BUG=90

Change-Id: Id2f5e1fa72847c823ee7f082e9e69f55e51ce9da
Reviewed-on: https://boringssl-review.googlesource.com/11122
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 19:51:45 +00:00
David Benjamin
e0ff767025 Remove SSL_set_fallback_version.
Ding-dong the fallback's dead.
https://mailarchive.ietf.org/arch/msg/tls/xfCh7D7hISFs5x-eA0xHwksoLrc

Also we'll need to tweak the versioning code slightly to implement
supported_versions and it's nice to have this out of the way.

Change-Id: I0961e19ea56b4afd828f6f48858ac6310129503d
Reviewed-on: https://boringssl-review.googlesource.com/11120
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 17:03:42 +00:00
David Benjamin
786793411a Do not distinguish NULL and empty PSK identity hints.
Plain PSK omits the ServerKeyExchange when there is no hint and includes
it otherwise (it should have always sent it), while other PSK ciphers
like ECDHE_PSK cannot omit the hint. Having different capabilities here
is odd and RFC 4279 5.2 suggests that all PSK ciphers are capable of
"[not] provid[ing] an identity hint".

Interpret this to mean no identity hint and empty identity hint are the
same state. Annoyingly, this gives a plain PSK implementation two
options for spelling an empty hint. The spec isn't clear and this is not
really a battle worth fighting, so I've left both acceptable and added a
test for this case.

See also https://android-review.googlesource.com/c/275217/. This is also
consistent with Android's PskKeyManager API, our only consumer anyway.

https://developer.android.com/reference/android/net/PskKeyManager.html

Change-Id: I8a8e6cc1f7dd1b8b202cdaf3d4f151bebfb4a25b
Reviewed-on: https://boringssl-review.googlesource.com/11087
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-09-20 23:00:47 +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
4fe3c90b7d Release TLS 1.3 key shares earlier in TLS 1.2.
This isn't hugely important since the hs object will actually be
released at the end of the handshake, but no sense in holding on to them
longer than needed.

Also release |public_key| when we no longer need it and document what
the fields mean.

Change-Id: If677cb4a915c75405dabe7135205630527afd8bc
Reviewed-on: https://boringssl-review.googlesource.com/10360
Reviewed-by: Steven Valdez <svaldez@google.com>
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-19 20:35:35 +00:00
David Benjamin
3d458dc048 Revert of Determining certificate_auth and key_exchange based on SSL.
Reason for revert:  Right now in TLS 1.3, certificate_auth is exactly
the same as whether we're doing resumption. With the weird reauth
stuff punted to later in the spec, having extra state is just more
room for bugs to creep in.

Original issue's description:
> Determining certificate_auth and key_exchange based on SSL.
> 
> This allows us to switch TLS 1.3 to use non-cipher based negotiation
> without needing to use separate functions between 1.3 and below.
> 
> BUG=77
> 
> Change-Id: I9207e7a6793cb69e8300e2c15afe3548cbf82af2
> Reviewed-on: https://boringssl-review.googlesource.com/10803
> 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>
> 

Change-Id: I240e3ee959ffd1f2481a06eabece3af554d20ffa
Reviewed-on: https://boringssl-review.googlesource.com/11008
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 22:54:08 +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
54091230cd Use C99 for size_t loops.
This was done just by grepping for 'size_t i;' and 'size_t j;'. I left
everything in crypto/x509 and friends alone.

There's some instances in gcm.c that are non-trivial and pulled into a
separate CL for ease of review.

Change-Id: I6515804e3097f7e90855f1e7610868ee87117223
Reviewed-on: https://boringssl-review.googlesource.com/10801
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-09-12 19:44:24 +00:00
Steven Valdez
bd09eccd6d Determining certificate_auth and key_exchange based on SSL.
This allows us to switch TLS 1.3 to use non-cipher based negotiation
without needing to use separate functions between 1.3 and below.

BUG=77

Change-Id: I9207e7a6793cb69e8300e2c15afe3548cbf82af2
Reviewed-on: https://boringssl-review.googlesource.com/10803
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 19:23:14 +00:00
David Benjamin
4d0be24319 Only allow SSL_set_session before the handshake.
Otherwise things break horribly. Explicitly abort to help catch bugs.

Change-Id: I66e2bf8808199b3331b3adde68d73758a601eb8c
Reviewed-on: https://boringssl-review.googlesource.com/10761
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-09-12 19:16:46 +00:00
David Benjamin
fc0c9d9665 Use a common state to begin the handshake.
This simplifies the logic around SSL_clear to reset the state for a new
handshake. The state around here is still a little iffy, but this is a
slight improvement.

The SSL_ST_CONNECT and SSL_ST_ACCEPT states are still kept separate to
avoid problems with the info callback reporting SSL_ST_INIT. Glancing
through info callback consumers, although they're all debugging, they
tend to assume that all intermediate states either have only
SSL_ST_CONNECT set or only SSL_ST_ACCEPT set.

(They also all look identical which makes me think it's copy-and-pasted
from OpenSSL command-line tool or something.)

Change-Id: I55503781e52b51b4ca829256c14de6f5942dae51
Reviewed-on: https://boringssl-review.googlesource.com/10760
Reviewed-by: Adam Langley <agl@google.com>
2016-09-12 19:00:50 +00:00
David Benjamin
5510863fbd Temporary remove the TLS 1.3 anti-downgrade mechanism.
This mechanism is incompatible with deploying draft versions of TLS 1.3.

Suppose a draft M client talks to a draft N server, M != N. (Either M or
N could also be the final standard revision should there be lingering
draft clients or servers.) The server will notice the mismatch and
pretend ClientHello.version is TLS 1.2, not TLS 1.3. But this will
trigger anti-downgrade signal and cause an interop failure! And if it
doesn't trigger, all the clever tricks around ServerHello.random being
signed in TLS 1.2 are moot.

We'll put this back when the dust has settled.

Change-Id: Ic3cf72b7c31ba91e5cca0cfd7a3fca830c493a43
Reviewed-on: https://boringssl-review.googlesource.com/11005
Reviewed-by: Steven Valdez <svaldez@google.com>
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 18:10:23 +00:00
David Benjamin
7aa31d68fc Remove ssl->verify_result.
Having two copies of this is confusing. This field is inherently tied to
the certificate chain, which lives on SSL_SESSION, so this should live
there too. This also wasn't getting reset correctly on SSL_clear, but
this is now resolved.

Change-Id: I22b1734a93320bb0bf0dc31faa74d77a8e1de906
Reviewed-on: https://boringssl-review.googlesource.com/10283
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-19 18:29:43 +00:00