Chromium no longer uses it.
Change-Id: I50cc55bad4124305686d299032a2e8ed2cb9d0d7
Reviewed-on: https://boringssl-review.googlesource.com/8691
Reviewed-by: David Benjamin <davidben@google.com>
Upstream added this in a18a31e49d266. The various *_up_ref functions
return a variety of types, but this one returns int because upstream
appears to be trying to unify around that. (See upstream's c5ebfcab713.)
Change-Id: I7e1cfe78c3a32f5a85b1b3c14428bd91548aba6d
Reviewed-on: https://boringssl-review.googlesource.com/8581
Reviewed-by: Adam Langley <alangley@gmail.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>
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>
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>
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>
Saves us some mess if they're never zero. This also fixes a bug in
ssl3_get_max_client_version where it didn't account for all versions being
disabled properly.
Change-Id: I4c95ff57cf8953cb4a528263b252379f252f3e01
Reviewed-on: https://boringssl-review.googlesource.com/8512
Reviewed-by: David Benjamin <davidben@google.com>
That both exist with nearly the same name is unfortunate. This also does away
with cert_req being unnecessarily tri-state.
Change-Id: Id83e13d0249b80700d9258b363d43b15d22898d8
Reviewed-on: https://boringssl-review.googlesource.com/8247
Reviewed-by: Adam Langley <agl@google.com>
SSL_set_bio has some rather complex ownership story because whether rbio/wbio
are both owning depends on whether they are equal. Moreover, whether
SSL_set_bio(ssl, rbio, wbio) frees ssl->rbio depends on whether rbio is the
existing rbio or not. The current logic doesn't even get it right; see tests.
Simplify this. First, rbio and wbio are always owning. All the weird ownership
cases which we're stuck with for compatibility will live in SSL_set_bio. It
will internally BIO_up_ref if necessary and appropriately no-op the left or
right side as needed. It will then call more well-behaved ssl_set_rbio or
ssl_set_wbio functions as necessary.
Change-Id: I6b4b34e23ed01561a8c0aead8bb905363ee413bb
Reviewed-on: https://boringssl-review.googlesource.com/8240
Reviewed-by: Adam Langley <agl@google.com>
This callback is used by BoringSSL tests in order to simulate the time,
so that the tests have repeatable results. This API will allow consumers
of BoringSSL to write the same sort of tests.
Change-Id: I79d72bce5510bbd83c307915cd2cc937579ce948
Reviewed-on: https://boringssl-review.googlesource.com/8200
Reviewed-by: David Benjamin <davidben@google.com>
The separation is purely historical (what happened to use an SSL_ctrl hook), so
put them all in one place. Make a vague attempt to match the order of the
header file, though we're still very far from matching.
Change-Id: Iba003ff4a06684a6be342e438d34bc92cab1cd14
Reviewed-on: https://boringssl-review.googlesource.com/8189
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@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>
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>
We've got it in entry points. That should be sufficient. (Do we even need it
there?)
Change-Id: I39b245a08fcde7b57e61b0bfc595c6ff4ce2a07a
Reviewed-on: https://boringssl-review.googlesource.com/8127
Reviewed-by: David Benjamin <davidben@google.com>
Windows SRWLOCK requires you call different functions here. Split
them up in preparation for switching Windows from CRITICAL_SECTION.
BUG=37
Change-Id: I7b5c6a98eab9ae5bb0734b805cfa1ff334918f35
Reviewed-on: https://boringssl-review.googlesource.com/8080
Reviewed-by: Adam Langley <agl@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>
This reverts commit c7eae5a326. pyOpenSSL
expects to be able to call |SSL_read| after a shutdown and get EOF.
Change-Id: Icc5faa09d644ec29aac99b181dac0db197f283e3
Reviewed-on: https://boringssl-review.googlesource.com/8060
Reviewed-by: Adam Langley <agl@google.com>
Constants representing TLS 1.3 are added to allow for future work to be
flagged on TLS1_3_VERSION. To prevent BoringSSL from negotiating the
non-existent TLS 1.3 version, it is explicitly disabled using
SSL_OP_NO_TLSv1_3.
Change-Id: Ie5258a916f4c19ef21646c4073d5b4a7974d6f3f
Reviewed-on: https://boringssl-review.googlesource.com/8041
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's bbio logic is kind of crazy. It would be good to eventually do the
buffering in a better way (notably, bbio is fragile, if not outright broken,
for DTLS). In the meantime, this fixes a number of bugs where the existence of
bbio was leaked in the public API and broke things.
- SSL_get_wbio returned the bbio during the handshake. It must always return
the BIO the consumer configured. In doing so, internal accesses of
SSL_get_wbio should be switched to ssl->wbio since those want to see bbio.
For consistency, do the same with rbio.
- The logic in SSL_set_rfd, etc. (which I doubt is quite right since
SSL_set_bio's lifetime is unclear) would get confused once wbio got wrapped.
Those want to compare to SSL_get_wbio.
- If SSL_set_bio was called mid-handshake, bbio would get disconnected and lose
state. It forgets to reattach the bbio afterwards. Unfortunately, Conscrypt
does this a lot. It just never ended up calling it at a point where the bbio
would cause problems.
- Make more explicit the invariant that any bbio's which exist are always
attached. Simplify a few things as part of that.
Change-Id: Ia02d6bdfb9aeb1e3021a8f82dcbd0629f5c7fb8d
Reviewed-on: https://boringssl-review.googlesource.com/8023
Reviewed-by: Kenny Root <kroot@google.com>
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>
This explicitly forbids an API pattern which formerly kind of worked, but was
extremely buggy (see preceding commits). Depending on how one interprets
close_notify and our API, one might wish to call SSL_shutdown only once
(morally shutdown(SHUT_WR)) and then SSL_read until EOF.
However, this exposes additional confusing states where we might try to send an
alert post-SHUT_WR, etc. Early commits made us more robust here (whether one is
allowed to touch the SSL* after an operattion failed because it read an alert
is... unclear), so we could support it if we wanted to, but this doesn't seem
worth the additional statespace. See if we can get away with not allowing it.
Change-Id: Ie7a7e5520b464360b1e6316c34ec9854b571782f
Reviewed-on: https://boringssl-review.googlesource.com/7433
Reviewed-by: David Benjamin <davidben@google.com>
The existing logic gets confused in a number of cases around close_notify vs.
fatal alert. SSL_shutdown, while still pushing to the error queue, will fail to
notice alerts. We also get confused if we try to send a fatal alert when we've
already sent something else.
Change-Id: I9b1d217fbf1ee8a9c59efbebba60165b7de9689e
Reviewed-on: https://boringssl-review.googlesource.com/7952
Reviewed-by: David Benjamin <davidben@google.com>
SSL_RECEIVED_SHUTDOWN checks in the record layer happen in two different
places. Some operations (but not all) check it, and so does read_bytes. Move it
to get_record.
This check should be at a low-level since it is otherwise duplicated in every
operation. It is also a signal which originates from around the peer's record
layer, so it makes sense to check it near the same code. (This one's in
get_record which is technically lower-level than read_bytes, but we're trying
to get rid of read_bytes. They're very coupled functions.)
Also, if we've seen a fatal alert, replay an error, not an EOF.
Change-Id: Idec35c5068ddabe5b1a9145016d8f945da2421cf
Reviewed-on: https://boringssl-review.googlesource.com/7436
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>
The logic is a little hairy, partly because we used to support multiple
certificate slots.
Change-Id: Iee8503e61f5e0e91b7bcb15f526e9ef7cc7ad860
Reviewed-on: https://boringssl-review.googlesource.com/7823
Reviewed-by: David Benjamin <davidben@google.com>
We reset it to SSL_NOTHING at the start of ever SSL_get_error-using operation.
Then we only set it to a non-NOTHING value in the rest of the stack on error
paths.
Currently, ssl->rwstate is set all over the place. Sometimes the pattern is:
ssl->rwstate = SSL_WRITING;
if (BIO_write(...) <= 0) {
goto err;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we only set it to the non-NOTHING value on error.
if (BIO_write(...) <= 0) {
ssl->rwstate = SSL_WRITING;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we just set it to SSL_NOTHING far from any callback in random places.
The third case is arbitrary and clearly should be removed.
But, in the second case, we sometimes forget to undo it afterwards. This is
largely harmless since an error in the error queue overrides rwstate, but we
don't always put something in the error queue (falling back to
SSL_ERROR_SYSCALL for "I'm not sure why it failed. Perhaps it was one of your
callbacks? Check your errno equivalent."), but in that case a stray rwstate
value will cause it to be wrong.
We could fix the cases where we fail to set SSL_NOTHING on success cases, but
this doesn't account for there being multiple SSL_get_error operations. The
consumer may have an SSL_read and an SSL_write running concurrently. Instead,
it seems the best option is to lift the SSL_NOTHING reset to the operations and
set SSL_WRITING and friends as in the second case.
(Someday hopefully we can fix this to just be an enum that is internally
returned. It can convert to something stateful at the API layer.)
Change-Id: I54665ec066a64eb0e48a06e2fcd0d2681a42df7f
Reviewed-on: https://boringssl-review.googlesource.com/7453
Reviewed-by: David Benjamin <davidben@google.com>
A lot of consumers of obj.h only want the NID values. Others didn't need
it at all. This also removes some OBJ_nid2sn and OBJ_nid2ln calls in EVP
error paths which isn't worth pulling a large table in for.
BUG=chromium:499653
Change-Id: Id6dff578f993012e35b740a13b8e4f9c2edc0744
Reviewed-on: https://boringssl-review.googlesource.com/7563
Reviewed-by: David Benjamin <davidben@google.com>
This removes the final use of in_handshake. Note that there is still a
rentrant call of read_bytes -> handshake_func when we see a
HelloRequest. That will need to be signaled up to ssl_read_impl
separately out of read_app_data.
Change-Id: I823de243f75e6b73eb40c6cf44157b4fc21eb8fb
Reviewed-on: https://boringssl-review.googlesource.com/7439
Reviewed-by: David Benjamin <davidben@google.com>
This removes one use of in_handshake and consolidates some DTLS and TLS
code.
Change-Id: Ibbdd38360a983dabfb7b18c7bd59cb5e316b2adb
Reviewed-on: https://boringssl-review.googlesource.com/7435
Reviewed-by: David Benjamin <davidben@google.com>
fatal_alert isn't read at all right now, and warn_alert is only checked
for close_notify. We only need three states:
- Not shutdown.
- Got a fatal alert (don't care which).
- Got a warning close_notify.
Leave ssl->shutdown alone for now as it's tied up with SSL_set_shutdown
and friends. To distinguish the remaining two, we only need a boolean.
Change-Id: I5877723af82b76965c75cefd67ec1f981242281b
Reviewed-on: https://boringssl-review.googlesource.com/7434
Reviewed-by: David Benjamin <davidben@google.com>
Most code already dereferences it directly.
Change-Id: I227fa91ecbf25a19077f7cfba21b0abd2bc2bd1d
Reviewed-on: https://boringssl-review.googlesource.com/7422
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I6267c9bfb66940d0b6fe5368514210a058ebd3cc
Reviewed-on: https://boringssl-review.googlesource.com/7494
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL historically made some poor API decisions. Rather than returning a
status enum in SSL_read, etc., these functions must be paired with
SSL_get_error which determines the cause of the last error's failure. This
requires SSL_read communicate with SSL_get_error with some stateful flag,
rwstate.
Further, probably as workarounds for bugs elsewhere, SSL_get_error does not
trust rwstate. Among other quirks, if the error queue is non-empty,
SSL_get_error overrides rwstate and returns a value based on that. This
requires that SSL_read, etc., be called with an empty error queue. (Or we hit
one of the spurious ERR_clear_error calls in the handshake state machine,
likely added as further self-workarounds.)
Since requiring callers consistently clear the error queue everywhere is
unreasonable (crbug.com/567501), clear ERR_clear_error *once* at the entry
point. Until/unless[*] we make SSL_get_error sane, this is the most reasonable
way to get to the point that clearing the error queue on error is optional.
With those in place, the calls in the handshake state machine are no longer
needed. (I suspect all the ERR_clear_system_error calls can also go, but I'll
investigate and think about that separately.)
[*] I'm not even sure it's possible anymore, thanks to the possibility of
BIO_write pushing to the error queue.
BUG=567501,593963
Change-Id: I564ace199e5a4a74b2554ad3335e99cd17120741
Reviewed-on: https://boringssl-review.googlesource.com/7455
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In OpenSSL, they create socket BIOs. The distinction isn't important on UNIX.
On Windows, file descriptors are provided by the C runtime, while sockets must
use separate recv and send APIs. Document how these APIs are intended to work.
Also add a TODO to resolve the SOCKET vs int thing. This code assumes that
Windows HANDLEs only use the bottom 32 bits of precision. (Which is currently
true and probably will continue to be true for the foreseeable future[*], but
it'd be nice to do this right.)
Thanks to Gisle Vanem and Daniel Stenberg for reporting the bug.
[*] Both so Windows can continue to run 32-bit programs and because of all the
random UNIX software, like OpenSSL and ourselves, out there which happily
assumes sockets are ints.
Change-Id: I67408c218572228cb1a7d269892513cda4261c82
Reviewed-on: https://boringssl-review.googlesource.com/7333
Reviewed-by: David Benjamin <davidben@google.com>
Node.js calls it but handles it failing. Since we have abstracted this
in the state machine, we mightn't even be using a cipher suite where the
server's key can be expressed as an EVP_PKEY.
Change-Id: Ic3f013dc9bcd7170a9eb2c7535378d478b985849
Reviewed-on: https://boringssl-review.googlesource.com/7272
Reviewed-by: David Benjamin <davidben@google.com>
Sending close_notify during init causes some problems for some
applications so we instead revert to the previous behavior returning an
error instead of silently passing.
(Imported from upstream's 64193c8218540499984cd63cda41f3cd491f3f59)
Change-Id: I5efed1ce152197d291e6c7ece6e5dbb8f3ad867d
Reviewed-on: https://boringssl-review.googlesource.com/7232
Reviewed-by: David Benjamin <davidben@google.com>
This stub returns an empty string rather than NULL (since some callers
might assume that NULL means there are no shared ciphers).
Change-Id: I9537fa0a80c76559b293d8518599b68fd9977dd8
Reviewed-on: https://boringssl-review.googlesource.com/7196
Reviewed-by: David Benjamin <davidben@google.com>
Calling SSL_shutdown while in init previously gave a "1" response,
meaning everything was successfully closed down (even though it
wasn't). Better is to send our close_notify, but fail when trying to
receive one.
The problem with doing a shutdown while in the middle of a handshake
is that once our close_notify is sent we shouldn't really do anything
else (including process handshake/CCS messages) until we've received a
close_notify back from the peer. However the peer might send a CCS
before acting on our close_notify - so we won't be able to read it
because we're not acting on CCS messages!
(Imported from upstream's f73c737c7ac908c5d6407c419769123392a3b0a9)
Change-Id: Iaad5c5e38983456d3697c955522a89919628024b
Reviewed-on: https://boringssl-review.googlesource.com/7207
Reviewed-by: David Benjamin <davidben@google.com>
While it's always safe to read |SSL_MAX_SSL_SESSION_ID_LENGTH| bytes
from an |SSL_SESSION|'s |session_id| array, the hash function would do
so with without considering if all those bytes had been written to.
This change checks |session_id_length| before possibly reading
uninitialised memory. Since the result of the hash function was already
attacker controlled, and since a lookup of a short session ID will
always fail, it doesn't appear that this is anything more than a clean
up.
BUG=586800
Change-Id: I5f59f245b51477d6d4fa2cdc20d40bb6b4a3eae7
Reviewed-on: https://boringssl-review.googlesource.com/7150
Reviewed-by: David Benjamin <davidben@google.com>
I switched up the endianness. Add some tests to make sure those work right.
Also tweak the DTLS semantics. SSL_get_read_sequence should return the highest
sequence number received so far. Include the epoch number in both so we don't
need a second API for it.
Change-Id: I9901a1665b41224c46fadb7ce0b0881dcb466bcc
Reviewed-on: https://boringssl-review.googlesource.com/7141
Reviewed-by: Adam Langley <agl@google.com>
C has implicit conversion of |void *| to other pointer types so these
casts are unnecessary. Clean them up to make the code easier to read
and to make it easier to find dangerous casts.
Change-Id: I26988a672e8ed4d69c75cfbb284413999b475464
Reviewed-on: https://boringssl-review.googlesource.com/7102
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL 1.1.0 doesn't seem to have these two, so this isn't based on anything.
Have them return uint64_t in preparation for switching the internal
representation to uint64_t so ssl_record_sequence_update can go away.
Change-Id: I21d55e9a29861c992f409ed293e0930a7aaef7a3
Reviewed-on: https://boringssl-review.googlesource.com/6941
Reviewed-by: Adam Langley <alangley@gmail.com>
We have the hook on the SSL_CTX, but it should be possible to set it without
reaching into SSL_CTX.
Change-Id: I93db070c7c944be374543442a8de3ce655a28928
Reviewed-on: https://boringssl-review.googlesource.com/6880
Reviewed-by: Adam Langley <alangley@gmail.com>
Move it into ssl->s3 so it automatically behaves correctly on SSL_clear.
ssl->version is still a mess though.
Change-Id: I17a692a04a845886ec4f8de229fa6cf99fa7e24a
Reviewed-on: https://boringssl-review.googlesource.com/6844
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>
node.js is, effectively, another bindings library. However, it's better
written than most and, with these changes, only a couple of tiny fixes
are needed in node.js. Some of these changes are a little depressing
however so we'll need to push node.js to use APIs where possible.
Changes:
∙ Support verify_recover. This is very obscure and the motivation
appears to be https://github.com/nodejs/node/issues/477 – where it's
not clear that anyone understands what it means :(
∙ Add a few, no-op #defines
∙ Add some members to |SSL_CTX| and |SSL| – node.js needs to not
reach into these structs in the future.
∙ Add EC_get_builtin_curves.
∙ Add EVP_[CIPHER|MD]_do_all_sorted – these functions are limited to
decrepit.
Change-Id: I9a3566054260d6c4db9d430beb7c46cc970a9d46
Reviewed-on: https://boringssl-review.googlesource.com/6952
Reviewed-by: Adam Langley <agl@google.com>
Conscrypt needs to, in the certificate verification callback, know the key
exchange + auth method of the current cipher suite to pass into
X509TrustManager.checkServerTrusted. Currently it reaches into the struct to
get it. Add an API for this.
Change-Id: Ib4e0a1fbf1d9ea24e0114f760b7524e1f7bafe33
Reviewed-on: https://boringssl-review.googlesource.com/6881
Reviewed-by: David Benjamin <davidben@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>
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>
wpa_supplicant needs to get at the client and server random. OpenSSL
1.1.0 added these APIs, so match their semantics.
Change-Id: I2b71ba850ac63e574c9ea79012d1d0efec5a979a
Reviewed-on: https://boringssl-review.googlesource.com/6830
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>
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>
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 callback is never used. The one caller I've ever seen is in Android
code which isn't built with BoringSSL and it was a no-op.
It also doesn't actually make much sense. A callback cannot reasonably
assume that it sees every, say, SSL_CTX created because the index may be
registered after the first SSL_CTX is created. Nor is there any point in
an EX_DATA consumer in one file knowing about an SSL_CTX created in
completely unrelated code.
Replace all the pointers with a typedef to int*. This will ensure code
which passes NULL or 0 continues to compile while breaking code which
passes an actual function.
This simplifies some object creation functions which now needn't worry
about CRYPTO_new_ex_data failing. (Also avoids bouncing on the lock, but
it's taking a read lock, so this doesn't really matter.)
BUG=391192
Change-Id: I02893883c6fa8693682075b7b130aa538a0a1437
Reviewed-on: https://boringssl-review.googlesource.com/6625
Reviewed-by: Adam Langley <agl@google.com>
Then deprecate the old functions. Thanks to upstream's
6977e8ee4a718a76351ba5275a9f0be4e530eab5 for the idea.
Change-Id: I916abd6fca2a3b2a439ec9902d9779707f7e41eb
Reviewed-on: https://boringssl-review.googlesource.com/6622
Reviewed-by: Adam Langley <agl@google.com>
It has no callers. I prepped for its removal earlier with
c05697c2c5
and then completely forgot.
Thanks to upstream's 6f78b9e824c053d062188578635c575017b587c5 for
the reminder. Quoth them:
> This only gets used to set a specific curve without actually checking
> that the peer supports it or not and can therefor result in handshake
> failures that can be avoided by selecting a different cipher.
It's also a very confusing API since it does NOT pass ownership of the
EC_KEY to the caller.
Change-Id: I6a00643b3a2d6746e9e0e228b47c2bc9694b0084
Reviewed-on: https://boringssl-review.googlesource.com/6621
Reviewed-by: Adam Langley <agl@google.com>
I don't think we're ever going to manage to enforce this, and it doesn't
seem worth the trouble. We don't support application protocols which use
renegotiation outside of the HTTP/1.1 mid-stream client auth hack.
There, it's on the server to reject legacy renegotiations.
This removes the last of SSL_OP_ALL.
Change-Id: I996fdeaabf175b6facb4f687436549c0d3bb0042
Reviewed-on: https://boringssl-review.googlesource.com/6580
Reviewed-by: Adam Langley <agl@google.com>
The keylog BIO is internally synchronized by the SSL_CTX lock, but an
application may wish to log keys from multiple SSL_CTXs. This is in
preparation for switching Chromium to use a separate SSL_CTX per profile
to more naturally split up the session caches.
It will also be useful for routing up SSLKEYLOGFILE in WebRTC. There,
each log line must be converted to an IPC up from the renderer
processes.
This will require changes in Chromium when we roll BoringSSL.
BUG=458365,webrtc:4417
Change-Id: I2945bdb4def0a9c36e751eab3d5b06c330d66b54
Reviewed-on: https://boringssl-review.googlesource.com/6514
Reviewed-by: Adam Langley <agl@google.com>
There seems to have been a merge error.
Change-Id: I72e5c2a45c148e31c90b28bedfff48f8ca6e3c8c
Reviewed-on: https://boringssl-review.googlesource.com/6455
Reviewed-by: Adam Langley <agl@google.com>
This exposes the ServerKeyExchange signature hash type used in the most recent
handshake, for histogramming on the client.
BUG=549662
Change-Id: I8a4e00ac735b1ecd2c2df824112c3a0bc62332a7
Reviewed-on: https://boringssl-review.googlesource.com/6413
Reviewed-by: Adam Langley <agl@google.com>
This function allows one to extract the current IVs from an SSL
connection. This is needed for the CBC cipher suites with implicit IVs
because, for those, the IV can't be extracted from the handshake key
material.
Change-Id: I247a1d0813b7a434b3cfc88db86d2fe8754344b6
Reviewed-on: https://boringssl-review.googlesource.com/6433
Reviewed-by: Adam Langley <agl@google.com>
This is a fairly timid, first step at trying to pack common structures a
little better.
This change reorders a couple of structures a little and turns some
variables into bit-fields. Much more can still be done.
Change-Id: Idbe0f54d66559c0ad654bf7e8dea277a771a568f
Reviewed-on: https://boringssl-review.googlesource.com/6394
Reviewed-by: Adam Langley <agl@google.com>
This change reduces unnecessary copying and makes the pre-RFC-7539
nonces 96 bits just like the AES-GCM, AES-CCM, and RFC 7539
ChaCha20-Poly1305 cipher suites. Also, all the symbols related to
the pre-RFC-7539 cipher suites now have "_OLD" appended, in
preparation for adding the RFC 7539 variants.
Change-Id: I1f85bd825b383c3134df0b6214266069ded029ae
Reviewed-on: https://boringssl-review.googlesource.com/6103
Reviewed-by: Adam Langley <alangley@gmail.com>
The internal session cache is keyed on session ID, so this is completely
useless for clients (indeed we never look it up internally). Along the way,
tidy up ssl_update_cache to be more readable. The slight behavior change is
that SSL_CTX_add_session's return code no longer controls the external
callback. It's not clear to me what that could have accomplished. (It can only
fail on allocation error. We only call it for new sessions, so the duplicate
case is impossible.)
The one thing of value the internal cache might have provided is managing the
timeout. The SSL_CTX_flush_sessions logic would flip the not_resumable bit and
cause us not to offer expired sessions (modulo SSL_CTX_flush_sessions's delay
and any discrepancies between the two caches). Instead, just check expiration
when deciding whether or not to offer a session.
This way clients that set SSL_SESS_CACHE_CLIENT blindly don't accidentally
consume gobs of memory.
BUG=531194
Change-Id: If97485beab21874f37737edc44df24e61ce23705
Reviewed-on: https://boringssl-review.googlesource.com/6321
Reviewed-by: Adam Langley <alangley@gmail.com>
A random 32-byte (so 256-bit) session ID is never going to collide with
an existing one. (And, if it does, SSL_CTX_add_session does account for
this, so the server won't explode. Just attempting to resume some
session will fail.)
That logic didn't completely work anyway as it didn't account for
external session caches or multiple connections picking the same ID in
parallel (generation and insertion happen at different times) or
multiple servers sharing one cache. In theory one could fix this by
passing in a sufficiently clever generate_session_id, but no one does
that.
I found no callers of these functions, so just remove them altogether.
Change-Id: I8500c592cf4676de6d7194d611b99e9e76f150a7
Reviewed-on: https://boringssl-review.googlesource.com/6318
Reviewed-by: Adam Langley <alangley@gmail.com>
Although Chromium actually uses SSL_(get_)state as part of its fallback
reason heuristic, that function really should go in the deprecated
bucket. I kept SSL_state_string_long since having a human-readable
string is probably useful for logging.
SSL_set_SSL_CTX was only half-documented as the behavior of this
function is very weird. This warrants further investigation and
rethinking.
SSL_set_shutdown is absurd. I added an assert to trip up clearing bits
and set it to a bitwise OR since clearing bits may mess up the state
machine. Otherwise there's enough consumers and it's not quite the same
as SSL_CTX_set_quiet_shutdown that I've left it alone for now.
Change-Id: Ie35850529373a5a795f6eb04222668ff76d84aaa
Reviewed-on: https://boringssl-review.googlesource.com/6312
Reviewed-by: Adam Langley <alangley@gmail.com>
SSL_in_connect_init and SSL_in_accept_init are removed as they're unused
both within the library and externally. They're also kind of silly.
Expand on how False Start works at the API level in doing so.
Change-Id: Id2a8e34b5bb8f28329e3b87b4c64d41be3f72410
Reviewed-on: https://boringssl-review.googlesource.com/6310
Reviewed-by: Adam Langley <alangley@gmail.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>
Also clean up the code slightly.
Change-Id: I066a389242c46cdc7d41b1ae9537c4b7716c92a2
Reviewed-on: https://boringssl-review.googlesource.com/6302
Reviewed-by: Adam Langley <alangley@gmail.com>
Now tls1.h is just a pile of protocol constants with no more circular
dependency problem.
I've preserved SSL_get_servername's behavior where it's simultaneously a
lookup of handshake state and local configuration. I've removed it from
SSL_get_servername_type. It got the logic wrong anyway with the order of
the s->session check.
(Searching through code, neither is used on the client, but the
SSL_get_servername one is easy.)
Change-Id: I61bb8fb0858b07d76a7835bffa6dc793812fb027
Reviewed-on: https://boringssl-review.googlesource.com/6298
Reviewed-by: Adam Langley <alangley@gmail.com>
The only reason you'd want it is to tls_unique, and we have a better API
for that. (It has one caller and that is indeed what that caller uses it
for.)
Change-Id: I39f8e353f56f18becb63dd6f7205ad31f4192bfd
Reviewed-on: https://boringssl-review.googlesource.com/6295
Reviewed-by: Adam Langley <alangley@gmail.com>
This is redundant with SSL_get_error. Neither is very good API, but
SSL_get_error is more common. SSL_get_error also takes a return code
which makes it harder to accidentally call it at some a point other than
immediately after an operation. (Any other point is confusing since you
can have SSL_read and SSL_write operations going on in parallel and
they'll get mixed up.)
Change-Id: I5818527c30daac28edb552c6c550c05c8580292d
Reviewed-on: https://boringssl-review.googlesource.com/6294
Reviewed-by: Adam Langley <alangley@gmail.com>
Also added a SSL_CTX_set_select_certificate_cb setter for
select_certificate_cb so code needn't access SSL_CTX directly. Plus it
serves as a convenient anchor for the documentation.
Change-Id: I23755b910e1d77d4bea7bb9103961181dd3c5efe
Reviewed-on: https://boringssl-review.googlesource.com/6291
Reviewed-by: Adam Langley <alangley@gmail.com>
Start converting the ones we can right now. Some of the messier ones
resize init_buf rather than assume the initial size is sufficient, so
those will probably wait until init_buf is gone and the handshake's
undergone some more invasive surgery. The async ones will also require
some thought. But some can be incrementally converted now.
BUG=468889
Change-Id: I0bc22e4dca37d9d671a488c42eba864c51933638
Reviewed-on: https://boringssl-review.googlesource.com/6190
Reviewed-by: Adam Langley <alangley@gmail.com>
Add a slightly richer API. Notably, one can configure ssl_renegotiate_once to
only accept the first renego.
Also, this API doesn't repeat the mistake I made with
SSL_set_reject_peer_renegotiations which is super-confusing with the negation.
Change-Id: I7eb5d534e3e6c553b641793f4677fe5a56451c71
Reviewed-on: https://boringssl-review.googlesource.com/6221
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's BIO_get_fd returns the fd or -1, not a boolean.
Change-Id: I12a3429c71bb9c9064f9f91329a88923025f1fb5
Reviewed-on: https://boringssl-review.googlesource.com/6080
Reviewed-by: Adam Langley <agl@google.com>
This didn't actually break anything, but it does make session lookup
quite slow.
Change-Id: I13615e8ccf6a46683a21774eb7c073318ae8c28c
Reviewed-on: https://boringssl-review.googlesource.com/6054
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The cipher suite rules could also be anchored on SSL_TXT_* if desired. I
currently documented them in prose largely because SSL_TXT_* also
defines protocol version strings and those are weird; SSL_TXT_TLSV1_1
isn't even a cipher rule. (And, in fact, those are the only SSL_TXT_*
macros that we can't blindly remove. I found some code that #ifdef's the
version SSL_TXT_* macros to decide if version-locked SSL_METHODs are
available.)
Also they clutter the header. I was thinking maybe we should dump a lot
of the random constants into a separate undocumented header or perhaps
just unexport them.
I'm slightly torn on this though and could easily be convinced in the
other direction. (Playing devil's advocate, anchoring on SSL_TXT_* means
we're less likely to forget to document one so long as adding a
SSL_TXT_* macro is the convention.)
Change-Id: Ide2ae44db9d6d8f29c24943090c210da0108dc37
Reviewed-on: https://boringssl-review.googlesource.com/5962
Reviewed-by: Adam Langley <agl@google.com>
Or at least group them together and make a passing attempt to document
them. The legacy X.509 stack itself remains largely untouched and most
of the parameters have to do with it.
Change-Id: I9e11e2ad1bbeef53478c787344398c0d8d1b3876
Reviewed-on: https://boringssl-review.googlesource.com/5942
Reviewed-by: Adam Langley <agl@google.com>
Existing documentation was moved to the header, very slightly tweaked.
Change-Id: Ife3c2351e2d7e6a335854284f996918039414446
Reviewed-on: https://boringssl-review.googlesource.com/5897
Reviewed-by: Adam Langley <agl@google.com>
These were already documented, though some of the documentation was
expanded on slightly.
Change-Id: I04c6276a83a64a03ab9cce9b9c94d4dea9ddf638
Reviewed-on: https://boringssl-review.googlesource.com/5896
Reviewed-by: Adam Langley <agl@google.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>
Forgot to fix these when I fixed the headers.
Change-Id: Ie45e624abc993e16e2d5a872ef00dba9029a38df
Reviewed-on: https://boringssl-review.googlesource.com/5891
Reviewed-by: Adam Langley <agl@google.com>
Also switch to the new variable names (SSL_CTX *ctx, SSL *ssl,
SSL_SESSION *session) for all documented functions.
Change-Id: I15e15a703b96af1727601108223c7ce3b0691f1d
Reviewed-on: https://boringssl-review.googlesource.com/5882
Reviewed-by: Adam Langley <agl@google.com>
This is arguably more commonly queried connection information than the
tls-unique.
Change-Id: I1f080536153ba9f178af8e92cb43b03df37110b5
Reviewed-on: https://boringssl-review.googlesource.com/5874
Reviewed-by: Adam Langley <agl@google.com>
Just the stuff that has been pulled out into sections already.
Change-Id: I3da6bc61d79ccfe2b18d888075dc32026a656464
Reviewed-on: https://boringssl-review.googlesource.com/5873
Reviewed-by: Adam Langley <agl@google.com>
Unfortunately, these are also some of the worst APIs in the SSL stack.
I've tried to capture all the things they expose to the caller. 0 vs -1
is intentionally left unexpanded on for now. Upstream's documentation
says 0 means transport EOF, which is a nice idea but isn't true. (A lot
of random functions return 0 on error and pass it up to the caller.)
https://crbug.com/466303 tracks fixing that.
SSL_set_bio is intentionally documented to NOT be usable when they're
already configured. The function tries to behave in this case and even
with additional cases when |rbio| and/or |wbio| are unchanged, but this
is buggy. For instance, this will explode:
SSL_set_bio(ssl, bio1, bio1);
SSL_set_bio(ssl, bio2, SSL_get_wbio(ssl));
As will this, though it's less clear this is part of the API contract
due to SSL taking ownership.
SSL_set_bio(ssl, bio1, bio2);
SSL_set_bio(ssl, bio2, bio1);
It also tries to handle ssl->bbio already existing, but I doubt it quite
works. Hopefully we can drop ssl->bbio eventually. (Why is this so
complicated...)
Change-Id: I5f9f3043915bffc67e2ebd282813e04afbe076e6
Reviewed-on: https://boringssl-review.googlesource.com/5872
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifa44fef160fc9d67771eed165f8fc277f28a0222
Reviewed-on: https://boringssl-review.googlesource.com/5840
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
There's not enough in that file to really justify its own file now.
Change-Id: I6130cfce6c40fe9d46aa83dd83e6f38d87fdcf64
Reviewed-on: https://boringssl-review.googlesource.com/5823
Reviewed-by: Adam Langley <agl@google.com>
