It's never called in outside code. This too seems to be a remnant of the DSA
PKIX optional parameter stuff. This is confirmed both by a removed comment and
by the brief documentation at http://www.umich.edu/~x509/ssleay/x509_pkey.html
RFC 5480 does not allow ECDSA keys to be missing parameters, so this logic is
incorrect for ECDSA anyway. It was also failing to check
EVP_PKEY_copy_parameters' return value. And that logic looks pretty suspect if
you have a chain made up multiple certificate types.
Change-Id: Id6c60659a0162356c7f3eae5c797047366baae1c
Reviewed-on: https://boringssl-review.googlesource.com/3485
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>
Change-Id: I34ee66fcc53d3371591beee3373c46598c31b5c5
Reviewed-on: https://boringssl-review.googlesource.com/3460
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Reduces number of silly casts in OpenSSL code and likely most
applications. Consistent with (char *) for "peername" value from
X509_check_host() and X509_VERIFY_PARAM_get0_peername().
(Imported from upstream's e83c913723fac7432a7706812f12394aaa00e8ce.)
Change-Id: Id0fc11773a0cee8933978cd4bdbd8251fd7cfb5f
Pass address of X509_VERIFY_PARAM_ID peername to X509_check_host().
(Imported from upstream's 55fe56837a65ff505b492aa6aee748bf5fa91fec.)
Change-Id: Ic21bfb361b8eb25677c4c2175882fa95ea44fc31
(Imported from upstream's 8abffa4a73fcbf6536e0a42d736ed9211a8204ea,
9624b50d51de25bb2e3a72e81fe45032d80ea5c2 and
41e3ebd5abacfdf98461cdeb6fa97a4175b7aad3.)
Change-Id: Ic9099eb5704b19b4500229e89351371cc6184f9d
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>
