This is a simpler implementation than OpenSSL's, lacking responder IDs
and request extensions support. This mirrors the client implementation
already present.
Change-Id: I54592b60e0a708bfb003d491c9250401403c9e69
Reviewed-on: https://boringssl-review.googlesource.com/5700
Reviewed-by: Adam Langley <agl@google.com>
This guarantees that we never read beyond the first record, even if the
first record is empty. Between removing SSL_set_read_ahead and DTLS
enforcing record boundaries, this means the buffer need never memmove
data.
The memmove isn't really much of a burden and we can probably just put
SSL_set_read_ahead back after the cleanup if desired. But while the
non-existant read_ahead is off, we should avoid reading more than needed.
(Also the current memmove logic is completely wrong for TLS. Checking
align != 0 doesn't make sense. The real reason to memmove is that the
next record may still be full size. So now line 209 of s3_pkt.c should
*actually* be unreachable.)
SSL_R_HTTPS_PROXY_REQUEST detection is now slightly less accurate, but
OpenSSL was already not parsing HTTP completely. We could asynchronously
read the extra 3 bytes once the first 5 match, but that seems
unnecessary. (Shall we just get rid of all these HTTP detectors? The
only consumer of those error codes is some diagnostics logic.)
BUG=468889
Change-Id: Ie3bf148ae7274795e1d048d78282d1d8063278ea
Reviewed-on: https://boringssl-review.googlesource.com/5714
Reviewed-by: Adam Langley <agl@google.com>
I'm not sure why I made a separate one. (Not quite how the V2ClientHello
code will look in the buffer-free API yet. Probably the future
refactored SSL_HANDSHAKE gadget will need separate entry points to
consume a handshake message or V2ClientHello and the driver deals with
framing.)
This also means that ssl3_setup_read_buffer is never called external to
ssl3_read_n.
BUG=468889
Change-Id: I872f1188270968bf53ee9d0488a761c772a11e9e
Reviewed-on: https://boringssl-review.googlesource.com/5713
Reviewed-by: Adam Langley <agl@google.com>
Rather than sniff for ClientHello, just fall through to standard logic
once weird cases are resolved.
This means that garbage will now read as WRONG_VERSION rather than
UNKNOWN_PROTOCOL, but the rules here were slightly odd anyway. This also
means we'll now accept empty records before the ClientHello (up to the
empty record limit), and process records of the wrong type with the
usual codepath during the handshake.
This shouldn't be any more risk as it just makes the ClientHello more
consistent with the rest of the protocol. A TLS implementation that
doesn't parse V2ClientHello would do the same unless it still
special-cased the first record. All newly-exposed states are reachable
by fragmenting ClientHello by one byte and then sending the record in
question.
BUG=468889
Change-Id: Ib701ae5d8adb663e158c391639b232a9d9cd1c6e
Reviewed-on: https://boringssl-review.googlesource.com/5712
Reviewed-by: Adam Langley <agl@google.com>
Rather than iterate over handshake_dgsts itself, it can just call
tls1_handshake_digest.
Change-Id: Ia518da540e47e65b13367eb1af184c0885908488
Reviewed-on: https://boringssl-review.googlesource.com/5617
Reviewed-by: Adam Langley <agl@google.com>
The handshake hash is initialized from the buffer as soon as the cipher
is known. When adding a message to the transcript, independently update
the buffer and rolling hash, whichever is active. This avoids the
complications around dont_free_handshake_buffer and EMS.
BUG=492371
Change-Id: I3b1065796a50fd1be5d42ead7210c2f253ef0aca
Reviewed-on: https://boringssl-review.googlesource.com/5615
Reviewed-by: Adam Langley <agl@google.com>
The RSA key exchange needs decryption and is still unsupported.
Change-Id: I8c13b74e25a5424356afbe6e97b5f700a56de41f
Reviewed-on: https://boringssl-review.googlesource.com/5467
Reviewed-by: Adam Langley <agl@google.com>
This also removes support for the “old” Channel ID extension.
Change-Id: I1168efb9365c274db6b9d7e32013336e4404ff54
Reviewed-on: https://boringssl-review.googlesource.com/5462
Reviewed-by: Adam Langley <agl@google.com>
This allows us to remove the confusing EVP_PKEY argument to the
SSL_PRIVATE_KEY_METHOD wrapper functions. It also simplifies some of the
book-keeping around the CERT structure, as well as the API for
configuring certificates themselves. The current one is a little odd as
some functions automatically route to the slot while others affect the
most recently touched slot. Others still (extra_certs) apply to all
slots, making them not terribly useful.
Consumers with complex needs should use cert_cb or the early callback
(select_certificate_cb) to configure whatever they like based on the
ClientHello.
BUG=486295
Change-Id: Ice29ffeb867fa4959898b70dfc50fc00137f01f3
Reviewed-on: https://boringssl-review.googlesource.com/5351
Reviewed-by: Adam Langley <agl@google.com>
The distinction was not well-enforced in the code. In fact, it wasn't
even possible to use the RSA_SIGN slot because ssl_set_pkey and
ssl_set_cert would always use the RSA_ENC slot.
A follow-up will fold away the mechanism altogether, but this is an easy
initial simplfication.
BUG=486295
Change-Id: I66b5bf3e6dc243dac7c75924c1c1983538e49060
Reviewed-on: https://boringssl-review.googlesource.com/5349
Reviewed-by: Adam Langley <agl@google.com>
After the custom key method support, the EVP_PKEY parameter is somewhat
confusing (to be resolved with the certificate slots removal) as it must
always refer to a private key. ssl3_cert_verify_hash is sometimes used
with the peer's public key. If custom keys were supported on the server,
this would break.
Fix this by passing a pkey_type parameter and letting the caller decide
whether this uses SSL_PRIVATE_KEY_METHOD or not.
Change-Id: I673b92579a84b4561f28026ec0b1c78a6bfee440
Reviewed-on: https://boringssl-review.googlesource.com/5341
Reviewed-by: Adam Langley <agl@google.com>
One tedious thing about using CBB is that you can't safely CBB_cleanup
until CBB_init is successful, which breaks the general 'goto err' style
of cleanup. This makes it possible:
CBB_zero ~ EVP_MD_CTX_init
CBB_init ~ EVP_DigestInit
CBB_cleanup ~ EVP_MD_CTX_cleanup
Change-Id: I085ecc4405715368886dc4de02285a47e7fc4c52
Reviewed-on: https://boringssl-review.googlesource.com/5267
Reviewed-by: Adam Langley <agl@google.com>
Use more sensible variable names. Also move some work between the helpers and
s3_srvr.c a little; the session lookup functions now only return a new session.
Whether to send a ticket is now an additional output to avoid the enum
explosion around renewal. The actual SSL state is not modified.
This is somewhat cleaner as s3_srvr.c may still reject a session for other
reasons, so we avoid setting ssl->session and ssl->verify_result to a session
that wouldn't be used. (They get fixed up in ssl_get_new_session, so it didn't
actually matter.)
Change-Id: Ib52fabbe993b5e2b7408395a02cdea3dee66df7b
Reviewed-on: https://boringssl-review.googlesource.com/5235
Reviewed-by: Adam Langley <agl@google.com>
This adds a new API, SSL_set_private_key_method, which allows the consumer to
customize private key operations. For simplicity, it is incompatible with the
multiple slots feature (which will hopefully go away) but does not, for now,
break it.
The new method is only routed up for the client for now. The server will
require a decrypt hook as well for the plain RSA key exchange.
BUG=347404
Change-Id: I35d69095c29134c34c2af88c613ad557d6957614
Reviewed-on: https://boringssl-review.googlesource.com/5049
Reviewed-by: Adam Langley <agl@google.com>
The client and server both have to decide on behaviour when resuming a
session where the EMS state of the session doesn't match the EMS state
as exchanged in the handshake.
Original handshake
| No Yes
------+--------------------------------------------------------------
|
R | Server: ok [1] Server: abort [3]
e No | Client: ok [2] Client: abort [4]
s |
u |
m |
e |
Yes | Server: don't resume No problem
| Client: abort; server
| shouldn't have resumed
[1] Servers want to accept legacy clients. The draft[5] says that
resumptions SHOULD be rejected so that Triple-Handshake can't be done,
but we'll rather enforce that EMS was used when using tls-unique etc.
[2] The draft[5] says that even the initial handshake should be aborted
if the server doesn't support EMS, but we need to be able to talk to the
world.
[3] This is a very weird case where a client has regressed without
flushing the session cache. Hopefully we can be strict and reject these.
[4] This can happen when a server-farm shares a session cache but
frontends are not all updated at once. If Chrome is strict here then
hopefully we can prevent any servers from existing that will try to
resume an EMS session that they don't understand. OpenSSL appears to be
ok here: https://www.ietf.org/mail-archive/web/tls/current/msg16570.html
[5] https://tools.ietf.org/html/draft-ietf-tls-session-hash-05#section-5.2
BUG=492200
Change-Id: Ie1225a3960d49117b05eefa5a36263d8e556e467
Reviewed-on: https://boringssl-review.googlesource.com/4981
Reviewed-by: Adam Langley <agl@google.com>
With SSL2 gone, there's no need for this split between the abstract
cipher framework and ciphers. Put the cipher suite table in ssl_cipher.c
and move other SSL_CIPHER logic there. With that gone, prune the
cipher-related hooks in SSL_PROTOCOL_METHOD.
BUG=468889
Change-Id: I48579de8bc4c0ea52781ba1b7b57bc5b4919d21c
Reviewed-on: https://boringssl-review.googlesource.com/4961
Reviewed-by: Adam Langley <agl@google.com>
This allows us to merge two of the ssl3_digest_cached_records calls which were
almost, but not completely, redundant. Also catches a missing case: the buffer
may be discarded if doing session resumption but otherwise enabling client
authentication.
BUG=492371
Change-Id: I78e9a4a9cca665e89899ef97b815454c6f5c7e02
Reviewed-on: https://boringssl-review.googlesource.com/4885
Reviewed-by: Adam Langley <agl@google.com>
Servers can no longer renegotiate.
Change-Id: Id79d5753562e29d2872871f4f571552a019215fa
Reviewed-on: https://boringssl-review.googlesource.com/4884
Reviewed-by: Adam Langley <agl@google.com>
This is documented as "Only request a client certificate on the initial TLS/SSL
handshake. Do not ask for a client certificate again in case of a
renegotiation." Server-side renegotiation is gone.
I'm not sure this flag has ever worked anyway, dating all the way back to
SSLeay 0.8.1b. ssl_get_new_session overwrites s->session, so the old
session->peer is lost.
Change-Id: Ie173243e189c63272c368a55167b8596494fd59c
Reviewed-on: https://boringssl-review.googlesource.com/4883
Reviewed-by: Adam Langley <agl@google.com>
Never send the time as a client. Always send it as a server.
Change-Id: I20c55078cfe199d53dc002f6ee5dd57060b086d5
Reviewed-on: https://boringssl-review.googlesource.com/4829
Reviewed-by: Adam Langley <agl@google.com>
They were added to avoid accidentally enabling renego for a consumer which set
them to zero to break the handshake on renego. Now that renego is off by
default, we can get rid of them again.
Change-Id: I2cc3bf567c55c6562352446a36f2b5af37f519ba
Reviewed-on: https://boringssl-review.googlesource.com/4827
Reviewed-by: Adam Langley <agl@google.com>
It's never called and the state is meaningless now.
Change-Id: I5429ec3eb7dc2b789c0584ea88323f0ff18920ae
Reviewed-on: https://boringssl-review.googlesource.com/4826
Reviewed-by: Adam Langley <agl@google.com>
The only case where renego is supported is if we are a client and the
server sends a HelloRequest. That is still needed to support the renego
+ client auth hack in Chrome. Beyond that, no other forms of renego will
work.
The messy logic where the handshake loop is repurposed to send
HelloRequest and the extremely confusing tri-state s->renegotiate (which
makes SSL_renegotiate_pending a lie during the initial handshake as a
server) are now gone. The next change will further simplify things by
removing ssl->s3->renegotiate and the renego deferral logic. There's
also some server-only renegotiation checks that can go now.
Also clean up ssl3_read_bytes' HelloRequest handling. The old logic relied on
the handshake state machine to reject bad HelloRequests which... actually that
code probably lets you initiate renego by sending the first four bytes of a
ServerHello and expecting the peer to read it later.
BUG=429450
Change-Id: Ie0f87d0c2b94e13811fe8e22e810ab2ffc8efa6c
Reviewed-on: https://boringssl-review.googlesource.com/4824
Reviewed-by: Adam Langley <agl@google.com>
This cuts down on one config knob as well as one case in the renego
combinatorial explosion. Since the only case we care about with renego
is the client auth hack, there's no reason to ever do resumption.
Especially since, no matter what's in the session cache:
- OpenSSL will only ever offer the session it just established,
whether or not a newer one with client auth was since established.
- Chrome will never cache sessions created on a renegotiation, so
such a session would never make it to the session cache.
- The new_session + SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
logic had a bug where it would unconditionally never offer tickets
(but would advertise support) on renego, so any server doing renego
resumption against an OpenSSL-derived client must not support
session tickets.
This also gets rid of s->new_session which is now pointless.
BUG=429450
Change-Id: I884bdcdc80bff45935b2c429b4bbc9c16b2288f8
Reviewed-on: https://boringssl-review.googlesource.com/4732
Reviewed-by: Adam Langley <agl@google.com>
There's multiple different versions of this check, between
s->s3->have_version (only works at some points), s->new_session (really
weird and not actually right), s->renegotiate (fails on the server
because it's always 2 after ClientHello), and s->s3->tmp.finish_md_len
(super confusing). Add an explicit bit with clear meaning. We'll prune
some of the others later; notably s->renegotiate can go away when
initiating renegotiation is removed.
This also tidies up the extensions to be consistent about whether
they're allowed during renego:
- ALPN failed to condition when accepting from the server, so even
if the client didn't advertise, the server could.
- SCTs now *are* allowed during renego. I think forbidding it was a
stray copy-paste. It wasn't consistently enforced in both ClientHello
and ServerHello, so the server could still supply it. Moreover, SCTs
are part of the certificate, so we should accept it wherever we accept
certificates, otherwise that session's state becomes incomplete. This
matches OCSP stapling. (NB: Chrome will never insert a session created
on renego into the session cache and won't accept a certificate
change, so this is moot anyway.)
Change-Id: Ic9bd1ebe2a2dbe75930ed0213bf3c8ed8170e251
Reviewed-on: https://boringssl-review.googlesource.com/4730
Reviewed-by: Adam Langley <agl@google.com>
Nothing should call ssl3_setup_read_buffer or ssl3_setup_write_buffer unless it
intends to write into the buffer. This way buffer management can later be an
implementation detail of the record layer.
Change-Id: Idb0effba00e77c6169764843793f40ec37868b61
Reviewed-on: https://boringssl-review.googlesource.com/4687
Reviewed-by: Adam Langley <agl@google.com>
This is an API wart that makes it easy to accidentally reuse the server
DHE half for every handshake. It's much simpler to have only one mode.
This mirrors the change made to the ECDHE code; align with that logic.
Change-Id: I47cccbb354d70127ab458f99a6d390b213e4e515
Reviewed-on: https://boringssl-review.googlesource.com/4565
Reviewed-by: Adam Langley <agl@google.com>
Also size them based on the limits in the quantities they control (after
checking bounds at the API boundary).
BUG=404754
Change-Id: Id56ba45465a473a1a793244904310ef747f29b63
Reviewed-on: https://boringssl-review.googlesource.com/4559
Reviewed-by: Adam Langley <agl@google.com>
When tlsext_ticket_key_cb is used, the full bounds aren't known until
after the callback has returned.
Change-Id: I9e89ffae6944c74c4ca04e6aa28afd3ec80aa1d4
Reviewed-on: https://boringssl-review.googlesource.com/4552
Reviewed-by: Adam Langley <agl@google.com>
This is a really dumb API wart. Now that we have a limited set of curves that
are all reasonable, the automatic logic should just always kick in. This makes
set_ecdh_auto a no-op and, instead of making it the first choice, uses it as
the fallback behavior should none of the older curve selection APIs be used.
Currently, by default, server sockets can only use the plain RSA key exchange.
BUG=481139
Change-Id: Iaabc82de766cd00968844a71aaac29bd59841cd4
Reviewed-on: https://boringssl-review.googlesource.com/4531
Reviewed-by: Adam Langley <agl@google.com>
See upstream's 3ae91cfb327c9ed689b9aaf7bca01a3f5a0657cb.
I misread that code and thought it was allowing empty cipher suites when there
*is* a session ID, but it was allowing them when there isn't. Which doesn't
make much sense because it'll get rejected later anyway. (Verified by toying
with handshake_client.go.)
Change-Id: Ia870a1518bca36fce6f3018892254f53ab49f460
Reviewed-on: https://boringssl-review.googlesource.com/4401
Reviewed-by: Adam Langley <agl@google.com>
This causes any unexpected handshake records to be met with a fatal
no_renegotiation alert.
In addition, restore the redundant version sanity-checks in the handshake state
machines. Some code would zero the version field as a hacky way to break the
handshake on renego. Those will be removed when switching to this API.
The spec allows for a non-fatal no_renegotiation alert, but ssl3_read_bytes
makes it difficult to find the end of a ClientHello and skip it entirely. Given
that OpenSSL goes out of its way to map non-fatal no_renegotiation alerts to
fatal ones, this seems probably fine. This avoids needing to account for
another source of the library consuming an unbounded number of bytes without
returning data up.
Change-Id: Ie5050d9c9350c29cfe32d03a3c991bdc1da9e0e4
Reviewed-on: https://boringssl-review.googlesource.com/4300
Reviewed-by: Adam Langley <agl@google.com>
The rest of ssl/ still includes things everywhere, but this at least fixes the
includes that were implicit from ssl/internal.h.
Change-Id: I7ed22590aca0fe78af84fd99a3e557f4b05f6782
Reviewed-on: https://boringssl-review.googlesource.com/4281
Reviewed-by: Adam Langley <agl@google.com>
Match the other internal headers.
Change-Id: Iff7e2dd06a1a7bf993053d0464cc15638ace3aaa
Reviewed-on: https://boringssl-review.googlesource.com/4280
Reviewed-by: Adam Langley <agl@google.com>
These are the remaining untested cipher suites. Rather than add support in
runner.go, just remove them altogether. Grepping for this is a little tricky,
but nothing enables aNULL (all occurrences disable it), and all occurrences of
["ALL:] seem to be either unused or explicitly disable anonymous ciphers.
Change-Id: I4fd4b8dc6a273d6c04a26e93839641ddf738343f
Reviewed-on: https://boringssl-review.googlesource.com/4258
Reviewed-by: Adam Langley <agl@google.com>
There's multiple sets of APIs for selecting the curve. Fold away
SSL_OP_SINGLE_ECDH_USE as failing to set it is either a no-op or a bug. With
that gone, the consumer only needs to control the selection of a curve, with
key generation from then on being uniform. Also clean up the interaction
between the three API modes in s3_srvr.c; they were already mutually exclusive
due to tls1_check_ec_tmp_key.
This also removes all callers of EC_KEY_dup (and thus CRYPTO_dup_ex_data)
within the library.
Change-Id: I477b13bd9e77eb03d944ef631dd521639968dc8c
Reviewed-on: https://boringssl-review.googlesource.com/4200
Reviewed-by: Adam Langley <agl@google.com>
Within the library, only ssl_update_cache read them, so add a dedicated field
to replace that use.
The APIs have a handful of uninteresting callers so I've left them in for now,
but they now always return zero.
Change-Id: Ie4e36fd4ab18f9bff544541d042bf3c098a46933
Reviewed-on: https://boringssl-review.googlesource.com/4101
Reviewed-by: Adam Langley <agl@google.com>
Align with upstream's renames from a while ago. These names are considerably
more standard. This also aligns with upstream in that both "ECDHE" and "EECDH"
are now accepted in the various cipher string parsing bits.
Change-Id: I84c3daeacf806f79f12bc661c314941828656b04
Reviewed-on: https://boringssl-review.googlesource.com/4053
Reviewed-by: Adam Langley <agl@google.com>
This callback receives information about the ClientHello and can decide
whether or not to allow the handshake to continue.
Change-Id: I21be28335fa74fedb5b73a310ee24310670fc923
Reviewed-on: https://boringssl-review.googlesource.com/3721
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 687eaf27a7e4bdfc58dd455e2566b915a7a25c20. I don't think any
of the *Update functions can actually fail (we should verify this and, if
accurate, document it), but HMAC_Final can. It internally copies an EVP_MD_CTX.
Change-Id: I318cb9d0771d536249a26b61d34fe0413a4d3a10
Reviewed-on: https://boringssl-review.googlesource.com/3830
Reviewed-by: Adam Langley <agl@google.com>
It may fail because the BIO_write to the memory BIO can allocate.
Unfortunately, this bubbles up pretty far up now that we've moved the handshake
hash to ssl3_set_handshake_header.
Change-Id: I58884347a4456bb974ac4783078131522167e29d
Reviewed-on: https://boringssl-review.googlesource.com/3483
Reviewed-by: Adam Langley <agl@google.com>
Found while diagnosing some crashes and hangs in the malloc tests. This (and
the follow-up) get us further but does not quite let the malloc tests pass
quietly, even without valgrind. DTLS silently ignores some malloc failures
(confusion with silently dropping bad packets) which then translate to hangs.
Change-Id: Ief06a671e0973d09d2883432b89a86259e346653
Reviewed-on: https://boringssl-review.googlesource.com/3482
Reviewed-by: Adam Langley <agl@google.com>
I found no users of this. We can restore it if needbe, but I don't expect
anyone to find it useful in its current form. The API is suspect for the same
reasons DTLSv1_listen was. An SSL object is stateful and assumes you already
have the endpoint separated out.
If we ever need it, server-side HelloVerifyRequest and DTLSv1_listen should be
implemented by a separate stateless listener that statelessly handles
cookieless ClientHello + HelloVerifyRequest. Once a ClientHello with a valid
cookie comes in, it sets up a stateful SSL object and passes control along to
that.
Change-Id: I86adc1dfb6a81bebe987784c36ad6634a9a1b120
Reviewed-on: https://boringssl-review.googlesource.com/3480
Reviewed-by: Adam Langley <agl@google.com>
Caught by malloc valgrind tests on Basic-Client-Sync. Also one by inspection
and verified with valgrind. Those should pass now with the exception of
CRYPTO_free_ex_data being internally implemented with malloc.
(Clearly we next should make our malloc tests assert that the containing
function fails to catch when we fail to check for some error and things
silently move one.)
Change-Id: I56c51dc8a32a7d3c7ac907d54015dc241728c761
Reviewed-on: https://boringssl-review.googlesource.com/3440
Reviewed-by: Adam Langley <agl@google.com>
