The logic to drop records really should be in the caller. Unless
ssl3_read_bytes is broken apart, condition on the type field which is more
robust.
If we manage to call, say, SSL_read after SSL_shutdown completes at 0 (instead
of 1), this logic can incorrectly cause unknown record types to be dropped.
Change-Id: Iab90e5d9190fcccbf6ff55e17079a2704ed99901
Reviewed-on: https://boringssl-review.googlesource.com/7953
Reviewed-by: Steven Valdez <svaldez@google.com>
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>
OpenSSL used to only forbid it on the server in plain PSK and allow it on the
client. Enforce it properly on both sides. My read of the rule in RFC 5246 ("A
non-anonymous server can optionally request a certificate") and in RFC 4279
("The Certificate and CertificateRequest payloads are omitted from the
response.") is that client auth happens iff we're certificate-based.
The line in RFC 4279 is under the plain PSK section, but that doesn't make a
whole lot of sense and there is only one diagram. PSK already authenticates
both sides. I think the most plausible interpretation is that this is for
certificate-based ciphers.
Change-Id: If195232c83f21e011e25318178bb45186de707e6
Reviewed-on: https://boringssl-review.googlesource.com/7942
Reviewed-by: David Benjamin <davidben@google.com>
A handshake message can go up to 2^24 bytes = 16MB which is a little large for
the peer to force us to buffer. Accordingly, we bound the size of a
handshake message.
Rather than have a global limit, the existing logic uses a different limit at
each state in the handshake state machine and, for certificates, allows
configuring the maximum certificate size. This is nice in that we engage larger
limits iff the relevant state is reachable from the handshake. Servers without
client auth get a tighter limit "for free".
However, this doesn't work for DTLS due to out-of-order messages and we use a
simpler scheme for DTLS. This scheme also is tricky on optional messages and
makes the handshake <-> message layer communication complex.
Apart from an ignored 20,000 byte limit on ServerHello, the largest
non-certificate limit is the common 16k limit on ClientHello. So this
complexity wasn't buying us anything. Unify everything on the DTLS scheme
except, so as not to regress bounds on client-auth-less servers, also correctly
check for whether client auth is configured. The value of 16k was chosen based
on this value.
(The 20,000 byte ServerHello limit makes no sense. We can easily bound the
ServerHello because servers may not send extensions we don't implement. But it
gets overshadowed by the certificate anyway.)
Change-Id: I00309b16d809a3c2a1543f99fd29c4163e3add81
Reviewed-on: https://boringssl-review.googlesource.com/7941
Reviewed-by: David Benjamin <davidben@google.com>
It's only used in one file.
Change-Id: I5d60cbc02799b22317f5f7593faf25eb8eea0a24
Reviewed-on: https://boringssl-review.googlesource.com/7943
Reviewed-by: David Benjamin <davidben@google.com>
The specification, sadly, did not say that servers MUST NOT send it, only that
they are "not expected to" do anything with the client extension. Accordingly,
we decided to tolerate this. Add a test for this so that we check this
behavior.
This test also ensures that the original session's value for it carries over.
Change-Id: I38c738f218a09367c9d8d1b0c4d68ab5cbec730e
Reviewed-on: https://boringssl-review.googlesource.com/7860
Reviewed-by: Adam Langley <agl@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>
When setting a new SRTP connection profile using
SSL_CTX_set_tlsext_use_srtp() or SSL_set_tlsext_use_srtp() we should
free any existing profile first to avoid a memory leak.
(Imported from upstream's fbdf0299dc98bc611d854c0a62c6ab1810d856fc.)
Change-Id: I738e711f1c23ed4a8ac97486d94c08cc0db7aea7
Reviewed-on: https://boringssl-review.googlesource.com/7910
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The SSL tests are fairly different from most test suites. Add some high-level
documentation so people know where to start.
Change-Id: Ie5ea108883dca82675571a3025b3fbc4b9d66da9
Reviewed-on: https://boringssl-review.googlesource.com/7890
Reviewed-by: Adam Langley <agl@google.com>
This is particularly questionable with ClientHello encompassing several states.
ssl->shutdown is already initialized to zero and further reset in
SSL_set_{connect,accept}_state. At any other state, if it manages to not be a
no-op, it will erase a close_notify we have sent or received, neither of which
is okay. (I don't think this is possible, but I'm not positive.)
This dates to the initial commit in OpenSSL, so git is not enlightening. The
state machine logic historically reset many fields it had no reason to reset,
so this is likely more of that.
Change-Id: Ie872316701720cb8ef2cfcb67b7f07a9fea3620f
Reviewed-on: https://boringssl-review.googlesource.com/7874
Reviewed-by: Adam Langley <agl@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>
That we're ignoring the return value is clearly wrong when
dtls1_retransmit_message has other code that doesn't ignore it, by way of
dtls1_do_handshake_write.
Change-Id: Ie3f8c0defdf1f5e709d67af4ca6fa4f0d83c76c9
Reviewed-on: https://boringssl-review.googlesource.com/7872
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>
There was only one function that required BoringSSL to know how to read
directories. Unfortunately, it does have some callers and it's not immediately
obvious whether the code is unreachable. Rather than worry about that, just
toss it all into decrepit.
In doing so, do away with the Windows and PNaCl codepaths. Only implement
OPENSSL_DIR_CTX on Linux.
Change-Id: Ie64d20254f2f632fadc3f248bbf5a8293ab2b451
Reviewed-on: https://boringssl-review.googlesource.com/7661
Reviewed-by: Adam Langley <agl@google.com>
The i2d_X509() function can return a negative value on error. Therefore
we should make sure we check it.
Issue reported by Yuan Jochen Kang.
(Imported from upstream's 8f43c80bfac15544820739bf035df946eeb603e8)
Change-Id: If247d5bf1d792eb7c6dc179b606ed21ea0ccdbb8
Reviewed-on: https://boringssl-review.googlesource.com/7743
Reviewed-by: David Benjamin <davidben@google.com>
Opaquifying SSL_SESSION is less important than the other structs, but this will
cause less turbulence in wpa_supplicant if we add this API too. Semantics and
name taken from OpenSSL 1.1.0 to match.
BUG=6
Change-Id: Ic39f58d74640fa19a60aafb434dd2c4cb43cdea9
Reviewed-on: https://boringssl-review.googlesource.com/7725
Reviewed-by: Adam Langley <agl@google.com>
Although the server_name extension was intended to be extensible to new name
types, OpenSSL 1.0.x had a bug which meant different name types will cause an
error. Further, RFC 4366 originally defined syntax inextensibly. RFC 6066
corrected this mistake, but adding new name types is no longer feasible.
Act as if the extensibility does not exist to simplify parsing. This also
aligns with OpenSSL 1.1.x's behavior. See upstream's
062178678f5374b09f00d70796f6e692e8775aca and
https://www.ietf.org/mail-archive/web/tls/current/msg19425.html
Change-Id: I5af26516e8f777ddc1dab5581ff552daf2ea59b5
Reviewed-on: https://boringssl-review.googlesource.com/7294
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>
The concern is if the peer denies our renegotiation attempt, but we will never
initiate renegotiation. We only support server-initiated renegotiation when we
are acting as the client.
(Strictly speaking, only the client ever initiates renegotiation. The server
sends a HelloRequest to ask the client to initiate it. But we forbid
application data interleave as soon as we see the HelloRequest, so we treat it
as part of the handshake.)
Change-Id: I1a625130de32a7227e4471f2f889255aba962ce4
Reviewed-on: https://boringssl-review.googlesource.com/7452
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This is just kind of a silly thing to do. NSS doesn't allow them either. Fatal
alerts would kill the connection regardless and warning alerts are useless. We
previously stopped accepting fragmented alerts but still allowed them doubled
up.
This is in preparation for pulling the shared alert processing code between TLS
and DTLS out of read_bytes into some common place.
Change-Id: Idbef04e39ad135f9601f5686d41f54531981e0cf
Reviewed-on: https://boringssl-review.googlesource.com/7451
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
With SSL_PRIVATE_KEY_METHOD, decryption can happen outside of BoringSSL. Rather than crash the process, it would be nicer if BoringSSL handled the error gracefully.
Change-Id: I3f24d066f7a329d41420b208a7e13c82ec966710
Reviewed-on: https://boringssl-review.googlesource.com/7683
Reviewed-by: David Benjamin <davidben@google.com>
There was only one function that required BoringSSL to know how to read
directories. Unfortunately, it does have some callers and it's not immediately
obvious whether the code is unreachable. Rather than worry about that, just
toss it all into decrepit.
In doing so, do away with the Windows and PNaCl codepaths. Only implement
OPENSSL_DIR_CTX on Linux.
Change-Id: I3eb55b098e3aa042b422bb7da115c0812685553e
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>
We reordered extensions some time ago to ensure a non-empty extension was last,
but the comment was since lost (or I forgot to put one in in the first place).
Add one now so we don't regress.
Change-Id: I2f6e2c3777912eb2c522a54bbbee579ee37ee58a
Reviewed-on: https://boringssl-review.googlesource.com/7570
Reviewed-by: David Benjamin <davidben@google.com>
These only affect the tests.
Change-Id: If22d047dc98023501c771787b485276ece92d4a2
Reviewed-on: https://boringssl-review.googlesource.com/7573
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We do an ad-hoc upper-bound check, but if the version is too low, we also
shouldn't offer the session. This isn't fatal to the connection and doesn't
have issues (we'll check the version later regardless), but offering a session
we're never going to accept is pointless. The check should match what we do in
ServerHello.
Credit to Matt Caswell for noticing the equivalent issue in an OpenSSL pull
request.
Change-Id: I17a4efd37afa63b34fca53f4c9b7ac3ae2fa3336
Reviewed-on: https://boringssl-review.googlesource.com/7543
Reviewed-by: David Benjamin <davidben@google.com>
The removes the last of OpenSSL's variables that count occurrences of a
function on the stack.
Change-Id: I1722c6d47bedb47b1613c4a5da01375b5c4cc220
Reviewed-on: https://boringssl-review.googlesource.com/7450
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>
Otherwise it's confusing if you mistype the test name.
Change-Id: Idf32081958f85f3b5aeb8993a07f6975c27644f8
Reviewed-on: https://boringssl-review.googlesource.com/7500
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
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>
Partially fixes build with -Wmissing-prototypes.
Change-Id: I828bcfb49b23c5a9ea403038bc3fb76750556ef8
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7514
Reviewed-by: David Benjamin <davidben@google.com>
Partially fixes build with -Wmissing-prototypes -Wmissing-declarations.
Change-Id: I6048f5b7ef31560399b25ed9880156bc7d8abac2
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/7511
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>
Also fix a long/unsigned-long cast. (ssl_get_message returns long. It really
shouldn't, but ssl_get_message needs much more work than just a long -> size_t
change, so leave it as long for now.)
Change-Id: Ice8741f62a138c0f35ca735eedb541440f57e114
Reviewed-on: https://boringssl-review.googlesource.com/7457
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Found by libFuzzer.
Change-Id: Ifa343a184cc65f71fb6591d290b2d47d24a2be80
Reviewed-on: https://boringssl-review.googlesource.com/7456
Reviewed-by: Steven Valdez <svaldez@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>
Align all unexpected messages on SSL_R_UNEXPECTED_MESSAGE. Make the SSL 3.0
case the exceptional case. In doing so, make sure the SSL 3.0
SSL_VERIFY_FAIL_IF_NO_PEER_CERT case has its own test as that's a different
handshake shape.
Change-Id: I1a539165093fbdf33e2c1b25142f058aa1a71d83
Reviewed-on: https://boringssl-review.googlesource.com/7421
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If we're doing substring matching, we should at least include the delimiter.
Change-Id: I98bee568140d0304bbb6a2788333dbfca044114c
Reviewed-on: https://boringssl-review.googlesource.com/7420
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The old logic was quite messy and grew a number of no-ops over the
years. It was also unreasonably fond of the variable name |i|.
The current logic wasn't even correct. It's overly fond of sending no
certificate, even when it pushes errors on the error queue for a fatal
error.
Change-Id: Ie5b2b38dd309f535af1d17fa261da7dc23185866
Reviewed-on: https://boringssl-review.googlesource.com/7418
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In TLS, you never skip the Certificate message. It may be empty, but its
presence is determined by CertificateRequest. (This is sensible.)
In SSL 3.0, the client omits the Certificate message. This means you need to
probe and may receive either Certificate or ClientKeyExchange (thankfully,
ClientKeyExchange is not optional, or we'd have to probe at ChangeCipherSpec).
We didn't have test coverage for this, despite some of this logic being a
little subtle asynchronously. Fix this.
Change-Id: I149490ae5506f02fa0136cb41f8fea381637bf45
Reviewed-on: https://boringssl-review.googlesource.com/7419
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also add no-certificate cases to the state machine coverage tests.
Change-Id: I88a80df6f3ea69aabc978dd356abcb9e309e156f
Reviewed-on: https://boringssl-review.googlesource.com/7417
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I5b38e2938811520f52ece6055245248c80308b4d
Reviewed-on: https://boringssl-review.googlesource.com/7416
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If a Read or Write blocks for too long, time out the operation. Otherwise, some
kinds of test failures result in hangs, which prevent the test harness from
progressing. (Notably, OpenSSL currently has a lot of those failure modes and
upstream expressed interest in being able to run the tests to completion.)
Go's APIs want you to send an absolute timeout, to avoid problems when a Read
is split into lots of little Reads. But we actively want the timer to reset in
that case, so this needs a trivial adapter.
The default timeout is set at 15 seconds for now. If this becomes a problem, we
can extend it or build a more robust deadlock detector given an out-of-band
channel (shim tells runner when it's waiting on data, abort if we're also
waiting on data at the same time). But I don't think we'll need that
complexity. 15 seconds appears fine for both valgrind and running tests on a
Nexus 4.
BUG=460189
Change-Id: I6463fd36058427d883b526044da1bbefba851785
Reviewed-on: https://boringssl-review.googlesource.com/7380
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
