Between TLS 1.2, TLS 1.3, and the early callback, we've got a lot of
ClientHello parsers. Unify everything on the early callback's parser. As
a side effect, this means we can parse a ClientHello fairly succinctly
from any function which will let us split up ClientHello states where
appropriate.
Change-Id: I2359b75f80926cc7d827570cf33f93029b39e525
Reviewed-on: https://boringssl-review.googlesource.com/10184
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>
tls13_process_certificate can take a boolean for whether anonymous is
allowed. This does change the error on the client slightly, but I think
this is correct anyway. It is not a syntax error for the server to send
no certificates in so far as the Certificate message allows it. It's
just illegal.
Change-Id: I1af80dacf23f50aad0b1fbd884bc068a40714399
Reviewed-on: https://boringssl-review.googlesource.com/9072
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>
Extend the DTLS mock clock to apply to sessions too and test that
resumption behaves as expected.
Change-Id: Ib8fdec91b36e11cfa032872b63cf589f93b3da13
Reviewed-on: https://boringssl-review.googlesource.com/9110
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>
We broke this to varying degrees ages ago.
This is the logic to implement the variations of rules in TLS to discard
sessions after a failed connection, where a failed connection could be
one of:
- A connection that was not cleanly shut down.
- A connection that received a fatal alert.
The first one is nonsense since close_notify does not actually work in
the real world. The second is a vaguely more plausible but...
- A stateless ticket-based server can't drop sessions anyway.
- In TLS 1.3, a client may receive many tickets over the lifetime of a
single connection. With an external session cache like ours which may,
in theory, but multithreaded, this will be a huge hassle to track.
- A client may well attempt to establish a connection and reuse the
session before we receive the fatal alert, so any application state we
hope to manage won't really work.
- An attacker can always close the connection before the fatal alert, so
whatever security policy clearing the session gave is easily
bypassable.
Implementation-wise, this has basically never worked. The
ssl_clear_bad_session logic called into SSL_CTX_remove_session which
relied on the internal session cache. (Sessions not in the internal
session cache don't get removed.) The internal session cache was only
useful for a server, where tickets prevent this mechanism from doing
anything. For a client, we since removed the internal session cache, so
nothing got removed. The API for a client also did not work as it gave
the SSL_SESSION, not the SSL, so a consumer would not know the key to
invalidate anyway.
The recent session state splitting change further broke this.
Moreover, calling into SSL_CTX_remove_session logic like that is
extremely dubious because it mutates the not_resumable flag on the
SSL_SESSION which isn't thread-safe.
Spec-wise, TLS 1.3 has downgraded the MUST to a SHOULD.
Given all that mess, just remove this code. It is no longer necessary to
call SSL_shutdown just to make session caching work.
Change-Id: Ib601937bfc5f6b40436941e1c86566906bb3165d
Reviewed-on: https://boringssl-review.googlesource.com/9091
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Save a little bit of typing at the call site.
Change-Id: I818535409b57a694e5e0ea0e9741d89f2be89375
Reviewed-on: https://boringssl-review.googlesource.com/9090
Reviewed-by: Adam Langley <agl@google.com>
We will now send tickets as a server and accept them as a
client. Correctly offering and resuming them in the handshake will be
implemented in a follow-up.
Now that we're actually processing draft 14 tickets, bump the draft
version.
Change-Id: I304320a29c4ffe564fa9c00642a4ace96ff8d871
Reviewed-on: https://boringssl-review.googlesource.com/8982
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: Id6a7443246479c62cbe0024e2131a2013959e21e
Reviewed-on: https://boringssl-review.googlesource.com/9078
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>
OpenSSL 1.1.0 added a function to tell if an SSL* is DTLS or not. This
is probably a good idea, especially since SSL_version returns
non-normalized versions.
BUG=91
Change-Id: I25c6cf08b2ebabf0c610c74691de103399f729bc
Reviewed-on: https://boringssl-review.googlesource.com/9077
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>
BUG=74
Change-Id: I72d52c1fbc3413e940dddbc0b20c7f22459da693
Reviewed-on: https://boringssl-review.googlesource.com/8981
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: I5cc194fc0a3ba8283049078e5671c924ee23036c
Reviewed-on: https://boringssl-review.googlesource.com/8980
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>
To prevent configuration/established session confusion, the handshake
session state is separated into the configured session (ssl->session)
and the newly created session (ssl->s3->new_session). Upon conclusion of
the handshake, the finalized session is stored
in (ssl->s3->established_session). During the handshake, any requests
for the session (SSL_get_session) return a non-resumable session, to
prevent resumption of a partially filled session. Sessions should only
be cached upon the completion of the full handshake, using the resulting
established_session. The semantics of accessors on the session are
maintained mid-renego.
Change-Id: I4358aecb71fce4fe14a6746c5af1416a69935078
Reviewed-on: https://boringssl-review.googlesource.com/8612
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This finishes getting rid of ssl_read_bytes! Now we have separate
entry-points for the various cases. For now, I've kept TLS handshake
consuming records partially. When we do the BIO-less API, I expect that
will need to change, since we won't have the record buffer available.
(Instead, the ssl3_read_handshake_bytes and extend_handshake_buffer pair
will look more like the DTLS side or Go and pull the entire record into
init_buf.)
This change opts to make read_app_data drive the message to completion
in anticipation of DTLS 1.3. That hasn't been specified, but
NewSessionTicket certainly will exist. Knowing that DTLS necessarily has
interleave seems something better suited for the SSL_PROTOCOL_METHOD
internals to drive.
It needs refining, but SSL_PROTOCOL_METHOD is now actually a half-decent
abstraction boundary between the higher-level protocol logic and
DTLS/TLS-specific record-layer and message dispatchy bits.
BUG=83
Change-Id: I9b4626bb8a29d9cb30174d9e6912bb420ed45aff
Reviewed-on: https://boringssl-review.googlesource.com/9001
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
With the previous DTLS change, the dispatch layer only cares about the
end of the handshake to know when to drop the current message. TLS 1.3
post-handshake messages will need a similar hook, so convert it to this
lower-level one.
BUG=83
Change-Id: I4c8c3ba55ba793afa065bf261a7bccac8816c348
Reviewed-on: https://boringssl-review.googlesource.com/8989
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
For TLS 1.3, we will need to process more complex post-handshake
messages. It is simplest if we use the same mechanism. In preparation,
allow ssl3_get_message to be called at any point.
Note that this stops reserving SSL3_RT_MAX_PLAIN_LENGTH in init_buf
right off the bat. Instead it will grow as-needed to accomodate the
handshake. SSL3_RT_MAX_PLAIN_LENGTH is rather larger than we probably
need to receive, particularly as a server, so this seems a good plan.
BUG=83
Change-Id: Id7f4024afc4c8a713b46b0d1625432315594350e
Reviewed-on: https://boringssl-review.googlesource.com/8985
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Alas, we will need a version fallback for TLS 1.3 again.
This deprecates SSL_MODE_SEND_FALLBACK_SCSV. Rather than supplying a
boolean, have BoringSSL be aware of the real maximum version so we can
change the TLS 1.3 anti-downgrade logic to kick in, even when
max_version is set to 1.2.
The fallback version replaces the maximum version when it is set for
almost all purposes, except for downgrade protection purposes.
BUG=chromium:630165
Change-Id: I4c841dcbc6e55a282b223dfe169ac89c83c8a01f
Reviewed-on: https://boringssl-review.googlesource.com/8882
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It really should take a few more parameters and save a bit of
long-winded initialization work.
Change-Id: I2823f0aa82be39914a156323f6f32b470b6d6a3b
Reviewed-on: https://boringssl-review.googlesource.com/8876
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>
[Tests added by davidben.]
Change-Id: I0d54a4f8b8fe91b348ff22658d95340cdb48b089
Reviewed-on: https://boringssl-review.googlesource.com/8850
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>
Share a bit more of it between TLS 1.2 and 1.3.
Change-Id: I43c9dbf785a3d33db1793cffb0fdbd3af075cc89
Reviewed-on: https://boringssl-review.googlesource.com/8849
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>
This adds three more formats to the SSLKEYLOGFILE format to support TLS
1.3:
EARLY_TRAFFIC_SECRET <client_random> <early_traffic_secret>
HANDSHAKE_TRAFFIC_SECRET <client_random> <handshake_traffic_secret>
TRAFFIC_SECRET_0 <client_random> <traffic_secret_0>
(We don't implement 0-RTT yet, so only the second two are implemented.)
Motivations:
1. If emitted the non-traffic secrets (early, handshake, and master) or
the IKMs, Wireshark needs to maintain a handshake hash. I don't
believe they need to do this today.
2. We don't store more than one non-traffic secret at a time and don't
keep traffic secrets for longer than needed. That suggests three
separate lines logged at different times rather than one line.
3. If 0-RTT isn't used, we probably won't even compute the early traffic
secret, so that further suggests three different lines.
4. If the handshake didn't get far enough to complete, we won't have an
TRAFFIC_SECRET_0 to log at all. That seems like exactly when
Wireshark would be handy, which means we want to log secrets as they
are computed.
MT from NSS has ACK'd over email that this format would be acceptable
for them, so let's go with it.
Change-Id: I4d685a1355dff4d4bd200310029d502bb6c511f9
Reviewed-on: https://boringssl-review.googlesource.com/8841
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Every flush but the last is always immediately followed by a read. Add a
combined wait mode to make things simpler. Unfortunately, both flights
we have (the state machine doesn't write the first ClientHello) are
followed immediately by a state change, which means we still need some
state in between because we must run code after write_message but before
read_message.
(This way to fix that is to get rid of the buffer BIO, change
write_message to write_flight, and allow things like init_message /
finish_message / init_message / finish_message / set_write_state /
init_message / finish_message / write_flight.)
Change-Id: Iebaa388ccbe7fcad48c1b2256e1c0d3a7c9c8a2a
Reviewed-on: https://boringssl-review.googlesource.com/8828
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>
This adds the machinery for doing TLS 1.3 1RTT.
Change-Id: I736921ffe9dc6f6e64a08a836df6bb166d20f504
Reviewed-on: https://boringssl-review.googlesource.com/8720
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is the equivalent of FragmentAcrossChangeCipherSuite for DTLS. It
is possible for us to, while receiving pre-CCS handshake messages, to
buffer up a message with sequence number meant for a post-CCS Finished.
When we then get to the new epoch and attempt to read the Finished, we
will process the buffered Finished although it was sent with the wrong
encryption.
Move ssl_set_{read,write}_state to SSL_PROTOCOL_METHOD hooks as this is
a property of the transport. Notably, read_state may fail. In DTLS
check the handshake buffer size. We could place this check in
read_change_cipher_spec, but TLS 1.3 has no ChangeCipherSpec message, so
we will need to implement this at the cipher change point anyway. (For
now, there is only an assert on the TLS side. This will be replaced with
a proper check in TLS 1.3.)
Change-Id: Ia52b0b81e7db53e9ed2d4f6d334a1cce13e93297
Reviewed-on: https://boringssl-review.googlesource.com/8790
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>
This allows us to implement custom RSA-PSS-based keys, so the async TLS
1.3 tests can proceed. For now, both sign and sign_digest exist, so
downstreams only need to manage a small change atomically. We'll remove
sign_digest separately.
In doing so, fold all the *_complete hooks into a single complete hook
as no one who implemented two operations ever used different function
pointers for them.
While I'm here, I've bumped BORINGSSL_API_VERSION. I do not believe we
have any SSL_PRIVATE_KEY_METHOD versions who cannot update atomically,
but save a round-trip in case we do. It's free.
Change-Id: I7f031aabfb3343805deee429b9e244aed5d76aed
Reviewed-on: https://boringssl-review.googlesource.com/8786
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>
This makes custom private keys and EVP_PKEYs symmetric again. There is
no longer a requirement that the caller pre-filter the configured
signing prefs.
Also switch EVP_PKEY_RSA to NID_rsaEncryption. These are identical, but
if some key types are to be NIDs, we should make them all NIDs.
Change-Id: I82ea41c27a3c57f4c4401ffe1ccad406783e4c64
Reviewed-on: https://boringssl-review.googlesource.com/8785
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I9ec1a8c87e29ffd4fabef68beb6d094aa7d9a215
Reviewed-on: https://boringssl-review.googlesource.com/8795
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>
This is already duplicated between client and server and otherwise will
get duplicated yet again for TLS 1.3.
Change-Id: Ia8a352f9bc76fab0f88c1629d08a1da4c13d2510
Reviewed-on: https://boringssl-review.googlesource.com/8778
Reviewed-by: David Benjamin <davidben@google.com>
This will get shared between TLS 1.2 and 1.3.
Change-Id: I9c0d73a087942ac4f8f2075a44bd55647c0dd70b
Reviewed-on: https://boringssl-review.googlesource.com/8777
Reviewed-by: David Benjamin <davidben@google.com>
These will all want to be shared with the TLS 1.3 handshake.
Change-Id: I4e50dc0ed2295d43c7ae800015d71c1406311801
Reviewed-on: https://boringssl-review.googlesource.com/8776
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>
For now, skip the 1.2 -> 1.1 signal since that will affect shipping
code. We may as well enable it too, but wait until things have settled
down. This implements the version in draft-14 since draft-13's isn't
backwards-compatible.
Change-Id: I46be43e6f4c5203eb4ae006d1c6a2fe7d7a949ec
Reviewed-on: https://boringssl-review.googlesource.com/8724
Reviewed-by: David Benjamin <davidben@google.com>
Implement in both C and Go. To test this, route config into all the
sign.go functions so we can expose bugs to skip the check.
Unfortunately, custom private keys are going to be a little weird since
we can't check their curve type. We may need to muse on what to do here.
Perhaps the key type bit should return an enum that includes the curve?
It's weird because, going forward, hopefully all new key types have
exactly one kind of signature so key type == sig alg == sig alg prefs.
Change-Id: I1f487ec143512ead931e3392e8be2a3172abe3d2
Reviewed-on: https://boringssl-review.googlesource.com/8701
Reviewed-by: David Benjamin <davidben@google.com>
ssl_verify_* already ought to be checking this, so there's only a need
to check against the configured preferences.
Change-Id: I79bc771969c57f953278e622084641e6e20108e3
Reviewed-on: https://boringssl-review.googlesource.com/8698
Reviewed-by: David Benjamin <davidben@google.com>
Rather than blindly select SHA-1 if we can't find a matching one, act as
if the peer advertised rsa_pkcs1_sha1 and ecdsa_sha1. This means that we
will fail the handshake if no common algorithm may be found.
This is done in preparation for removing the SHA-1 default in TLS 1.3.
Change-Id: I3584947909d3d6988b940f9404044cace265b20d
Reviewed-on: https://boringssl-review.googlesource.com/8695
Reviewed-by: David Benjamin <davidben@google.com>
Instead, in SSL_set_private_key_digest_prefs, convert the NID list to a
sigalgs list. We'll need to add a new API later when custom key callers
are ready to start advertising RSA-PSS.
This removes all callers of tls12_get_hash except inside the signing and
verifying functions.
Change-Id: Ie534f3b736c6ac6ebeb0d7770d489f72e3321865
Reviewed-on: https://boringssl-review.googlesource.com/8693
Reviewed-by: David Benjamin <davidben@google.com>
Instead have ssl3_cert_verify_hash output the hash, since it already
knows it. Also add a missing EVP_PKEY_CTX_set_signature_md call on the
client half. (Although, the call isn't actually necessary.)
Also remove now unnecessary static assert. Since EVP_md5_sha1 is an
EVP_MD itself, EVP_MAX_MD_SIZE is required to fit it already.
Change-Id: Ief74fdbdf08e9f124679475bafba2f6f1d8fc687
Reviewed-on: https://boringssl-review.googlesource.com/8692
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
It still places the current message all over the place, but remove the
bizarre init_num/error/ok split. Now callers get the message length out
of init_num, which mirrors init_msg. Also fix some signedness.
Change-Id: Ic2e97b6b99e234926504ff217b8aedae85ba6596
Reviewed-on: https://boringssl-review.googlesource.com/8690
Reviewed-by: David Benjamin <davidben@google.com>
This machinery is so different between TLS and DTLS that there is no
sense in having them share structures. This switches us to maintaining
the full reassembled message in hm_fragment and get_message just lets
the caller read out of that when ready.
This removes the last direct handshake dependency on init_buf,
ssl3_hash_message.
Change-Id: I4eccfb6e6021116255daead5359a0aa3f4d5be7b
Reviewed-on: https://boringssl-review.googlesource.com/8667
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Both DTLS and TLS still use it, but that will change in the following
commit. This also removes the handshake's knowledge of the
dtls_clear_incoming_messages function.
(It's possible we'll want to get rid of begin_handshake in favor of
allocating it lazily depending on how TLS 1.3 post-handshake messages
end up working out. But this should work for now.)
Change-Id: I0f512788bbc330ab2c947890939c73e0a1aca18b
Reviewed-on: https://boringssl-review.googlesource.com/8666
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In order to delay the digest of the handshake transcript and unify
around message-based signing callbacks, a copy of the transcript is kept
around until we are sure there is no certificate authentication.
This removes support for SSL_PRIVATE_KEY_METHOD as a client in SSL 3.0.
Change-Id: If8999a19ca021b4ff439319ab91e2cd2103caa64
Reviewed-on: https://boringssl-review.googlesource.com/8561
Reviewed-by: David Benjamin <davidben@google.com>
This replaces the old key_exchange_info APIs and does not require the
caller be aware of the mess around SSL_SESSION management. They
currently have the same bugs around renegotiation as before, but later
work to fix up SSL_SESSION tracking will fix their internals.
For consistency with the existing functions, I've kept the public API at
'curve' rather than 'group' for now. I think it's probably better to
have only one name with a single explanation in the section header
rather than half and half. (I also wouldn't be surprised if the IETF
ends up renaming 'group' again to 'key exchange' at some point. We'll
see what happens.)
Change-Id: I8e90a503bc4045d12f30835c86de64ef9f2d07c8
Reviewed-on: https://boringssl-review.googlesource.com/8565
Reviewed-by: Adam Langley <agl@google.com>
We ended up switching this from a curve to a cipher suite, so the group
ID isn't used. This is in preparation for adding an API for the curve
ID, at which point leaving the protocol constants undefined seems
somewhat bad manners.
Change-Id: Icb8bf4594879dbbc24177551868ecfe89bc2f8c3
Reviewed-on: https://boringssl-review.googlesource.com/8563
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's SSL_OP_NO_* flags allow discontinuous version ranges. This is a
nuisance for two reasons. First it makes it unnecessarily difficult to answer
"are any versions below TLS 1.3 enabled?". Second the protocol does not allow
discontinuous version ranges on the client anyway. OpenSSL instead picks the
first continous range of enabled versions on the client, but not the server.
This is bizarrely inconsistent. It also doesn't quite do this as the
ClientHello sending logic does this, but not the ServerHello processing logic.
So we actually break some invariants slightly. The logic is also cumbersome in
DTLS which kindly inverts the comparison logic.
First, switch min_version/max_version's storage to normalized versions. Next
replace all the ad-hoc version-related functions with a single
ssl_get_version_range function. Client and server now consistently pick a
contiguous range of versions. Note this is a slight behavior change for
servers. Version-range-sensitive logic is rewritten to use this new function.
BUG=66
Change-Id: Iad0d64f2b7a917603fc7da54c9fc6656c5fbdb24
Reviewed-on: https://boringssl-review.googlesource.com/8513
Reviewed-by: David Benjamin <davidben@google.com>
The signing logic itself still depends on pre-hashed messages and will be fixed
in later commits.
Change-Id: I901b0d99917c311653d44efa34a044bbb9f11e57
Reviewed-on: https://boringssl-review.googlesource.com/8545
Reviewed-by: David Benjamin <davidben@google.com>
They're not necessary.
Change-Id: Ifeb3fae73a8b22f88019e6ef9f9ba5e64ed3cfab
Reviewed-on: https://boringssl-review.googlesource.com/8543
Reviewed-by: David Benjamin <davidben@google.com>
As part of the SignatureAlgorithm change in the TLS 1.3 specification,
the existing signature/hash combinations are replaced with a combined
signature algorithm identifier. This change maintains the existing APIs
while fixing the internal representations. The signing code currently
still treats the SignatureAlgorithm as a decomposed value, which will be
fixed as part of a separate CL.
Change-Id: I0cd1660d74ad9bcf55ce5da4449bf2922660be36
Reviewed-on: https://boringssl-review.googlesource.com/8480
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
buffer buffer buffer buffer buffer. At some point, words lose their meaning if
they're used too many times. Notably, the DTLS code can't decide whether a
"buffered message" is an incoming message to be reassembled or an outgoing
message to be (re)transmitted.
Change-Id: Ibdde5c00abb062c603d21be97aff49e1c422c755
Reviewed-on: https://boringssl-review.googlesource.com/8500
Reviewed-by: Adam Langley <agl@google.com>
This allows us to use CBB for all handshake messages. Now, SSL_PROTOCOL_METHOD
is responsible for implementing a trio of CBB-related hooks to assemble
handshake messages.
Change-Id: I144d3cac4f05b6637bf45d3f838673fc5c854405
Reviewed-on: https://boringssl-review.googlesource.com/8440
Reviewed-by: Adam Langley <agl@google.com>
It has size 7. There's no need for a priority queue structure, especially one
that's O(N^2) anyway.
Change-Id: I7609794aac1925c9bbf3015744cae266dcb79bff
Reviewed-on: https://boringssl-review.googlesource.com/8437
Reviewed-by: Adam Langley <agl@google.com>
The pair was a remnant of some weird statefulness and also ChangeCipherSpec
having a "sequence number" to make the pqueue turn into an array.
Change-Id: Iffd82594314df43934073bd141faee0fc167ed5f
Reviewed-on: https://boringssl-review.googlesource.com/8436
Reviewed-by: Adam Langley <agl@google.com>
Now that retransitting is a lot less stateful, a lot of surrounding code can
lose statefulness too. Rather than this overcomplicated pqueue structure,
hardcode that a handshake flight is capped at 7 messages (actually, DTLS can
only get up to 6 because we don't support NPN or Channel ID in DTLS) and used a
fixed size array.
This also resolves several TODOs.
Change-Id: I2b54c3441577a75ad5ca411d872b807d69aa08eb
Reviewed-on: https://boringssl-review.googlesource.com/8435
Reviewed-by: Adam Langley <agl@google.com>
Now dtls1_do_handshake_write takes in a serialized form of the full message and
writes it. It's a little weird to serialize and deserialize the header a bunch,
but msg_callback requires that we keep the full one around in memory anyway.
Between that and the handshake hash definition, DTLS really wants messages to
mean the assembled header, redundancies and all, so we'll just put together
messages that way.
This also fixes a bug where ssl_do_msg_callback would get passed in garbage
where the header was supposed to be. The buffered messages get sampled before
writing the fragment rather than after.
Change-Id: I4e3b8ce4aab4c4ab4502d5428dfb8f3f729c6ef9
Reviewed-on: https://boringssl-review.googlesource.com/8433
Reviewed-by: Adam Langley <agl@google.com>
It doesn't really convey anything useful. Leave ssl_get_message alone for now
since it's called everywhere in the handshake and I'm about to tweak it
further.
Change-Id: I6f3a74c170e818f624be8fbe5cf6b796353406df
Reviewed-on: https://boringssl-review.googlesource.com/8430
Reviewed-by: Adam Langley <agl@google.com>
There's a __pragma expression which allows this. Android builds us Windows with
MinGW for some reason, so we actually do have to tolerate non-MSVC-compatible
Windows compilers. (Clang for Windows is much more sensible than MinGW and
intentionally mimicks MSVC.)
MinGW doesn't understand MSVC's pragmas and warns a lot. #pragma warning is
safe to suppress, so wrap those to shush them. This also lets us do away with a
few ifdefs.
Change-Id: I1f5a8bec4940d4b2d947c4c1cc9341bc15ec4972
Reviewed-on: https://boringssl-review.googlesource.com/8236
Reviewed-by: Adam Langley <agl@google.com>
While most of OpenSSL's assembly allows out < in too, some of it doesn't.
Upstream seems to not consider this a problem (or, at least, they're failing to
make a decision on whether it is a problem, so we should assume they'll stay
their course). Accordingly, require aliased buffers to exactly align so we
don't have to keep chasing this down.
Change-Id: I00eb3df3e195b249116c68f7272442918d7077eb
Reviewed-on: https://boringssl-review.googlesource.com/8231
Reviewed-by: Adam Langley <agl@google.com>
Decrypting is very easy to do in-place, but encrypting in-place is a hassle.
The rules actually were wrong due to record-splitting. The aliasing prefix and
the alignment prefix actually differ by 1. Take it out for now in preparation
for tightening the aliasing rules.
If we decide to do in-place encrypt later, probably it'd be more useful to
return header + in-place ciphertext + trailer. (That, in turn, needs a
scatter/gather thing on the AEAD thanks to TLS 1.3's padding and record type
construction.) We may also wish to rethink how record-splitting works here.
Change-Id: I0187d39c541e76ef933b7c2c193323164fd8a156
Reviewed-on: https://boringssl-review.googlesource.com/8230
Reviewed-by: Adam Langley <agl@google.com>
Reorder states and functions by where they appear in the handshake. Remove
unnecessary hooks on SSL_PROTOCOL_METHOD.
Change-Id: I78dae9cf70792170abed6f38510ce870707e82ff
Reviewed-on: https://boringssl-review.googlesource.com/8184
Reviewed-by: David Benjamin <davidben@google.com>
It can be folded into dtls1_read_app_data. This code, since it still takes an
output pointer, does not yet process records atomically. (Though, being DTLS,
it probably should...)
Change-Id: I57d60785c9c1dd13b5b2ed158a08a8f5a518db4f
Reviewed-on: https://boringssl-review.googlesource.com/8177
Reviewed-by: David Benjamin <davidben@google.com>
This was probably the worst offender of them all as read_bytes is the wrong
abstraction to begin with. Note this is a slight change in how processing a
record works. Rather than reading one fragment at a time, we process all
fragments in a record and return. The intent here is so that all records are
processed atomically since the connection eventually will not be able to retain
a buffer holding the record.
This loses a ton of (though not quite all yet) those a2b macros.
Change-Id: Ibe4bbcc33c496328de08d272457d2282c411b38b
Reviewed-on: https://boringssl-review.googlesource.com/8176
Reviewed-by: David Benjamin <davidben@google.com>
The business with ssl_record_prefix_len is rather a hassle. Instead, have
tls_open_record always decrypt in-place and give back a CBS to where the body
is.
This way the caller doesn't need to do an extra check all to avoid creating an
invalid pointer and underflow in subtraction.
Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee
Reviewed-on: https://boringssl-review.googlesource.com/8173
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Alert handling is more-or-less identical across all contexts. Push it down from
read_bytes into the low-level record functions. This also deduplicates the code
shared between TLS and DTLS.
Now the only type mismatch managed by read_bytes is if we get handshake data in
read_app_data.
Change-Id: Ia8331897b304566e66d901899cfbf31d2870194e
Reviewed-on: https://boringssl-review.googlesource.com/8124
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is getting a little repetitive.
Change-Id: Ib0fa8ab10149557c2d728b88648381b9368221d9
Reviewed-on: https://boringssl-review.googlesource.com/8126
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Move this logic out of dtls1_read_bytes and into dtls1_get_record. Only trigger
it when reading from the buffer fails. The other one shouldn't be necessary.
This exists to handle the blocking BIO case when the
BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT signal triggers, so we only need to do it when
timeouts actually trigger.
There also doesn't seem to be a need for most of the machinery. The
BIO_set_flags call seems to be working around a deficiency in the underlying
BIO. There also shouldn't be a need to check the handshake state as there
wouldn't be a timer to restart otherwise.
Change-Id: Ic901ccfb5b82aeb409d16a9d32c04741410ad6d7
Reviewed-on: https://boringssl-review.googlesource.com/8122
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is easier to deploy, and more obvious. This commit reverts a few
pieces of e25775bc, but keeps most of it.
Change-Id: If8d657a4221c665349c06041bb12fffca1527a2c
Reviewed-on: https://boringssl-review.googlesource.com/8061
Reviewed-by: Adam Langley <agl@google.com>
The TLS 1.3 spec has an explicit nonce construction for AEADs that
requires xoring the IV and sequence number.
Change-Id: I77145e12f7946ffb35ebeeb9b2947aa51058cbe9
Reviewed-on: https://boringssl-review.googlesource.com/8042
Reviewed-by: Adam Langley <agl@google.com>
The 'elliptic_curves' extension is being renamed to 'supported_groups'
in the TLS 1.3 draft, and most of the curve-specific methods are
generalized to groups/group IDs.
Change-Id: Icd1a1cf7365c8a4a64ae601993dc4273802610fb
Reviewed-on: https://boringssl-review.googlesource.com/7955
Reviewed-by: David Benjamin <davidben@google.com>
CECPQ1 is a new key exchange that concatenates the results of an X25519
key agreement and a NEWHOPE key agreement.
Change-Id: Ib919bdc2e1f30f28bf80c4c18f6558017ea386bb
Reviewed-on: https://boringssl-review.googlesource.com/7962
Reviewed-by: David Benjamin <davidben@google.com>
They're completely unused now. The handshake message reassembly logic should
not depend on the state machine. This should partially free it up (ugly as it
is) to be shared with a future TLS 1.3 implementation while, in parallel, it
and the layers below, get reworked. This also cuts down on the number of states
significantly.
Partially because I expect we'd want to get ssl_hash_message_t out of there
too. Having it in common code is fine, but it needs to be in the (supposed to
be) protocol-agnostic handshake state machine, not the protocol-specific
handshake message layer.
Change-Id: I12f9dc57bf433ceead0591106ab165d352ef6ee4
Reviewed-on: https://boringssl-review.googlesource.com/7949
Reviewed-by: Adam Langley <agl@google.com>
Previously, SSL_ECDH_METHOD consisted of two methods: one to produce a
public key to be sent to the peer, and another to produce the shared key
upon receipt of the peer's message.
This API does not work for NEWHOPE, because the client-to-server message
cannot be produced until the server's message has been received by the
client.
Solve this by introducing a new method which consumes data from the
server key exchange message and produces data for the client key
exchange message.
Change-Id: I1ed5a2bf198ca2d2ddb6d577888c1fa2008ef99a
Reviewed-on: https://boringssl-review.googlesource.com/7961
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL used to only forbid it on the server in plain PSK and allow it on the
client. Enforce it properly on both sides. My read of the rule in RFC 5246 ("A
non-anonymous server can optionally request a certificate") and in RFC 4279
("The Certificate and CertificateRequest payloads are omitted from the
response.") is that client auth happens iff we're certificate-based.
The line in RFC 4279 is under the plain PSK section, but that doesn't make a
whole lot of sense and there is only one diagram. PSK already authenticates
both sides. I think the most plausible interpretation is that this is for
certificate-based ciphers.
Change-Id: If195232c83f21e011e25318178bb45186de707e6
Reviewed-on: https://boringssl-review.googlesource.com/7942
Reviewed-by: David Benjamin <davidben@google.com>
A handshake message can go up to 2^24 bytes = 16MB which is a little large for
the peer to force us to buffer. Accordingly, we bound the size of a
handshake message.
Rather than have a global limit, the existing logic uses a different limit at
each state in the handshake state machine and, for certificates, allows
configuring the maximum certificate size. This is nice in that we engage larger
limits iff the relevant state is reachable from the handshake. Servers without
client auth get a tighter limit "for free".
However, this doesn't work for DTLS due to out-of-order messages and we use a
simpler scheme for DTLS. This scheme also is tricky on optional messages and
makes the handshake <-> message layer communication complex.
Apart from an ignored 20,000 byte limit on ServerHello, the largest
non-certificate limit is the common 16k limit on ClientHello. So this
complexity wasn't buying us anything. Unify everything on the DTLS scheme
except, so as not to regress bounds on client-auth-less servers, also correctly
check for whether client auth is configured. The value of 16k was chosen based
on this value.
(The 20,000 byte ServerHello limit makes no sense. We can easily bound the
ServerHello because servers may not send extensions we don't implement. But it
gets overshadowed by the certificate anyway.)
Change-Id: I00309b16d809a3c2a1543f99fd29c4163e3add81
Reviewed-on: https://boringssl-review.googlesource.com/7941
Reviewed-by: David Benjamin <davidben@google.com>
It's only used in one file.
Change-Id: I5d60cbc02799b22317f5f7593faf25eb8eea0a24
Reviewed-on: https://boringssl-review.googlesource.com/7943
Reviewed-by: David Benjamin <davidben@google.com>
This allows an application to override the default of 1 second, which
is what's instructed in RFC 6347 but is not an absolute requirement.
Change-Id: I0bbb16e31990fbcab44a29325b6ec7757d5789e5
Reviewed-on: https://boringssl-review.googlesource.com/7930
Reviewed-by: David Benjamin <davidben@google.com>
Having bbio be tri-state (not allocated, allocated but not active, and
allocated and active) is confusing.
The extra state is only used in the client handshake, where ClientHello is
special-cased to not go through the buffer while everything else is. This dates
to OpenSSL's initial commit and doesn't seem to do much. I do not believe it
can affect renego as the buffer only affects writes; although OpenSSL accepted
interleave on read (though this logic predates it slightly), it never sent
application data while it believed a handshake was active. The handshake would
always be driven to completion first.
My guess is this was to save a copy since the ClientHello is a one-message
flight so it wouldn't need to be buffered? This is probably not worth the extra
variation in the state. (Especially with the DTLS state machine going through
ClientHello twice and pushing the BIO in between the two. Though I suspect that
was a mistake in itself. If the optimization guess is correct, there was no
need to do that.)
Change-Id: I6726f866e16ee7213cab0c3e6abb133981444d47
Reviewed-on: https://boringssl-review.googlesource.com/7873
Reviewed-by: Adam Langley <agl@google.com>
The DTLS bbio logic is rather problematic, but this shouldn't make things
worse. In the in-handshake case, the new code merges the per-message
(unchecked) BIO_flush calls into one call at the end but otherwise the BIO is
treated as is. Otherwise any behavior around non-block writes should be
preserved.
In the post-handshake case, we now install the buffer when we didn't
previously. On write error, the buffer will have garbage in it, but it will be
discarded, so that will preserve any existing retry behavior. (Arguably the
existing retry behavior is a bug, but that's another matter.)
Add a test for all this, otherwise it is sure to regress. Testing for
record-packing is a little fuzzy, but we can assert ChangeCipherSpec always
shares a record with something.
BUG=57
Change-Id: I8603f20811d502c71ded2943b0e72a8bdc4e46f2
Reviewed-on: https://boringssl-review.googlesource.com/7871
Reviewed-by: Adam Langley <agl@google.com>
This was dropped in d27441a9cb due to lack
of use, but node.js now needs it.
Change-Id: I1e207d4b46fc746cfae309a0ea7bbbc04ea785e8
Reviewed-on: https://boringssl-review.googlesource.com/7270
Reviewed-by: David Benjamin <davidben@google.com>
For TLS, this machinery only exists to swallow no_certificate alerts
which only get sent in an SSL 3.0 codepath anyway. It's much less a
no-op for SSL 3.0 which, strictly speaking, has only a subset of TLS's
alerts.
This gets messy around version negotiation because of the complex
relationship between enc_method, have_version, and version which all get
set at different times. Given that SSL 3.0 is nearly dead and all these
alerts are fatal to the connection anyway, this doesn't seem worth
carrying around. (It doesn't work very well anyway. An SSLv3-only server
may still send a record_overflow alert before version negotiation.)
This removes the last place enc_method is accessed prior to version
negotiation.
Change-Id: I79a704259fca69e4df76bd5a6846c9373f46f5a9
Reviewed-on: https://boringssl-review.googlesource.com/6843
Reviewed-by: Adam Langley <alangley@gmail.com>
This removes the various non-PRF checks from SSL3_ENC_METHOD so that can
have a clearer purpose. It also makes TLS 1.0 through 1.2's
SSL3_ENC_METHOD tables identical and gives us an assert to ensure
nothing accesses the version bits before version negotiation.
Accordingly, ssl_needs_record_splitting was reordered slightly so we
don't rely on enc_method being initialized to TLS 1.2
pre-version-negotiation.
This leaves alert_value as the only part of SSL3_ENC_METHOD which may be
accessed before version negotiation.
Change-Id: If9e299e2ef5511b5fa442b2af654eed054c3e675
Reviewed-on: https://boringssl-review.googlesource.com/6842
Reviewed-by: Adam Langley <alangley@gmail.com>
I got that from the TLS 1.3 draft, but it's kind of silly-looking. X25519
already refers to a Diffie-Hellman primitive.
Also hopefully the WG will split NamedGroups and SignatureAlgorithms per the
recent proposal, so it won't be needed anyway. (Most chatter is about what
hashes should be allowed with what NIST curves, so it seems like people like
the split itself? We'll see.)
Change-Id: I7bb713190001199a3ebd30b67df2c00d29132431
Reviewed-on: https://boringssl-review.googlesource.com/6912
Reviewed-by: Adam Langley <agl@google.com>
We have need to normalize other versions during version negotiation, but
almost all will be post-negotiation. Hopefully later this can be
replaced with a value explicitly stored on the object and we do away
with ssl->version.
Change-Id: I595db9163d0af2e7c083b9a09310179aaa9ac812
Reviewed-on: https://boringssl-review.googlesource.com/6841
Reviewed-by: Adam Langley <alangley@gmail.com>
The various SSL3_ENC_METHODs ought to be defined in the same file their
functions are defined in, so they can be static.
Change-Id: I34a1d3437e8e61d4d50f2be70312e4630ea89c19
Reviewed-on: https://boringssl-review.googlesource.com/6840
Reviewed-by: Adam Langley <alangley@gmail.com>
This is a companion to SSL_get_rc4_state and SSL_get_ivs which doesn't
require poking at internal state. Partly since it aligns with the
current code and partly the off chance we ever need to get
wpa_supplicant's EAP-FAST code working, the API allows one to generate
more key material than is actually in the key block.
Change-Id: I58bc3f2b017482dbb8567dcd0cd754947a95397f
Reviewed-on: https://boringssl-review.googlesource.com/6839
Reviewed-by: Adam Langley <alangley@gmail.com>
There's not much point in putting those in the interface as the
final_finished_mac implementation is itself different between SSL 3.0
and TLS.
Change-Id: I76528a88d255c451ae008f1a34e51c3cb57d3073
Reviewed-on: https://boringssl-review.googlesource.com/6838
Reviewed-by: Adam Langley <alangley@gmail.com>
As things stand now, they don't actually do anything.
Change-Id: I9f8b4cbf38a0dffabfc5265805c52bb8d7a8fb0d
Reviewed-on: https://boringssl-review.googlesource.com/6837
Reviewed-by: Adam Langley <alangley@gmail.com>
Both are connection state rather than configuration state. Notably this
cuts down more of SSL_clear that can't just use ssl_free + ssl_new.
Change-Id: I3c05b3ae86d4db8bd75f1cd21656f57fc5b55ca9
Reviewed-on: https://boringssl-review.googlesource.com/6835
Reviewed-by: Adam Langley <alangley@gmail.com>
It's the same between TLS and SSL 3.0. There's also no need for the
do_change_cipher_spec wrapper (it no longer needs checks to ensure it
isn't called at a bad place). Finally fold the setup_key_block call into
change_cipher_spec.
Change-Id: I7917f48e1a322f5fbafcf1dfb8ad53f66565c314
Reviewed-on: https://boringssl-review.googlesource.com/6834
Reviewed-by: Adam Langley <alangley@gmail.com>
Move the actual SSL_AEAD_CTX swap into the record layer. Also revise the
intermediate state we store between setup_key_block and
change_cipher_state. With SSL_AEAD_CTX_new abstracted out, keeping the
EVP_AEAD around doesn't make much sense. Just store enough to partition
the key block.
Change-Id: I773fb46a2cb78fa570f00c0a89339c15bbb1d719
Reviewed-on: https://boringssl-review.googlesource.com/6832
Reviewed-by: Adam Langley <alangley@gmail.com>
This is a minor regression from
https://boringssl-review.googlesource.com/5235.
If the client, for whatever reason, had an ID-based session but also
supports tickets, it will send non-empty ID + empty ticket extension.
If the ticket extension is non-empty, then the ID is not an ID but a
dummy signaling value, so 5235 avoided looking it up. But if it is
present and empty, the ID is still an ID and should be looked up.
This shouldn't have any practical consequences, except if a server
switched from not supporting tickets and then started supporting it,
while keeping the session cache fixed.
Add a test for this case, and tighten up existing ID vs ticket tests so
they fail if we resume with the wrong type.
Change-Id: Id4d08cd809af00af30a2b67fe3a971078e404c75
Reviewed-on: https://boringssl-review.googlesource.com/6554
Reviewed-by: Adam Langley <alangley@gmail.com>
That we're half and half is really confusing.
Change-Id: I1c2632682e8a3e63d01dada8e0eb3b735ff709ce
Reviewed-on: https://boringssl-review.googlesource.com/6785
Reviewed-by: Adam Langley <agl@google.com>
This unifies the ClientKeyExchange code rather nicely. ServerKeyExchange
is still pretty specialized. For simplicity, I've extended the yaSSL bug
workaround for clients as well as servers rather than route in a
boolean.
Chrome's already banished DHE to a fallback with intention to remove
altogether later, and the spec doesn't say anything useful about
ClientDiffieHellmanPublic encoding, so this is unlikely to cause
problems.
Change-Id: I0355cd1fd0fab5729e8812e4427dd689124f53a2
Reviewed-on: https://boringssl-review.googlesource.com/6784
Reviewed-by: Adam Langley <agl@google.com>
The new curve is not enabled by default.
As EC_GROUP/EC_POINT is a bit too complex for X25519, this introduces an
SSL_ECDH_METHOD abstraction which wraps just the raw ECDH operation. It
also tidies up some of the curve code which kept converting back and
force between NIDs and curve IDs. Now everything transits as curve IDs
except for API entry points (SSL_set1_curves) which take NIDs. Those
convert immediately and act on curve IDs from then on.
Note that, like the Go implementation, this slightly tweaks the order of
operations. The client sees the server public key before sending its
own. To keep the abstraction simple, SSL_ECDH_METHOD expects to
generate a keypair before consuming the peer's public key. Instead, the
client handshake stashes the serialized peer public value and defers
parsing it until it comes time to send ClientKeyExchange. (This is
analogous to what it was doing before where it stashed the parsed peer
public value instead.)
It still uses TLS 1.2 terminology everywhere, but this abstraction should also
be compatible with TLS 1.3 which unifies (EC)DH-style key exchanges.
(Accordingly, this abstraction intentionally does not handle parsing the
ClientKeyExchange/ServerKeyExchange framing or attempt to handle asynchronous
plain RSA or the authentication bits.)
BUG=571231
Change-Id: Iba09dddee5bcdfeb2b70185308e8ab0632717932
Reviewed-on: https://boringssl-review.googlesource.com/6780
Reviewed-by: Adam Langley <agl@google.com>
clang-format keeps getting annoyed at it. Also remove some long-dead
constants.
Change-Id: I61e773f5be1e60ca28f1ea085e3afa7cb2c97b9e
Reviewed-on: https://boringssl-review.googlesource.com/6778
Reviewed-by: Adam Langley <agl@google.com>
In doing so, make the asynchronous portion look more like
ssl3_send_server_key_exchange. This is a considerably simpler structure,
so the save/resume doesn't need any state.
Mostly this means writing out the signature algorithm can now go through
CBB rather than a uint8_t* without bounds check.
Change-Id: If99fcffd0d41a84514c3d23034062c582f1bccb2
Reviewed-on: https://boringssl-review.googlesource.com/6771
Reviewed-by: Adam Langley <agl@google.com>
There is some messiness around saving and restoring the CBB, but this is
still significantly clearer.
Note that the BUF_MEM_grow line is gone in favor of a fixed CBB like the
other functions ported thus far. This line was never necessary as
init_buf is initialized to 16k and none of our key exchanges get that
large. (The largest one can get is DHE_RSA. Even so, it'd take a roughly
30k-bit DH group with a 30k-bit RSA key.)
Having such limits and tight assumptions on init_buf's initial size is
poor (but on par for the old code which usually just blindly assumed the
message would not get too large) and the size of the certificate chain
is much less obviously bounded, so those BUF_MEM_grows can't easily go.
My current plan is convert everything but those which legitimately need
BUF_MEM_grow to CBB, then atomically convert the rest, remove init_buf,
and switch everything to non-fixed CBBs. This will hopefully also
simplify async resumption. In the meantime, having a story for
resumption means the future atomic change is smaller and, more
importantly, relieves some complexity budget in the ServerKeyExchange
code for adding Curve25519.
Change-Id: I1de6af9856caaed353453d92a502ba461a938fbd
Reviewed-on: https://boringssl-review.googlesource.com/6770
Reviewed-by: Adam Langley <agl@google.com>
This relieves some complexity budget for adding Curve25519 to this
code.
This also adds a BN_bn2cbb_padded helper function since this seems to be a
fairly common need.
Change-Id: Ied0066fdaec9d02659abd6eb1a13f33502c9e198
Reviewed-on: https://boringssl-review.googlesource.com/6767
Reviewed-by: Adam Langley <agl@google.com>
Only ECDHE-based ciphers are implemented. To ease the transition, the
pre-standard cipher shares a name with the standard one. The cipher rule parser
is hacked up to match the name to both ciphers. From the perspective of the
cipher suite configuration language, there is only one cipher.
This does mean it is impossible to disable the old variant without a code
change, but this situation will be very short-lived, so this is fine.
Also take this opportunity to make the CK and TXT names align with convention.
Change-Id: Ie819819c55bce8ff58e533f1dbc8bef5af955c21
Reviewed-on: https://boringssl-review.googlesource.com/6686
Reviewed-by: Adam Langley <agl@google.com>
This uses ssl3_read_bytes for now. We still need to dismantle that
function and then invert the handshake state machine, but this gets
things closer to the right shape as an intermediate step and is a large
chunk in itself. It simplifies a lot of the CCS/handshake
synchronization as a lot of the invariants much more clearly follow from
the handshake itself.
Tests need to be adjusted since this changes some error codes. Now all
the CCS/Handshake checks fall through to the usual
SSL_R_UNEXPECTED_RECORD codepath. Most of what used to be a special-case
falls out naturally. (If half of Finished was in the same record as the
pre-CCS message, that part of the handshake record would have been left
unconsumed, so read_change_cipher_spec would have noticed, just like
read_app_data would have noticed.)
Change-Id: I15c7501afe523d5062f0e24a3b65f053008d87be
Reviewed-on: https://boringssl-review.googlesource.com/6642
Reviewed-by: Adam Langley <agl@google.com>
