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>
Rather than reset the timer on every message, start it up immediately after
flushing one of our flights.
Change-Id: I97f8b4f572ceff62c546c94933b2700975c50a02
Reviewed-on: https://boringssl-review.googlesource.com/8180
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
It's unreachable and wouldn't work anyway. We'd never bubble up to the caller
to retry. As a consequence, the TLS side doesn't actually need to pay attention
to init_off.
(For now anyway. We'll probably need state of this sort once the write half is
all reworked. All the craziness with wpend_buf ought to be limited to the
SSL_write bits.)
Change-Id: I951534f6bbeb547ce0492d5647aaf76be42108a3
Reviewed-on: https://boringssl-review.googlesource.com/8179
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
It can be folded into dtls1_read_app_data. This code, since it still takes an
output pointer, does not yet process records atomically. (Though, being DTLS,
it probably should...)
Change-Id: I57d60785c9c1dd13b5b2ed158a08a8f5a518db4f
Reviewed-on: https://boringssl-review.googlesource.com/8177
Reviewed-by: David Benjamin <davidben@google.com>
This was probably the worst offender of them all as read_bytes is the wrong
abstraction to begin with. Note this is a slight change in how processing a
record works. Rather than reading one fragment at a time, we process all
fragments in a record and return. The intent here is so that all records are
processed atomically since the connection eventually will not be able to retain
a buffer holding the record.
This loses a ton of (though not quite all yet) those a2b macros.
Change-Id: Ibe4bbcc33c496328de08d272457d2282c411b38b
Reviewed-on: https://boringssl-review.googlesource.com/8176
Reviewed-by: David Benjamin <davidben@google.com>
read_close_notify is a very straight-forward hook and doesn't need much.
Change-Id: I7407d842321ea1bcb47838424a0d8f7550ad71ca
Reviewed-on: https://boringssl-review.googlesource.com/8174
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The business with ssl_record_prefix_len is rather a hassle. Instead, have
tls_open_record always decrypt in-place and give back a CBS to where the body
is.
This way the caller doesn't need to do an extra check all to avoid creating an
invalid pointer and underflow in subtraction.
Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee
Reviewed-on: https://boringssl-review.googlesource.com/8173
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Alert handling is more-or-less identical across all contexts. Push it down from
read_bytes into the low-level record functions. This also deduplicates the code
shared between TLS and DTLS.
Now the only type mismatch managed by read_bytes is if we get handshake data in
read_app_data.
Change-Id: Ia8331897b304566e66d901899cfbf31d2870194e
Reviewed-on: https://boringssl-review.googlesource.com/8124
Reviewed-by: Steven Valdez <svaldez@google.com>
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>
Move this logic out of dtls1_read_bytes and into dtls1_get_record. Only trigger
it when reading from the buffer fails. The other one shouldn't be necessary.
This exists to handle the blocking BIO case when the
BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT signal triggers, so we only need to do it when
timeouts actually trigger.
There also doesn't seem to be a need for most of the machinery. The
BIO_set_flags call seems to be working around a deficiency in the underlying
BIO. There also shouldn't be a need to check the handshake state as there
wouldn't be a timer to restart otherwise.
Change-Id: Ic901ccfb5b82aeb409d16a9d32c04741410ad6d7
Reviewed-on: https://boringssl-review.googlesource.com/8122
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The two modes are quite different. One of them requires the BIO honor an
extra BIO_ctrl. Also add an explanation at the top of
addDTLSRetransmitTests for how these tests work. The description is
scattered across many different places.
BUG=63
Change-Id: Iff4cdd1fbf4f4439ae0c293f565eb6780c7c84f9
Reviewed-on: https://boringssl-review.googlesource.com/8121
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>
This cannot happen.
Change-Id: Ib1b473aa91d6479eeff43f7eaf94906d0b2c2a8f
Reviewed-on: https://boringssl-review.googlesource.com/8123
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
ssl->cert is never NULL. It gets created in SSL_new unconditionally.
Change-Id: I5c54c9c73e281e61a554820d61421226d763d33a
Reviewed-on: https://boringssl-review.googlesource.com/8125
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In TLS 1.3, the actual record type is hidden within the encrypted data
and the record layer defaults to using a TLS 1.0 {3, 1} record version
for compatibility. Additionally the record layer no longer checks the
minor version of the record layer to maintain compatibility with the
TLS 1.3 spec.
Change-Id: If2c08e48baab170c1658e0715c33929d36c9be3a
Reviewed-on: https://boringssl-review.googlesource.com/8091
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL was actually super-buggy here (though known bugs on our end have been
fixed), but pyOpenSSL was confused and incorrectly documented that callers call
SSL_read after SSL_shutdown to do bidi shutdown, so we should probably support
this. Add a test that it works.
Change-Id: I2b6d012161330aeb4cf894bae3a0b6a55d53c70d
Reviewed-on: https://boringssl-review.googlesource.com/8093
Reviewed-by: Adam Langley <agl@google.com>
This keeps the naming convention in line with the actual spec.
Change-Id: I34673f78dbc29c1659b4da0e49677ebe9b79636b
Reviewed-on: https://boringssl-review.googlesource.com/8090
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>
The TLS 1.3 spec has an explicit nonce construction for AEADs that
requires xoring the IV and sequence number.
Change-Id: I77145e12f7946ffb35ebeeb9b2947aa51058cbe9
Reviewed-on: https://boringssl-review.googlesource.com/8042
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>
This renames the Channel ID EncryptedExtensions message to allow for
compatibility with TLS 1.3 EncryptedExtensions.
Change-Id: I5b67d00d548518045554becb1b7213fba86731f2
Reviewed-on: https://boringssl-review.googlesource.com/8040
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>
GetConfigPtr was a silly name. GetTestConfig matches the type and GetTestState.
Change-Id: I9998437a7be35dbdaab6e460954acf1b95375de0
Reviewed-on: https://boringssl-review.googlesource.com/8024
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>
CECPQ1 is a new key exchange that concatenates the results of an X25519
key agreement and a NEWHOPE key agreement.
Change-Id: Ib919bdc2e1f30f28bf80c4c18f6558017ea386bb
Reviewed-on: https://boringssl-review.googlesource.com/7962
Reviewed-by: David Benjamin <davidben@google.com>
Those checks contradict an assert up in read_app_data. This is part of
shrinking read_bytes further into get_record and its callers until it goes
away. Here, this kind of policy should be controlled by the callers.
Change-Id: If8f9a45b8b95093beab1b3d4abcd31da55c65322
Reviewed-on: https://boringssl-review.googlesource.com/7954
Reviewed-by: Adam Langley <agl@google.com>
There is no good reason why this needs to be this way. Later work should make
this all use a much more appropriate design. In the meantime, leave a note here
so this does not look accidental.
Change-Id: I7599dea7a474f54e26d9ab175b0e3cada99a974d
Reviewed-on: https://boringssl-review.googlesource.com/7951
Reviewed-by: Adam Langley <agl@google.com>
This was needed because ssl3_get_message would get confused if init_num were
not set back to zero when reading the next message. However, ssl3_get_message
now treats init_num only as an output, not an input. (The message sending logic
and the individual handshake states still use it, so we can't get rid of it
altogether yet.)
I've kept the init_num reset at the start and end of the handshake loop alone
for now since that's more about initialization and cleanup. Though I believe
they too do not do anything.
Change-Id: I64bbdd82122498de32364e7edb3b00b166059ecd
Reviewed-on: https://boringssl-review.googlesource.com/7950
Reviewed-by: Adam Langley <agl@google.com>
They're completely unused now. The handshake message reassembly logic should
not depend on the state machine. This should partially free it up (ugly as it
is) to be shared with a future TLS 1.3 implementation while, in parallel, it
and the layers below, get reworked. This also cuts down on the number of states
significantly.
Partially because I expect we'd want to get ssl_hash_message_t out of there
too. Having it in common code is fine, but it needs to be in the (supposed to
be) protocol-agnostic handshake state machine, not the protocol-specific
handshake message layer.
Change-Id: I12f9dc57bf433ceead0591106ab165d352ef6ee4
Reviewed-on: https://boringssl-review.googlesource.com/7949
Reviewed-by: Adam Langley <agl@google.com>
Rather than this confusing coordination with the handshake state machine and
init_num changing meaning partway through, use the length field already in
BUF_MEM. Like the new record layer parsing, is no need to keep track of whether
we are reading the header or the body. Simply keep extending the handshake
message until it's far enough along.
ssl3_get_message still needs tons of work, but this allows us to disentangle it
from the handshake state.
Change-Id: Ic2b3e7cfe6152a7e28a04980317d3c7c396d9b08
Reviewed-on: https://boringssl-review.googlesource.com/7948
Reviewed-by: Adam Langley <agl@google.com>
On Windows, if we write to our socket and then close it, the peer sometimes
doesn't get all the data. This was working for our shimShutsDown tests because
we send close_notify in parallel with the peer and sendAlert(alertCloseNotify)
did not internally return an error.
For convenience, sendAlert returns a local error for non-close_notify alerts.
Suppress that error to avoid the race condition. This makes it behave like the
other shimShutsDown tests.
Change-Id: Iad256e3ea5223285793991e2eba9c7d61f2e3ddf
Reviewed-on: https://boringssl-review.googlesource.com/7980
Reviewed-by: Adam Langley <agl@google.com>
Previously, SSL_ECDH_METHOD consisted of two methods: one to produce a
public key to be sent to the peer, and another to produce the shared key
upon receipt of the peer's message.
This API does not work for NEWHOPE, because the client-to-server message
cannot be produced until the server's message has been received by the
client.
Solve this by introducing a new method which consumes data from the
server key exchange message and produces data for the client key
exchange message.
Change-Id: I1ed5a2bf198ca2d2ddb6d577888c1fa2008ef99a
Reviewed-on: https://boringssl-review.googlesource.com/7961
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 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>
