Instead, each module defines a static CRYPTO_EX_DATA_CLASS to hold the values.
This makes CRYPTO_cleanup_all_ex_data a no-op as spreading the
CRYPTO_EX_DATA_CLASSes across modules (and across crypto and ssl) makes cleanup
slightly trickier. We can make it do something if needbe, but it's probably not
worth the trouble.
Change-Id: Ib6f6fd39a51d8ba88649f0fa29c66db540610c76
Reviewed-on: https://boringssl-review.googlesource.com/4375
Reviewed-by: Adam Langley <agl@google.com>
Callers are required to use the wrappers now. They still need OPENSSL_EXPORT
since crypto and ssl get built separately in the standalone shared library
build.
Change-Id: I61186964e6099b9b589c4cd45b8314dcb2210c89
Reviewed-on: https://boringssl-review.googlesource.com/4372
Reviewed-by: Adam Langley <agl@google.com>
This causes any unexpected handshake records to be met with a fatal
no_renegotiation alert.
In addition, restore the redundant version sanity-checks in the handshake state
machines. Some code would zero the version field as a hacky way to break the
handshake on renego. Those will be removed when switching to this API.
The spec allows for a non-fatal no_renegotiation alert, but ssl3_read_bytes
makes it difficult to find the end of a ClientHello and skip it entirely. Given
that OpenSSL goes out of its way to map non-fatal no_renegotiation alerts to
fatal ones, this seems probably fine. This avoids needing to account for
another source of the library consuming an unbounded number of bytes without
returning data up.
Change-Id: Ie5050d9c9350c29cfe32d03a3c991bdc1da9e0e4
Reviewed-on: https://boringssl-review.googlesource.com/4300
Reviewed-by: Adam Langley <agl@google.com>
Just about everything depends on SSL_CIPHER. Move it to the top as the first
section in ssl.h. Match the header order and the source file order and document
everything. Also make a couple of minor style guide tweaks.
Change-Id: I6a810dbe79238278ac480e5ced1447055715a79f
Reviewed-on: https://boringssl-review.googlesource.com/4290
Reviewed-by: Adam Langley <agl@google.com>
That block is slightly unreachable.
Change-Id: I1b4b2d8b1cd4bb7137ce0aac4b65079545cd9264
Reviewed-on: https://boringssl-review.googlesource.com/4286
Reviewed-by: Adam Langley <agl@google.com>
It's no longer needed to distinguish ciphers from fake ciphers.
Change-Id: I1ad4990ba936b1059eb48f3d2f309eb832dd1cb5
Reviewed-on: https://boringssl-review.googlesource.com/4285
Reviewed-by: Adam Langley <agl@google.com>
Rather than shoehorn real ciphers and cipher aliases into the same type (that's
what cipher->valid is used for), treat them separately. Make
ssl_cipher_apply_rule match ciphers by cipher_id (the parameter was ignored and
we assumed that masks uniquely identify a cipher) and remove the special cases
around zero for all the masks. This requires us to remember which fields
default to 0 and which default to ~0u, but the logic is much clearer.
Finally, now that ciphers and cipher aliases are different, don't process rules
which sum together an actual cipher with cipher aliases. This would AND
together the masks for the alias with the values in the cipher and do something
weird around alg_ssl. (alg_ssl is just weird in general, as everyone trying to
disable SSLv3 in OpenSSL recently discovered.)
With all that, we can finally remove cipher->valid which was always one.
Change-Id: Iefcfe159bd6c22dbaea3a5f1517bd82f756dcfe1
Reviewed-on: https://boringssl-review.googlesource.com/4284
Reviewed-by: Adam Langley <agl@google.com>
Beyond generally eliminating unnecessary includes, eliminate as many
includes of headers that declare/define particularly error-prone
functionality like strlen, malloc, and free. crypto/err/internal.h was
added to remove the dependency on openssl/thread.h from the public
openssl/err.h header. The include of <stdlib.h> in openssl/mem.h was
retained since it defines OPENSSL_malloc and friends as macros around
the stdlib.h functions. The public x509.h, x509v3.h, and ssl.h headers
were not changed in order to minimize breakage of source compatibility
with external code.
Change-Id: I0d264b73ad0a720587774430b2ab8f8275960329
Reviewed-on: https://boringssl-review.googlesource.com/4220
Reviewed-by: Adam Langley <agl@google.com>
A previous change in BoringSSL renamed ERR_print_errors_fp to
BIO_print_errors_fp as part of refactoring the code to improve the
layering of modules within BoringSSL. Rename it back for better
compatibility with code that was using the function under the original
name. Move its definition back to crypto/err using an implementation
that avoids depending on crypto/bio.
Change-Id: Iee7703bb1eb4a3d640aff6485712bea71d7c1052
Reviewed-on: https://boringssl-review.googlesource.com/4310
Reviewed-by: Adam Langley <agl@google.com>
These are all masks of some sort (except id which is a combined version and
cipher), so they should use fixed-size unsigned integers.
Change-Id: I058dd8ad231ee747df4b4fb17d9c1e2cbee21918
Reviewed-on: https://boringssl-review.googlesource.com/4283
Reviewed-by: Adam Langley <agl@google.com>
The rest of ssl/ still includes things everywhere, but this at least fixes the
includes that were implicit from ssl/internal.h.
Change-Id: I7ed22590aca0fe78af84fd99a3e557f4b05f6782
Reviewed-on: https://boringssl-review.googlesource.com/4281
Reviewed-by: Adam Langley <agl@google.com>
Match the other internal headers.
Change-Id: Iff7e2dd06a1a7bf993053d0464cc15638ace3aaa
Reviewed-on: https://boringssl-review.googlesource.com/4280
Reviewed-by: Adam Langley <agl@google.com>
These are the remaining untested cipher suites. Rather than add support in
runner.go, just remove them altogether. Grepping for this is a little tricky,
but nothing enables aNULL (all occurrences disable it), and all occurrences of
["ALL:] seem to be either unused or explicitly disable anonymous ciphers.
Change-Id: I4fd4b8dc6a273d6c04a26e93839641ddf738343f
Reviewed-on: https://boringssl-review.googlesource.com/4258
Reviewed-by: Adam Langley <agl@google.com>
This drops in a copy of a subset of golang.org/x/crypto/poly1305 to implement
Poly1305. Hopefully this will keep them from regression as we rework the record
layer.
Change-Id: Ic1e0d941a0a9e5ec260151ced8acdf9215c4b887
Reviewed-on: https://boringssl-review.googlesource.com/4257
Reviewed-by: Adam Langley <agl@google.com>
MSVC doesn't like unary - on unsigned numbers. Also switch ssl3_read_n's
version to uintptr_t to match the write half. This gets us closer to clearing
through C4311 violations. (The remaining one is in asn1_add_error which can go
after verifying that most of asn1_mac.h is safe to drop.)
Change-Id: Idb33dda8863bf1a3408b14d5513a667338311b6b
Reviewed-on: https://boringssl-review.googlesource.com/4255
Reviewed-by: Adam Langley <agl@google.com>
After sharding the session cache for fallbacks, the numbers have been pretty
good; 0.03% on dev and 0.02% on canary. Stable is at 0.06% but does not have
the sharded session cache. Before sharding, stable, beta, and dev had been
fairly closely aligned. Between 0.03% being low and the fallback saving us in
all but extremely contrived cases, I think this should be fairly safe.
Add tests for both the cipher suite and protocol version mismatch checks.
BUG=441456
Change-Id: I2374bf64d0aee0119f293d207d45319c274d89ab
Reviewed-on: https://boringssl-review.googlesource.com/3972
Reviewed-by: Adam Langley <agl@google.com>
This conceivably has a use, but NSS doesn't do this buffer either and it still
suffers from the same problems as the other uses of record_pqueue. This removes
the last use of record_pqueue. It also opens the door to removing pqueue
altogether as it isn't the right data structure for either of the remaining
uses either. (It's not clear it was right for record_pqueue either, but I don't
feel like digging into this code.)
Change-Id: If8a43e7332b3cd11a78a516f3e8ebf828052316f
Reviewed-on: https://boringssl-review.googlesource.com/4239
Reviewed-by: Adam Langley <agl@google.com>
It was only ever enabled for handshake and alert messages. The comments cite
renego as a use case though even then I'm not clear on why. The only use I see
is if, say, the Finished message and ClientKeyExchange came in out-of-order.
DTLS is unreliable so leaning on retransmit seems fine, and usually flights
will be packed into one packet where possible. NSS doesn't have any such
buffer and doesn't seem to have problems.
The buffering mechanism is also rather dubious. It stows away the entire packet
and read buffer---all 16K of it---and there may have been other records in that
packet.
Change-Id: Ic3b7bf817be380dc73102eec62c690ed093e6667
Reviewed-on: https://boringssl-review.googlesource.com/4238
Reviewed-by: Adam Langley <agl@google.com>
At this point, has_version has been set and we may even have a non-null cipher.
Trying to assign meaning to the record-layer version number is not worth making
s->version's semantics even more complicated.
Change-Id: Ia1cf341cf7306eb48d2d11241316dc2116306968
Reviewed-on: https://boringssl-review.googlesource.com/4237
Reviewed-by: Adam Langley <agl@google.com>
Compression is gone, so don't allow for compression overhead. With that fixed,
the second rr->length check in ssl3_get_record matches the length computation
which sizes the read buffer. The first is wrong and doesn't account for the
alignment padding. Move the second to the first.
Change-Id: I3f4f05de9fdf5c645ff24493bbfdf303dcc1aa90
Reviewed-on: https://boringssl-review.googlesource.com/4236
Reviewed-by: Adam Langley <agl@google.com>
Also check for overflow, although it really shouldn't happen.
Change-Id: I34dfe8eaf635aeaa8bef2656fda3cd0bad7e1268
Reviewed-on: https://boringssl-review.googlesource.com/4235
Reviewed-by: Adam Langley <agl@google.com>
These are redundant with the lower level ones in s3_pkt.c just before BIO_read.
Only the operation which actually failed an operation on the BIO should set
the wait state.
Not all failure paths in ssl3_read_bytes and dtls1_read_bytes set SSL_READING,
but those that don't leave the BIO in a retry state, so SSL_READING doesn't
matter.
Change-Id: I2ae064ecc8b2946cc8ae8f724be09dfe49e077b5
Reviewed-on: https://boringssl-review.googlesource.com/4230
Reviewed-by: Adam Langley <agl@google.com>
Fix up the variable names. Also avoid the messy logic of checking whether the
label and context collide with the normal key expansion ones in the face of
adverserial inputs. Make that the caller's responsibility, just as it's already
the caller's responsibility to ensure that different calls don't overlap. (The
label should be a constant string in an IANA registry anyway.)
Change-Id: I062fadb7b6a18fa946b883be660ea9b3f0f6277c
Reviewed-on: https://boringssl-review.googlesource.com/4216
Reviewed-by: Adam Langley <agl@google.com>
Separate actually writing the fragment to the network from assembling it so
there is no need for is_fragment. record_split_done also needn't be a global;
as of 7fdeaf1101, it is always reset to 0 whether
or not SSL3_WANT_WRITE occurred, despite the comment.
I believe this is sound, but the pre-7fdeaf1 logic wasn't quiiite right;
ssl3_write_pending allows a retry to supply *additional* data, so not all
plaintext had been commited to before the IV was randomized. We could fix this
by tracking how many bytes were committed to the last time we fragmented, but
this is purely an optimization and doesn't seem worth the complexity.
This also fixes the alignment computation in the record-splitting case. The
extra byte was wrong, as demonstrated by the assert.
Change-Id: Ia087a45a6622f4faad32e501942cc910eca1237b
Reviewed-on: https://boringssl-review.googlesource.com/4234
Reviewed-by: Adam Langley <agl@google.com>
It's still rather a mess, but this is at least somewhat clearer. The old one
had a lot of remnants of compression, etc.
Change-Id: Iffcb4dd4e8c4ab14f60abf917d22b7af960c93ba
Reviewed-on: https://boringssl-review.googlesource.com/4233
Reviewed-by: Adam Langley <agl@google.com>
If the key is missing, it seems the failure is assumed to be expected.
BUG=473924
Change-Id: I62edd9110fa74bee5e6425fd6786badf5398728c
Reviewed-on: https://boringssl-review.googlesource.com/4231
Reviewed-by: Adam Langley <agl@google.com>
Move the state to TestState rather than passing pointers to them everywhere.
Also move SSL_read and SSL_write retry loops into helper functions so they
aren't repeated everywhere. This also makes the SSL_write calls all
consistently account for partial writes.
Change-Id: I9bc083a03da6a77ab2fc03c29d4028435fc02620
Reviewed-on: https://boringssl-review.googlesource.com/4214
Reviewed-by: Adam Langley <agl@google.com>
Extend the False Start tests to optionally send an alert (thus avoiding
deadlock) before waiting for the out-of-order app data. Based on whether the
peer shuts off the connection before or after sending app data, we can
determine whether the peer False Started by observing purely external effects.
Change-Id: I8b9fecc29668e0b0c34b5fd19d0f239545011bae
Reviewed-on: https://boringssl-review.googlesource.com/4213
Reviewed-by: Adam Langley <agl@google.com>
Based on whether -false-start is passed, we expect SSL_CB_HANDSHAKE_DONE to or
not to fire. Also add a flag that asserts SSL_CB_HANDSHAKE_DONE does *not* fire
in any False Start test where the handshake fails after SSL_connect returns.
Change-Id: I6c5b960fff15e297531e15b16abe0b98be95bec8
Reviewed-on: https://boringssl-review.googlesource.com/4212
Reviewed-by: Adam Langley <agl@google.com>
That's a pretty obvious thing to test. I'm not sure how we forgot that one.
Change-Id: I7e1a7df6c6abbdd587e0f7723117f50d09faa5c4
Reviewed-on: https://boringssl-review.googlesource.com/4211
Reviewed-by: Adam Langley <agl@google.com>
There's multiple sets of APIs for selecting the curve. Fold away
SSL_OP_SINGLE_ECDH_USE as failing to set it is either a no-op or a bug. With
that gone, the consumer only needs to control the selection of a curve, with
key generation from then on being uniform. Also clean up the interaction
between the three API modes in s3_srvr.c; they were already mutually exclusive
due to tls1_check_ec_tmp_key.
This also removes all callers of EC_KEY_dup (and thus CRYPTO_dup_ex_data)
within the library.
Change-Id: I477b13bd9e77eb03d944ef631dd521639968dc8c
Reviewed-on: https://boringssl-review.googlesource.com/4200
Reviewed-by: Adam Langley <agl@google.com>
ASan's own malloc interceptor isn't compatible with this mechanism; it doesn't
see calls to __libc_malloc.
Change-Id: Ibac5aa05c6e40f1c72dcee3a2597e96deffca62c
Reviewed-on: https://boringssl-review.googlesource.com/4191
Reviewed-by: Adam Langley <agl@google.com>
SSL_CIPHER_get_rfc_name still returns an allocated string.
Change-Id: Ie2f14626c1ff22d0ea613b22439b7de5c04c9062
Reviewed-on: https://boringssl-review.googlesource.com/4190
Reviewed-by: Adam Langley <agl@google.com>
This is in preparation for using RAII in the unit tests. Those tests are built
in Chromium as well, but Chromium does not have C++11 library support across
all its toolchains. Compiler support is available, so add a partial
reimplementation of std::unique_ptr and std::move under crypto/test/. The
scopers for the crypto/ library are also moved there while the ones for ssl/
stay in ssl/test/.
Change-Id: I38f769acbc16a870db34649928575c7314b6e9f6
Reviewed-on: https://boringssl-review.googlesource.com/4120
Reviewed-by: Adam Langley <agl@google.com>
MinGW on Linux needs lowercase include files. On Windows this doesn't
matter since the filesystems are case-insensitive, but building
BoringSSL on Linux with MinGW has case-sensitive filesystems.
Change-Id: Id9c120d819071b041341fbb978352812d6d073bc
Reviewed-on: https://boringssl-review.googlesource.com/4090
Reviewed-by: Adam Langley <agl@google.com>
Partly inspired by the new state exposed in
dc3da93899, stress this codepath by spamming our
poor shim with warning alerts.
Change-Id: I876c6e52911b6eb57493cf3e1782b37ea96d01f8
Reviewed-on: https://boringssl-review.googlesource.com/4112
Reviewed-by: Adam Langley <agl@google.com>
Google C++ style allows these. It's also considerably less tedious and
error-prone than defining an out-of-line constructor.
Change-Id: Ib76ccf6079be383722433046ac5c5d796dd1f525
Reviewed-on: https://boringssl-review.googlesource.com/4111
Reviewed-by: Adam Langley <agl@google.com>
It was failing only on 32-bit for some reason. Part of TestConfig wasn't
getting initialized.
Change-Id: I2a3747a347a47b47e2357f34d32f8db86d6cc629
Reviewed-on: https://boringssl-review.googlesource.com/4110
Reviewed-by: Adam Langley <agl@google.com>
Within the library, only ssl_update_cache read them, so add a dedicated field
to replace that use.
The APIs have a handful of uninteresting callers so I've left them in for now,
but they now always return zero.
Change-Id: Ie4e36fd4ab18f9bff544541d042bf3c098a46933
Reviewed-on: https://boringssl-review.googlesource.com/4101
Reviewed-by: Adam Langley <agl@google.com>
I'm guessing a previous iteration used dlsym to look up the real malloc.
Change-Id: I18be9ef4db4ed059400074c8507d4e2fea882fbc
Reviewed-on: https://boringssl-review.googlesource.com/4100
Reviewed-by: Adam Langley <agl@google.com>
Quite a few functions reported wrong function names when pushing
to the error stack.
Change-Id: I84d89dbefd2ecdc89ffb09799e673bae17be0e0f
Reviewed-on: https://boringssl-review.googlesource.com/4080
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Both on the client and the server.
Change-Id: I9892c6dbbb29938154aba4f53b10e8b5231f9c47
Reviewed-on: https://boringssl-review.googlesource.com/4071
Reviewed-by: Adam Langley <agl@google.com>
Align with upstream's renames from a while ago. These names are considerably
more standard. This also aligns with upstream in that both "ECDHE" and "EECDH"
are now accepted in the various cipher string parsing bits.
Change-Id: I84c3daeacf806f79f12bc661c314941828656b04
Reviewed-on: https://boringssl-review.googlesource.com/4053
Reviewed-by: Adam Langley <agl@google.com>
This is really just scar tissue with https://crbug.com/468889 being the real
underlying problem. But the test is pretty easy.
Change-Id: I5eca18fdcbde8665c0e6c3ac419a28152647d66f
Reviewed-on: https://boringssl-review.googlesource.com/4052
Reviewed-by: Adam Langley <agl@google.com>
A casual grep would suggest this function has the same problems as
CVE-2015-0291, but the structure is memset to 0, so the calls are unnecessary.
Also use BUF_memdup rather than an OPENSSL_malloc + mempcy pair.
Change-Id: Id605374d99cff32e2dccb7f9b8a9da226faf7715
Reviewed-on: https://boringssl-review.googlesource.com/4051
Reviewed-by: Adam Langley <agl@google.com>
In verifying the fix for CVE-2015-0291, I noticed we don't actually have any
test coverage for full handshakes on renegotiation. All our tests always do
resumptions.
Change-Id: Ia9b701e8a50ba9353fefb8cc4fb86e78065d0b40
Reviewed-on: https://boringssl-review.googlesource.com/4050
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 34e3edbf3a10953cb407288101fd56a629af22f9. This fixes
CVE-2015-0291. Also bubble up malloc failures in tls1_set_shared_sigalgs. Tidy
up style a bit and remove unnecessary check (it actually is unnecessary; see
https://boringssl-review.googlesource.com/4042).
Change-Id: Idfb31a90fb3e56ef6fe7701464748a5c1603f064
Reviewed-on: https://boringssl-review.googlesource.com/4047
Reviewed-by: Adam Langley <agl@google.com>
Noticed these as I was poking around.
Change-Id: I93833a152583feced374c9febf7485bec7abc1c7
Reviewed-on: https://boringssl-review.googlesource.com/3973
Reviewed-by: Adam Langley <agl@google.com>
This too isn't version-specific. This removes the final difference between TLS
and DTLS SSL3_ENC_METHODs and we can fold them together. (We should be able to
fold away the version-specific differences too, but all in due time.)
Change-Id: I6652d3942a0970273d46d28d7052629c81f848b5
Reviewed-on: https://boringssl-review.googlesource.com/3771
Reviewed-by: Adam Langley <agl@google.com>
This fixes test flakiness on Windows.
BUG=467767
Change-Id: Ie69b5b43ddd524aadb15c53705f6ec860e928786
Reviewed-on: https://boringssl-review.googlesource.com/4001
Reviewed-by: Adam Langley <agl@google.com>
It created the cert structure in SSL_CTX or SSL if it was NULL, but they can
never be NULL as the comments already said.
(Imported from upstream's 2c3823491d8812560922a58677e3ad2db4b2ec8d.)
Change-Id: I97c7bb306d6f3c18597850db9f08023b2ef74839
Reviewed-on: https://boringssl-review.googlesource.com/4042
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's e1b568dd2462f7cacf98f3d117936c34e2849a6b.)
Our RAND_bytes secretly can't actually fail, but we should propagate the check
upwards.
Change-Id: Ieaaea98dad00bf73b1c0a42c039507d76b10ac78
Reviewed-on: https://boringssl-review.googlesource.com/4003
Reviewed-by: Adam Langley <agl@google.com>
This allows the current RC4 state of an SSL* to be extracted. We have
internal uses for this functionality.
Change-Id: Ic124c4b253c8325751f49e7a4c021768620ea4b7
Reviewed-on: https://boringssl-review.googlesource.com/3722
Reviewed-by: Adam Langley <agl@google.com>
This callback receives information about the ClientHello and can decide
whether or not to allow the handshake to continue.
Change-Id: I21be28335fa74fedb5b73a310ee24310670fc923
Reviewed-on: https://boringssl-review.googlesource.com/3721
Reviewed-by: Adam Langley <agl@google.com>
This mostly[*] doesn't matter for TLS since the message would have been
rejected anyway, but, in DTLS, if the peer rejects our Finished, it will send
an encrypted alert. This will then cause it to hang, which isn't very helpful.
I've made the change on both TLS and DTLS so the two protocols don't diverge on
this point. It is true that we're accepting nominally encrypted and
authenticated alerts before Finished, but, prior to ChangeCipherSpec, the
alerts are sent in the clear anyway so an attacker could already inject alerts.
A consumer could only be sensitive to it being post-CCS if it was watching
msg_callback. The only non-debug consumer of msg_callback I've found anywhere
is some hostapd code to detect Heartbeat.
See https://code.google.com/p/webrtc/issues/detail?id=4403 for an instance
where the equivalent behavior in OpenSSL masks an alert.
[*] This does change behavior slightly if the peer sends a warning alert
between CCS and Finished. I believe this is benign as warning alerts are
usually ignored apart from info_callback and msg_callback. The one exception is
a close_notify which is a slightly new state (accepting close_notify during a
handshake seems questionable...), but they're processed pre-CCS too.
Change-Id: Idd0d49b9f9aa9d35374a9f5e2f815cdb931f5254
Reviewed-on: https://boringssl-review.googlesource.com/3883
Reviewed-by: Adam Langley <agl@google.com>
May as well use this convenience function when we can. A little tidier. Even
fixes a leak on malloc failure in eckey_type2param.
Change-Id: Ie48dd98f2fe03fa9911bd78db4423ab9faefc63d
Reviewed-on: https://boringssl-review.googlesource.com/3772
Reviewed-by: Adam Langley <agl@google.com>
Fix a few remnants of them being unsigned long. Also rename extremely unhelpful
variable names in SSL_get_error. i is now ret_code to match the header.
Change-Id: Ic31d6626bfe09c9e21c03691dfc716c5573833ea
Reviewed-on: https://boringssl-review.googlesource.com/3881
Reviewed-by: Adam Langley <agl@google.com>
Some things were misindented in the reformatting.
Change-Id: I97642000452ce4d5b4c8a39b794cec13097d8760
Reviewed-on: https://boringssl-review.googlesource.com/3870
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 687eaf27a7e4bdfc58dd455e2566b915a7a25c20. I don't think any
of the *Update functions can actually fail (we should verify this and, if
accurate, document it), but HMAC_Final can. It internally copies an EVP_MD_CTX.
Change-Id: I318cb9d0771d536249a26b61d34fe0413a4d3a10
Reviewed-on: https://boringssl-review.googlesource.com/3830
Reviewed-by: Adam Langley <agl@google.com>
signed/unsigned comparison. Just add a cast for now as in s3_both.c. Later
we'll properly size_t it alongside other tightening of this interface.
Change-Id: Idc8441d65e8ca65e39ab7172a8ec87d9ad710ed6
Reviewed-on: https://boringssl-review.googlesource.com/3860
Reviewed-by: Adam Langley <agl@google.com>
The assert was supposed to be *added* in fcf25833 but instead replaced
the check.
BUG=465557
Change-Id: I0d3db5038515021e5bdd1ccb9ff08d4f78552621
Reviewed-on: https://boringssl-review.googlesource.com/3850
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
It's never called or implemented.
Change-Id: Id41c2fbd23d27cc440e8a23ac1b2d590e50ff20f
Reviewed-on: https://boringssl-review.googlesource.com/3770
Reviewed-by: Adam Langley <agl@google.com>
None of these are version-specific. SSL_PROTOCOL_METHOD's interface will change
later, but this gets us closer to folding away SSL3_ENC_METHOD.
Change-Id: Ib427cdff32d0701a18fe42a52cdbf798f82ba956
Reviewed-on: https://boringssl-review.googlesource.com/3769
Reviewed-by: Adam Langley <agl@google.com>
Turn them into static functions that take in an hm_fragment. It's not
immediately obvious that the frag_off/frag_len bounds checks and the msg_len
consistency check are critical to avoiding an out-of-bounds write. Better to
have dtls1_hm_fragment_mark also check internally.
Also rework the bitmask logic to be clearer and avoid a table.
Change-Id: Ica54e98f66295efb323e033cb6c67ab21e7d6cbc
Reviewed-on: https://boringssl-review.googlesource.com/3765
Reviewed-by: Adam Langley <agl@google.com>
Notably, drop all special cases around receiving a message in order and
receiving a full message. It makes things more complicated and was the source
of bugs (the MixCompleteMessageWithFragments tests added in this CL did not
pass before). Instead, every message goes through an hm_fragment, and
dtls1_get_message always checks buffered_messages to see if the next is
complete.
The downside is that we pay one more copy of the message data in the common
case. This is only during connection setup, so I think it's worth the
simplicity. (If we want to optimize later, we could either tighten
ssl3_get_message's interface to allow the handshake data being in the
hm_fragment's backing store rather than s->init_buf or swap out s->init_buf
with the hm_fragment's backing store when a mesasge completes.
This CL does not address ssl_read_bytes being an inappropriate API for DTLS.
Future work will revise the handshake/transport boundary to align better with
DTLS's needs. Also other problems that I've left as TODOs.
Change-Id: Ib4570d45634b5181ecf192894d735e8699b1c86b
Reviewed-on: https://boringssl-review.googlesource.com/3764
Reviewed-by: Adam Langley <agl@google.com>
We actually don't really care about this special-case since we only test client
full handshakes where the runner sends the second Finished not the shim
(otherwise the overlap logic and retransmitting on every fragment would
probably break us), but it should probably live next to the fragmentation
logic.
Change-Id: I54097d84ad8294bc6c42a84d6f22f496e63eb2a8
Reviewed-on: https://boringssl-review.googlesource.com/3763
Reviewed-by: Adam Langley <agl@google.com>
If the peer fragments Finished into multiple pieces, there is no need to
retransmit multiple times.
Change-Id: Ibf708ad079e1633afd420ff1c9be88a80020cba9
Reviewed-on: https://boringssl-review.googlesource.com/3762
Reviewed-by: Adam Langley <agl@google.com>
We don't support DTLS renego. Removing this separately from the rewrite to call
out intentionally dropping this logic.
Change-Id: Ie4428eea0d2dbbb8b4b8b6474df4821de62558cc
Reviewed-on: https://boringssl-review.googlesource.com/3761
Reviewed-by: Adam Langley <agl@google.com>
Instead, add a separate init_with_direction hook. Normal AEADs ignore the
direction, while legacy AEADs must be initialized with it. This avoids
maintaining extra state to support the delayed initialization.
Change-Id: I25271f0e56ee2783a2fd4d4026434154d58dc0a8
Reviewed-on: https://boringssl-review.googlesource.com/3731
Reviewed-by: Adam Langley <agl@google.com>
The old test just sent an empty ServerKeyExchange which is sufficient as we
reject the message early. But be more thorough and implement the actual
ephemeral key logic in the test server.
Change-Id: I016658762e4502c928c051e14d69eea67b5a495f
Reviewed-on: https://boringssl-review.googlesource.com/3650
Reviewed-by: Adam Langley <agl@google.com>
That might be a reasonable check to make, maybe.
DTLS handshake message reading has a ton of other bugs and needs a complete
rewrite. But let's fix this and get a test in now.
Change-Id: I4981fc302feb9125908bb6161ed1a18288c39e2b
Reviewed-on: https://boringssl-review.googlesource.com/3600
Reviewed-by: Adam Langley <agl@google.com>
Test both asynchronous and synchronous versions. This callback is somewhat
different from others. It's NOT called a second time when the handshake is
resumed. This appears to be intentional and not a mismerge from the internal
patch. The caller is expected to set up any state before resuming the handshake
state machine.
Also test the early callback returning an error.
Change-Id: If5e6eddd7007ea5cdd7533b4238e456106b95cbd
Reviewed-on: https://boringssl-review.googlesource.com/3590
Reviewed-by: Adam Langley <agl@google.com>
This involves more synchronization with child exits as the kernel no longer
closes the pre-created pipes for free, but it works on Windows. As long as
TCP_NODELAY is set, the performance seems comparable. Though it does involve
dealing with graceful socket shutdown. I couldn't get that to work on Windows
without draining the socket; not even SO_LINGER worked. Current (untested)
theory is that Windows refuses to gracefully shutdown a socket if the peer
sends data after we've stopped reading.
cmd.ExtraFiles doesn't work on Windows; it doesn't use fds natively, so you
can't pass fds 4 and 5. (stdin/stdout/stderr are special slots in
CreateProcess.) We can instead use the syscall module directly and mark handles
as inheritable (and then pass the numerical values out-of-band), but that
requires synchronizing all of our shim.Start() calls and assuming no other
thread is spawning a process.
PROC_THREAD_ATTRIBUTE_HANDLE_LIST fixes threading problems, but requires
wrapping more syscalls. exec.Cmd also doesn't let us launch the process
ourselves. Plus it still requires every handle in the list be marked
inheritable, so it doesn't help if some other thread is launching a process
with bInheritHandles TRUE but NOT using PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
(Like Go, though we can take syscall.ForkLock there.)
http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx
The more natively Windows option seems to be named pipes, but that too requires
wrapping more system calls. (To be fair, that isn't too painful.) They also
involve a listening server, so we'd still have to synchronize with shim.Wait()
a la net.TCPListener.
Then there's DuplicateHandle, but then we need an out-of-band signal.
All in all, one cross-platform implementation with a TCP sockets seems
simplest.
Change-Id: I38233e309a0fa6814baf61e806732138902347c0
Reviewed-on: https://boringssl-review.googlesource.com/3563
Reviewed-by: Adam Langley <agl@google.com>
Align the DTLS and TLS implementations more. s3_pkt.c's version still has
remnants of fragmentable alerts and only one side marks some variables as
const. Also use warning/fatal constants rather than the numbers with comments.
Change-Id: Ie62d3af1747b6fe4336496c047dfccc9d71fde3f
Reviewed-on: https://boringssl-review.googlesource.com/3562
Reviewed-by: Adam Langley <agl@google.com>
Saves making a temporary SSL_CTX and looking at its insides.
Change-Id: Ia351b9b91aec8b813ad7b6e373773396f0975f9a
Reviewed-on: https://boringssl-review.googlesource.com/3561
Reviewed-by: Adam Langley <agl@google.com>
These were added in upstream's 7e159e0133d28bec9148446e8f4dd86c0216d819 for
SCTP. As far as I can tell, they were a no-op there too. The corresponding RT
ticket makes no mention of them.
SSL_get_error checks the retry flags of the BIO already. Specifically it checks
BIO_should_read and BIO_should_write, but those two automatically set
BIO_should_retry.
(Minor, but I noticed them idly. One less thing to think about when the state
machines finally unify.)
Change-Id: I17a956a51895fba383063dee574e0fbe3209f9b0
Reviewed-on: https://boringssl-review.googlesource.com/3560
Reviewed-by: Adam Langley <agl@google.com>
(Which is just an exported wrapper around ssl3_get_cipher_by_value.)
Change-Id: Ibba166015ce59e337ff50963ba20237ac4949aaf
Reviewed-on: https://boringssl-review.googlesource.com/3543
Reviewed-by: Adam Langley <agl@google.com>
Upstream settled in this API, and it's also the one that we expect
internally and that third_party code will expect.
Change-Id: Id7af68cf0af1f2e4d9defd37bda2218d70e2aa7b
Reviewed-on: https://boringssl-review.googlesource.com/3542
Reviewed-by: Adam Langley <agl@google.com>
We can pass the fragment pointer to dtls1_retransmit_message rather than
having it look it up again.
Change-Id: If6957428418a44e7ceac91a93f7c6032d331d9d8
Reviewed-on: https://boringssl-review.googlesource.com/3510
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Also move BIO_print_errors_fp up a level so it's less repetitive. There's
enough exit points now that it doesn't seem like adding a separate return exit
code for each has held up. (Maybe there should be a macro that samples
__LINE__...)
Change-Id: I120e59caaa96185e80cf51ea801a5e1f149b1b39
Reviewed-on: https://boringssl-review.googlesource.com/3530
Reviewed-by: Adam Langley <agl@google.com>
Tighten up the requirements for False Start. At this point, neither
AES-CBC or RC4 are something that we want to use unless we're sure that
the server wants to speak them.
Rebase of original CL at: https://boringssl-review.googlesource.com/#/c/1980/
BUG=427721
Change-Id: I9ef7a596edeb8df1ed070aac67c315b94f3cc77f
Reviewed-on: https://boringssl-review.googlesource.com/3501
Reviewed-by: Adam Langley <agl@google.com>
Also adds a flag to runner.go to make it more suitable for printing to a pipe.
Change-Id: I26fae21f3e4910028f6b8bfc4821c8c595525504
Reviewed-on: https://boringssl-review.googlesource.com/3490
Reviewed-by: Adam Langley <agl@google.com>
I believe this is a remnant of DSA. The logic strangely fails to check for
failure and then goes out of its way to ERR_clear_error. I believe this is so
that keys that are missing parameters silently move on. This dates to
upstream's dfeab0689f69c0b4bd3480ffd37a9cacc2f17d9c, which is SSLeay 0.9.1b. At
that time, EVP_PKEY_copy_parameters only did anything for DSA. (Now it only
does anything for ECDSA.)
My read is that this comes from DSA in PKIX's "optional domain parameters"
craziness. RFC 3279 says:
If the DSA domain parameters are omitted from the SubjectPublicKeyInfo
AlgorithmIdentifier and the CA signed the subject certificate using a
signature algorithm other than DSA, then the subject's DSA domain parameters
are distributed by other means.
This was probably part of some weird thing where, if your certificate is
missing parameters, the server would know what to use based on the private key.
(Also this was making the malloc tests unhappy.)
Change-Id: I8d8122a9f50a19e2bbe067f311a8e2d30774935c
Reviewed-on: https://boringssl-review.googlesource.com/3484
Reviewed-by: Adam Langley <agl@google.com>
It may fail because the BIO_write to the memory BIO can allocate.
Unfortunately, this bubbles up pretty far up now that we've moved the handshake
hash to ssl3_set_handshake_header.
Change-Id: I58884347a4456bb974ac4783078131522167e29d
Reviewed-on: https://boringssl-review.googlesource.com/3483
Reviewed-by: Adam Langley <agl@google.com>
Found while diagnosing some crashes and hangs in the malloc tests. This (and
the follow-up) get us further but does not quite let the malloc tests pass
quietly, even without valgrind. DTLS silently ignores some malloc failures
(confusion with silently dropping bad packets) which then translate to hangs.
Change-Id: Ief06a671e0973d09d2883432b89a86259e346653
Reviewed-on: https://boringssl-review.googlesource.com/3482
Reviewed-by: Adam Langley <agl@google.com>
False Start is the name it's known by now. Deprecate the old API and expose new
ones with the new name.
Change-Id: I32d307027e178fd7d9c0069686cc046f75fdbf6f
Reviewed-on: https://boringssl-review.googlesource.com/3481
Reviewed-by: Adam Langley <agl@google.com>
I found no users of this. We can restore it if needbe, but I don't expect
anyone to find it useful in its current form. The API is suspect for the same
reasons DTLSv1_listen was. An SSL object is stateful and assumes you already
have the endpoint separated out.
If we ever need it, server-side HelloVerifyRequest and DTLSv1_listen should be
implemented by a separate stateless listener that statelessly handles
cookieless ClientHello + HelloVerifyRequest. Once a ClientHello with a valid
cookie comes in, it sets up a stateful SSL object and passes control along to
that.
Change-Id: I86adc1dfb6a81bebe987784c36ad6634a9a1b120
Reviewed-on: https://boringssl-review.googlesource.com/3480
Reviewed-by: Adam Langley <agl@google.com>
Caught by malloc valgrind tests on Basic-Client-Sync. Also one by inspection
and verified with valgrind. Those should pass now with the exception of
CRYPTO_free_ex_data being internally implemented with malloc.
(Clearly we next should make our malloc tests assert that the containing
function fails to catch when we fail to check for some error and things
silently move one.)
Change-Id: I56c51dc8a32a7d3c7ac907d54015dc241728c761
Reviewed-on: https://boringssl-review.googlesource.com/3440
Reviewed-by: Adam Langley <agl@google.com>
pqueue_free requires the queue be empty.
Change-Id: I633e18fe71ddec51d6005210fcb6570ef53b9808
Reviewed-on: https://boringssl-review.googlesource.com/3410
Reviewed-by: Adam Langley <agl@google.com>
Win64 fires significantly more warnings than Win32. Also some recent
changes made it grumpy.
(We might want to reconsider enabling all of MSVC's warnings. Given the sorts
of warnings some of these are, I'm not sure MSVC's version of -Wall -Werror is
actually tenable. Plus, diverging from the Chromium build, especially before
the bots are ready, is going to break pretty readily.)
Change-Id: If3b8feccf910ceab4a233b0731e7624d7da46f87
Reviewed-on: https://boringssl-review.googlesource.com/3420
Reviewed-by: Adam Langley <agl@google.com>
We have got to get rid of that parameter...
Change-Id: I17f2d1282636f7d077f21dabdc135eecf9300998
Reviewed-on: https://boringssl-review.googlesource.com/3384
Reviewed-by: Adam Langley <agl@google.com>
Previously, error strings were kept in arrays for each subdirectory and
err.c would iterate over them all and insert them at init time to a hash
table.
This means that, even if you have a shared library and lots of processes
using that, each process has ~30KB of private memory from building that
hash table.
This this change, all the error strings are built into a sorted list and
are thus static data. This means that processes can share the error
information and it actually saves binary space because of all the
pointer overhead in the old scheme. Also it saves the time taken
building the hash table at startup.
This removes support for externally-supplied error string data.
Change-Id: Ifca04f335c673a048e1a3e76ff2b69c7264635be
OpenSSL's internal names for the ciphers are not the standard ones and are not
easy to consistently map to the standard ones. Add an API to get the real names
out. (WebRTC wants an API to get the standard names out.)
Also change some incorrect flags on SHA-256 TLS 1.2 ciphers;
SSL_HANDSHAKE_MAC_DEFAULT and SSL_HANDSHAKE_MAC_SHA256 are the same after TLS
1.2. A TLS 1.2 cipher should be tagged explicitly with SHA-256. (This avoids
tripping a check in SSL_CIPHER_get_rfc_name which asserts that default-hash
ciphers only ever use SHA-1 or MD5 for the bulk cipher MAC.)
Change-Id: Iaec2fd4aa97df29883094d3c2ae60f0ba003bf07
Nothing else on SSL* is thread-safe. (Also SSL_set_generate_session_id is never
called.) This removes the last use of CRYPTO_LOCK_SSL.
Change-Id: I4cf8c05d7cef4ea27962ce29902649317c22f74d
Reviewed-on: https://boringssl-review.googlesource.com/3361
Reviewed-by: Adam Langley <agl@google.com>
Found by running malloc tests with -valgrind. Unfortunately, the next one is
deep in crypto/asn1 itself, so I'm going to stop here for now.
Change-Id: I7a33971ee07c6b7b7a98715f2f18e0f29380c0a1
Reviewed-on: https://boringssl-review.googlesource.com/3350
Reviewed-by: Adam Langley <agl@google.com>
We have a stateful object hanging off the SSL* now. May as well use it and
avoid having to remember to reset that.
Change-Id: I5fc5269aa9b158517dd551036e658afaa2ef9acd
Reviewed-on: https://boringssl-review.googlesource.com/3349
Reviewed-by: Adam Langley <agl@google.com>
It's currently a mix of GoogleCPlusPlusStyle and unix_hacker_style. Since it's
now been thoroughly C++-ified, let's go with the former. This also matches the
tool, our other bit of C++ code.
Change-Id: Ie90a166006aae3b8f41628dbb35fcd64e99205df
Reviewed-on: https://boringssl-review.googlesource.com/3348
Reviewed-by: Adam Langley <agl@google.com>
This is more consistent with other asynchronous hooks and gets it working in
DTLS.
Change-Id: Ia17d9d23910e8665b2756516ba729dffc79af8c0
Reviewed-on: https://boringssl-review.googlesource.com/3346
Reviewed-by: Adam Langley <agl@google.com>
It doesn't appear that logic (added in upstream's
b7cfcfb7f8e17c17f457b3384010eb027f3aad72) is protecting against anything. On
the contrary, it prohibits implementing CRYPTO_add with real atomic operations!
There's no guarantee that those operations will interact with the locked
implementation.
https://www.mail-archive.com/openssl-users@openssl.org/msg63176.html
As long as ssl->session points to the same session, we know the session won't
be freed. There is no lock protecting, say, SSL_set_session, but a single SSL*
does not appear to be safe to use across threads. If this were to be supported,
both should be guarded by CRYPTO_LOCK_SSL (which is barely used).
CRYPTO_LOCK_SSL_SESSION isn't sufficient anyway; it could sample while
SSL_set_session is busy swapping out the now freed old session with the new.
Change-Id: I54623d0690c55c2c86720406ceff545e2e5f2f8f
Reviewed-on: https://boringssl-review.googlesource.com/3345
Reviewed-by: Adam Langley <agl@google.com>
The fact that an SSL_SESSION is reference-counted is already part of the API.
If an external application (like, say, the test code) wishes to participate, we
should let it.
Change-Id: If04d26a35141da14fd8d917de6cc1c10537ad11a
Reviewed-on: https://boringssl-review.googlesource.com/3344
Reviewed-by: Adam Langley <agl@google.com>
Start exercising the various async callbacks, starting with channel ID. These
will run under the existing state machine coverage tests; -async will also
enable every asynchronous callback we can.
Change-Id: I173148d93d3a9c575b3abc3e2aceb77968b88f0e
Reviewed-on: https://boringssl-review.googlesource.com/3342
Reviewed-by: Adam Langley <agl@google.com>
bssl_shim rather needs it. It doesn't even free the SSL* properly most of the
time. Now that it does, this opens the door to running malloc tests under
a leak checker (because it's just not slow enough right now).
Change-Id: I37d2004de27180c41b42a6d9e5aea02caf9b8b32
Reviewed-on: https://boringssl-review.googlesource.com/3340
Reviewed-by: Adam Langley <agl@google.com>
This is consistent with ignoring writeRecord failures. Without doing this, the
DTLS MinimumVersion test now flakily fails with:
FAILED (MinimumVersion-Client-TLS12-TLS1-DTLS)
bad error (wanted ':UNSUPPORTED_PROTOCOL:' / 'remote error: protocol version not supported'): local error 'write unix @: broken pipe', child error 'exit status 2', stdout:
2092242157:error:1007b1a7:SSL routines:ssl3_get_server_hello:UNSUPPORTED_PROTOCOL:../ssl/s3_clnt.c:783:
This is because the MinimumVersion tests assert on /both/ expectedError and
expectedLocalError. The latter is valuable as it asserts on the alert the peer
returned. (I would like us to add more such assertions to our tests where
appropriate.) However, after we send ServerHello, we also send a few messages
following it. This races with the peer shutdown and we sometimes get EPIPE
before reading the alert.
Change-Id: I3fe37940a6a531379673a00976035f8e76e0f825
Reviewed-on: https://boringssl-review.googlesource.com/3337
Reviewed-by: Adam Langley <agl@google.com>
This makes the following changes:
- SSL_cutthrough_complete no longer rederives whether cutthrough happened and
just maintains a handshake bit.
- SSL_in_init no longer returns true if we are False Starting but haven't
completed the handshake. That logic was awkward as it depended on querying
in_read_app_data to force SSL_read to flush the entire handshake. Defaulting
SSL_in_init to continue querying the full handshake and special-casing
SSL_write is better. E.g. the check in bidirectional SSL_shutdown wants to know
if we're in a handshake. No internal consumer of
SSL_MODE_HANDSHAKE_CUTTHROUGH ever queries SSL_in_init directly.
- in_read_app_data is gone now that the final use is dead.
Change-Id: I05211a116d684054dfef53075cd277b1b30623b5
Reviewed-on: https://boringssl-review.googlesource.com/3336
Reviewed-by: Adam Langley <agl@google.com>
It may take up to two iterations of s->handshake_func before it is safe to
continue. Fortunately, even if anything was using False Start this way
(Chromium doesn't), we don't inherit NSS's security bug. The "redundant" check
in the type match case later on in this function saves us.
Amusingly, the success case still worked before this fix. Even though we fall
through to the post-handshake codepath and get a handshake record while
"expecting" app data, the handshake state machine is still pumped thanks to a
codepath meant for renego!
Change-Id: Ie129d83ac1451ad4947c4f86380879db8a3fd924
Reviewed-on: https://boringssl-review.googlesource.com/3335
Reviewed-by: Adam Langley <agl@google.com>
The new V2ClientHello sniff asserts, for safety, that nothing else has
initialized the record layer before it runs. However, OpenSSL allows you to
avoid explicitly calling SSL_connect/SSL_accept and instead let
SSL_read/SSL_write implicitly handshake for you. This check happens at a fairly
low-level in the ssl3_read_bytes function, at which point the record layer has
already been initialized.
Add some tests to ensure this mode works.
(Later we'll lift the handshake check to a higher-level which is probably
simpler.)
Change-Id: Ibeb7fb78e5eb75af5411ba15799248d94f12820b
Reviewed-on: https://boringssl-review.googlesource.com/3334
Reviewed-by: Adam Langley <agl@google.com>
This works fine, but I believe NSS had a bug here a couple years ago. Also move
all the Skip* bug options next to each other in order.
Change-Id: I72dcb3babeee7ba73b3d7dc5ebef2e2298e37438
Reviewed-on: https://boringssl-review.googlesource.com/3333
Reviewed-by: Adam Langley <agl@google.com>
This is the source of much of renegotiation's complexity, and of OpenSSL's
implementation of it. In practice, we only care about renegotiation because of
the client auth hack. There, we can safely assume that no server will send
application data between sending the HelloRequest and completing the handshake.
BUG=429450
Change-Id: I37f5abea5fdedb1d53e24ceb11f71287c74bb777
Reviewed-on: https://boringssl-review.googlesource.com/3332
Reviewed-by: Adam Langley <agl@google.com>
EVP_Digest can fail on malloc failure. May as well tidy that. Also make that
humongous comment less verbose.
Change-Id: I0ba74b901a5ac68711b9ed268b4202dc19242909
Reviewed-on: https://boringssl-review.googlesource.com/3331
Reviewed-by: Adam Langley <agl@google.com>
We have test coverage for invalid alerts, but not for normal ones on the DTLS
side.
Change-Id: I359dce8d4dc80dfa99b5d8bacd73f48a8e4ac310
Reviewed-on: https://boringssl-review.googlesource.com/3291
Reviewed-by: Adam Langley <agl@google.com>
The check on the DTLS side was broken anyway. On the TLS side, the spec does
say to ignore them, but there should be no need for this in future-proofing and
NSS doesn't appear to be lenient here. See also
https://boringssl-review.googlesource.com/#/c/3233/
Change-Id: I0846222936c5e08acdcfd9d6f854a99df767e468
Reviewed-on: https://boringssl-review.googlesource.com/3290
Reviewed-by: Adam Langley <agl@google.com>
SSL_AEAD_CTX ownership is currently too confusing. Instead, rely on the lack of
renego, so the previous epoch always uses the NULL cipher. (Were we to support
DTLS renego, we could keep track of s->d1->last_aead_write_ctx like
s->d1->last_write_sequence, but it isn't worth it.)
Buffered messages also tracked an old s->session, but this is unnecessary. The
s->session NULL check in tls1_enc dates to the OpenSSL initial commit and is
redundant with the aead NULL check.
Change-Id: I9a510468d95934c65bca4979094551c7536980ae
Reviewed-on: https://boringssl-review.googlesource.com/3234
Reviewed-by: Adam Langley <agl@google.com>
Nothing recognized through those codepaths is fragmentable in DTLS. Also remove
an unnecessary epoch check. It's not possible to process a record from the
wrong epoch.
Change-Id: I9d0f592860bb096563e2bdcd2c8e50a0d2b65f59
Reviewed-on: https://boringssl-review.googlesource.com/3232
Reviewed-by: Adam Langley <agl@google.com>
This is only applicable for renego and is wrong anyway. The handshake_read_seq
check doesn't account for message reordering. The correct check is if we
haven't yet processed the peer's CCS in the current handshake.
(The other Finished special-case needs to stay, however.)
Change-Id: Ic42897aab7140285ce2f3be24d52b81851b912b5
Reviewed-on: https://boringssl-review.googlesource.com/3231
Reviewed-by: Adam Langley <agl@google.com>
We will not support any form of DTLS renego.
Change-Id: I6eab4ed12a131ad27fdb9b5ea7cc1f35d872cd43
Reviewed-on: https://boringssl-review.googlesource.com/3230
Reviewed-by: Adam Langley <agl@google.com>
It has no callers in internal code.
Change-Id: I53cf1769b71be6a0441533b6af7d3f64aab5098a
Reviewed-on: https://boringssl-review.googlesource.com/3219
Reviewed-by: Adam Langley <agl@google.com>
We have no use for DTLS renego and it's even more complex than TLS renego.
Change-Id: I8680ab361cc8761dd7fc8dfb1bfe1ff4abc6612f
Reviewed-on: https://boringssl-review.googlesource.com/3218
Reviewed-by: Adam Langley <agl@google.com>
For now, only test reorderings when we always or never fragment messages.
There's a third untested case: when full messages and fragments are mixed. That
will be tested later after making it actually work.
Change-Id: Ic4efb3f5e87b1319baf2d4af31eafa40f6a50fa6
Reviewed-on: https://boringssl-review.googlesource.com/3216
Reviewed-by: Adam Langley <agl@google.com>
No behavior change. This is in preparation for buffering a flight of handshake
messages to reorder vigorously on flush.
Change-Id: Ic348829b340bf58d28f332027646559cb11046ac
Reviewed-on: https://boringssl-review.googlesource.com/3215
Reviewed-by: Adam Langley <agl@google.com>
It dates to 2000 from upstream and is only used when serving client auth to
Netscape. It will also get in the way when we get to merging DTLS and TLS
handshake functions because NETSCAPE_HANG_BUG is not valid for DTLS as it is
(the handshake fragmentation code will get confused).
Removing per comment on https://boringssl-review.googlesource.com/#/c/2602/
Change-Id: Ia2d086205bbfed002dc33b2203a47206f373b820
Reviewed-on: https://boringssl-review.googlesource.com/3214
Reviewed-by: Adam Langley <agl@google.com>
All but one field is a no-op.
Change-Id: Ib7bc59a12ce792d5e42fb6e04a4aff54f42643a9
Reviewed-on: https://boringssl-review.googlesource.com/3213
Reviewed-by: Adam Langley <agl@google.com>
This extends the packet adaptor protocol to send three commands:
type command =
| Packet of []byte
| Timeout of time.Duration
| TimeoutAck
When the shim processes a Timeout in BIO_read, it sends TimeoutAck, fails the
BIO_read, returns out of the SSL stack, advances the clock, calls
DTLSv1_handle_timeout, and continues.
If the Go side sends Timeout right between sending handshake flight N and
reading flight N+1, the shim won't read the Timeout until it has sent flight
N+1 (it only processes packet commands in BIO_read), so the TimeoutAck comes
after N+1. Go then drops all packets before the TimeoutAck, thus dropping one
transmit of flight N+1 without having to actually process the packets to
determine the end of the flight. The shim then sees the updated clock, calls
DTLSv1_handle_timeout, and re-sends flight N+1 for Go to process for real.
When dropping packets, Go checks the epoch and increments sequence numbers so
that we can continue to be strict here. This requires tracking the initial
sequence number of the next epoch.
The final Finished message takes an additional special-case to test. DTLS
triggers retransmits on either a timeout or seeing a stale flight. OpenSSL only
implements the former which should be sufficient (and is necessary) EXCEPT for
the final Finished message. If the peer's final Finished message is lost, it
won't be waiting for a message from us, so it won't time out anything. That
retransmit must be triggered on stale message, so we retransmit the Finished
message in Go.
Change-Id: I3ffbdb1de525beb2ee831b304670a3387877634c
Reviewed-on: https://boringssl-review.googlesource.com/3212
Reviewed-by: Adam Langley <agl@google.com>
This reverts commit c67a3ae6ba. With a
deterministic clock, we can now go back to being strict about retransmits. Our
tests will now require that the shim only retransmit when we expect it to.
Change-Id: Iab1deb9665dcd294790c8253d920089e83a9140c
Reviewed-on: https://boringssl-review.googlesource.com/3211
Reviewed-by: Adam Langley <agl@google.com>
This is so the tests needn't be sensitive to the clock. It is, unfortunately, a
test-only hook, but the DTLS retransmit/timeout logic more-or-less requires it
currently. Use this hook to, for now, freeze the clock at zero. This makes the
tests deterministic.
It might be worth designing a saner API in the future. The current one,
notably, requires that the caller's clock be compatible with the one we
internally use. It's also not clear whether the caller needs to call
DTLSv1_handle_timeout or can just rely on the state machine doing it internally
(as it does do). But mock clocks are relatively tame and WebRTC wants to
compile against upstream OpenSSL for now, so we're limited in how much new API
we can build.
Change-Id: I7aad51570596f69275ed0fc1a8892393e4b7ba13
Reviewed-on: https://boringssl-review.googlesource.com/3210
Reviewed-by: Adam Langley <agl@google.com>
Including string.h in base.h causes any file that includes a BoringSSL
header to include string.h. Generally this wouldn't be a problem,
although string.h might slow down the compile if it wasn't otherwise
needed. However, it also causes problems for ipsec-tools in Android
because OpenSSL didn't have this behaviour.
This change removes string.h from base.h and, instead, adds it to each
.c file that requires it.
Change-Id: I5968e50b0e230fd3adf9b72dd2836e6f52d6fb37
Reviewed-on: https://boringssl-review.googlesource.com/3200
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The variable switches the default type for add_library from STATIC to SHARED.
We can condition additional stuff on that for convenience. (tabtest still
doesn't build.)
BoringSSL as any kind of stable system shared library is still very much
unsupported, but this is probably handy for making sure we don't forget all
those pesky OPENSSL_EXPORTs.
Change-Id: I66ab80bcddbf3724e03e85384141fdf4f4acbc2e
Reviewed-on: https://boringssl-review.googlesource.com/3092
Reviewed-by: Adam Langley <agl@google.com>
This is fatal for TLS but buffered in DTLS. The buffering isn't strictly
necessary (it would be just as valid to drop the record on the floor), but so
long as we want this behavior it should have a test.
Change-Id: I5846bb2fe80d78e25b6dfad51bcfcff2dc427c3f
Reviewed-on: https://boringssl-review.googlesource.com/3029
Reviewed-by: Adam Langley <agl@google.com>
They're not in the duplicated handshake state machines anyway. But we still
shouldn't negotiate them. d1_pkt.c assumes Finished is the only post-CCS
handshake message. An unexpected handshake message in the current epoch may
either be a retransmit/out-of-order message from the previous handshake, or a
message from the next handshake (also potentially out-of-order). In the former
case, we shouldn't spin up another handshake state machine instance.
(This assumption is required due to a protocol bug. DTLS resets sequence
numbers after a handshake, so it is necessary to categorize handshake fragments
by pre-CCS and post-CCS to distinguish between retransmit and renego.)
Change-Id: Ib3c1c7085c729e36a40f7ff14494733156924a24
Reviewed-on: https://boringssl-review.googlesource.com/3028
Reviewed-by: Adam Langley <agl@google.com>
This regressed in e95d20dcb8. EVP_AEAD will push
errors on the error queue (unlike the EVP_CIPHER codepath which checked
everything internally to ssl/ and didn't bother pushing anything). This meant
that a dropped packet would leave junk in the error queue.
Later, when SSL_read returns <= 0 (EOF or EWOULDBLOCK), the non-empty error
queue check in SSL_get_error kicks in and SSL_read looks to have failed.
BUG=https://code.google.com/p/webrtc/issues/detail?id=4214
Change-Id: I1e5e41c77a3e5b71e9eb0c72294abf0da677f840
Reviewed-on: https://boringssl-review.googlesource.com/2982
Reviewed-by: Adam Langley <agl@google.com>
This regressed in fcf25833bc. 0 return code on
unclean shutdown means the underlying BIO returned EOF, didn't push any error
code, but we haven't seen close_notify yet. The intent seems to be that you go
check errno or some BIO-specific equivalent if you care about close_notify.
Make sure test code routes all SSL_read return codes through SSL_get_error
since that's supposed to work in all cases.
(Note that rv == 0 can still give SSL_ERROR_SSL if the error queue is not
empty.)
Change-Id: I45bf9614573f876d93419ce169a4e0d9ceea9052
Reviewed-on: https://boringssl-review.googlesource.com/2981
Reviewed-by: Adam Langley <agl@google.com>
The distinction between publicly and non-publicly invalid is barely acted upon
and slightly silly now that the CBC padding check has been folded into
EVP_AEAD.
Change-Id: Idce4b9b8d29d624e3c95243a147265d071612127
Reviewed-on: https://boringssl-review.googlesource.com/2980
Reviewed-by: Adam Langley <agl@google.com>
This is an initial cut at aarch64 support. I have only qemu to test it
however—hopefully hardware will be coming soon.
This also affects 32-bit ARM in that aarch64 chips can run 32-bit code
and we would like to be able to take advantage of the crypto operations
even in 32-bit mode. AES and GHASH should Just Work in this case: the
-armx.pl files can be built for either 32- or 64-bit mode based on the
flavour argument given to the Perl script.
SHA-1 and SHA-256 don't work like this however because they've never
support for multiple implementations, thus BoringSSL built for 32-bit
won't use the SHA instructions on an aarch64 chip.
No dedicated ChaCha20 or Poly1305 support yet.
Change-Id: Ib275bc4894a365c8ec7c42f4e91af6dba3bd686c
Reviewed-on: https://boringssl-review.googlesource.com/2801
Reviewed-by: Adam Langley <agl@google.com>
SSL_library_init already loads the error strings (unlike upstream). Code which
calls both will end up loading error strings twice. Instead make the second
call a no-op.
Change-Id: Ifd34ab20ed46aabeba14661e58f8dac2bbb29f69
Reviewed-on: https://boringssl-review.googlesource.com/2790
Reviewed-by: Adam Langley <agl@google.com>
The under 32 constraint is silly; it's to check for duplicate curves in
library-supplied configuration. That API is new as of 1.0.2. It doesn't seem
worth bothering; if the caller supplies a repeated value, may as well emit a
repeated one and so be it. (Probably no one will ever call that function
outside of maybe test code anyway.)
While I'm here, remove the 0 constraint too. It's not likely to change, but
removing the return value overload seems easier than keeping comments about it
comments about it.
Change-Id: I01d36dba1855873875bb5a0ec84b040199e0e9bc
Reviewed-on: https://boringssl-review.googlesource.com/2844
Reviewed-by: Adam Langley <agl@google.com>
We only implement four curves (P-224, P-256, P-384, and P-521) and only
advertise the latter three by default. Don't maintain entries corresponding to
all the unimplemented curves.
Change-Id: I1816a10c6f849ca1d9d896bc6f4b64cd6b329481
Reviewed-on: https://boringssl-review.googlesource.com/2843
Reviewed-by: Adam Langley <agl@google.com>
They both happen to be zero, but OBJ_undef is a type error; OBJ_foo expands to
a comma-separated list of integers.
Change-Id: Ia5907dd3bc83240b7cc98af6456115d2efb48687
Reviewed-on: https://boringssl-review.googlesource.com/2842
Reviewed-by: Adam Langley <agl@google.com>
When parsing ClientHello clear any existing extension state from
SRP login and SRTP profile.
(Imported from upstream's 4f605ccb779e32a770093d687e0554e0bbb137d3)
More state that should be systematically reset across handshakes. Add a reset
on the ServerHello end too since that was missed.
Change-Id: Ibb4549acddfd87caf7b6ff853e2adbfa4b7e7856
Reviewed-on: https://boringssl-review.googlesource.com/2838
Reviewed-by: Adam Langley <agl@google.com>
write_quota should only be decremented by 1 in datagram mode, otherwise we'll
underflow and always allow writes through. This does not cause any existing
tests to fail.
(It will be useful once the bug in dtls1_do_write is fixed.)
Change-Id: I42aa001d7264790a3726269890635f679497fb1c
Reviewed-on: https://boringssl-review.googlesource.com/2831
Reviewed-by: Adam Langley <agl@google.com>
The existing comments are not very helpful. This code is also quite buggy.
Document two of them as TODOs.
Change-Id: Idfaf93d9c3b8b1ee92f2fb0d292ef513b5f6d824
Reviewed-on: https://boringssl-review.googlesource.com/2830
Reviewed-by: Adam Langley <agl@google.com>
BIO_ctrls do not have terribly well-defined return values on error. (Though the
existing ones seem to all return 0, not -1, on nonexistant operation.)
Change-Id: I08497f023ce3257c253aa71517a98b2fe73c3f74
Reviewed-on: https://boringssl-review.googlesource.com/2829
Reviewed-by: Adam Langley <agl@google.com>
g_probably_mtu and dtls1_guess_mtu is a bunch of logic for guessing the right
MTU, but it only ever returns the maximum (the input is always zero). Trim that
down to only what it actually does.
Change-Id: If3afe3f68ccb36cbf9c4525372564d16a4bbb73f
Reviewed-on: https://boringssl-review.googlesource.com/2828
Reviewed-by: Adam Langley <agl@google.com>
The retry doesn't actually work when we're sending a non-initial fragment; the
s->init_off != 0 block will get re-run each iteration through and continually
prepend headers. It can also infinite loop if the BIO reports
BIO_CTRL_DGRAM_MTU_EXCEEDED but either fails to report an MTU or reports an MTU
that always rounds up to the minimum. See upstream's
d3d9eef31661633f5b003a9e115c1822f79d1870.
WebRTC doesn't participate in any of the MTU logic and inherits the default
MTU, so just remove it for now.
Change-Id: Ib2ed2ba016b7c229811741fb7369c015ba0b551f
Reviewed-on: https://boringssl-review.googlesource.com/2827
Reviewed-by: Adam Langley <agl@google.com>
That setting means that the MTU is provided externally via SSL_set_mtu.
(Imported from upstream's 001235778a6e9c645dc0507cad6092d99c9af8f5)
Change-Id: I4e5743a9dee734ddd0235f080aefe98a7365aaf6
Reviewed-on: https://boringssl-review.googlesource.com/2826
Reviewed-by: Adam Langley <agl@google.com>
Based in part on upstream's cf75017bfd60333ff65edf9840001cd2c49870a3. This
situation really shouldn't be able to happen, but between no static asserts
that the minimum MTU is always large enough and a bug in reseting the MTU later
(to be fixed be a follow-up import from upstream), check these and return a
useful error code.
Change-Id: Ie853e5d35a6a7bc9c0032e74ae71529d490f4fe2
Reviewed-on: https://boringssl-review.googlesource.com/2825
Reviewed-by: Adam Langley <agl@google.com>
BoringSSL currently retransmits non-deterministically on an internal timer
(rather than one supplied externally), so the tests currently fail flakily
depending on timing. Valgrind is a common source for this. We still assume an
in-order and reliable channel, but drop retransmits silently:
- Handshake messages may arrive with old sequence numbers.
- Retransmitted CCS records arrive from the previous epoch.
- We may receive a retransmitted Finished after we believe the handshake has
completed. (Aside: even in a real implementation, only Finished is possible
here. Even with out-of-order delivery, retransmitted or reordered messages
earlier in the handshake come in under a different epoch.)
Note that because DTLS renego and a Finished retransmit are ambiguous at the
record layer[*], this precludes us writing tests for DTLS renego. But DTLS
renego should get removed anyway. As BoringSSL currently implements renego,
this ambiguity is also a source of complexity in the real implementation. (See
the SSL3_MT_FINISHED check in dtls1_read_bytes.)
[*] As a further fun aside, it's also complex if dispatching renego vs Finished
after handshake message reassembly. The spec doesn't directly say the sequence
number is reset across renegos, but it says "The first message each side
transmits in /each/ handshake always has message_seq = 0". This means that such
an implementation needs the handshake message reassembly logic be aware that a
Finished fragment with high sequence number is NOT an out-of-order fragment for
the next handshake.
Change-Id: I35d13560f82bcb5eeda62f4de1571d28c818cc36
Reviewed-on: https://boringssl-review.googlesource.com/2770
Reviewed-by: Adam Langley <agl@google.com>
As of our 82b7da271f, an SSL_SESSION created
externally always has a cipher set. Unknown ciphers are rejected early. Prior
to that, an SSL_SESSION would only have a valid cipher or valid cipher_id
depending on whether it came from an internal or external session cache.
See upstream's 6a8afe2201cd888e472e44225d3c9ca5fae1ca62 and
c566205319beeaa196e247400c7eb0c16388372b for more context.
Since we don't get ourselves into this strange situation and s->cipher is now
always valid for established SSL_SESSION objects (the existence of
unestablished SSL_SESSION objects during a handshake is awkward, but something
to deal with later), do away with s->cipher_id altogether. An application
should be able to handle failing to parse an SSL_SESSION instead of parsing it
successfuly but rejecting all resumptions.
Change-Id: I2f064a815e0db657b109c7c9269ac6c726d1ffed
Reviewed-on: https://boringssl-review.googlesource.com/2703
Reviewed-by: Adam Langley <agl@google.com>
This CL removes the last of the EVP_CIPHER codepath in ssl/. The dead code is
intentionally not pruned for ease of review, except in DTLS-only code where
adding new logic to support both, only to remove half, would be cumbersome.
Fixes made:
- dtls1_retransmit_state is taught to retain aead_write_ctx rather than
enc_write_ctx.
- d1_pkt.c reserves space for the variable-length nonce when echoed into the
packet.
- dtls1_do_write sizes the MTU based on EVP_AEAD max overhead.
- tls1_change_cipher_state_cipher should not free AEAD write contexts in DTLS.
This matches the (rather confused) ownership for the EVP_CIPHER contexts.
I've added a TODO to resolve this craziness.
A follow-up CL will remove all the resultant dead code.
Change-Id: I644557f4db53bbfb182950823ab96d5e4c908866
Reviewed-on: https://boringssl-review.googlesource.com/2699
Reviewed-by: Adam Langley <agl@google.com>
This introduces another knob into SSL_AEAD_CTX to omit the version from the ad
parameter. It also allows us to fold a few more SSL3_ENC_METHOD hooks together.
Change-Id: I6540d410d4722f734093554fb434dab6e5217d4f
Reviewed-on: https://boringssl-review.googlesource.com/2698
Reviewed-by: Adam Langley <agl@google.com>
This lets us fold away the SSLv3-specific generate_master_secret. Once SSLv3
uses AEADs, others will fold away as well.
Change-Id: I27c1b75741823bc6db920d35f5dd5ce71b6fdbb3
Reviewed-on: https://boringssl-review.googlesource.com/2697
Reviewed-by: Adam Langley <agl@google.com>
Fix up the generate_master_secret parameter while we're here.
Change-Id: I1c80796d1f481be0c3eefcf3222f2d9fc1de4a51
Reviewed-on: https://boringssl-review.googlesource.com/2696
Reviewed-by: Adam Langley <agl@google.com>
size_t all the parameters. Also explicitly label label as label. This is in
preparation for pulling the PRF out into SSL3_ENC_METHOD so more of the
SSL3_ENC_METHOD hooks may be shared between SSLv3 and TLS once SSLv3 uses
stateful AEADs.
Also port away from EVP_PKEY_HMAC and use HMAC_CTX directly. The abstraction
doesn't buy much and is different from all the other EVP_DigestSign* functions.
There are few enough users within BoringSSL and Google that we can probably
deprecate and eventually remove it altogether.
Change-Id: I5d4529438c8a2a992fc199388a0c9e73bd6d2e06
Reviewed-on: https://boringssl-review.googlesource.com/2695
Reviewed-by: Adam Langley <agl@google.com>
The EVP_CIPHER codepath should no longer be used with TLS. It still exists for
DTLS and SSLv3. The AEAD construction in TLS does not allow for
variable-overhead AEADs, so stateful AEADs do not include the length in the ad
parameter. Rather the AEADs internally append the unpadded length once it is
known. EVP_aead_rc4_md5_tls is modified to account for this.
Tests are added (and RC4-MD5's regenerated) for each of the new AEADs. The
cipher tests are all moved into crypto/cipher/test because there's now a lot of
them and they clutter the directory listing.
In ssl/, the stateful AEAD logic is also modified to account for stateful AEADs
with a fixed IV component, and for AEADs which use a random nonce (for the
explicit-IV CBC mode ciphers).
The new implementation fixes a bug/quirk in stateless CBC mode ciphers where
the fixed IV portion of the keyblock was generated regardless. This is at the
end, so it's only relevant for EAP-TLS which generates a MSK from the end of
the key block.
Change-Id: I2d8b8aa11deb43bde2fd733f4f90b5d5b8cb1334
Reviewed-on: https://boringssl-review.googlesource.com/2692
Reviewed-by: Adam Langley <agl@google.com>
It's not worth saving the extra mallocs. This is preparation for moving SSLv3
to stateful AEADs; it'll share code TLS's SSL3_ENC_METHOD, but
tls1_generate_key_block is different, so that'll be pulled out into its own
hook.
Change-Id: I3f2136600758465c66ce23736041bb47f74efa6d
Reviewed-on: https://boringssl-review.googlesource.com/2690
Reviewed-by: Adam Langley <agl@google.com>
More modern versions of GCC (at least with aarch64) are warning about an
unused value in these locations. It's incorrect, but I guess that the
macro is confusing it.
Using a (void) tag is a little ugly but solves the problem.
Change-Id: If6ba5083ab6e501c81e7743ae1ed99a89565e57c
Reviewed-on: https://boringssl-review.googlesource.com/2810
Reviewed-by: Adam Langley <agl@google.com>
The minimum MTU (not consistently enforced) is just under 256, so it's
difficult to test everything, but this is a basic test. (E.g., without renego,
the only handshake message with encryption is Finished which fits in the MTU.)
It tests the server side because the Certificate message is large enough to
require fragmentation.
Change-Id: Ida11f1057cebae2b800ad13696f98bb3a7fbbc5e
Reviewed-on: https://boringssl-review.googlesource.com/2824
Reviewed-by: Adam Langley <agl@google.com>
This should have been removed with its dtls1_clear cousin in
8c88153465.
Change-Id: Ibf4ee67348f603285b26766568cbb92183b62cee
Reviewed-on: https://boringssl-review.googlesource.com/2823
Reviewed-by: Adam Langley <agl@google.com>
State on s3 gets freed in both ssl3_clear and ssl3_free. Considate to just
ssl3_free. This replaces the (SSL,ssl,ssl3)_clear calls in (SSL,ssl,ssl3)_new
with the state that was initialized. This results in a little code duplication
between SSL_new and SSL_clear because state is on the wrong object. I've just
left TODOs for now; some of it will need disentangling.
We're far from it, but going forward, separate state between s and s->s3 as:
- s contains configuration state, DTLS or TLS. It is initialized from SSL_CTX,
configurable directly afterwards, and preserved across SSL_clear calls.
(Including when it's implicitly set as part of a handshake callback.)
- Connection state hangs off s->s3 (TLS) and s->d1 (DTLS). It is reset across
SSL_clear. This should happen naturally out of a ssl_free/ssl_new pair.
The goal is to avoid needing separate initialize and reset code for anything;
the point any particular state is reset is the point its owning context is
destroyed and recreated.
Change-Id: I5d779010778109f8c339c07433a0777feaf94d1f
Reviewed-on: https://boringssl-review.googlesource.com/2822
Reviewed-by: Adam Langley <agl@google.com>
Configuration data inherited from the ctx happens in SSL_new. (This also gets
in the way of using ssl3_free/ssl3_new to implement SSL_clear.)
Change-Id: I2773af91abf4e1edc0c1a324bc1e94088d7c2274
Reviewed-on: https://boringssl-review.googlesource.com/2821
Reviewed-by: Adam Langley <agl@google.com>
A memory leak can occur in dtls1_buffer_record if either of the calls to
ssl3_setup_buffers or pqueue_insert fail. The former will fail if there
is a malloc failure, whilst the latter will fail if attempting to add a
duplicate record to the queue. This should never happen because
duplicate records should be detected and dropped before any attempt to
add them to the queue. Unfortunately records that arrive that are for
the next epoch are not being recorded correctly, and therefore replays
are not being detected. Additionally, these "should not happen" failures
that can occur in dtls1_buffer_record are not being treated as fatal and
therefore an attacker could exploit this by sending repeated replay
records for the next epoch, eventually causing a DoS through memory
exhaustion.
Thanks to Chris Mueller for reporting this issue and providing initial
analysis and a patch. Further analysis and the final patch was performed
by Matt Caswell from the OpenSSL development team.
CVE-2015-0206
(Imported from upstream's 7c6a3cf2375f5881ef3f3a58ac0fbd0b4663abd1).
Change-Id: I765fe61c75bc295bcc4ab356b8a5ce88c8964764
Reviewed-on: https://boringssl-review.googlesource.com/2782
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Add a dedicated error code to the queue for a handshake_failure alert in
response to ClientHello. This matches NSS's client behavior and gives a better
error on a (probable) failure to negotiate initial parameters.
BUG=https://crbug.com/446505
Change-Id: I34368712085a6cbf0031902daf2c00393783d96d
Reviewed-on: https://boringssl-review.googlesource.com/2751
Reviewed-by: Adam Langley <agl@google.com>
The |skip_message| variable was overly complex and, since we have at
least 32-bit ints, we know that a 24-bit value doesn't overflow an int.
Change-Id: I5c16fa979e1716f39cc47882c033bcf5bce3284c
Reviewed-on: https://boringssl-review.googlesource.com/2610
Reviewed-by: Adam Langley <agl@google.com>
RAND_pseudo_bytes just calls RAND_bytes now and only returns 0 or 1. Switch all
callers within the library call the new one and use the simpler failure check.
This fixes a few error checks that no longer work (< 0) and some missing ones.
Change-Id: Id51c79deec80075949f73fa1fbd7b76aac5570c6
Reviewed-on: https://boringssl-review.googlesource.com/2621
Reviewed-by: Adam Langley <agl@google.com>
No current use of ssl_cert_type passes a NULL EVP_PKEY, so it can be simplified
a little.
Change-Id: I2052cc3b6069cd30e4685ba8a6d0014016a4d712
Reviewed-on: https://boringssl-review.googlesource.com/2620
Reviewed-by: Adam Langley <agl@google.com>
Currently we don't express an opinion. Most sites aren't likely to have a
choice since it depends on what certificates they have available. But we may as
well order them.
Change-Id: I4fffa5e392f42e19823cb8faa2e9e15a6bb91086
Reviewed-on: https://boringssl-review.googlesource.com/2607
Reviewed-by: Adam Langley <agl@google.com>
Turns out the EVP_CIPH_FLAG_CUSTOM_CIPHER ciphers (i.e. legacy EVP_CIPHER
AES-GCM) have a completely different return value setup than the normal ones
which are the standard one/zero. (Except that they never return zero; see
TODO.)
Fix checks in ssl/ and remove remnants of EVP_CIPH_FLAG_CUSTOM_CIPHER in ssl/
as we're using EVP_AEAD now.
See CHANGES entry added in upstream's 3da0ca796cae6625bd26418afe0a1dc47bf5a77f.
Change-Id: Ia4d0ff59b03c35fab3a08141c60b9534cb7172e2
Reviewed-on: https://boringssl-review.googlesource.com/2606
Reviewed-by: Adam Langley <agl@google.com>
Those version checks are if renego tried to change the version, but at that
point we're out of the initial null cipher and should leave the version fixed.
(On the server end, the code in question was dead after the version negotiation
rewrite anyway.)
Change-Id: I3242ba11bc9981ccf7fdb867176d59846cc49dd9
Reviewed-on: https://boringssl-review.googlesource.com/2605
Reviewed-by: Adam Langley <agl@google.com>
This avoids needing a should_add_to_finished_hash boolean on do_write. The
logic in do_write was a little awkward because do_write would be called
multiple times if the write took several iterations. This also gets complex if
DTLS retransmits are involved. (At a glance, it's not obvious the
BIO_CTRL_DGRAM_MTU_EXCEEDED case actually works.)
Doing it as the handshake message is being prepared avoids this concern. It
also gives a natural point for the extended master secret logic which needs to
do work after the finished hash has been sampled.
As a bonus, we can remove s->d1->retransmitting which was only used to deal
with this issue.
Change-Id: Ifedf23ee4a6c5e08f960d296a6eb1f337a16dc7a
Reviewed-on: https://boringssl-review.googlesource.com/2604
Reviewed-by: Adam Langley <agl@google.com>
That comment is wrong as of TLS 1.2.
Change-Id: I900d5efc09d7468f2601d85f867833e43d046f6a
Reviewed-on: https://boringssl-review.googlesource.com/2603
Reviewed-by: Adam Langley <agl@google.com>
(Or should we just drop this? It only matters for servers trying to use client
auth.)
Change-Id: I50b6999375dc8f9246bf617f17929ae304503c57
Reviewed-on: https://boringssl-review.googlesource.com/2602
Reviewed-by: Adam Langley <agl@google.com>
Some of the messages did the computation manually which would bite us if we
tried to transplant them between DTLS and TLS. More importantly, it precludes
moving the handshake hash computation from ssl_do_write to
ssl_set_handshake_header.
Change-Id: I9d400deb0720e62cb1ab905242eb0679ad650a46
Reviewed-on: https://boringssl-review.googlesource.com/2600
Reviewed-by: Adam Langley <agl@google.com>
The frag_off/frag_len parameters are always zero, and the return value is never
used.
Change-Id: If7487b23c55f2a996e411b25b76a8e1651f25d8b
Reviewed-on: https://boringssl-review.googlesource.com/2601
Reviewed-by: Adam Langley <agl@google.com>
This change has no semantic effect (I hope!). It's just a reformatting
of a few files in ssl/. This is just a start – the other files in ssl/
should follow in the coming days.
Change-Id: I5eb3f4b18d0d46349d0f94d3fe5ab2003db5364e
David is heading out so I didn't want to block the previous batch of
changes for weeks. Thus I landed them as-is and this change tweaks a
couple of things that would normally have been addressed in code-review.
Change-Id: I2579dbc43d93fea34a52b4041f5511d70217aaf7
Ensure that both the client and the server emit a protocol_version alert
(except in SSLv3 where it doesn't exist) with a record-layer version which the
peer will recognize.
Change-Id: I31650a64fe9b027ff3d51e303711910a00b43d6f
This makes SSLv23_method go through DTLS_ANY_VERSION's version negotiation
logic. This allows us to get rid of duplicate ClientHello logic. For
compatibility, SSL_METHOD is now split into SSL_PROTOCOL_METHOD and a version.
The legacy version-locked methods set min_version and max_version based this
version field to emulate the original semantics.
As a bonus, we can now handle fragmented ClientHello versions now.
Because SSLv23_method is a silly name, deprecate that too and introduce
TLS_method.
Change-Id: I8b3df2b427ae34c44ecf972f466ad64dc3dbb171
Tested manually by replacing SSLv23_method() with TLSv1_2_method() in
bssl_shim. This is a large chunk of code which is not run in SSLv23_method(),
but it will be run after unification. It's split out separately to ease review.
Change-Id: I6bd241daca17aa0f9b3e36e51864a29755a41097
Match the DTLS code. Rather than sniffing the handshake state, use the
have_version bit.
Change-Id: I40e92f187647417c34b4cfdc3ad258f5562e781b
Reviewed-on: https://boringssl-review.googlesource.com/2588
Reviewed-by: Adam Langley <agl@google.com>
These tests use both APIs. This also modifies the inline version negotiation's
error codes (currently only used for DTLS) to align with SSLv23's error codes.
Note: the peer should send a protocol_version alert which is currently untested
because it's broken.
Upstream would send such an alert if TLS 1.0 was supported but not otherwise,
which is somewhat bizarre. We've actually regressed and never send the alert in
SSLv23. When version negotiation is unified, we'll get the alerts back.
Change-Id: I4c77bcef3a3cd54a039a642f189785cd34387410
Reviewed-on: https://boringssl-review.googlesource.com/2584
Reviewed-by: Adam Langley <agl@google.com>
Amend the version negotiation tests to test this new spelling of max_version.
min_version will be tested in a follow-up.
Change-Id: Ic4bfcd43bc4e5f951140966f64bb5fd3e2472b01
Reviewed-on: https://boringssl-review.googlesource.com/2583
Reviewed-by: Adam Langley <agl@google.com>
DTLS_method() can now negotiate versions without switching methods.
Change-Id: I0655b3221b6e7e4b3ed4acc45f1f41c594447021
Reviewed-on: https://boringssl-review.googlesource.com/2582
Reviewed-by: Adam Langley <agl@google.com>
SSL3_ENC_METHOD will remain version-specific while SSL_METHOD will become
protocol-specific. This finally removes all the version-specific portions of
SSL_METHOD but the version tag itself.
(SSL3_ENC_METHOD's version-specific bits themselves can probably be handled by
tracking a canonicalized protocol version. It would simplify version
comparisons anyway. The one catch is SSLv3 has a very different table. But
that's a cleanup for future. Then again, perhaps a version-specific method
table swap somewhere will be useful later for TLS 1.3.)
Much of this commit was generated with sed invocation:
s/method->ssl3_enc/enc_method/g
Change-Id: I2b192507876aadd4f9310240687e562e56e6c0b1
Reviewed-on: https://boringssl-review.googlesource.com/2581
Reviewed-by: Adam Langley <agl@google.com>
Now SSLv23 and DTLS_ANY_VERSION share version-related helper functions.
ssl3_get_method is temporary until the method switch is no longer necessary.
Put them all together so there's one place to refactor them when we add a new
version or implement min_version/max_version controls.
Change-Id: Ic28a145cad22db08a87fdb854480b22886c451c6
Reviewed-on: https://boringssl-review.googlesource.com/2580
Reviewed-by: Adam Langley <agl@google.com>
Missed this one. It requires that we be able to change an SSL_METHOD after the
after, which complicates compiling the version locking into min_version /
max_version configurations.
Change-Id: I24ba54b7939360bbfafe3feb355a65840bda7611
Reviewed-on: https://boringssl-review.googlesource.com/2579
Reviewed-by: Adam Langley <agl@google.com>
SSL_ST_BEFORE isn't a possible state anymore. It seems this state meant the
side wasn't known, back in the early SSLeay days. Now upstream guesses
(sometimes incorrectly with generic methods), and we don't initialize until
later. SSL_shutdown also doesn't bother to call ssl3_shutdown at all if the
side isn't initialized and SSL_ST_BEFORE isn't the uninitialized state, which
seems a much more sensible arrangement.
Likewise, because bare SSL_ST_BEFOREs no longer exist, SSL_in_init implies
SSL_in_before and there is no need to check both.
Change-Id: Ie680838b2f860b895073dabb4d759996e21c2824
Reviewed-on: https://boringssl-review.googlesource.com/2564
Reviewed-by: Adam Langley <agl@google.com>
These may as well be replaced with assertions. Get them out of the way of the
initialization.
Change-Id: Ie4ab8bdc018e4a1def7d3f6b3b172a77896bfc0a
Reviewed-on: https://boringssl-review.googlesource.com/2563
Reviewed-by: Adam Langley <agl@google.com>
They're always known now. Also fix the SSLv23_{client,server}_method
definitions still had their own macro invocations.
Change-Id: Ia13f29a27f2331d25a4051e83f2d5abc62fab981
Reviewed-on: https://boringssl-review.googlesource.com/2562
Reviewed-by: Adam Langley <agl@google.com>
There's an undefined one not used anywhere. The others ought to be const. Also
move the forward declaration to ssl.h so we don't have to use the struct name.
Change-Id: I76684cf65255535c677ec19154cac74317c289ba
Reviewed-on: https://boringssl-review.googlesource.com/2561
Reviewed-by: Adam Langley <agl@google.com>
else block got lost in a rewrite of this code.
Change-Id: I51f1655474ec8bbd4eccb4297124e8584329444e
Reviewed-on: https://boringssl-review.googlesource.com/2560
Reviewed-by: Adam Langley <agl@google.com>
The client_version needs to be preserved, both for the RSA key exchange and
(when this codepath is used for TLS) for the SChannel renego workaround. Fix
the tests to enforce this so the cipher suite version tests catch this.
Change-Id: I0c42dc3ec4830f3724026b400e5066e7a7f1ee97
Reviewed-on: https://boringssl-review.googlesource.com/2551
Reviewed-by: Adam Langley <agl@google.com>
There's no need to make that conditional.
Change-Id: Idac1aba42b22e3fe8e7731ae4ecb5ebc4183336c
Reviewed-on: https://boringssl-review.googlesource.com/2550
Reviewed-by: Adam Langley <agl@google.com>
Comparing data is a much easier idiom than CBS_skip + a CBS_len check.
Change-Id: I3efe925734c76f3494cad682445291ae83750a7e
Reviewed-on: https://boringssl-review.googlesource.com/2500
Reviewed-by: Adam Langley <agl@google.com>
The record-layer version of the ServerHello should match the final version. The
record-layer version of the ClientHello should be the advertised version, but
clamped at TLS 1.0. This is to ensure future rewrites do not regress this.
Change-Id: I96f1f0674944997ff38b562453a322ce61652635
Reviewed-on: https://boringssl-review.googlesource.com/2540
Reviewed-by: Adam Langley <agl@google.com>
Bruce Dawson pointed out that the shadowing of |ret| in |s3_srvr.c|
looked dodgy. It was actually deliberate (we don't want to reset the
default value of the function's |ret| variable with a successful return
from the callback) but it does look dodgy.
This change adds -Wshadow to ban variable shadowing and fixes all
current instances.
Change-Id: I1268f88b9f26245c7d16d6ead5bb9014ea471c01
Reviewed-on: https://boringssl-review.googlesource.com/2520
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 7a04b854d655785798d471df25ffd5036f3cc46b.)
This does not affect BoringSSL as ssl3_get_client_hello advances to yet another
state immediately after reading the message. But the state advance is correct.
It matches the normal exit for this function.
Change-Id: I8a664f2ad5f80beacbaf3e17a7786a5c9e8ef30e
Reviewed-on: https://boringssl-review.googlesource.com/2480
Reviewed-by: Piotr Sikora <piotr@cloudflare.com>
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 4b87706d20f0a2fdf2e8f1b90256e141c487ef47 and
eceef8fb865eb5de329b27ea472d4fdea4c290fe.)
Dead code.
Change-Id: I58120c3a9c42cb9db27f404774778222c3bb642a
Reviewed-on: https://boringssl-review.googlesource.com/2479
Reviewed-by: Adam Langley <agl@google.com>
PR#1767
(Imported from upstream's fe78f08d1541211566a5656395186bfbdc61b6f8)
Not sure this is reachable (upstream's PR references custom engines), but
better be tidy. Note this is slightly different from upstream's: EVP_Cipher is
documented to return -1 on failure, not 0.
Change-Id: I836f12b73c6912a8ae8cbd37cfd3d33466acbc9e
Reviewed-on: https://boringssl-review.googlesource.com/2478
Reviewed-by: Adam Langley <agl@google.com>
PR#3613
(Imported from upstream's fc3968a25ce0c16cab8730ec0d68a59856158029)
We don't care about GOST, but removing redundant code is reasonable. Also
switch that CRYPTO_add to EVP_PKEY_dup. Missed a spot.
Change-Id: I768ec546d987fb3d8bc3decf7ebf1a5590fbb6c2
Reviewed-on: https://boringssl-review.googlesource.com/2477
Reviewed-by: Adam Langley <agl@google.com>
Some code predated the RFCs themselves, but the RFCs now exist. Also remove
now obsolete comments and some unused #defines.
See upstream's cffeacd91e70712c99c431bf32a655fa1b561482. (Though this predates
it; I just remembered I never uploaded it.)
Change-Id: I5e56f0ab6b7f558820f72e84dfdbc71a8c23cb91
Reviewed-on: https://boringssl-review.googlesource.com/2475
Reviewed-by: Adam Langley <agl@google.com>
The ClientHello record is padded to 1024 bytes when
fastradio_padding is enabled. As a result, the 3G cellular radio
is fast forwarded to DCH (high data rate) state. This mechanism
leads to a substantial redunction in terms of TLS handshake
latency, and benefits mobile apps that are running on top of TLS.
Change-Id: I3d55197b6d601761c94c0f22871774b5a3dad614
MSVC doesn't like it when you compare the two.
Change-Id: I03c5ff2e2668ac2e536de8278e3a7c98a3dfd117
Reviewed-on: https://boringssl-review.googlesource.com/2460
Reviewed-by: Adam Langley <agl@google.com>
It just inserts extra flushes everywhere and isn't used.
Change-Id: I082e4bada405611f4986ba852dd5575265854036
Reviewed-on: https://boringssl-review.googlesource.com/2456
Reviewed-by: Adam Langley <agl@google.com>
Use it in ssl3_cert_verify_hash so signing a pre-TLS-1.2 handshake hash can go
through RSA_sign and be intercepted via RSA_METHOD appropriately. This avoids
Windows needing to intercept sign_raw. (CAPI keys cannot provide sign_raw,
unless the input size happens to be that of NID_md5_sha1.)
Also use it in processing ServerKeyExchange to avoid special-casing RSA.
BUG=crbug.com/437023
Change-Id: Ia07433f468b75fdf7bfc8fa90c9751639b2478e6
Reviewed-on: https://boringssl-review.googlesource.com/2420
Reviewed-by: David Benjamin <davidben@google.com>
Don't link with dl, except on Linux where we have malloc tests.
Change-Id: I7b23acc854172e64628a55acecfaa9a661f74f77
Reviewed-on: https://boringssl-review.googlesource.com/2453
Reviewed-by: Adam Langley <agl@google.com>
The comment has it right, but the rewritten code was wrong.
Change-Id: I450193c39fb62eae32aae090a3834dd83db53421
Reviewed-on: https://boringssl-review.googlesource.com/2444
Reviewed-by: Adam Langley <agl@google.com>
Replace the comment with a clearer one and reimplement it much more tidily. The
mask thing was more complicated than was needed.
This slightly changes behavior on the DTLS_ANY_VERSION side in that, if only
one method is enabled, we no longer short-circuit to the version-locked method
early. This "optimization" seems unnecessary.
Change-Id: I571c8b60ed16bd4357c67d65df0dd1ef9cc5eb57
Reviewed-on: https://boringssl-review.googlesource.com/2451
Reviewed-by: Adam Langley <agl@google.com>
It should be set correctly prior to entering the handshake. Don't mask bugs by
assigning it.
Change-Id: Ib9bca8fad68916b3b242aad8819e3760e59e777a
Reviewed-on: https://boringssl-review.googlesource.com/2443
Reviewed-by: Adam Langley <agl@google.com>
first_packet is a temporary connection-global flag set for the duration of some
call and then queried from other code. This kind of logic is too difficult to
reason through. It also incorrectly treats renegotiate ClientHellos as
pre-version-negotiation records. This eliminates the need to query
enc_write_ctx (which wasn't EVP_AEAD-aware anyway).
Instead, take a leaf from Go TLS's book and add a have_version bit. This is
placed on s->s3 as it is connection state; s->s3 automatically gets reset on
SSL_clear while s doesn't.
This new flag will also be used to determine whether to do the V2ClientHello
sniff when the version-locked methods merge into SSLv23_method. It will also
replace needing to condition s->method against a dummy DTLS_ANY_VERSION value
to determine whether DTLS version negotiation has happened yet.
Change-Id: I5c8bc6258b182ba4ab175a48a84eab6d3a001333
Reviewed-on: https://boringssl-review.googlesource.com/2442
Reviewed-by: Adam Langley <agl@google.com>
Supporting both schemes seems pointless. Now that s->server and s->state are
set appropriately late and get_ssl_method is gone, the only difference is that
the client/server ones have non-functional ssl_accept or ssl_connect hooks. We
can't lose the generic ones, so let's unify on that.
Note: this means a static linker will no longer drop the client or server
handshake code if unused by a consumer linking statically. However, Chromium
needs the server half anyway for DTLS and WebRTC, so that's probably a lost
cause. Android also exposes server APIs.
Change-Id: I290f5fb4ed558f59fadb5d1f84e9d9c405004c23
Reviewed-on: https://boringssl-review.googlesource.com/2440
Reviewed-by: Adam Langley <agl@google.com>
If SSL_clear is called before SSL_set_{connect,accept}_state (as SSL_new does
internally), s->state will get set prematurely. Likewise, s->server is set
based on the method's ssl_accept hook, but client SSL's may be initialized from
a generic SSL_METHOD too.
Since we can't easily get rid of the generic SSL_METHODs, defer s->state and
s->server initialization until the side is known.
Change-Id: I0972e17083df22a3c09f6f087011b54c699a22e7
Reviewed-on: https://boringssl-review.googlesource.com/2439
Reviewed-by: Adam Langley <agl@google.com>
It's unused. Also per the previous commit message, it historically had a bug
anyway.
Change-Id: I5868641e7938ddebbc0ffd72d218c81cd17c7739
Reviewed-on: https://boringssl-review.googlesource.com/2437
Reviewed-by: Adam Langley <agl@google.com>
