Decrypting is very easy to do in-place, but encrypting in-place is a hassle.
The rules actually were wrong due to record-splitting. The aliasing prefix and
the alignment prefix actually differ by 1. Take it out for now in preparation
for tightening the aliasing rules.
If we decide to do in-place encrypt later, probably it'd be more useful to
return header + in-place ciphertext + trailer. (That, in turn, needs a
scatter/gather thing on the AEAD thanks to TLS 1.3's padding and record type
construction.) We may also wish to rethink how record-splitting works here.
Change-Id: I0187d39c541e76ef933b7c2c193323164fd8a156
Reviewed-on: https://boringssl-review.googlesource.com/8230
Reviewed-by: Adam Langley <agl@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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
This was dropped in d27441a9cb due to lack
of use, but node.js now needs it.
Change-Id: I1e207d4b46fc746cfae309a0ea7bbbc04ea785e8
Reviewed-on: https://boringssl-review.googlesource.com/7270
Reviewed-by: David Benjamin <davidben@google.com>
For TLS, this machinery only exists to swallow no_certificate alerts
which only get sent in an SSL 3.0 codepath anyway. It's much less a
no-op for SSL 3.0 which, strictly speaking, has only a subset of TLS's
alerts.
This gets messy around version negotiation because of the complex
relationship between enc_method, have_version, and version which all get
set at different times. Given that SSL 3.0 is nearly dead and all these
alerts are fatal to the connection anyway, this doesn't seem worth
carrying around. (It doesn't work very well anyway. An SSLv3-only server
may still send a record_overflow alert before version negotiation.)
This removes the last place enc_method is accessed prior to version
negotiation.
Change-Id: I79a704259fca69e4df76bd5a6846c9373f46f5a9
Reviewed-on: https://boringssl-review.googlesource.com/6843
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>
I got that from the TLS 1.3 draft, but it's kind of silly-looking. X25519
already refers to a Diffie-Hellman primitive.
Also hopefully the WG will split NamedGroups and SignatureAlgorithms per the
recent proposal, so it won't be needed anyway. (Most chatter is about what
hashes should be allowed with what NIST curves, so it seems like people like
the split itself? We'll see.)
Change-Id: I7bb713190001199a3ebd30b67df2c00d29132431
Reviewed-on: https://boringssl-review.googlesource.com/6912
Reviewed-by: Adam Langley <agl@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>
The various SSL3_ENC_METHODs ought to be defined in the same file their
functions are defined in, so they can be static.
Change-Id: I34a1d3437e8e61d4d50f2be70312e4630ea89c19
Reviewed-on: https://boringssl-review.googlesource.com/6840
Reviewed-by: Adam Langley <alangley@gmail.com>
This is a companion to SSL_get_rc4_state and SSL_get_ivs which doesn't
require poking at internal state. Partly since it aligns with the
current code and partly the off chance we ever need to get
wpa_supplicant's EAP-FAST code working, the API allows one to generate
more key material than is actually in the key block.
Change-Id: I58bc3f2b017482dbb8567dcd0cd754947a95397f
Reviewed-on: https://boringssl-review.googlesource.com/6839
Reviewed-by: Adam Langley <alangley@gmail.com>
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>
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>
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>
Move the actual SSL_AEAD_CTX swap into the record layer. Also revise the
intermediate state we store between setup_key_block and
change_cipher_state. With SSL_AEAD_CTX_new abstracted out, keeping the
EVP_AEAD around doesn't make much sense. Just store enough to partition
the key block.
Change-Id: I773fb46a2cb78fa570f00c0a89339c15bbb1d719
Reviewed-on: https://boringssl-review.googlesource.com/6832
Reviewed-by: Adam Langley <alangley@gmail.com>
This is a minor regression from
https://boringssl-review.googlesource.com/5235.
If the client, for whatever reason, had an ID-based session but also
supports tickets, it will send non-empty ID + empty ticket extension.
If the ticket extension is non-empty, then the ID is not an ID but a
dummy signaling value, so 5235 avoided looking it up. But if it is
present and empty, the ID is still an ID and should be looked up.
This shouldn't have any practical consequences, except if a server
switched from not supporting tickets and then started supporting it,
while keeping the session cache fixed.
Add a test for this case, and tighten up existing ID vs ticket tests so
they fail if we resume with the wrong type.
Change-Id: Id4d08cd809af00af30a2b67fe3a971078e404c75
Reviewed-on: https://boringssl-review.googlesource.com/6554
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 unifies the ClientKeyExchange code rather nicely. ServerKeyExchange
is still pretty specialized. For simplicity, I've extended the yaSSL bug
workaround for clients as well as servers rather than route in a
boolean.
Chrome's already banished DHE to a fallback with intention to remove
altogether later, and the spec doesn't say anything useful about
ClientDiffieHellmanPublic encoding, so this is unlikely to cause
problems.
Change-Id: I0355cd1fd0fab5729e8812e4427dd689124f53a2
Reviewed-on: https://boringssl-review.googlesource.com/6784
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>
clang-format keeps getting annoyed at it. Also remove some long-dead
constants.
Change-Id: I61e773f5be1e60ca28f1ea085e3afa7cb2c97b9e
Reviewed-on: https://boringssl-review.googlesource.com/6778
Reviewed-by: Adam Langley <agl@google.com>
In doing so, make the asynchronous portion look more like
ssl3_send_server_key_exchange. This is a considerably simpler structure,
so the save/resume doesn't need any state.
Mostly this means writing out the signature algorithm can now go through
CBB rather than a uint8_t* without bounds check.
Change-Id: If99fcffd0d41a84514c3d23034062c582f1bccb2
Reviewed-on: https://boringssl-review.googlesource.com/6771
Reviewed-by: Adam Langley <agl@google.com>
There is some messiness around saving and restoring the CBB, but this is
still significantly clearer.
Note that the BUF_MEM_grow line is gone in favor of a fixed CBB like the
other functions ported thus far. This line was never necessary as
init_buf is initialized to 16k and none of our key exchanges get that
large. (The largest one can get is DHE_RSA. Even so, it'd take a roughly
30k-bit DH group with a 30k-bit RSA key.)
Having such limits and tight assumptions on init_buf's initial size is
poor (but on par for the old code which usually just blindly assumed the
message would not get too large) and the size of the certificate chain
is much less obviously bounded, so those BUF_MEM_grows can't easily go.
My current plan is convert everything but those which legitimately need
BUF_MEM_grow to CBB, then atomically convert the rest, remove init_buf,
and switch everything to non-fixed CBBs. This will hopefully also
simplify async resumption. In the meantime, having a story for
resumption means the future atomic change is smaller and, more
importantly, relieves some complexity budget in the ServerKeyExchange
code for adding Curve25519.
Change-Id: I1de6af9856caaed353453d92a502ba461a938fbd
Reviewed-on: https://boringssl-review.googlesource.com/6770
Reviewed-by: Adam Langley <agl@google.com>
This relieves some complexity budget for adding Curve25519 to this
code.
This also adds a BN_bn2cbb_padded helper function since this seems to be a
fairly common need.
Change-Id: Ied0066fdaec9d02659abd6eb1a13f33502c9e198
Reviewed-on: https://boringssl-review.googlesource.com/6767
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 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>
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>
