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>
When the peer or caller requests a renegotiation, OpenSSL doesn't
renegotiate immediately. It sets a flag to begin a renegotiation as soon
as record-layer read and write buffers are clear. One reason is that
OpenSSL's record layer cannot write a handshake record while an
application data record is being written. The buffer consistency checks
around partial writes will break.
None of these cases are relevant for the client auth hack. We already
require that renego come in at a quiescent part of the application
protocol by forbidding handshake/app_data interleave.
The new behavior is now: when a HelloRequest comes in, if the record
layer is not idle, the renegotiation is rejected as if
SSL_set_reject_peer_renegotiations were set. Otherwise we immediately
begin the new handshake. The server may not send any application data
between HelloRequest and completing the handshake. The HelloRequest may
not be consumed if an SSL_write is pending.
Note this does require that Chromium's HTTP stack not attempt to read
the HTTP response until the request has been written, but the
renegotiation logic already assumes it. Were Chromium to drive the
SSL_read state machine early and the server, say, sent a HelloRequest
after reading the request headers but before we've sent the whole POST
body, the SSL state machine may racily enter renegotiate early, block
writing the POST body on the new handshake, which would break Chromium's
ERR_SSL_CLIENT_AUTH_CERT_NEEDED plumbing.
BUG=429450
Change-Id: I6278240c3bceb5d2e1a2195bdb62dd9e0f4df718
Reviewed-on: https://boringssl-review.googlesource.com/4825
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 dates back to upstream's a2a0158959.
It seems to be a remnant of those SSL_clear calls in the handshake state
machine which... were also bizarre and since gone.
Since SSL_clear is to drop the current connection but retain the
configuration, it doesn't really make sense to forbid it while you're
mid-handshake.
This removes another consumer of s->renegotiate.
BUG=429450
Change-Id: Ifac6bf11644447fd5571262bed7421684739bc39
Reviewed-on: https://boringssl-review.googlesource.com/4823
Reviewed-by: Adam Langley <agl@google.com>
ssl_cipher_list_to_bytes is client-only, so s->renegotiate worked, but
the only reason the other two worked is because s->renegotiate isn't a
lie on the server before ServerHello.
BUG=429450
Change-Id: If68a986c6ec4a0f16e57a6187238e05b50ecedfc
Reviewed-on: https://boringssl-review.googlesource.com/4822
Reviewed-by: Adam Langley <agl@google.com>
Now that WebRTC honors packet boundaries (https://crbug.com/447431), we
can start enforcing them correctly. Configuring read-ahead now does
nothing. Instead DTLS will always set "read-ahead" and also correctly
enforce packet boundaries when reading records. Add tests to ensure that
badly fragmented packets are ignored. Because such packets don't fail
the handshake, the tests work by injecting an alert in the front of the
handshake stream and ensuring the DTLS implementation ignores them.
ssl3_read_n can be be considerably unraveled now, but leave that for
future cleanup. For now, make it correct.
BUG=468889
Change-Id: I800cfabe06615af31c2ccece436ca52aed9fe899
Reviewed-on: https://boringssl-review.googlesource.com/4820
Reviewed-by: Adam Langley <agl@google.com>
This isn't exhaustive. There are still failures in some tests which probably
ought to get C++'d first.
Change-Id: Iac58df9d98cdfd94603d54374a531b2559df64c3
Reviewed-on: https://boringssl-review.googlesource.com/4795
Reviewed-by: Adam Langley <agl@google.com>
Currently far from passing and I haven't even tried with a leak checker yet.
Also bn_test is slow.
Change-Id: I4fe2783aa5f7897839ca846062ae7e4a367d2469
Reviewed-on: https://boringssl-review.googlesource.com/4794
Reviewed-by: Adam Langley <agl@google.com>
This is considerably less scary than swapping out connection state. It also
fixes a minor bug where, if dtls1_do_write had an alert to dispatch and we
happened to retry during a rexmit, it would use the wrong epoch.
BUG=468889
Change-Id: I754b0d46bfd02f797f4c3f7cfde28d3e5f30c52b
Reviewed-on: https://boringssl-review.googlesource.com/4793
Reviewed-by: Adam Langley <agl@google.com>
tls1_enc is now SSL_AEAD_CTX_{open,seal}. This starts tidying up a bit
of the record-layer logic. This removes rr->input, as encrypting and
decrypting records no longer refers to various globals. It also removes
wrec altogether. SSL3_RECORD is now only used to maintain state about
the current incoming record. Outgoing records go straight to the write
buffer.
This also removes the outgoing alignment memcpy and simply calls
SSL_AEAD_CTX_seal with the parameters as appropriate. From bssl speed
tests, this seems to be faster on non-ARM and a bit of a wash on ARM.
Later it may be worth recasting these open/seal functions to write into
a CBB (tweaked so it can be malloc-averse), but for now they take an
out/out_len/max_out trio like their EVP_AEAD counterparts.
BUG=468889
Change-Id: Ie9266a818cc053f695d35ef611fd74c5d4def6c3
Reviewed-on: https://boringssl-review.googlesource.com/4792
Reviewed-by: Adam Langley <agl@google.com>
Chromium's session cache has since been rewritten and no longer needs to
muck with those functions in tests.
Change-Id: I2defad81513210dca5e105757e04cbb677583251
Reviewed-on: https://boringssl-review.googlesource.com/4788
Reviewed-by: Adam Langley <agl@google.com>
Current thought is to organize this by:
- Core SSL_CTX APIs (creating, destroying)
- Core SSL APIs (creating destroying, maybe handshake, read, write as
well)
- APIs to configure SSL_CTX/SSL, roughly grouped by feature. Probably
options and modes are the first two sections. SSL_TXT_* constants can
be part of documenting cipher suite configuration.
- APIs to query state from SSL_CTX/SSL, roughly grouped by feature. (Or
perhaps these should be folded into the configuration sections?)
The functions themselves aren't reordered or reorganized to match the
eventual header order yet. Though I did do the s -> ssl rename on the
ones I've touched.
Also formally deprecate SSL_clear. It would be a core SSL API
except it's horrible.
Change-Id: Ia7e4fdcb7bad4e9ccdee8cf8c3136dc63aaaa772
Reviewed-on: https://boringssl-review.googlesource.com/4784
Reviewed-by: Adam Langley <agl@google.com>
Rather than duplicate all the various modifiers, which is quite
error-prone, write all the tests to a temporary array and then apply
modifiers afterwards.
Change-Id: I19bfeb83b722ed34e973f17906c5e071471a926a
Reviewed-on: https://boringssl-review.googlesource.com/4782
Reviewed-by: Adam Langley <agl@google.com>
We should be testing asynchronous renego.
BUG=429450
Change-Id: Ib7a5d42f2ac728f9ea0d80158eef63ad77cd77a4
Reviewed-on: https://boringssl-review.googlesource.com/4781
Reviewed-by: Adam Langley <agl@google.com>
Since we hope to eventually lose server-side renegotiation support
altogether, get the client-side version of those tests. We should have
had those anyway to test that the default is to allow it.
BUG=429450
Change-Id: I4a18f339b55f3f07d77e22e823141e10a12bc9ff
Reviewed-on: https://boringssl-review.googlesource.com/4780
Reviewed-by: Adam Langley <agl@google.com>
|SSL_CTX| and |X509_STORE| have grown their own locks. Several static
locks have been added to hack around not being able to use a
|CRYPTO_once_t| in public headers. Lastly, support for calling
|SSL_CTX_set_generate_session_id| concurrently with active connections
has been removed. No other property of an |SSL_CTX| works like that.
Change-Id: Iff5fe3ee3fdd6ea9c9daee96f850b107ad8a6bca
Reviewed-on: https://boringssl-review.googlesource.com/4775
Reviewed-by: Adam Langley <agl@google.com>
Convert reference counts in ssl/ to use |CRYPTO_refcount_t|.
Change-Id: I5d60f641b0c89b1ddfe38bfbd9d7285c60377f4c
Reviewed-on: https://boringssl-review.googlesource.com/4773
Reviewed-by: Adam Langley <agl@google.com>
This change converts the reference counts in crypto/ to use
|CRYPTO_refcount_t|. The reference counts in |X509_PKEY| and |X509_INFO|
were never actually used and so were dropped.
Change-Id: I75d572cdac1f8c1083c482e29c9519282d7fd16c
Reviewed-on: https://boringssl-review.googlesource.com/4772
Reviewed-by: Adam Langley <agl@google.com>
DH groups less than 1024 bits are clearly not very safe. Ideally servers
would switch to ECDHE because 1024 isn't great either, but this will
serve for the short term.
BUG=490240
Change-Id: Ic9aac714cdcdcbfae319b5eb1410675d3b903a69
Reviewed-on: https://boringssl-review.googlesource.com/4813
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
We can't actually catch this with MSan because it requires all code be
instrumented, so it needs a NO_ASM build which no disables that code. valgrind
doesn't notice either, possibly because there's some computation being done on
it. Still, we shouldn't use uninitialized memory.
Also get us closer to being instrumentable by MSan, but the runner tests will
need to build against an instrumented STL and I haven't tried that yet.
Change-Id: I2d65697a3269b5b022899f361730a85c51ecaa12
Reviewed-on: https://boringssl-review.googlesource.com/4760
Reviewed-by: Adam Langley <agl@google.com>
SSL_get_current_cipher is documented by upstream to return the cipher actually
being used. However, because it reads s->session, it returns information
pertaining to the session to be offered if queried before ServerHello or early
in an abbreviated handshake.
Logic around s->session needs more comprehensive cleanup but for just this
function, defining it to be the current outgoing cipher is close to the current
semantics but for fixing the initial state (s->session->cipher is populated
when sending CCS). Store it in the SSL_AEAD_CTX which seems a natural place to
associate state pertaining to a connection half.
BUG=484744
Change-Id: Ife8db27a16615d0dbb2aec65359537243e08af7c
Reviewed-on: https://boringssl-review.googlesource.com/4733
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>
As of crbug.com/484543, Chromium's SSLClientSocket is not sensitive to whether
renegotiation is enabled or not. Disable it by default and require consumers to
opt into enabling this protocol mistake.
BUG=429450
Change-Id: I2329068284dbb851da010ff1fd398df3d663bcc3
Reviewed-on: https://boringssl-review.googlesource.com/4723
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>
It's completely redundant with the extend bit. If extend is 0, we're reading a
new record, and rbuf.len is passed. Then it needs to get clamped by ssl3_read_n
post alignment anyway. If extend is 1, we're reading the rest of the current
record and max is always n. (For TLS, we actually could just read more, but not
for DTLS. Basically no one sets it on the TLS side of things, so instead, after
WebRTC's broken DTLS handling is fixed, read_ahead can go away altogether and
DTLS/TLS record layers can be separated.)
This removes ssl3_read_n's callers' dependency on ssl3_setup_read_buffer
setting up rbuf.len.
Change-Id: Iaf11535d01017507a52a33b19240f42984d6cf52
Reviewed-on: https://boringssl-review.googlesource.com/4686
Reviewed-by: Adam Langley <agl@google.com>
They date to https://rt.openssl.org/Ticket/Display.html?id=2533, but no
particularly good justification was given for them. It seems it was just a
bandaid because d1_pkt.c forgot to initialize the buffer. I went through
codesearch for all accesses to SSL3_BUFFER::buf and SSL::packet. They seem
appropriately guarded but for this one.
Change-Id: Ife4e7afdb7a7c137d6be4791542eb5de6dd5b1b6
Reviewed-on: https://boringssl-review.googlesource.com/4685
Reviewed-by: Adam Langley <agl@google.com>
s->packet points into the read buffer. It shouldn't leave a dangling pointer.
Change-Id: Ia7def2f50928ea9fca8cb0b69d614a92f9f47f57
Reviewed-on: https://boringssl-review.googlesource.com/4684
Reviewed-by: Adam Langley <agl@google.com>
There's no real need to ever disable it, so this is one fewer configuration to
test. It's still disabled for DTLS, but a follow-up will resolve that.
Change-Id: Ia95ad8c17ae8236ada516b3968a81c684bf37fd9
Reviewed-on: https://boringssl-review.googlesource.com/4683
Reviewed-by: Adam Langley <agl@google.com>
With DTLSv1_get_timeout de-ctrl-ified, the type checker complains about
OPENSSL_timeval. Existing callers all use the real timeval.
Now that OPENSSL_timeval is not included in any public structs, simply
forward-declare timeval itself in ssl.h and pull in winsock2.h in internal
headers.
Change-Id: Ieaf110e141578488048c28cdadb14881301a2ce1
Reviewed-on: https://boringssl-review.googlesource.com/4682
Reviewed-by: Adam Langley <agl@google.com>
Nothing ever uses those structs. This to avoid having any structs in the
public header which use struct timeval.
In doing so, move the protocol version constants up to ssl.h so dtls1.h
may be empty. This also removes TLS1_get_version and TLS1_get_client_version
as they're unused and depend on TLS1_VERSION_MAJOR. This still lets tls1.h
be included independently from ssl.h (though I don't think anyone ever includes
it...).
Change-Id: Ieac8b90cf94f7f1e742a88bb75c0ee0aa4b1414c
Reviewed-on: https://boringssl-review.googlesource.com/4681
Reviewed-by: Adam Langley <agl@google.com>
Per earlier review comment. The number is wrong anyway. (Neither version does
anything since init_buf is initialized to a large size and most functions don't
bother sizing it. Future work should rewrite all of this to use a CBB.)
Change-Id: I3b58672b328396459a34c6403f8bfb77c96efe9c
Reviewed-on: https://boringssl-review.googlesource.com/4650
Reviewed-by: Adam Langley <agl@google.com>
With SSL_get0_raw_cipherlist gone, there's no need to hold onto it.
Change-Id: I258f8bfe21cc354211a777660df680df6c49df2a
Reviewed-on: https://boringssl-review.googlesource.com/4616
Reviewed-by: Adam Langley <agl@google.com>
The only place using it is export keying material which can do the
version check inline.
Change-Id: I1893966c130aa43fa97a6116d91bb8b04f80c6fb
Reviewed-on: https://boringssl-review.googlesource.com/4615
Reviewed-by: Adam Langley <agl@google.com>
clang-format got a little confused there.
Change-Id: I46df523e8a7813a2b4e243da3df22851b3393873
Reviewed-on: https://boringssl-review.googlesource.com/4614
Reviewed-by: Adam Langley <agl@google.com>
It's only called for client certificates with NULL. The interaction with
extra_certs is more obvious if we handle that case externally. (We
shouldn't attach extra_certs if there is no leaf.)
Change-Id: I9dc26f32f582be8c48a4da9aae0ceee8741813dc
Reviewed-on: https://boringssl-review.googlesource.com/4613
Reviewed-by: Adam Langley <agl@google.com>
Next batch. Mostly a bunch of deprecated things. This switches
SSL_CTX_set_tmp_rsa from always failing to always succeeding. The latter
is probably a safer behavior; a consumer may defensively set a temporary
RSA key. We'll successfully "set it" and just never use the result.
Change-Id: Idd3d6bf4fc1a20bc9a26605bb9c77c9f799f993c
Reviewed-on: https://boringssl-review.googlesource.com/4566
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>
The only difference is SSL_clear_num_renegotiations which is never
called.
Change-Id: Id661c71e89d34d834349ad1f1a296e332606e6cc
Reviewed-on: https://boringssl-review.googlesource.com/4564
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley
Adam Langley