Commit Graph

1920 Commits

Author SHA1 Message Date
Adam Langley
0bdef09263 Check CA names during the handshake.
Rather than store CA names and only find out that they're unparsable
when we're asked for a |STACK_OF(X509_NAME)|, check that we can parse
them all during the handshake. This avoids changing the semantics with
the previous change that kept CA names as |CRYPTO_BUFFER|s.

Change-Id: I0fc7a4e6ab01685347e7a5be0d0579f45b8a4818
Reviewed-on: https://boringssl-review.googlesource.com/13969
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>
2017-02-28 22:33:05 +00:00
Adam Langley
34b4c829fd Hold CA names as |CRYPTO_BUFFER|s.
This change converts the CA names that are parsed from a server's
CertificateRequest, as well as the CA names that are configured for
sending to clients in the same, to use |CRYPTO_BUFFER|.

The |X509_NAME|-based interfaces are turned into compatibility wrappers.

Change-Id: I95304ecc988ee39320499739a0866c7f8ff5ed98
Reviewed-on: https://boringssl-review.googlesource.com/13585
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>
2017-02-28 22:24:31 +00:00
Adam Langley
2ff7933f22 Add test for |SSL_get_client_CA_list|.
Change-Id: Ibea6a9d52b000876740097f98c6891db4772371b
Reviewed-on: https://boringssl-review.googlesource.com/14008
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>
2017-02-28 22:17:39 +00:00
Adam Langley
898be92b67 Support P-224 certificates as a server.
Change-Id: I3bc1e46fb94104c4ae31c1c98fa0d5a931e5f954
Reviewed-on: https://boringssl-review.googlesource.com/13974
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-27 21:27:39 +00:00
David Benjamin
adec7726ec Remove SSL_CIPHER_has_MD5_HMAC.
It has no more callers.

Change-Id: I587ccb3b63810ed167febf7a65ba85106d17a300
Reviewed-on: https://boringssl-review.googlesource.com/13911
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>
2017-02-22 22:20:56 +00:00
Matthew Braithwaite
a57dcfb69c Add new cipherlist-setting APIs that reject nonsense.
The new APIs are SSL_CTX_set_strict_cipher_list() and
SSL_set_strict_cipher_list().  They have two motivations:

First, typos in cipher lists can go undetected for a long time, and
can have surprising consequences when silently ignored.

Second, there is a tendency to use superstition in the construction of
cipher lists, for example by "turning off" things that do not actually
exist.  This leads to the corrosive belief that DEFAULT and ALL ought
not to be trusted.  This belief is false.

Change-Id: I42909b69186e0b4cf45457e5c0bc968f6bbf231a
Reviewed-on: https://boringssl-review.googlesource.com/13925
Commit-Queue: Matt Braithwaite <mab@google.com>
Reviewed-by: Matt Braithwaite <mab@google.com>
2017-02-22 00:09:27 +00:00
David Benjamin
fc02b59b27 Move tmp.extended_master_secret to SSL_HANDSHAKE.
The two non-trivial changes are:

1. The public API now queries it out of the session. There is a long
   comment over the old field explaining why the state was separate, but
   this predates EMS being forbidden from changing across resumption. It
   is not possible for established_session and the socket to disagree on
   EMS.

2. Since SSL_HANDSHAKE gets reset on each handshake, the check that EMS
   does not change on renego looks different. I've reworked that function a
   bit, but it should have the same effect.

Change-Id: If72e5291f79681381cf4d8ceab267f76618b7c3d
Reviewed-on: https://boringssl-review.googlesource.com/13910
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-17 21:36:37 +00:00
David Benjamin
45738dd496 Move new_cipher and new_session to SSL_HANDSHAKE.
This lets us trim another two pointers of per-connection state.

Change-Id: I2145d529bc25b7e24a921d01e82ee99f2c98867c
Reviewed-on: https://boringssl-review.googlesource.com/13804
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>
2017-02-17 20:48:31 +00:00
David Benjamin
12709db355 Stop reporting SSL_R_SHUTDOWN_WHILE_IN_INIT.
This effectively reverts b9824e2417. This
error seems to have mostly just caused confusion in logs and the
occasional bug around failing to ERR_clear_error. Consumers tend to
blindly call SSL_shutdown when tearing down an SSL (to avoid
invalidating sessions). This means handshake failures trigger two
errors, which is screwy.

Go back to the old behavior where SSL_shutdown while SSL_in_init
silently succeeds.

Change-Id: I1fcfc92d481b97c840847dc39afe59679cd995f2
Reviewed-on: https://boringssl-review.googlesource.com/13909
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-17 19:49:59 +00:00
David Benjamin
5960a90964 Move sid_ctx from SSL/SSL_CTX to CERT.
This reduces us from seven different configuration patterns to six (see
comment #2 of linked bug). I do not believe there is any behavior change
here as SSL_set_SSL_CTX already manually copied the field. It now gives
us a nice invariant: SSL_set_SSL_CTX overrides all and only the
dual-SSL/SSL_CTX options hanging off of CERT.

BUG=123

Change-Id: I1ae06b791fb869917a6503cee41afb2d9be53d89
Reviewed-on: https://boringssl-review.googlesource.com/13865
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>
2017-02-16 18:59:24 +00:00
David Benjamin
26e1ff3dfb Remove some unnecessary return values.
I'm not sure why the SSL versions of these functions return int while
the SSL_CTX version returns void. It looks like this dates to
https://boringssl-review.googlesource.com/c/1491/, of which the initial
upload was an SSL_ctrl macro. I guess one of the ints got accidentally
preserved in conversion.

(No existing caller, aside from bssl_shim, checks the result.)

Change-Id: Id54309c1aa03462d520b9a45cdfdefdd2cdd1298
Reviewed-on: https://boringssl-review.googlesource.com/13866
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>
2017-02-16 18:21:47 +00:00
Steven Valdez
27a9e6ae1b Adding ALPN to session.
0-RTT requires matching the selected ALPN parameters against those in
the session. Stash the ALPN value in the session in TLS 1.3, so we can
recover it.

BUG=76

Change-Id: I8668b287651ae4deb0bf540c0885a02d189adee0
Reviewed-on: https://boringssl-review.googlesource.com/13845
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>
2017-02-16 17:33:23 +00:00
David Benjamin
83a321231b Move SCT lists and OCSP responses to CERT.
Recent changes added SSL-level setters to these APIs. Unfortunately,
this has the side effect of breaking SSL_set_SSL_CTX, which is how SNI
is typically handled. SSL_set_SSL_CTX is kind of a weird function in
that it's very sensitive to which of the hodge-podge of config styles is
in use. I previously listed out all the config styles here, but it was
long and unhelpful. (I counted up to 7.)

Of the various SSL_set_SSL_CTX-visible config styles, the sanest seems
to be to move it to CERT. In this case, it's actually quite reasonable
since they're very certificate-related.

Later we may wish to think about whether we can cut down all 7 kinds of
config styles because this is kinda nuts. I'm wondering we should do
CERT => SSL_CONFIG, move everything there, and make that be the same
structure that is dropped post-handshake (supposing the caller has
disavowed SSL_clear and renego). Fruit for later thought. (Note though
that comes with a behavior change for all the existing config.)

Change-Id: I9aa47d8bd37bf2847869e0b577739d4d579ee4ae
Reviewed-on: https://boringssl-review.googlesource.com/13864
Reviewed-by: Martin Kreichgauer <martinkr@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>
2017-02-15 00:42:38 +00:00
David Benjamin
16b1b1d4d1 Simplify state and info_callback management.
All the business with rewinding hs->state back or skipping states based
on reuse_message or a skip parameter isn't really worth the trouble for
a debugging callback. With SSL_state no longer exposed, we don't have to
worry about breaking things.

BUG=177

Change-Id: I9a0421f01c8b2f24c80a6b3e44de9138ea023f58
Reviewed-on: https://boringssl-review.googlesource.com/13829
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>
2017-02-13 19:52:27 +00:00
David Benjamin
35ed5233c2 Remove an unnecessary state transition.
The split was there out of paranoia that some caller may notice the
change in initial state. Now that SSL_state is neutered, simplify.

BUG=177

Change-Id: I7e2138c2b56821b0c79eec98bb09a82fc28238e8
Reviewed-on: https://boringssl-review.googlesource.com/13828
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>
2017-02-13 19:52:09 +00:00
David Benjamin
77458a436e Avoid transitioning into SSL_ST_OK and back out.
I doubt this matters, but this seems a little odd. In particular, this
avoids info_callback seeing the SSL_ST_OK once we stop switching
hs->state back and forth.

BUG=177

Change-Id: Ied39c0e94c242af9d5d0f26795d6e0f2f0b12406
Reviewed-on: https://boringssl-review.googlesource.com/13827
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>
2017-02-13 19:52:02 +00:00
David Benjamin
9e766d7532 Unexport the handshake's internal state.
Code which manages to constrain itself on this will limit our ability to
rework the handshake. I believe, at this point, we only need to expose
one bit of information (there's some code that compares SSL_state to
SSL_ST_OK), if even that.

BUG=177

Change-Id: Ie1c43006737db0b974811f1819755c629ae68e7b
Reviewed-on: https://boringssl-review.googlesource.com/13826
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2017-02-13 19:44:30 +00:00
David Benjamin
07ab5d44d9 Stop skipping stray HelloRequests.
This makes sense to do if we are a client and initiate a renegotiation
at the same time as the server requesting one. Since we will never
initiate a renegotiation, this should not be necessary.

Change-Id: I5835944291fdb8dfcc4fed2ebf1064e91ccdbe6a
Reviewed-on: https://boringssl-review.googlesource.com/13825
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>
2017-02-13 19:44:22 +00:00
David Benjamin
7d7554b6b3 Add a helper for comparing byte strings.
We compare pointer/length pairs constantly. To avoid needing to type it
everywhere and get GTest's output, add a StringPiece-alike for byte
slices which supports ==, !=, and std::ostream.

BUG=129

Change-Id: I108342cbd2c6a58fec0b9cb87ebdf50364bda099
Reviewed-on: https://boringssl-review.googlesource.com/13625
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>
2017-02-11 00:42:00 +00:00
Adam Langley
46db7af2c9 Remove |X509| things from SSL_SESSION.
|SSL_SESSION_from_bytes| now takes an |SSL_CTX*|, from which it uses the
|X509_METHOD| and buffer pool. This is our API so we can do this.

This also requires adding an |SSL_CTX*| argument to |SSL_SESSION_new|
for the same reason. However, |SSL_SESSION_new| already has very few
callers (and none in third-party code that I can see) so I think we can
get away with this.

Change-Id: I1337cd2bd8cff03d4b9405ea3146b3b59584aa72
Reviewed-on: https://boringssl-review.googlesource.com/13584
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: Adam Langley <alangley@gmail.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-10 19:12:04 +00:00
David Benjamin
7ebe61a585 Fix server-side KeyUpdates.
We sized the post-handshake message limit for the older zero-length
KeyUpdate and forgot to update it when it got larger.

Thanks to Matt Caswell for catching this.

Change-Id: I7d2189479e9516fbfb6c195dfa367794d383582c
Reviewed-on: https://boringssl-review.googlesource.com/13805
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>
2017-02-10 18:20:52 +00:00
Steven Valdez
908ac19e8e Moving transcript and PRF functions to SSL_TRANSCRIPT.
Change-Id: I98903df561bbf8c5739f892d2ad5e89ac0eb8e6f
Reviewed-on: https://boringssl-review.googlesource.com/13369
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>
2017-02-10 16:33:42 +00:00
David Benjamin
d4c349b56c Test various lengths of ticket session IDs.
In honor of CVE-2016-9244. Although that particular bug BoGo was already
testing since it uses 16 bytes here.

The empty session ID case is particularly worth testing to make sure we
don't get confused somewhere. RFC 5077 allows clients to offer tickets
with no session ID. This is absurd since the client then has no way of
detecting resumption except by lookahead. We'll never do this as a
client, but should handle it correctly as a server.

Change-Id: I49695d19f03c4efdef43749c07372d590a010cda
Reviewed-on: https://boringssl-review.googlesource.com/13740
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-02-09 23:09:16 +00:00
Adam Langley
5e3777830c Rename ssl_rsa.c to ssl_privkey.c.
ssl_rsa.c now basically deals with private-key functions, so rename to
reflect that.

Change-Id: Ia87ed4c0f9b34af134844e2eeb270fc45ff3f23f
Reviewed-on: https://boringssl-review.googlesource.com/13583
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>
2017-02-09 18:45:11 +00:00
Adam Langley
52940c4945 Move a number of certificate-related functions from ssl_rsa.c to ssl_cert.c
This leaves ssl_rsa.c dealing only with private-key matters.

Change-Id: I46aa80abd8374d8cc6d2b0f7a9a345214ec86084
Reviewed-on: https://boringssl-review.googlesource.com/13582
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>
2017-02-09 18:36:18 +00:00
Adam Langley
3509dacc3c Add |X509_METHOD| and, using it, move many functions to ssl_x509.c.
Change-Id: I266af0c2bdcebcc1dd1026f816b9ef6ece5a592f
Reviewed-on: https://boringssl-review.googlesource.com/13581
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>
2017-02-09 18:30:35 +00:00
Adam Langley
04540a7fa7 Set the default value of a last |alert|.
I even made a note to update my change in light of this but still
managed to forget. With this, grep tells me that all |alert| values have
the correct default value now.

Change-Id: If37c4f2f6b36cf69e53303a3924a8eda4cfffed8
Reviewed-on: https://boringssl-review.googlesource.com/13721
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-09 18:19:27 +00:00
Adam Langley
c68e5b99a7 Establish that the default value of an out-arg for alerts is SSL_AD_DECODE_ERROR.
We already have some cases where the default is DECODE_ERROR and, rather
than have two defaults, just harmonise on that. (INTERNAL_ERROR might
make more sense in some cases, but we don't want to have to remember
what the default is in each case and nobody really cares what the actual
value is anyway.)

Change-Id: I28007898e8d6e7415219145eb9f43ea875028ab2
Reviewed-on: https://boringssl-review.googlesource.com/13720
Reviewed-by: Adam Langley <agl@google.com>
2017-02-09 18:07:30 +00:00
David Benjamin
dc8c1d962e Remove some dead state constants.
Change-Id: If97da565155292d5f0de5c6a8b0fd8508398768a
Reviewed-on: https://boringssl-review.googlesource.com/13564
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-02-09 02:15:44 +00:00
David Benjamin
59bae5aa3a Split TLS 1.2 ClientHello processing into separate functions.
This ABCD thing with multiple ways to enter the same function is
confusing. ClientHello processing is the most egregious of these, so
split it up ahead of time as an intermediate step.

States remain named as-is due to them being exposed as public API. We
should have a story for which subset of states we need to promise as
public API and to intentionally break all other cases (map to some
generic value) before we go too far there.

BUG=128

Change-Id: Id9d28c6de14bd53c3294552691cebe705748f489
Reviewed-on: https://boringssl-review.googlesource.com/13563
Reviewed-by: Adam Langley <agl@google.com>
2017-02-09 02:15:27 +00:00
Adam Langley
7dccc71e08 Set output alert when failing to parse public key.
Previously, the alert was uninitialised.

(Thanks to Robert Swiecki and honggfuzz.)

Change-Id: I2d4eb96b0126f3eb502672b2600ad43ae140acec
Reviewed-on: https://boringssl-review.googlesource.com/13700
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-08 17:09:03 +00:00
David Benjamin
cdb6fe90cb Test the exporter at all versions + ciphers + side.
The Go side (thankfully not the C side) was not fully updated for the
exporter secret derivation being earlier at some point. Also TLS 1.2
upgrades the PRF hash for pre-1.2 ciphers to SHA-256, so make sure we
cover that.

Change-Id: Ibdf50ef500e7e48a52799ac75577822bc304a613
Reviewed-on: https://boringssl-review.googlesource.com/13663
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>
2017-02-07 21:37:44 +00:00
David Benjamin
3bd57e3a6f Remove remnants of non-standard ECDHE_PSK GCM ciphers.
Change-Id: I471880d785c38123e038279f67348bf02b47d091
Reviewed-on: https://boringssl-review.googlesource.com/13662
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>
2017-02-07 21:37:22 +00:00
David Benjamin
6b0edfb9e6 Add a common TestEventListener for the error queue.
Replicate the logic in the AllTests targets to dump the error queue on
failure. GTest seems to print to stdout, so we do here too.

BUG=129

Change-Id: I623b695fb9a474945834c3653728f54e5b122187
Reviewed-on: https://boringssl-review.googlesource.com/13623
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>
2017-02-07 21:33:22 +00:00
David Benjamin
f0d8e22078 Convert some of ssl_test to GTest more thoroughly.
The more complex ones will want a TEST_P, but here are a few easy ones
to start with.

BUG=129

Change-Id: I2e341d04910c0b05a5bc7afec961c4541ca7db41
Reviewed-on: https://boringssl-review.googlesource.com/13622
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>
2017-02-07 21:33:15 +00:00
Alessandro Ghedini
8df6766d01 Support setting per-connection SCT list
Right now the only way to set an SCT list is the per-context function
SSL_CTX_set_signed_cert_timestamp_list. However this assumes that all the
SSLs generated from a SSL_CTX share the same SCT list, which is wrong.

In order to avoid memory duplication in case SSL_CTX has its own list, a
CRYPTO_BUFFER is used for both SSL_CTX and SSL.

Change-Id: Id20e6f128c33cf3e5bff1be390645441be6518c6
Reviewed-on: https://boringssl-review.googlesource.com/13642
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>
2017-02-07 17:50:13 +00:00
Alessandro Ghedini
33fe4a0d14 Remove support for setting per-connection default session timeout
As previously discussed, it turns out we don't actually need this, so
there's no point in keeping it.

Change-Id: If549c917b6bd818cd36948e37cb7839c8d122b1a
Reviewed-on: https://boringssl-review.googlesource.com/13641
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>
2017-02-07 17:46:25 +00:00
David Benjamin
023d419eae Test that we tolerate server name acknowledgements.
The SNI extension may be ACKed by the server. This is kind of pointless,
but make sure we cover these codepaths.

Change-Id: I14b25ab865dd6e35a30f11ebc9027a1518bbeed9
Reviewed-on: https://boringssl-review.googlesource.com/13633
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>
2017-02-06 23:18:47 +00:00
Nick Harper
ab20cec1c1 Read 0-RTT data in Bogo.
Change-Id: I878dfb9f5d3736c3ec0d5fa39052cca58932dbb7
Reviewed-on: https://boringssl-review.googlesource.com/12981
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>
2017-02-06 22:36:53 +00:00
Nick Harper
f2511f19b9 Send 0-RTT data in bogo.
Change-Id: I38cd04fa40edde4e4dd31fdc16bbf92985430198
Reviewed-on: https://boringssl-review.googlesource.com/12702
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>
2017-02-06 22:35:45 +00:00
David Benjamin
3f2611a98f Hide SSL struct.
BUG=6

Change-Id: I5383ad230f1fdc54f9536c9922bfbf991401a00c
Reviewed-on: https://boringssl-review.googlesource.com/13632
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-06 18:45:50 +00:00
Steven Valdez
2f82a0e51b Don't stash tlsext_hostname in ssl_get_new_session.
ssl_get_new_session would stash a copy of the configured hostname
into the SSL_SESSION on the server. Servers have no reason to
configuring that anyway, but, if one did, we'd leak when filling in
the client-supplied SNI later.

Remove this code and guard against this by remembering to OPENSSL_free
when overwriting that field (although it should always be NULL).

Reported-By: Robert Swiecki <swiecki@google.com>
Change-Id: Ib901b5f82e5cf818060ef47a9585363e05dd9932
Reviewed-on: https://boringssl-review.googlesource.com/13631
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>
2017-02-06 18:42:53 +00:00
David Benjamin
b2ff2623a8 Add a basic SSL_get_certificate test.
With the CRYPTO_BUFFER stuff, this API is now slightly more complex. Add
some tests as a sanity-check.

Change-Id: I9da20e3eb6391fc86ed215c5fabec71aa32ef56f
Reviewed-on: https://boringssl-review.googlesource.com/13620
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>
2017-02-03 22:19:51 +00:00
Adam Langley
bdcfd1366f Move the SSL BIO into ssl/ from decrepit/.
This is purely to support curl, which now has HTTPS proxy support that,
sadly, uses the BIO SSL. Don't use the BIO SSL for anything else.

Change-Id: I9ef6c9773ec87a11e0b5a93968386ac4b351986d
Reviewed-on: https://boringssl-review.googlesource.com/13600
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-03 21:08:10 +00:00
David Benjamin
daa0539276 Remove an unnecessary TLS 1.3 ClientHello state.
The TLS 1.2 and 1.3 state machines do the exact same thing at the
beginning. Let them process the ClientHello extensions, etc., and
finalize the certificate in common code. Once we start picking
parameters, we begin to diverge. Everything before this point is
arguably part of setting up the configuration, which is
version-agnostic.

BUG=128

Change-Id: I293ea3087ecbc3267bd8cdaa011c98d26a699789
Reviewed-on: https://boringssl-review.googlesource.com/13562
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-02-03 20:03:37 +00:00
David Benjamin
42bfeb3623 Remove an unnecessary TLS 1.2 ClientHello state.
The version negotiation logic was a little bizarrely wedged in the
middle of the state machine. (We don't support server renegotiation, so
have_version is always false here.)

BUG=128

Change-Id: I9448dce374004b92e8bd5172c36a4e0eea51619c
Reviewed-on: https://boringssl-review.googlesource.com/13561
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-02-03 20:01:31 +00:00
David Benjamin
8671c47bd8 Fold ssl3_write_bytes into ssl3_write_app_data.
It has no other callers, now that the handshake is written elsewhere.

Change-Id: Ib04bbdc4a54fc7d01405d9b3f765fa9f186244de
Reviewed-on: https://boringssl-review.googlesource.com/13540
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>
2017-02-02 22:23:46 +00:00
David Benjamin
17b3083373 Use a separate timeout scheme for TLS 1.3.
In TLS 1.2, resumption's benefits are more-or-less subsumed by False
Start. TLS 1.2 resumption lifetime is bounded by how much traffic we are
willing to encrypt without fresh key material, so the lifetime is short.
Renewal uses the same key, so we do not allow it to increase lifetimes.

In TLS 1.3, resumption unlocks 0-RTT. We do not implement psk_ke, so
resumption incorporates fresh key material into both encrypted traffic
(except for early data) and renewed tickets. Thus we are both more
willing to and more interested in longer lifetimes for tickets. Renewal
is also not useless. Thus in TLS 1.3, lifetime is bound separately by
the lifetime of a given secret as a psk_dhe_ke authenticator and the
lifetime of the online signature which authenticated the initial
handshake.

This change maintains two lifetimes on an SSL_SESSION: timeout which is
the renewable lifetime of this ticket, and auth_timeout which is the
non-renewable cliff. It also separates the TLS 1.2 and TLS 1.3 timeouts.
The old session timeout defaults and configuration apply to TLS 1.3, and
we define new ones for TLS 1.3.

Finally, this makes us honor the NewSessionTicket timeout in TLS 1.3.
It's no longer a "hint" in 1.3 and there's probably value in avoiding
known-useless 0-RTT offers.

BUG=120

Change-Id: Iac46d56e5a6a377d8b88b8fa31f492d534cb1b85
Reviewed-on: https://boringssl-review.googlesource.com/13503
Reviewed-by: Adam Langley <agl@google.com>
2017-02-02 19:51:49 +00:00
David Benjamin
0b1bb12ce8 Push the SSL_CTX session_timeout zero logic up.
This special-case is almost unexposed (the timeout is initialized to the
default) except if the caller calls SSL_CTX_set_timeout(0). Preserve
that behavior by mapping 0 to SSL_DEFAULT_SESSION_TIMEOUT in
SSL_CTX_set_timeout but simplify the internal state.

Change-Id: Ice03a519c25284b925f1e0cf485f2d8c54dc5038
Reviewed-on: https://boringssl-review.googlesource.com/13502
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-02-02 17:52:07 +00:00
David Benjamin
0efa7592e3 dispatch_alert is not an incidental write.
It is impossible to have to call dispatch_alert when writing application
data. Now that we don't send warning alerts through ssl3_send_alert, all
alerts are closure alerts, which means attempts to write will fail.

This prunes a lot of dead code, avoiding the re-entrancy in the write
path. With that gone, tracking alert_dispatch is much more
straightforward.

BUG=146

Change-Id: Ie5fe677daee71e463d79562f3d2cea822a92581d
Reviewed-on: https://boringssl-review.googlesource.com/13500
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2017-02-02 17:49:44 +00:00
David Benjamin
e79fe70be9 Bit-pack SSL_AEAD_CTX's various toggles.
Change-Id: Ibb479a0a739a44d0568e37cdfdb30b30e5410c02
Reviewed-on: https://boringssl-review.googlesource.com/13520
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-02-01 23:37:34 +00:00
David Benjamin
b5c58db9ff TLS 1.3 sessions should not be added to the server session cache.
Fix this and add a test. Otherwise enabling TLS 1.3 will cause a server
to blow through its session cache.

Change-Id: I67edbc468faedfd94a6c30cf842af085a6543b50
Reviewed-on: https://boringssl-review.googlesource.com/13501
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>
2017-02-01 23:07:55 +00:00
Adam Langley
c26692cfdd Push the use of X509 upwards, out of |ssl_set_cert|.
This change moves the interface between |X509| and |CRYPTO_BUFFER| a
little further out, towards the API.

Change-Id: I1c014d20f12ad83427575843ca0b3bb22de1a694
Reviewed-on: https://boringssl-review.googlesource.com/13365
Reviewed-by: Adam Langley <agl@google.com>
2017-02-01 20:00:10 +00:00
Adam Langley
e1e78130f5 Keep a reference to |X509|s appended to a chain.
The recent CRYPTO_BUFFER changes meant that |X509| objects passed to
SSL_CTX_add_extra_chain_cert would be |free|ed immediately. However,
some third-party code (at least serf and curl) continue to use the
|X509| even after handing over ownership.

In order to unblock things, keep the past |X509| around for a while to
paper over the issues with those libraries while we try and upstream
changes.

Change-Id: I832b458af9b265749fed964658c5c34c84d518df
Reviewed-on: https://boringssl-review.googlesource.com/13480
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>
2017-02-01 00:24:24 +00:00
Nick Harper
7cd0a978cc Bogo: Send and receive 0.5-RTT data.
Change-Id: I44202457841f06a899e140f78ae8afa7ac720283
Reviewed-on: https://boringssl-review.googlesource.com/12600
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>
2017-02-01 00:04:04 +00:00
David Benjamin
f71036e4e3 Remove ssl_hash_message_t from ssl_get_message.
Move to explicit hashing everywhere, matching TLS 1.2 with TLS 1.3. The
ssl_get_message calls between all the handshake states are now all
uniform so, when we're ready, we can rewire the TLS 1.2 state machine to
look like the TLS 1.3 one. (ssl_get_message calls become an
ssl_hs_read_message transition, reuse_message becomes an ssl_hs_ok
transition.)

This avoids some nuisance in processing the ServerHello at the 1.2 / 1.3
transition.

The downside of explicit hashing is we may forget to hash something, but
this will fail to interop with our tests and anyone else, so we should
be able to catch it.

BUG=128

Change-Id: I01393943b14dfaa98eec2a78f62c3a41c29b3a0e
Reviewed-on: https://boringssl-review.googlesource.com/13266
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>
2017-01-27 23:23:57 +00:00
David Benjamin
1a444daca6 Detach V2ClientHello hashing hack from ssl_hash_message_t.
This is kind of annoying (even new state is needed to keep the layering
right). As part of aligning the read paths of the TLS 1.2 and TLS 1.3
state machine, we'll want to move to states calling
ssl_hash_current_message when the process the message, rather than when
the message is read. Right now the TLS 1.2 optional message story
(reuse_message) depends on all messages preceded by an optional message
using ssl_hash_message. For instance, if TLS 1.2 decided to place
CertificateStatus before ServerKeyExchange, we would not be able to
handle it.

However, V2ClientHello, by being handled in the message layer, relies on
ssl_get_message-driven hashing to replace the usual ClientHello hash
with a hash of something custom. This switches things so rather than
ClientHellos being always pre-hashed by the message layer, simulated
ClientHellos no-op ssl_hash_current_message.

This just replaces one hack with another (V2ClientHello is inherently
nasty), but this hack should be more compatible with future plans.

BUG=128

Change-Id: If807ea749d91e306a37bb2362ecc69b84bf224c9
Reviewed-on: https://boringssl-review.googlesource.com/13265
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>
2017-01-27 23:22:14 +00:00
David Benjamin
276b7e8127 Move optional message type checks out of ssl_get_message.
This aligns the TLS 1.2 state machine closer with the TLS 1.3 state
machine. This is more work for the handshake, but ultimately the
plan is to take the ssl_get_message call out of the handshake (so it is
just the state machine rather than calling into BIO), so the parameters
need to be folded out as in TLS 1.3.

The WrongMessageType-* family of tests should make sure we don't miss
one of these.

BUG=128

Change-Id: I17a1e6177c52a7540b2bc6b0b3f926ab386c4950
Reviewed-on: https://boringssl-review.googlesource.com/13264
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>
2017-01-27 23:15:52 +00:00
Adam Langley
6f07d726c9 Don't up_ref a NULL |CRYPTO_BUFFER|.
If an existing chain had a NULL placeholder for a leaf we could end up
trying to increment its reference count. That results in a crash at
configuration time. Found via the SSL_CTX API fuzzer.

BUG=oss-fuzz:480

Change-Id: I0ddc2cbde2e625015768f1bdc8da625e8a4f05fd
Reviewed-on: https://boringssl-review.googlesource.com/13383
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>
2017-01-27 22:09:49 +00:00
David Benjamin
42e3e191e4 Restore mapping BIO_flush errors to -1.
This was originally changed so that flush_flight could forward BIO_write
errors as-is, but we can and probably should still map BIO_flush errors.
This is unlikely to matter (every relevant BIO likely just has a no-op
flush which returns one), but, e.g., our file BIO returns 0, not -1, on
error.

We possibly should also be mapping BIO_write errors, but I'll leave that
alone for now. It's primarily BIO_read where the BIO return value must
be preserved due to error vs. EOF.

(We probably can just remove the BIO_flush calls altogether, but since
the buffer BIO forwarded the flush signal it would be a user-visible
behavior change to confirm.)

Change-Id: Ib495cc5d043867cf964f99b7ee4535114f7b2230
Reviewed-on: https://boringssl-review.googlesource.com/13367
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-27 16:24:19 +00:00
Adam Langley
830f7009eb Rename some single-letter argument names.
(I split this change off to minimise the noise in future diffs that
actually do something meaningful.)

Change-Id: I7a054dcfc90a44ab5bb89b8f46704e5e3410e524
Reviewed-on: https://boringssl-review.googlesource.com/13364
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-27 16:21:23 +00:00
Adam Langley
3a2b47ab5b Don't use |X509| objects in |CERT|, by default.
This change converts the |CERT| struct to holding certificates as binary
blobs, rather than in parsed form. The members for holding the parsed
form are still there, however, but are only used as a cache for the
event that someone asks us for a non-owning pointer to the parsed leaf
or chain.

Next steps:
  * Move more functions in to ssl_x509.c
  * Create an X509_OPS struct of function pointers that will hang off
    the |SSL_METHOD| to abstract out the current calls to crypto/x509
    operations.

BUG=chromium:671420

Change-Id: Ifa05d88c49a987fd561b349705c9c48f106ec868
Reviewed-on: https://boringssl-review.googlesource.com/13280
Reviewed-by: Adam Langley <agl@google.com>
2017-01-27 16:21:05 +00:00
David Benjamin
2fe6e227fb Remove mask_a and mask_k from CERT.
This resolves a TODO, trims per-connection memory, and makes more sense.
These masks have nothing to do with certificate configuration.

Change-Id: I783e6158e51f58cce88e3e68dfa0ed965bdc894c
Reviewed-on: https://boringssl-review.googlesource.com/13368
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>
2017-01-27 15:44:17 +00:00
David Benjamin
9b16066654 Ignore 0-RTT-capable tickets unless enabled.
Until we've gotten it fully working, we should not mint any of these
SSL_SESSIONs, to avoid constraining future versions of our client code.

Notably, if any of our TLS 1.3 clients today serialized sessions, we
would need to rev the serialization format. Without opting into 0-RTT, a
TLS 1.3 client will create SSL_SESSIONs tagged as 0-RTT-capable but
missing important fields (ALPN, etc.). When that serialized session
makes its way to a future version of our client code, it would disagree
with the server about the ALPN value stored in the ticket and cause
interop failures.

I believe the only client code enabling TLS 1.3 right now is Chrome, and
the window is small, so it should be fine. But fix this now before it
becomes a problem.

Change-Id: Ie2b109f8d158017a6f3b4cb6169050d38a66b31c
Reviewed-on: https://boringssl-review.googlesource.com/13342
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2017-01-26 21:29:32 +00:00
Steven Valdez
258508fce1 Adding V2ClientHello counter.
Change-Id: I324743e7d1864fbbb9653209ff93e4da872c8d31
Reviewed-on: https://boringssl-review.googlesource.com/13340
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>
2017-01-26 20:32:00 +00:00
Nick Harper
47383aadff Skip over early data in bogo.
Change-Id: Idc93fdca2f1c5c23e4ba48c4efed2edbad1e857b
Reviewed-on: https://boringssl-review.googlesource.com/12521
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>
2017-01-26 02:38:56 +00:00
David Benjamin
16315f7cc7 Remove the rest of write_message.
The TLS 1.2 state machine now looks actually much closer to the TLS 1.3
one on the write side. Although the write states still have a BIO-style
return, they don't actually send anything anymore. Only the BIO flush
state does. Reads are still integrated into the states themselves
though, so I haven't made it match TLS 1.3 yet.

BUG=72

Change-Id: I7708162efca13cd335723efa5080718a5f2808ab
Reviewed-on: https://boringssl-review.googlesource.com/13228
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 23:39:23 +00:00
David Benjamin
0f24bedb05 Rename tls13_prepare_* to tls13_add_*.
The SSL code suffers from needing too many verbs for variations on
writing things without actually writing them. We used to have queuing
the message up to be written to the buffer BIO, writing to the buffer
BIO, and flushing the buffer BIO. (Reading, conversely, has a similar
mess of verbs.)

Now we just have adding to the pending flight and flushing the pending
flight, match the SSL_PROTOCOL_METHOD naming.

BUG=72

Change-Id: I332966928bf13f03dfb8eddd519c2fefdd7f24d4
Reviewed-on: https://boringssl-review.googlesource.com/13227
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 23:37:30 +00:00
David Benjamin
81b7bc3979 Trim unnecessary TLS 1.3 states.
Large chunks of contiguous messages can now be sent in a row. Notably,
the ServerHello flight involves a number of optional messages which can
now be collapsed into straight-line code.

BUG=72

Change-Id: I1429d22a12401aa0f811a04e495bd5d754c084a4
Reviewed-on: https://boringssl-review.googlesource.com/13226
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 23:36:19 +00:00
David Benjamin
25ac251a1d Remove write_message from TLS 1.3 handshakes.
BUG=72

Change-Id: I4aad718762925191d85f0a468eeec4aa5d85d1e8
Reviewed-on: https://boringssl-review.googlesource.com/13225
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 23:36:02 +00:00
David Benjamin
daf207a52a Don't use the buffer BIO in TLS.
On the TLS side, we introduce a running buffer of ciphertext. Queuing up
pending data consists of encrypting the record into the buffer. This
effectively reimplements what the buffer BIO was doing previously, but
this resizes to fit the whole flight.

As part of this, rename all the functions to add to the pending flight
to be more uniform. This CL proposes "add_foo" to add to the pending
flight and "flush_flight" to drain it.

We add an add_alert hook for alerts but, for now, only the SSL 3.0
warning alert (sent mid-handshake) uses this mechanism.  Later work will
push this down to the rest of the write path so closure alerts use it
too, as in DTLS. The intended end state is that all the ssl_buffer.c and
wpend_ret logic will only be used for application data and eventually
optionally replaced by the in-place API, while all "incidental" data
will be handled internally.

For now, the two buffers are mutually exclusive. Moving closure alerts
to "incidentals" will change this, but flushing application data early
is tricky due to wpend_ret. (If we call ssl_write_buffer_flush,
do_ssl3_write doesn't realize it still has a wpend_ret to replay.) That
too is all left alone in this change.

To keep the diff down, write_message is retained for now and will be
removed from the state machines in a follow-up change.

BUG=72

Change-Id: Ibce882f5f7196880648f25d5005322ca4055c71d
Reviewed-on: https://boringssl-review.googlesource.com/13224
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 23:35:47 +00:00
David Benjamin
1a999cf54d Don't use the buffer BIO in DTLS.
Instead, "writing" a message merely adds it to the outgoing_messages
structure. The code to write the flight then loops over it all and now
shares code with retransmission. The verbs here are all a little odd,
but they'll be fixed in later commits.

In doing so, this fixes a slight miscalculation of the record-layer
overhead when retransmitting a flight that spans two epochs. (We'd use
the encrypted epoch's overhead for the unencrypted epoch.)

BUG=72

Change-Id: I8ac897c955cc74799f8b5ca6923906e97d6dad17
Reviewed-on: https://boringssl-review.googlesource.com/13223
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 23:35:32 +00:00
David Benjamin
6d50f475e3 Remove support for RSA premaster logging.
This was replaced by the more general CLIENT_RANDOM scheme that records
the master secret. Support was added in Wireshark 1.8.0, released in
June 2012. At this point, ECDHE is sufficiently widely deployed that
anyone that cares about this feature must have upgraded their Wireshark
by now.

Change-Id: I9b708f245ec8728c1999daf91aca663be7d25661
Reviewed-on: https://boringssl-review.googlesource.com/13263
Reviewed-by: David Benjamin <davidben@google.com>
2017-01-25 16:48:35 +00:00
David Benjamin
a772b16f9f Allow dtls_seal_record to work in-place.
This will let us avoid a scratch buffer when assembling DTLS handshake
packets in the write_message-less flow.

BUG=72

Change-Id: I15e78efe3a9e3933c307e599f0043427330f4a9e
Reviewed-on: https://boringssl-review.googlesource.com/13262
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 16:27:32 +00:00
David Benjamin
3b584332ee Fix ssl_test with BORINGSSL_ANDROID_SYSTEM.
We need to suppress a few tests on the system Android build until
RSA-PSS is shipped there.

Change-Id: I5843997aae9fa499ec08d76f44fdf3b523599e1c
Reviewed-on: https://boringssl-review.googlesource.com/13267
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-25 16:23:10 +00:00
David Benjamin
5db7c9b8c2 Get OPENSSL_COMPILE_ASSERT working in function bodies.
Change-Id: Ifc28887cbf91c7a80bdaf56e3bf80b2f8cfa7d53
Reviewed-on: https://boringssl-review.googlesource.com/13260
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-24 21:30:33 +00:00
Adam Langley
d1515a3b0a Move a number of X.509 functions from ssl_lib.c to ssl_x509.c
Eventually, all uses of crypto/x509 will be from ssl_x509.c, but this is
just a start.

Change-Id: I2f38cdcbf18b1f26add0aac10a70af10a79dee0e
Reviewed-on: https://boringssl-review.googlesource.com/13242
Reviewed-by: Adam Langley <agl@google.com>
2017-01-24 17:53:07 +00:00
Adam Langley
03b96d70f9 Remove unused |ssl_parse_x509|.
Change-Id: Id81297add5dcba8b861ca107a57a322df4c41c3d
Reviewed-on: https://boringssl-review.googlesource.com/13241
Reviewed-by: Adam Langley <agl@google.com>
2017-01-24 17:51:04 +00:00
David Benjamin
8d5f9da2e3 Abstract away BIO_flush calls in the handshake.
This is the first part to removing the buffer BIO. The eventual end
state is the SSL_PROTOCOL_METHOD is responsible for maintaining one
flight's worth of messages. In TLS, it will just be a buffer containing
the flight's ciphertext. In DTLS, it's the existing structure for
retransmit purposes. There will be hooks:

- add_message (synchronous)
- add_change_cipher_spec (synchronous)
- add_warning_alert (synchronous; needed until we lose SSLv3 client auth
  and TLS 1.3 draft 18; draft 19 will switch end_of_early_data to a
  handshake message)
- write_flight (BIO; flush_flight will be renamed to this)

This also preserves the exact return value of BIO_flush. Eventually all
the BIO_write calls will be hidden behind BIO_flush to, to be consistent
with other BIO-based calls, preserve the return value.

BUG=72

Change-Id: I74cd23759a17356aab3bb475a8ea42bd2cd115c9
Reviewed-on: https://boringssl-review.googlesource.com/13222
Reviewed-by: Adam Langley <agl@google.com>
2017-01-24 16:16:02 +00:00
Nick Harper
44c1a65760 Run go fmt on bogo code.
Change-Id: I15363a9c9ebb4e08bd9cf45ba2c95368766bb19b
Reviewed-on: https://boringssl-review.googlesource.com/13240
Reviewed-by: David Benjamin <davidben@google.com>
2017-01-24 00:29:38 +00:00
David Benjamin
5b410b6bec Remove unnecessary CBS_get_asn1_element.
EVP_parse_public_key already acts like CBS_get_* in that it peels one
element off and leaves a remainder.

Change-Id: Ic90952785005ed81664a6f46503b13ecd293176c
Reviewed-on: https://boringssl-review.googlesource.com/13045
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2017-01-21 00:50:13 +00:00
David Benjamin
966284337d Do a cursory conversion of a few tests to GTest.
For now, this is the laziest conversion possible. The intent is to just
get the build setup ready so that we can get everything working in our
consumers. The intended end state is:

- The standalone build produces three test targets, one per library:
  {crypto,ssl,decrepit}_tests.

- Each FOO_test is made up of:
    FOO/**/*_test.cc
    crypto/test/gtest_main.cc
    test_support

- generate_build_files.py emits variables crypto_test_sources and
  ssl_test_sources. These variables are populated with FindCFiles,
  looking for *_test.cc.

- The consuming file assembles those variables into the two test targets
  (plus decrepit) from there. This avoids having generate_build_files.py
  emit actual build rules.

- Our standalone builders, Chromium, and Android just run the top-level
  test targets using whatever GTest-based reporting story they have.

In transition, we start by converting one of two tests in each library
to populate the three test targets. Those are added to all_tests.json
and all_tests.go hacked to handle them transparently. This keeps our
standalone builder working.

generate_build_files.py, to start with, populates the new source lists
manually and subtracts them out of the old machinery. We emit both for
the time being. When this change rolls in, we'll write all the build
glue needed to build the GTest-based tests and add it to consumers'
continuous builders.

Next, we'll subsume a file-based test and get the consumers working with
that. (I.e. make sure the GTest targets can depend on a data file.)

Once that's all done, we'll be sure all this will work. At that point,
we start subsuming the remaining tests into the GTest targets and,
asynchronously, rewriting tests to use GTest properly rather than
cursory conversion here.

When all non-GTest tests are gone, the old generate_build_files.py hooks
will be removed, consumers updated to not depend on them, and standalone
builders converted to not rely on all_tests.go, which can then be
removed. (Unless bits end up being needed as a malloc test driver. I'm
thinking we'll want to do something with --gtest_filter.)

As part of this CL, I've bumped the CMake requirements (for
target_include_directories) and added a few suppressions for warnings
that GTest doesn't pass.

BUG=129

Change-Id: I881b26b07a8739cc0b52dbb51a30956908e1b71a
Reviewed-on: https://boringssl-review.googlesource.com/13232
Reviewed-by: Adam Langley <agl@google.com>
2017-01-21 00:17:05 +00:00
Alessandro Ghedini
958346a5e7 Run select_certificate_cb multiple times
It's not completely clear to me why select_cetificate_cb behaves the way it
does, however not only is it confusing, but it makes assumptions about the
application using BoringSSL (it's not always possible to implement custom
logic outside of the callbacks provided by libssl), that make this callback
somewhat useless.

Case in point, the callback can be used for changing min/max protocol versions
based on per-site policies, and select_certificate_cb is the only place where
SSL_set_min/max_proto_version() can be used (e.g. you can't call them in
cert_cb because it's too late), but the decision on the specific versions to
use might depend on configuration that needs retrieving asynchronously from
over the network, which requires re-running the callback multiple times.

Change-Id: Ia8e151b163628545373e7fd1f327e9af207478a6
Reviewed-on: https://boringssl-review.googlesource.com/13000
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2017-01-20 23:55:50 +00:00
David Benjamin
07820b5cee Add a getter for SSL_set_session_id_context.
We have a test somewhere which tries to read off of it. Align the getter
roughly with upstream's SSL_SESSION_get0_id_context (which we don't
currently expose).

BUG=6

Change-Id: Iab240868838ba56c1f08d112888d9536574347b4
Reviewed-on: https://boringssl-review.googlesource.com/12636
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>
2017-01-20 04:39:42 +00:00
Adam Langley
2e839244b0 Remove old ChaCha20-Poly1305 AEAD.
Before RFC 7539 we had a ChaCha20-Poly1305 cipher suite that had a 64/64
nonce/counter split (as DJB's original ChaCha20 did). RFC 7539 changed
that to 96/32 and we've supported both for some time.

This change removes the old version and the TLS cipher suites that used
it.

BUG=chromium:682816

Change-Id: I2345d6db83441691fe0c1ab6d7c6da4d24777849
Reviewed-on: https://boringssl-review.googlesource.com/13203
Reviewed-by: Adam Langley <agl@google.com>
2017-01-19 23:27:54 +00:00
Adam Langley
5322010405 Revert "Remove old ChaCha20-Poly1305 AEAD."
This reverts commit def9b46801.

(I should have uploaded a new version before sending to the commit queue.)

Change-Id: Iaead89c8d7fc1f56e6294d869db9238b467f520a
Reviewed-on: https://boringssl-review.googlesource.com/13202
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>
2017-01-19 23:07:06 +00:00
David Benjamin
6752efdeaf Never send SNI warning alerts.
TLS 1.3 forbids warning alerts, and sending these is a bad idea. Per RFC
6066:

   If the server understood the ClientHello extension but
   does not recognize the server name, the server SHOULD take one of two
   actions: either abort the handshake by sending a fatal-level
   unrecognized_name(112) alert or continue the handshake.  It is NOT
   RECOMMENDED to send a warning-level unrecognized_name(112) alert,
   because the client's behavior in response to warning-level alerts is
   unpredictable.

The motivation is to cut down on the number of places where we send
non-closing alerts. We can't remove them yet (SSL 3.0 and TLS 1.3 draft
18 need to go), but eventually this can be a simplifying assumption.
Already this means DTLS never sends warning alerts, which is good
because DTLS can't retransmit them.

Change-Id: I577a1eb9c23e66d28235c0fbe913f00965e19486
Reviewed-on: https://boringssl-review.googlesource.com/13221
Reviewed-by: Adam Langley <agl@google.com>
2017-01-19 23:03:11 +00:00
David Benjamin
a8c8b387f1 Don't call the SNI callback as a client.
This doesn't do anything useful. Every caller either never sets the
callback as a client or goes out of their way to filter out clients in
the callback.

Change-Id: I6f07d000a727f9ccba080f812e6b8e7a38e04350
Reviewed-on: https://boringssl-review.googlesource.com/13220
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-19 22:57:46 +00:00
Adam Langley
def9b46801 Remove old ChaCha20-Poly1305 AEAD.
Before RFC 7539 we had a ChaCha20-Poly1305 cipher suite that had a 64/64
nonce/counter split (as DJB's original ChaCha20 did). RFC 7539 changed
that to 96/32 and we've supported both for some time.

This change removes the old version and the TLS cipher suites that used
it.

Change-Id: Icd9c2117c657f3aa6df55990c618d562194ef0e8
Reviewed-on: https://boringssl-review.googlesource.com/13201
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
2017-01-19 22:54:50 +00:00
David Benjamin
1252f8758a Convert one libssl function to C++11.
This is to make sure all of libssl's consumers' have sufficiently
reasonable toolchains. Once this bakes, we can go about moving
libssl to C++.

This is just starting with libssl for now because libcrypto has more
consumers and libssl would benefit more from C++ than libcrypto (though
libcrypto also has code that would benefit).

BUG=132

Change-Id: Ie02f7b0a8a95defd289cc7e62451d4b16408ca2a
Reviewed-on: https://boringssl-review.googlesource.com/13161
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-17 21:51:06 +00:00
Alessandro Ghedini
0726fb76eb Add SSL_CIPHER_is_AEAD.
Change-Id: Ia6598ee4b2d4623abfc140d6a5c0eca4bcb30427
Reviewed-on: https://boringssl-review.googlesource.com/13180
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>
2017-01-17 16:41:49 +00:00
David Benjamin
c253864993 Remove some node.js hacks.
These are no longer needed.

Change-Id: I909f7d690f57dafcdad6254948b5683757da69f4
Reviewed-on: https://boringssl-review.googlesource.com/13160
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-13 21:50:39 +00:00
David Benjamin
745745df03 Add SSL_CIPHER_is_static_RSA.
Change-Id: Id0013a2441da206b051a05a39aa13e4eca937e03
Reviewed-on: https://boringssl-review.googlesource.com/13109
Reviewed-by: Adam Langley <agl@google.com>
2017-01-12 18:37:19 +00:00
David Benjamin
5fc99c6603 There are no more MD5 ciphers.
The last one was an RC4 cipher and those are gone.

Change-Id: I3473937ff6f0634296fc75a346627513c5970ddb
Reviewed-on: https://boringssl-review.googlesource.com/13108
Reviewed-by: Adam Langley <agl@google.com>
2017-01-12 18:36:54 +00:00
David Benjamin
e3fbb36005 Test SSL_set_max_send_fragment.
This gives coverage over needing to fragment something over multiple
records.

Change-Id: I2373613608ef669358d48f4e12f68577fa5a40dc
Reviewed-on: https://boringssl-review.googlesource.com/13101
Reviewed-by: Adam Langley <agl@google.com>
2017-01-12 18:22:08 +00:00
David Benjamin
d261004048 Report TLS 1.3 as supporting secure renegotiation.
TLS 1.3 doesn't support renegotiation in the first place, but so callers
don't report TLS 1.3 servers as missing it, always report it as
(vacuously) protected against this bug.

BUG=chromium:680281

Change-Id: Ibfec03102b2aec7eaa773c331d6844292e7bb685
Reviewed-on: https://boringssl-review.googlesource.com/13046
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>
2017-01-11 22:19:17 +00:00
David Benjamin
9c33ae8562 Fix TLS 1.3 NewSessionTicket processing.
08b65f4e31 introduced a memory leak and
also got enums confused. Also fix a codepath that was missing an error
code.

Thanks to OSS-Fuzz which appears to have found it in a matter of hours.

Change-Id: Ia9e926c28a01daab3e6154d363d0acda91209a22
Reviewed-on: https://boringssl-review.googlesource.com/13104
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-09 03:37:19 +00:00
Steven Valdez
08b65f4e31 Enabling 0-RTT on new Session Tickets.
This adds support for setting 0-RTT mode on tickets minted by
BoringSSL, allowing for testing of the initial handshake knowledge.

BUG=76

Change-Id: Ic199842c03b5401ef122a537fdb7ed9e9a5c635a
Reviewed-on: https://boringssl-review.googlesource.com/12740
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>
2017-01-06 16:24:43 +00:00
David Benjamin
053fee9f79 Enforce the SSL 3.0 no_certificate alert in tests.
As long as we still have this code, we should make sure it doesn't
regress.

Change-Id: I0290792aedcf667ec49b251d747ffbc141c0cec4
Reviewed-on: https://boringssl-review.googlesource.com/13053
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-04 13:41:56 +00:00
David Benjamin
48063c2aea Add tests around SSL_write's retry behavior.
SSL_write is remarkably complicated.

Change-Id: I1cb8d00af1b4c5e2d90187d5f87951f25e27f224
Reviewed-on: https://boringssl-review.googlesource.com/13050
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>
2017-01-04 04:54:04 +00:00
David Benjamin
a660e7ab67 Don't clear cert_request in ssl3_send_client_certificate.
Instead, add ssl_has_certificate to the ssl3_send_cert_verify check. If
writing the empty Certificate does not complete synchronously (it almost
always does due to the buffer BIO), but if the buffer boundary is at
exactly the wrong place, write_message will need a retry but, having
cleared cert_request, we never re-enter ssl3_send_client_certificate.

This will later be moot when we've gotten rid of the buffer BIO, but
this is cleaner anyway and is closer to the TLS 1.3 code.

With this change, blindly taking away the BIO buffer in TLS (which is
not what we want since we want the entire flight in one write but is a
nice sanity check), only the SSL 3.0 no client certificate tests fail.
They too rely on some writes completing synchronously due to SSL 3.0
sending a warning alert. There is a similar bug when
tlsext_servername_callback returns SSL_TLSEXT_ERR_ALERT_WARNING.

Those will be resolved after reworking the write path since it's a bit
of a mess.

Change-Id: I56b4df6163cae1df263cf36f0d93046d0375a5ac
Reviewed-on: https://boringssl-review.googlesource.com/13052
Reviewed-by: David Benjamin <davidben@google.com>
2017-01-04 04:50:00 +00:00
David Benjamin
2be4aa7164 Add a helper function for resetting SSL_get_error state.
We repeat this in a bunch of places.

Change-Id: Iee2c95a13e1645453f101d8be4be9ac78d520387
Reviewed-on: https://boringssl-review.googlesource.com/13051
Reviewed-by: David Benjamin <davidben@google.com>
2017-01-04 04:48:44 +00:00
David Benjamin
a1eaba1dc6 Add a test for renegotiation on busy write buffer.
The write path for TLS is going to need some work. There are some fiddly
cases when there is a write in progress. Start adding tests to cover
this logic.

Later I'm hoping we can extend this flag so it drains the unfinished
write and thus test the interaction of read/write paths in 0-RTT. (We
may discover 1-RTT keys while we're in the middle of writing data.)

Change-Id: Iac2c417e4b5e84794fb699dd7cbba26a883b64ef
Reviewed-on: https://boringssl-review.googlesource.com/13049
Reviewed-by: Adam Langley <agl@google.com>
2017-01-04 01:54:57 +00:00
David Benjamin
2214f4e422 Remove call to SSL_CTX_set_tls_channel_id_enabled in bssl_shim.
Channel ID is already enabled on the SSL. This dates to
49c7af1c42 which converted an instance of
tlsext_channel_id_enabled_new to it, but tlsext_channel_id_enabled_new
meant "if Channel ID is enabled, use the new one", not "enable Channel
ID".

Thanks to Eric Rescorla for catching this.

Change-Id: I2d5a82b930ffcbe5527a62a9aa5605ebb71a6b9f
Reviewed-on: https://boringssl-review.googlesource.com/13042
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2017-01-04 01:46:10 +00:00
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
f650c71ac0 Use SSL_CTX_up_ref to up-ref an SSL_CTX.
We have this function now. Probably good to use it.

Change-Id: I00fe1f4cf5c8cb6f61a8f6600cac4667e95ad7f3
Reviewed-on: https://boringssl-review.googlesource.com/13040
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>
2017-01-03 13:15:30 +00:00
David Benjamin
a81967b47c Add tests for the point format extension.
Upstream accidentally started rejecting server-sent point formats in
https://github.com/openssl/openssl/issues/2133. Our own test coverage
here is also lacking, so flesh it out.

Change-Id: I99059558bd28d3a540c9687649d6db7e16579d29
Reviewed-on: https://boringssl-review.googlesource.com/12979
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-22 15:33:40 +00:00
David Benjamin
9c70b89d0b Update fuzzer mode suppressions.
Change-Id: Ie4c566c29c20faac7a9a5e04c88503fc2e1ff4db
Reviewed-on: https://boringssl-review.googlesource.com/12970
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-12-22 03:18:19 +00:00
David Benjamin
1444c3ace0 Add tests for auto-chaining.
Alas, wpa_supplicant relies on the auto-chaining feature, so we can't
easily remove it. Write tests for it to ensure it stays working.

These test currently do not work for TLS 1.3 because the feature is
broken in 1.3. A follow-up change will fix this.

BUG=70

Change-Id: I2c04f55b712d66f5af1556254a5b017ebc3244f7
Reviewed-on: https://boringssl-review.googlesource.com/12965
Reviewed-by: Adam Langley <agl@google.com>
2016-12-21 23:10:07 +00:00
David Benjamin
6f600d6bae Add experimental TLS 1.3 short record header extension.
This extension will be used to test whether
https://github.com/tlswg/tls13-spec/pull/762 is deployable against
middleboxes. For simplicity, it is mutually exclusive with 0-RTT. If
client and server agree on the extension, TLS 1.3 records will use the
format in the PR rather than what is in draft 18.

BUG=119

Change-Id: I1372ddf7b328ddf73d496df54ac03a95ede961e1
Reviewed-on: https://boringssl-review.googlesource.com/12684
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-21 22:06:44 +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
David Benjamin
56cadc3daf Assert on the alert sent on FALLBACK_SCSV.
We were only asserting on the shim-side error code.

Change-Id: Idc08c253a7723a2a7fd489da761a56c72f7a3b96
Reviewed-on: https://boringssl-review.googlesource.com/12923
Reviewed-by: Adam Langley <agl@google.com>
2016-12-21 20:12:19 +00:00
David Benjamin
b442dee388 Rename FallbackSCSV-MatchVersion.
It should probably have a TLS 1.3 in the name to be clear that's what
it's testing.

Change-Id: I50b5f503a8038715114136179bde83e7da064e9b
Reviewed-on: https://boringssl-review.googlesource.com/12961
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-20 20:47:30 +00:00
David Benjamin
458334a159 Test name-based version APIs are reported as expected.
Notably, Conscrypt uses SSL_SESSION_get_version, so we should have tests
for it.

Change-Id: I670f1b1b9951f840f27cb62dd36ef4f05042c974
Reviewed-on: https://boringssl-review.googlesource.com/12881
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2016-12-19 21:50:06 +00:00
Steven Valdez
4cb8494d25 Splitting handshake traffic derivation from key change.
This is in preparation for implementing 0-RTT where, like
with client_traffic_secret_0, client_handshake_secret must
be derived slightly earlier than it is used. (The secret is
derived at ServerHello, but used at server Finished.)

Change-Id: I6a186b84829800704a62fda412992ac730422110
Reviewed-on: https://boringssl-review.googlesource.com/12920
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-16 20:29:23 +00:00
David Benjamin
65fb425811 Remove version-specific cipher lists.
There are no longer any consumers of these APIs.

These were useful back when the CBC vs. RC4 tradeoff varied by version
and it was worth carefully tuning this cutoff. Nowadays RC4 is
completely gone and there's no use in configuring these anymore.

To avoid invalidating the existing ssl_ctx_api corpus and requiring it
regenerated, I've left the entries in there. It's probably reasonable
for new API fuzzers to reuse those slots.

Change-Id: I02bf950e3828062341e4e45c8871a44597ae93d5
Reviewed-on: https://boringssl-review.googlesource.com/12880
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-12-16 19:11:02 +00:00
David Benjamin
76bb1411ac Remove the BORINGSSL_ANDROID_SYSTEM P-521 special-case.
This dates to
62d05888d1
which intended to be removed in a later Android release once X25519 was
added. That has since happened.

This intentionally leaves the P-521 hooked up for now. Detaching it
completely is a more aggressive change (since it's slightly tied up with
SHA-512) that should wait until removing ECDSA+SHA512 has stuck in Chrome.

Change-Id: I04553c3eddf33a13b6e3e9a6e7ac4c4725676cb0
Reviewed-on: https://boringssl-review.googlesource.com/10923
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-15 15:47:39 +00:00
Adam Langley
629db8cd0c Add |SSL_get_peer_full_cert_chain|.
This function always returns the full chain and will hopefully eliminate
the need for some code in Conscrypt.

Change-Id: Ib662005322c40824edf09d100a784ff00492896a
Reviewed-on: https://boringssl-review.googlesource.com/12780
Reviewed-by: Adam Langley <agl@google.com>
2016-12-14 18:01:10 +00:00
Adam Langley
a4b91981f8 Make TLS 1.3 check ECDSA KeyUsage and add test.
Change-Id: Ibb5c5f6b945f72585f58c457158a386dfb4dae98
Reviewed-on: https://boringssl-review.googlesource.com/12710
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-14 17:56:55 +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
Adam Langley
0c294254b5 Don't use |X509_get_pubkey| in TLS 1.3 code either.
Change-Id: I7050c74ac38503f450760a857442e6fc0863d5df
Reviewed-on: https://boringssl-review.googlesource.com/12708
Reviewed-by: Adam Langley <agl@google.com>
2016-12-14 17:49:10 +00:00
David Benjamin
aa01204175 Move per-cipher-suite tests into a separate function.
The loop is getting a little deeply nested and hard to read.

Change-Id: I3a99fba54c2f352850b83aef91ab72d5d9aabfb8
Reviewed-on: https://boringssl-review.googlesource.com/12685
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:51:37 +00:00
David Benjamin
db5bd72152 Move key_share extension check with ECDHE code.
Also fix the error code. It's a missing extension, not an unexpected
one.

Change-Id: I48e48c37e27173f6d7ac5e993779948ead3706f2
Reviewed-on: https://boringssl-review.googlesource.com/12683
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:46:34 +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
8a55ce4954 Test SSL_get_curve_id behavior on resume.
Also test that TLS 1.3 can be resumed at a different curve.

Change-Id: Ic58e03ad858c861958b7c934813c3e448fb2829c
Reviewed-on: https://boringssl-review.googlesource.com/12692
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:08:02 +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
David Benjamin
54881224e8 Remove SSL_get_dhe_group_size.
Nothing calls this anymore. DHE is nearly gone. This unblocks us from
making key_exchange_info only apply to the curve.

Change-Id: I3099e7222a62441df6e01411767d48166a0729b1
Reviewed-on: https://boringssl-review.googlesource.com/12691
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:06:22 +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
2644a13d71 Set up the SSL_HANDSHAKE object earlier.
This is to free up moving ssl->state into SSL_HANDSHAKE. ssl->state
serves two purposes right now. First, it is the state tracking for
SSL_HANDSHAKE. Second, it lets the system know there is a handshake
waiting to complete.

Instead, arrange things so that, if there is a handshake waiting to
complete, hs is not NULL. That means we need to initialize it when
creating a new connection and when discovering a renego.

Note this means we cannot make initializing an SSL_HANDSHAKE depend on
client vs. server.

Change-Id: I585a8d7e700c4ffe4d372248d34c44106ad7e7a0
Reviewed-on: https://boringssl-review.googlesource.com/12696
Reviewed-by: David Benjamin <davidben@google.com>
2016-12-12 21:59:06 +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
David Benjamin
5888946777 Remove SSL_CTX_get_client_cert_cb.
This is never used. Removing it allows us to implement the old callback
using the new one.

Change-Id: I4be70cc16e609ce79b51836c19fec565c67ff3d6
Reviewed-on: https://boringssl-review.googlesource.com/12689
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-12-12 21:43:32 +00:00
David Benjamin
287fc4ff7b Don't use SSL_want_* macros internally.
Reduce the amount of boilerplate needed to add more of these. Also tidy
things up a little.

Change-Id: I90ea7f70dba5a2b38a1fb716faff97eb4f6afafc
Reviewed-on: https://boringssl-review.googlesource.com/12687
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-12-12 21:42:44 +00:00
David Benjamin
3977f30317 Rename hs->state to hs->tls13_state.
This is to free up the hs->state name for the upper-level handshake
state.

Change-Id: I1183a329f698c56911f3879a91809edad5b5e94e
Reviewed-on: https://boringssl-review.googlesource.com/12695
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 21:15:28 +00:00
Matthew Braithwaite
f440e827f1 Remove New Hope key agreement.
Change-Id: Iaac633616a54ba1ed04c14e4778865c169a68621
Reviewed-on: https://boringssl-review.googlesource.com/12703
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-10 01:06:31 +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
5a6e616961 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.

Change-Id: I0ea4589d7a8b5c41df225ad7f282b6d1376a8db4
Reviewed-on: https://boringssl-review.googlesource.com/12164
Reviewed-by: Adam Langley <alangley@gmail.com>
2016-12-09 18:22:06 +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
Alessandro Ghedini
559f0644a5 Support setting per-connection OCSP staple
Right now the only way to set an OCSP response is SSL_CTX_set_ocsp_response
however this assumes that all the SSLs generated from a SSL_CTX share the
same OCSP response, which is wrong.

This is similar to the OpenSSL "function" SSL_get_tlsext_status_ocsp_resp,
the main difference being that this doesn't take ownership of the OCSP buffer.

In order to avoid memory duplication in case SSL_CTX has its own response,
a CRYPTO_BUFFER is used for both SSL_CTX and SSL.

Change-Id: I3a0697f82b805ac42a22be9b6bb596aa0b530025
Reviewed-on: https://boringssl-review.googlesource.com/12660
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-08 20:29:43 +00:00
David Benjamin
7c5728649a Remove SSL_set_reject_peer_renegotiations.
All callers were long since updated.

Change-Id: Ibdc9b186076dfbcbc3bd7dcc72610c8d5a522cfc
Reviewed-on: https://boringssl-review.googlesource.com/12624
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-08 17:23:10 +00:00
David Benjamin
b79cc84635 Fix SSL_clear's interaction with session resumption.
Prior to 87eab4902d, due to some
confusions between configuration and connection state, SSL_clear had the
side effect of offering the previously established session on the new
connection.

wpa_supplicant relies on this behavior, so restore it for TLS 1.2 and
below and add a test. (This behavior is largely incompatible with TLS
1.3's post-handshake tickets, so it won't work in 1.3. It'll act as if
we configured an unresumable session instead.)

Change-Id: Iaee8c0afc1cb65c0ab7397435602732b901b1c2d
Reviewed-on: https://boringssl-review.googlesource.com/12632
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-08 16:57:57 +00:00
David Benjamin
30c4c30d4a Revise some integer sizes.
size_t at the public API, uint8_t on the SSL structs since everything
fits in there comfortably.

Change-Id: I837c3b21e04e03dfb957c1a3e6770300d0b49c0b
Reviewed-on: https://boringssl-review.googlesource.com/12638
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-08 16:48:44 +00:00
David Benjamin
813fc01ff1 Remove unreachable check.
It is impossible to have an SSL* without a corresponding method.

Change-Id: Icaf826a06aaaa2c7caf98b1e4a950f9c1d48e6bd
Reviewed-on: https://boringssl-review.googlesource.com/12621
Reviewed-by: Adam Langley <agl@google.com>
2016-12-08 16:40:15 +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
a2bda9fb95 Make more functions static.
These too have no reason to be called across files.

Change-Id: Iee477e71f956c2fa0d8817bf2777cb3a81e1c853
Reviewed-on: https://boringssl-review.googlesource.com/12585
Reviewed-by: Adam Langley <agl@google.com>
2016-12-08 16:29:58 +00:00
David Benjamin
0be6fc4c98 Move a few more functions into *_method.c.
s3_lib.c is nearly gone. ssl_get_cipher_preferences will fall away once
we remove the version-specific cipher lists. ssl_get_algorithm_prf and
the PRF stuff in general needs some revising (it was the motivation for
all the SSL_HANDSHAKE business). I've left ssl3_new / ssl3_free alone
for now because we don't have a good separation between common TLS/DTLS
connection state and state internal to the TLS SSL_PROTOCOL_METHOD.
Leaving that alone for now as there's lower-hanging fruit.

Change-Id: Idf7989123a387938aa89b6a052161c9fff4cbfb3
Reviewed-on: https://boringssl-review.googlesource.com/12584
Reviewed-by: Adam Langley <agl@google.com>
2016-12-08 16:29:19 +00:00