clang-format has changed a little. This is a semantic no-op but it makes
the diff in the next change smaller.
Change-Id: Ia492a81340a868b888d619a1c7740d1a86845e92
(This change is for a future change that increases the range of the
return values.)
(Imported from upstream's 3fc0b1edad0c75d7beb51fa77f63ffe817295e2c.)
Change-Id: I221d4ee0e90586f89f731e01ff4d813058173211
Just store NUL-terminated strings. This works better when we add
support for multiple hostnames.
(Imported from upstream's d93edc0aab98377f42dd19312248597a018a7889.)
Change-Id: Ib3bf8a8c654b829b4432782ba21ba55c3d4a0582
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>
Go is not required for running all the tests and bash isn't.
Change-Id: I04d5981dbd2203e8bae27a1265a5db5e35ae5279
Reviewed-on: https://boringssl-review.googlesource.com/3450
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>
This supports -valgrind as with runner.go. It also works on Windows and
provides a place for implementing Chrome infra's JSON test output format in the
future, as well as whatever magic may be needed for Android.
Change-Id: I26eb68053f95e825561a142dbcdc4fbd84e3687d
Reviewed-on: https://boringssl-review.googlesource.com/3411
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>
This saves about 6-7k of error data.
Change-Id: Ic28593d4a1f5454f00fb2399d281c351ee57fb14
Reviewed-on: https://boringssl-review.googlesource.com/3385
Reviewed-by: Adam Langley <agl@google.com>
Some files in crypto/x509 were moved from crypto/asn1, so they emit errors from
another module. Fix make_errors.go to account for this: cross module errors
must use the foreign module as the first argument to OPENSSL_PUT_ERROR. Both
the function code and the error code should be declared in the foreign module.
Update make_errors.go to ignore cross-module error lines when deciding which
function tokens to emit.
Change-Id: Ic38377ddd56e22d033ef91318c30510762f6445d
Reviewed-on: https://boringssl-review.googlesource.com/3383
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>
Otherwise running git grep for a single function gives a ton of noise.
Change-Id: I18900d6269fd2be39ef9b579419aee1c7eca9143
Reviewed-on: https://boringssl-review.googlesource.com/3382
Reviewed-by: Adam Langley <agl@google.com>
Sort all the files before processing them.
Change-Id: Id6b4519fa22f1770bb2ba2a792f5c27de9ea452d
Reviewed-on: https://boringssl-review.googlesource.com/3380
Reviewed-by: Adam Langley <agl@google.com>
Also, Clang doesn't like static asserts with the same message and
ERR_free_strings should still free the error queues, although it's badly
misnamed now.
Change-Id: Ibff8eb50f93c0b56c3eeb17a300e8501a31c3ab8
Reviewed-on: https://boringssl-review.googlesource.com/3370
Reviewed-by: David Benjamin <davidben@chromium.org>
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>
The ENGINE code had a concept of a stable-ABI for METHODs, because that
might be a useful thing in the future when people want to have blobs
that wrap PKCS#11 or something.
However, at the moment nobody uses this feature and it didn't work very
well anyway: I hadn't updated |ENGINE_free| to free them all and
|set_method| was copying the methods, but not resetting the |is_static|
flag.
This change removes support for non-static methods. We can always put it
back later if we need.
Change-Id: Ic7401c8cb1cadd46b26a215f85bc48562efe9919
Reviewed-on: https://boringssl-review.googlesource.com/3300
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>
We assume that AArch64 systems are modern enough to have getauxval.
Change-Id: I4cc74f04ca5ed50b8ca1cfd00afeaaa01c6caca0
Reviewed-on: https://boringssl-review.googlesource.com/3280
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>
