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>
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>
Doing it at ChangeCipherSpec makes it be set twice and, more
importantly, causes us to touch SSL_SESSION objects on resumption. (With
a no-op change, but this still isn't a good idea.)
This should actually let us get rid of ssl->s3->tmp.new_cipher but some
of external code accesses that field directly.
Change-Id: Ia6b7e0964c1b430f963ad0b1a5417b339b7b19d3
Reviewed-on: https://boringssl-review.googlesource.com/6833
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 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>
TLS resets it in t1_enc.c while DTLS has it sprinkled everywhere.
Change-Id: I78f0f0e646b4dc82a1058199c4b00f2e917aa5bc
Reviewed-on: https://boringssl-review.googlesource.com/6511
Reviewed-by: Adam Langley <agl@google.com>
This callback is some combination of arguably useful stuff (bracket
handshakes, alerts) and completely insane things (find out when the
state machine advances). Deprecate the latter.
Change-Id: Ibea5b32cb360b767b0f45b302fd5f1fe17850593
Reviewed-on: https://boringssl-review.googlesource.com/6305
Reviewed-by: Adam Langley <alangley@gmail.com>
ssl.h should be first. Also two lines after includes and the rest of the
file.
Change-Id: Icb7586e00a3e64170082c96cf3f8bfbb2b7e1611
Reviewed-on: https://boringssl-review.googlesource.com/5892
Reviewed-by: Adam Langley <agl@google.com>
The handshake state machine is still rather messy (we should switch to CBB,
split the key exchanges apart, and also pull reading and writing out), but this
version makes it more obvious to the compiler that |p| and |sig_len| are
initialized. The old logic created a synchronous-only state which, if enterred
directly, resulted in some variables being uninitialized.
Change-Id: Ia3ac9397d523fe299c50a95dc82a9b26304cea96
Reviewed-on: https://boringssl-review.googlesource.com/5765
Reviewed-by: Adam Langley <agl@google.com>
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>
They get initialized in SSL_new and SSL_CTX_new, respectively.
Change-Id: Ib484108987a99f654d1a77fc473103f5cb393bd7
Reviewed-on: https://boringssl-review.googlesource.com/5676
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 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>
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>
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>
These are all masks of some sort (except id which is a combined version and
cipher), so they should use fixed-size unsigned integers.
Change-Id: I058dd8ad231ee747df4b4fb17d9c1e2cbee21918
Reviewed-on: https://boringssl-review.googlesource.com/4283
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>
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>
These were added in upstream's 7e159e0133d28bec9148446e8f4dd86c0216d819 for
SCTP. As far as I can tell, they were a no-op there too. The corresponding RT
ticket makes no mention of them.
SSL_get_error checks the retry flags of the BIO already. Specifically it checks
BIO_should_read and BIO_should_write, but those two automatically set
BIO_should_retry.
(Minor, but I noticed them idly. One less thing to think about when the state
machines finally unify.)
Change-Id: I17a956a51895fba383063dee574e0fbe3209f9b0
Reviewed-on: https://boringssl-review.googlesource.com/3560
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>
It dates to 2000 from upstream and is only used when serving client auth to
Netscape. It will also get in the way when we get to merging DTLS and TLS
handshake functions because NETSCAPE_HANG_BUG is not valid for DTLS as it is
(the handshake fragmentation code will get confused).
Removing per comment on https://boringssl-review.googlesource.com/#/c/2602/
Change-Id: Ia2d086205bbfed002dc33b2203a47206f373b820
Reviewed-on: https://boringssl-review.googlesource.com/3214
Reviewed-by: Adam Langley <agl@google.com>
This CL removes the last of the EVP_CIPHER codepath in ssl/. The dead code is
intentionally not pruned for ease of review, except in DTLS-only code where
adding new logic to support both, only to remove half, would be cumbersome.
Fixes made:
- dtls1_retransmit_state is taught to retain aead_write_ctx rather than
enc_write_ctx.
- d1_pkt.c reserves space for the variable-length nonce when echoed into the
packet.
- dtls1_do_write sizes the MTU based on EVP_AEAD max overhead.
- tls1_change_cipher_state_cipher should not free AEAD write contexts in DTLS.
This matches the (rather confused) ownership for the EVP_CIPHER contexts.
I've added a TODO to resolve this craziness.
A follow-up CL will remove all the resultant dead code.
Change-Id: I644557f4db53bbfb182950823ab96d5e4c908866
Reviewed-on: https://boringssl-review.googlesource.com/2699
Reviewed-by: Adam Langley <agl@google.com>
This avoids needing a should_add_to_finished_hash boolean on do_write. The
logic in do_write was a little awkward because do_write would be called
multiple times if the write took several iterations. This also gets complex if
DTLS retransmits are involved. (At a glance, it's not obvious the
BIO_CTRL_DGRAM_MTU_EXCEEDED case actually works.)
Doing it as the handshake message is being prepared avoids this concern. It
also gives a natural point for the extended master secret logic which needs to
do work after the finished hash has been sampled.
As a bonus, we can remove s->d1->retransmitting which was only used to deal
with this issue.
Change-Id: Ifedf23ee4a6c5e08f960d296a6eb1f337a16dc7a
Reviewed-on: https://boringssl-review.googlesource.com/2604
Reviewed-by: Adam Langley <agl@google.com>
Some of the messages did the computation manually which would bite us if we
tried to transplant them between DTLS and TLS. More importantly, it precludes
moving the handshake hash computation from ssl_do_write to
ssl_set_handshake_header.
Change-Id: I9d400deb0720e62cb1ab905242eb0679ad650a46
Reviewed-on: https://boringssl-review.googlesource.com/2600
Reviewed-by: Adam Langley <agl@google.com>
SSL3_ENC_METHOD will remain version-specific while SSL_METHOD will become
protocol-specific. This finally removes all the version-specific portions of
SSL_METHOD but the version tag itself.
(SSL3_ENC_METHOD's version-specific bits themselves can probably be handled by
tracking a canonicalized protocol version. It would simplify version
comparisons anyway. The one catch is SSLv3 has a very different table. But
that's a cleanup for future. Then again, perhaps a version-specific method
table swap somewhere will be useful later for TLS 1.3.)
Much of this commit was generated with sed invocation:
s/method->ssl3_enc/enc_method/g
Change-Id: I2b192507876aadd4f9310240687e562e56e6c0b1
Reviewed-on: https://boringssl-review.googlesource.com/2581
Reviewed-by: Adam Langley <agl@google.com>
These may as well be replaced with assertions. Get them out of the way of the
initialization.
Change-Id: Ie4ab8bdc018e4a1def7d3f6b3b172a77896bfc0a
Reviewed-on: https://boringssl-review.googlesource.com/2563
Reviewed-by: Adam Langley <agl@google.com>
It just inserts extra flushes everywhere and isn't used.
Change-Id: I082e4bada405611f4986ba852dd5575265854036
Reviewed-on: https://boringssl-review.googlesource.com/2456
Reviewed-by: Adam Langley <agl@google.com>
It should be set correctly prior to entering the handshake. Don't mask bugs by
assigning it.
Change-Id: Ib9bca8fad68916b3b242aad8819e3760e59e777a
Reviewed-on: https://boringssl-review.googlesource.com/2443
Reviewed-by: Adam Langley <agl@google.com>
Supporting both schemes seems pointless. Now that s->server and s->state are
set appropriately late and get_ssl_method is gone, the only difference is that
the client/server ones have non-functional ssl_accept or ssl_connect hooks. We
can't lose the generic ones, so let's unify on that.
Note: this means a static linker will no longer drop the client or server
handshake code if unused by a consumer linking statically. However, Chromium
needs the server half anyway for DTLS and WebRTC, so that's probably a lost
cause. Android also exposes server APIs.
Change-Id: I290f5fb4ed558f59fadb5d1f84e9d9c405004c23
Reviewed-on: https://boringssl-review.googlesource.com/2440
Reviewed-by: Adam Langley <agl@google.com>
If the state is SSL_ST_BEFORE, the SSL* was just initialized. Otherwise, we
don't want to call SSL_clear. The one case I found where we do is if a
handshake message is received and someone sets
SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS. This is apparently intended for external
consumers to set, but I see no code in Google that does.
Which is fortunate because it'll trigger SSL_clear. This retains the BIOs but
drops all connection state, including the record. If the client just initiated
renego, that's the ClientHello that's lost. The connection then hangs: the now
reset SSL* wants a ClientHello (under the null cipher because that too's been
dropped) while the peer wants an encrypted ServerHello.
Change-Id: Iddb3e0bb86d39d98155b060f9273a0856f2d1409
Reviewed-on: https://boringssl-review.googlesource.com/2436
Reviewed-by: Adam Langley <agl@google.com>
SSL_clear sets s->state and dtls1_clear sets cookie_len on the server. Setting
cookie_len on the server seems to serve no purpose but to let the callback know
how large the buffer is. This can be done just before calling the callback.
It also avoids a bug where the cookie check can be bypassed, should the server
not specify an app_verify_cookie_cb, by supplying a cookie of all zeros of the
maximum size. (Zero is fine because an empty cookie is rejected.)
The goal here is to avoid needing the SSL_clear calls in the handshake
functions. They are currently needed to fix the cookie_len setting when using
the generic method. (They get set wrong and then flipped back.)
Change-Id: I5095891bc0f7df62d83a9c84312fcf0b84826faa
Reviewed-on: https://boringssl-review.googlesource.com/2435
Reviewed-by: Adam Langley <agl@google.com>
SSL_ST_BEFORE is never standalone. As of upstream's
413c4f45ed0508d2242638696b7665f499d68265, SSL_ST_BEFORE is only ever set paired
with SSL_ST_ACCEPT or SSL_ST_CONNECT.
Conversely, SSL_ST_OK is never paired with SSL_ST_ACCEPT or SSL_ST_CONNECT. As
far as I can tell, this combination has never been possible.
Change-Id: Ifbc8f147be821026cf59f3d5038f0dbad3b0a1d2
Reviewed-on: https://boringssl-review.googlesource.com/2433
Reviewed-by: Adam Langley <agl@google.com>
It should already be assigned, as of upstream's
b31b04d951e9b65bde29657e1ae057b76f0f0a73. I believe these assignments are part
of the reason it used to appear to work. Replace them with assertions. So the
assertions are actually valid, check in SSL_connect / SSL_accept that they are
never called if the socket had been placed in the opposite state. (Or we'd be
in another place where it would have appeared to work with the handshake
functions fixing things afterwards.)
Now the only places handshake_func is set are in SSL_set_{connect,accept}_state
and the method switches.
Change-Id: Ib249212bf4aa889b94c35965a62ca06bdbcf52e1
Reviewed-on: https://boringssl-review.googlesource.com/2432
Reviewed-by: Adam Langley <agl@google.com>
This is a bit of cleanup that probably should have been done at the same time
as 30ddb434bf.
For now, version negotiation is implemented with a method swap. It also
performs this swap on SSL_set_session, but this was neutered in
30ddb434bf. Rather than hackishly neuter it,
remove it outright. In addition, remove SSL_set_ssl_method. Now all method
swaps are internal: SSLv23_method switch to a version-specific method and
SSL_clear undoing it.
Note that this does change behavior: if an SSL* is created with one
version-specific method and we SSL_set_session to a session from a /different/
version, we would switch to the /other/ version-specific method. This is
extremely confusing, so it's unlikely anyone was actually expecting it.
Version-specific methods in general don't work well.
Change-Id: I72a5c1f321ca9aeb1b52ebe0317072950ba25092
Reviewed-on: https://boringssl-review.googlesource.com/2390
Reviewed-by: Adam Langley <agl@google.com>
This commit fixes a number of crashes caused by malloc failures. They
were found using the -malloc-test=0 option to runner.go which runs tests
many times, causing a different allocation call to fail in each case.
(This test only works on Linux and only looks for crashes caused by
allocation failures, not memory leaks or other errors.)
This is not the complete set of crashes! More can be found by collecting
core dumps from running with -malloc-test=0.
Change-Id: Ia61d19f51e373bccb7bc604642c51e043a74bd83
Reviewed-on: https://boringssl-review.googlesource.com/2320
Reviewed-by: Adam Langley <agl@google.com>
