Share a bit more of it between TLS 1.2 and 1.3.
Change-Id: I43c9dbf785a3d33db1793cffb0fdbd3af075cc89
Reviewed-on: https://boringssl-review.googlesource.com/8849
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Our CBB patterns do not make it safe to use a CBB after any operation
failed. Suppose one does:
int add_to_cbb(CBB *cbb) {
CBB child;
return CBB_add_u8(cbb, 1) &&
CBB_add_u8_length_prefixed(cbb, &child) &&
CBB_add_u8(&child, 2) &&
/* Flush |cbb| before |child| goes out of scoped. */
CBB_flush(cbb);
}
If one of the earlier operations fails, any attempt to use |cbb| (except
CBB_cleanup) would hit a memory error. Doing this would be a bug anyway,
since the CBB would be in an undefined state anyway (wrote only half my
object), but the memory error is bad manners.
Officially document that using a CBB after failure is illegal and, to
avoid the memory error, set a poison bit on the cbb_buffer_st to prevent
all future operations. In theory we could make failure +
CBB_discard_child work, but this is not very useful and would require a
more complex CBB pattern.
Change-Id: I4303ee1c326785849ce12b5f7aa8bbde6b95d2ec
Reviewed-on: https://boringssl-review.googlesource.com/8840
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This adds the machinery for doing TLS 1.3 1RTT.
Change-Id: I736921ffe9dc6f6e64a08a836df6bb166d20f504
Reviewed-on: https://boringssl-review.googlesource.com/8720
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is the equivalent of FragmentAcrossChangeCipherSuite for DTLS. It
is possible for us to, while receiving pre-CCS handshake messages, to
buffer up a message with sequence number meant for a post-CCS Finished.
When we then get to the new epoch and attempt to read the Finished, we
will process the buffered Finished although it was sent with the wrong
encryption.
Move ssl_set_{read,write}_state to SSL_PROTOCOL_METHOD hooks as this is
a property of the transport. Notably, read_state may fail. In DTLS
check the handshake buffer size. We could place this check in
read_change_cipher_spec, but TLS 1.3 has no ChangeCipherSpec message, so
we will need to implement this at the cipher change point anyway. (For
now, there is only an assert on the TLS side. This will be replaced with
a proper check in TLS 1.3.)
Change-Id: Ia52b0b81e7db53e9ed2d4f6d334a1cce13e93297
Reviewed-on: https://boringssl-review.googlesource.com/8790
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
prk should be a const parameter.
Change-Id: I2369ed9f87fc3c59afc07d3b667b86aec340052e
Reviewed-on: https://boringssl-review.googlesource.com/8810
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It was pointed out that the equivalent values may sometimes be hard to
find.
Change-Id: I02a1790e026047b3dc2034c2f9ad75abc9e59eb7
Reviewed-on: https://boringssl-review.googlesource.com/8800
Reviewed-by: Adam Langley <agl@google.com>
This allows us to implement custom RSA-PSS-based keys, so the async TLS
1.3 tests can proceed. For now, both sign and sign_digest exist, so
downstreams only need to manage a small change atomically. We'll remove
sign_digest separately.
In doing so, fold all the *_complete hooks into a single complete hook
as no one who implemented two operations ever used different function
pointers for them.
While I'm here, I've bumped BORINGSSL_API_VERSION. I do not believe we
have any SSL_PRIVATE_KEY_METHOD versions who cannot update atomically,
but save a round-trip in case we do. It's free.
Change-Id: I7f031aabfb3343805deee429b9e244aed5d76aed
Reviewed-on: https://boringssl-review.googlesource.com/8786
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This makes custom private keys and EVP_PKEYs symmetric again. There is
no longer a requirement that the caller pre-filter the configured
signing prefs.
Also switch EVP_PKEY_RSA to NID_rsaEncryption. These are identical, but
if some key types are to be NIDs, we should make them all NIDs.
Change-Id: I82ea41c27a3c57f4c4401ffe1ccad406783e4c64
Reviewed-on: https://boringssl-review.googlesource.com/8785
Reviewed-by: David Benjamin <davidben@google.com>
This gives us a sigalg-based API for configuring signing algorithms.
Change-Id: Ib746a56ebd1061eadd2620cdb140d5171b59bc02
Reviewed-on: https://boringssl-review.googlesource.com/8784
Reviewed-by: Adam Langley <agl@google.com>
clang-format is being really insistent on reformatting these even when I
tell it only to reformat a small region far away.
Change-Id: I46cfd40e8c8658b73caee9c7deae65265c42f762
Reviewed-on: https://boringssl-review.googlesource.com/8787
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
For now, skip the 1.2 -> 1.1 signal since that will affect shipping
code. We may as well enable it too, but wait until things have settled
down. This implements the version in draft-14 since draft-13's isn't
backwards-compatible.
Change-Id: I46be43e6f4c5203eb4ae006d1c6a2fe7d7a949ec
Reviewed-on: https://boringssl-review.googlesource.com/8724
Reviewed-by: David Benjamin <davidben@google.com>
[Rebased and tests added by davidben.]
In doing so, regenerate the test RSA certificate to be 2048-bit RSA.
RSA-PSS with SHA-512 is actually too large for 1024-bit RSA. Also make
the sigalg test loop test versions that do and don't work which subsumes
the ecdsa_sha1 TLS 1.3 test.
For now, RSA-PKCS1 is still allowed because NSS has yet to implement
RSA-PSS and we'd like to avoid complicated interop testing.
Change-Id: I686b003ef7042ff757bdaab8d5838b7a4d6edd87
Reviewed-on: https://boringssl-review.googlesource.com/8613
Reviewed-by: David Benjamin <davidben@google.com>
Upstream have added |EVP_PKEY_up_ref|, but their version returns an int.
Having this function with a different signature like that is dangerous
so this change aligns BoringSSL with upstream. Users of this function in
Chromium and internally should already have been updated.
Change-Id: I0a7aeaf1a1ca3b0f0c635e2ee3826aa100b18157
Reviewed-on: https://boringssl-review.googlesource.com/8736
Reviewed-by: David Benjamin <davidben@google.com>
libssh2 expects this function.
Change-Id: Ie2d6ceb25d1b633e1363e82f8a6c187b75a4319f
Reviewed-on: https://boringssl-review.googlesource.com/8735
Reviewed-by: David Benjamin <davidben@google.com>
Rather than blindly select SHA-1 if we can't find a matching one, act as
if the peer advertised rsa_pkcs1_sha1 and ecdsa_sha1. This means that we
will fail the handshake if no common algorithm may be found.
This is done in preparation for removing the SHA-1 default in TLS 1.3.
Change-Id: I3584947909d3d6988b940f9404044cace265b20d
Reviewed-on: https://boringssl-review.googlesource.com/8695
Reviewed-by: David Benjamin <davidben@google.com>
This reverts commits:
8d79ed674019fdcb52348d79ed6740
Because WebRTC (at least) includes our headers in an extern "C" block,
which precludes having any C++ in them.
Change-Id: Ia849f43795a40034cbd45b22ea680b51aab28b2d
MSVC doesn't define __cplusplus as 201103 to indicate C++11 support, so
just assume that the compiler supports C++11 if _MSC_VER is defined.
Change-Id: I27f6eeefe6e8dc522470f36fab76ab36d85eebac
Reviewed-on: https://boringssl-review.googlesource.com/8734
Reviewed-by: David Benjamin <davidben@google.com>
Now that we have template code in them, that doesn't work.
Change-Id: I9ead5d202b0d8c9b848cf25a1f247f824394a168
Reviewed-on: https://boringssl-review.googlesource.com/8733
Reviewed-by: David Benjamin <davidben@google.com>
This change scatters the contents of the two scoped_types.h files into
the headers for each of the areas of the code. The types are now in the
|bssl| namespace.
Change-Id: I802b8de68fba4786b6a0ac1bacd11d81d5842423
Reviewed-on: https://boringssl-review.googlesource.com/8731
Reviewed-by: Adam Langley <agl@google.com>
We currently have the situation where the |tool| and |bssl_shim| code
includes scoped_types.h from crypto/test and ssl/test. That's weird and
shouldn't happen. Also, our C++ consumers might quite like to have
access to the scoped types.
Thus this change moves some of the template code to base.h and puts it
all in a |bssl| namespace to prepare for scattering these types into
their respective headers. In order that all the existing test code be
able to access these types, it's all moved into the same namespace.
Change-Id: I3207e29474dc5fcc344ace43119df26dae04eabb
Reviewed-on: https://boringssl-review.googlesource.com/8730
Reviewed-by: David Benjamin <davidben@google.com>
It still places the current message all over the place, but remove the
bizarre init_num/error/ok split. Now callers get the message length out
of init_num, which mirrors init_msg. Also fix some signedness.
Change-Id: Ic2e97b6b99e234926504ff217b8aedae85ba6596
Reviewed-on: https://boringssl-review.googlesource.com/8690
Reviewed-by: David Benjamin <davidben@google.com>
Chromium no longer uses it.
Change-Id: I50cc55bad4124305686d299032a2e8ed2cb9d0d7
Reviewed-on: https://boringssl-review.googlesource.com/8691
Reviewed-by: David Benjamin <davidben@google.com>
V2ClientHello is going to be ugly wherever we do it, but this hides it
behind the transport method table. It removes a place where the
handshake state machine reaches into ssl3_get_message's internal state.
ssl3_get_message will now silently translate V2ClientHellos into true
ClientHellos and manage the handshake hash appropriately.
Now the only accesses of init_buf from the handshake state machines are
to create and destroy the buffer.
Change-Id: I81467a038f6ac472a465eec7486a443fe50a98e1
Reviewed-on: https://boringssl-review.googlesource.com/8641
Reviewed-by: Adam Langley <agl@google.com>
To match the Go side. That message will never be used for anything else,
so there's not much need to give it such a long name.
Change-Id: I3396c9d513d02d873e59cd8e81ee64005c5c706c
Reviewed-on: https://boringssl-review.googlesource.com/8620
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also, update the documentation about aliasing for |BN_usub|. It might
be better to find a way to factor out the shared logic between the
tests of these functions and the tests of |BN_add| and |BN_usub|, but
doing so would end up up creating a lot of parameters due to the many
distinct strings used in the messages.
Change-Id: Ic9d714858212fc92aa6bbcc3959576fe6bbf58c3
Reviewed-on: https://boringssl-review.googlesource.com/8593
Reviewed-by: David Benjamin <davidben@google.com>
Also update the documentation for |BN_sub|.
Change-Id: I544dbfc56f22844f6ca08e9e472ec13e76baf8c4
Reviewed-on: https://boringssl-review.googlesource.com/8592
Reviewed-by: David Benjamin <davidben@google.com>
Upstream added this in a18a31e49d266. The various *_up_ref functions
return a variety of types, but this one returns int because upstream
appears to be trying to unify around that. (See upstream's c5ebfcab713.)
Change-Id: I7e1cfe78c3a32f5a85b1b3c14428bd91548aba6d
Reviewed-on: https://boringssl-review.googlesource.com/8581
Reviewed-by: Adam Langley <alangley@gmail.com>
In order to delay the digest of the handshake transcript and unify
around message-based signing callbacks, a copy of the transcript is kept
around until we are sure there is no certificate authentication.
This removes support for SSL_PRIVATE_KEY_METHOD as a client in SSL 3.0.
Change-Id: If8999a19ca021b4ff439319ab91e2cd2103caa64
Reviewed-on: https://boringssl-review.googlesource.com/8561
Reviewed-by: David Benjamin <davidben@google.com>
I'd meant to change the other -latest to -13 when I merged this, but we
may as well group the two together anyway. Also remove ticket_age as
that's likely to go away in PR#503.
Change-Id: Ibb2f447e344d0b13c937291de69ace37ac9a5e8d
Reviewed-on: https://boringssl-review.googlesource.com/8567
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This replaces the old key_exchange_info APIs and does not require the
caller be aware of the mess around SSL_SESSION management. They
currently have the same bugs around renegotiation as before, but later
work to fix up SSL_SESSION tracking will fix their internals.
For consistency with the existing functions, I've kept the public API at
'curve' rather than 'group' for now. I think it's probably better to
have only one name with a single explanation in the section header
rather than half and half. (I also wouldn't be surprised if the IETF
ends up renaming 'group' again to 'key exchange' at some point. We'll
see what happens.)
Change-Id: I8e90a503bc4045d12f30835c86de64ef9f2d07c8
Reviewed-on: https://boringssl-review.googlesource.com/8565
Reviewed-by: Adam Langley <agl@google.com>
This isn't filled in on the client and Chromium no longer uses it for
plain RSA. It's redundant with existing APIs. This is part of removing
the need for callers to call SSL_get_session where possible.
SSL_get_session is ambiguous when it comes to renego. Some code wants
the current connection state which should not include the pending
handshake and some code wants the handshake scratch space which should.
Renego doesn't exist in TLS 1.3, but TLS 1.3 makes NewSessionTicket a
post-handshake message, so SSL_get_session is somewhat silly of an API
there too.
SSL_SESSION_get_key_exchange_info is a BoringSSL-only API, so we can
freely change it and replace it with APIs keyed on SSL. In doing so, I
think it is better to provide APIs like "SSL_get_dhe_group_size" and
"SSL_get_curve_id" rather than make the caller do the multi-step
SSL_get_current_cipher / SSL_CIPHER_is_ECDHE dance. To that end, RSA
key_exchange_info is pointless as it can already be determined from the
peer certificate.
Change-Id: Ie90523083d8649701c17934b7be0383502a0caa3
Reviewed-on: https://boringssl-review.googlesource.com/8564
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's SSL_OP_NO_* flags allow discontinuous version ranges. This is a
nuisance for two reasons. First it makes it unnecessarily difficult to answer
"are any versions below TLS 1.3 enabled?". Second the protocol does not allow
discontinuous version ranges on the client anyway. OpenSSL instead picks the
first continous range of enabled versions on the client, but not the server.
This is bizarrely inconsistent. It also doesn't quite do this as the
ClientHello sending logic does this, but not the ServerHello processing logic.
So we actually break some invariants slightly. The logic is also cumbersome in
DTLS which kindly inverts the comparison logic.
First, switch min_version/max_version's storage to normalized versions. Next
replace all the ad-hoc version-related functions with a single
ssl_get_version_range function. Client and server now consistently pick a
contiguous range of versions. Note this is a slight behavior change for
servers. Version-range-sensitive logic is rewritten to use this new function.
BUG=66
Change-Id: Iad0d64f2b7a917603fc7da54c9fc6656c5fbdb24
Reviewed-on: https://boringssl-review.googlesource.com/8513
Reviewed-by: David Benjamin <davidben@google.com>
The signing logic itself still depends on pre-hashed messages and will be fixed
in later commits.
Change-Id: I901b0d99917c311653d44efa34a044bbb9f11e57
Reviewed-on: https://boringssl-review.googlesource.com/8545
Reviewed-by: David Benjamin <davidben@google.com>
They're not necessary.
Change-Id: Ifeb3fae73a8b22f88019e6ef9f9ba5e64ed3cfab
Reviewed-on: https://boringssl-review.googlesource.com/8543
Reviewed-by: David Benjamin <davidben@google.com>
Otherwise how would callers know what these functions do!
Change-Id: Icbd8b8b614fede82b8d78068353539c300cbacab
Reviewed-on: https://boringssl-review.googlesource.com/8542
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Code search confirms they're never used externally either.
Change-Id: Id90bc15e18555dcfd757b318ab7e2d3ca7c31661
Reviewed-on: https://boringssl-review.googlesource.com/8540
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
As part of the SignatureAlgorithm change in the TLS 1.3 specification,
the existing signature/hash combinations are replaced with a combined
signature algorithm identifier. This change maintains the existing APIs
while fixing the internal representations. The signing code currently
still treats the SignatureAlgorithm as a decomposed value, which will be
fixed as part of a separate CL.
Change-Id: I0cd1660d74ad9bcf55ce5da4449bf2922660be36
Reviewed-on: https://boringssl-review.googlesource.com/8480
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This allows us to use CBB for all handshake messages. Now, SSL_PROTOCOL_METHOD
is responsible for implementing a trio of CBB-related hooks to assemble
handshake messages.
Change-Id: I144d3cac4f05b6637bf45d3f838673fc5c854405
Reviewed-on: https://boringssl-review.googlesource.com/8440
Reviewed-by: Adam Langley <agl@google.com>
OBJ_NAME in OpenSSL has an 'alias' field which some code consumes. We never
report anything OpenSSL considers an alias, so just leave it zero. It also has
a 'data' field which, confusingly, is a pointer to the EVP_CIPHER or EVP_MD
despite being a char pointer.
See calls to and implementation of OBJ_NAME_add in OpenSSL for comparison.
Change-Id: Ifc5c70424569db8783deb2fda7736c1954b5dd3a
Reviewed-on: https://boringssl-review.googlesource.com/8515
Reviewed-by: Adam Langley <agl@google.com>
It was missing. Writing NewSessionTicket will need it.
Change-Id: I39de237894f2e8356bd6861da2b8a4d805dcd2d6
Reviewed-on: https://boringssl-review.googlesource.com/8439
Reviewed-by: Adam Langley <agl@google.com>
It is an explicit copy of something, but it's a lot easier to reason about than
the init_buf/init_num gynmastics we were previously doing. This is along the
way to getting init_buf out of here.
Change-Id: Ia1819ba9db60ef6db09dd60d208dbc95fcfb4bd2
Reviewed-on: https://boringssl-review.googlesource.com/8432
Reviewed-by: Adam Langley <agl@google.com>
Saves us some mess if they're never zero. This also fixes a bug in
ssl3_get_max_client_version where it didn't account for all versions being
disabled properly.
Change-Id: I4c95ff57cf8953cb4a528263b252379f252f3e01
Reviewed-on: https://boringssl-review.googlesource.com/8512
Reviewed-by: David Benjamin <davidben@google.com>
This also adds a missing check to the C half to ensure fake record types are
always correct, to keep implementations honest.
Change-Id: I1d65272e647ffa67018c721d52c639f8ba47d647
Reviewed-on: https://boringssl-review.googlesource.com/8510
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
As of 67cb49d045 and the corresponding upstream
change, BN_mod_word may fail, like BN_div_word. Handle this properly and
document in bn.h. Thanks to Brian Smith for pointing this out.
Change-Id: I6d4f32dc37bcabf70847c9a8b417d55d31b3a380
Reviewed-on: https://boringssl-review.googlesource.com/8491
Reviewed-by: Adam Langley <agl@google.com>
That both exist with nearly the same name is unfortunate. This also does away
with cert_req being unnecessarily tri-state.
Change-Id: Id83e13d0249b80700d9258b363d43b15d22898d8
Reviewed-on: https://boringssl-review.googlesource.com/8247
Reviewed-by: Adam Langley <agl@google.com>
This implements the cipher suite constraints in "fake TLS 1.3". It also makes
bssl_shim and runner enable it by default so we can start adding MaxVersion:
VersionTLS12 markers to tests as 1.2 vs. 1.3 differences begin to take effect.
Change-Id: If1caf6e43938c8d15b0a0f39f40963b8199dcef5
Reviewed-on: https://boringssl-review.googlesource.com/8340
Reviewed-by: David Benjamin <davidben@google.com>
It's a prime, so computing a constant-time mod inverse is straight-forward.
Change-Id: Ie09b84363c3d5da827989300a844c470437fd8f2
Reviewed-on: https://boringssl-review.googlesource.com/8308
Reviewed-by: Adam Langley <agl@google.com>
The case where ec_group_get_mont_data is NULL is only for arbitrary groups
which we now require to be prime order. BN_mod_exp_mont is fine with a NULL
BN_MONT_CTX. It will just compute it. Saves a bit of special-casing.
Also don't mark p-2 as BN_FLG_CONSTTIME as the exponent is public anyway.
Change-Id: Ie868576d52fc9ae5f5c9f2a4039a729151bf84c7
Reviewed-on: https://boringssl-review.googlesource.com/8307
Reviewed-by: Adam Langley <agl@google.com>
The Conscrypt revert cycled in long ago.
Change-Id: If3cdb211d7347dca88bd70bdc643f80b19a7e528
Reviewed-on: https://boringssl-review.googlesource.com/8306
Reviewed-by: Adam Langley <agl@google.com>
(This change will be sent upstream. Since the legacy X.509 stack is just
kept around for compatibility, if they decide to fix it in a different
way, we may wish to revert this and apply their fix.)
Dating back to SSLeay, X509_LOOKUP_METHOD had this X509_LU_RETRY
machinery. But it's not documented and it appears to have never worked.
Problems with the existing logic:
- X509_LU_* is not sure whether it is a type enum (to be passed into
X509_LOOKUP_by_*) or a return enum (to be retained by those same
functions).
- X509_LOOKUP_by_* is not sure whether it returns 0/1 or an X509_LU_*
value. Looking at the functions themselves, one might think it's the
latter, but for X509_LOOKUP_by_subject returning both 0 and
X509_LU_FAIL. But looking at the call sites, some expect 0/1 (such as
X509_STORE_get1_certs) while others expect an X509_LU_* enum (such as
X509_STORE_CTX_get1_issuer). It is very fortunate that FAIL happens to
be 0 and X509 happens to be 1.
These functions primarily call to X509_LOOKUP_METHOD hooks. Looking
through OpenSSL itself and code checked into Google, I found no
evidence that any hooks have been implemented except for
get_by_subject in by_dir.c. We take that one as definitive and observe
it believes it returns 0/1. Notably, it returns 1 on success even if
asked for a type other than X509_LU_X509. (X509_LU_X509 = 1. Others are
different.) I found another piece of third-party software which corroborates
this worldview.
- X509_STORE_get_by_subject's handling of X509_LU_RETRY (it's the j < 0
check) is broken. It saves j into vs->current_method where it probably
meant to save i. (This bug has existed since SSLeay.)
It also returns j (supposedly X509_LU_RETRY) while all callers of
X509_STORE_get_by_subject expect it to return 0/1 by checking with !
instead of <= 0. (Note that all other codepaths return 0 and 1 so this
function did not actually believe it returned X509_LU_* most of the
time.)
This, in turn, gives us a free of uninitialized pointers in
X509_STORE_get1_certs and other functions which expect that *ret is
filled in if X509_STORE_get_by_subject returns success. GCC 4.9 with
optimizations from the Android NDK noticed this, which trigged this
saga.
(It's only reachable if any X509_LOOKUP_METHOD returned
X509_LU_RETRY.)
- Although the code which expects X509_STORE_get_by_subject return 0/1
does not date to SSLeay, the X509_STORE_get_by_subject call in
X509_STORE_CTX_get1_issuer *does* (though, at the time, it was inline
in X509_verify_cert. That code believes X509_STORE_get_by_subject
returns an X509_LU_* enum, but it doesn't work either! It believes
*ret is filled in on X509_LU_RETRY, thus freeing another uninitialized
pointer (GCC noticed this too).
Since this "retry" code has clearly never worked, from SSLeay onwards,
unwind it completely rather than attempt to fix it. No
X509_LOOKUP_METHOD can possibly have depended on it.
Matching all non-broken codepaths X509_LOOKUP_by_* now returns 0/1 and
X509_STORE_get_by_subject returns 0/1. X509_LU_* is purely a type enum
with X509_LU_{REJECT,FAIL} being legacy constants to keep old code
compiling. (Upstream is recommended to remove those values altogether
for 1.1.0.)
On the off chance any get_by_* X509_LOOKUP_METHOD implementations did
not return 0/1 (I have found no evidence anywhere of this, and I believe
it wouldn't have worked anyway), the X509_LOOKUP_by_* wrapper functions
will coerce the return values back to 0/1 before passing up to the
callers which want 0/1. This both avoids the error-prone -1/0/1 calling
convention and, more importantly, avoids problems with third-party
callers which expect a X509_LU_* return code. 0/1 collide with FAIL/X509
while -1 will collide with RETRY and might confuse things.
Change-Id: I98ecf6fa7342866b9124dc6f0b422cb9ce4a1ae7
Reviewed-on: https://boringssl-review.googlesource.com/8303
Reviewed-by: Adam Langley <agl@google.com>
I named the compatibility function wrong.
Change-Id: Idc289c317c5826c338c1daf58a2d3b26b09a7e49
Reviewed-on: https://boringssl-review.googlesource.com/8301
Reviewed-by: Adam Langley <agl@google.com>
SSL_set_bio has some rather complex ownership story because whether rbio/wbio
are both owning depends on whether they are equal. Moreover, whether
SSL_set_bio(ssl, rbio, wbio) frees ssl->rbio depends on whether rbio is the
existing rbio or not. The current logic doesn't even get it right; see tests.
Simplify this. First, rbio and wbio are always owning. All the weird ownership
cases which we're stuck with for compatibility will live in SSL_set_bio. It
will internally BIO_up_ref if necessary and appropriately no-op the left or
right side as needed. It will then call more well-behaved ssl_set_rbio or
ssl_set_wbio functions as necessary.
Change-Id: I6b4b34e23ed01561a8c0aead8bb905363ee413bb
Reviewed-on: https://boringssl-review.googlesource.com/8240
Reviewed-by: Adam Langley <agl@google.com>
These are more remnants of CMS. Nothing uses them directly. Removing them means
more code we don't have to think about when importing upstream patches.
Also take out a bunch of dead prototypes nearby.
Change-Id: Ife094d9d2078570006d1355fa4e3323f435be608
Reviewed-on: https://boringssl-review.googlesource.com/8244
Reviewed-by: David Benjamin <davidben@google.com>
These are more pretty-printers for generic ASN.1 structures. They're never
called externally and otherwise are only used in the X509V3_EXT_PARSE_UNKNOWN
mode for the X509 pretty-print functions. That makes unknown extensions
pretty-print as ASN.1 structures.
This is a rather useless feature, so have that fall through to
X509V3_EXT_DUMP_UNKNOWN which does a hexdump instead.
(The immediate trigger is I don't know what |op| is in upstream's
8c918b7b9c93ba38790ffd1a83e23c3684e66f57 and don't think it is worth the time
to puzzle that out and verify it. Better ditch this code completely.)
Change-Id: I0217906367d83056030aea64ef344d4fedf74763
Reviewed-on: https://boringssl-review.googlesource.com/8243
Reviewed-by: David Benjamin <davidben@google.com>
These functions are never instantiated. (They're a remnant of the PKCS#7 and
CMS bits.) Next time upstream touches this code, we don't have to puzzle
through the diff and import it.
Change-Id: I67c2102ae13e8e0527d858e1c63637dd442a4ffb
Reviewed-on: https://boringssl-review.googlesource.com/8242
Reviewed-by: David Benjamin <davidben@google.com>
There's a __pragma expression which allows this. Android builds us Windows with
MinGW for some reason, so we actually do have to tolerate non-MSVC-compatible
Windows compilers. (Clang for Windows is much more sensible than MinGW and
intentionally mimicks MSVC.)
MinGW doesn't understand MSVC's pragmas and warns a lot. #pragma warning is
safe to suppress, so wrap those to shush them. This also lets us do away with a
few ifdefs.
Change-Id: I1f5a8bec4940d4b2d947c4c1cc9341bc15ec4972
Reviewed-on: https://boringssl-review.googlesource.com/8236
Reviewed-by: Adam Langley <agl@google.com>
While most of OpenSSL's assembly allows out < in too, some of it doesn't.
Upstream seems to not consider this a problem (or, at least, they're failing to
make a decision on whether it is a problem, so we should assume they'll stay
their course). Accordingly, require aliased buffers to exactly align so we
don't have to keep chasing this down.
Change-Id: I00eb3df3e195b249116c68f7272442918d7077eb
Reviewed-on: https://boringssl-review.googlesource.com/8231
Reviewed-by: Adam Langley <agl@google.com>
Set ctx->error = X509_V_ERR_OUT_OF_MEM when verification cannot
continue due to malloc failure. Similarly for issuer lookup failures
and caller errors (bad parameters or invalid state).
Also, when X509_verify_cert() returns <= 0 make sure that the
verification status does not remain X509_V_OK, as a last resort set
it it to X509_V_ERR_UNSPECIFIED, just in case some code path returns
an error without setting an appropriate value of ctx->error.
Add new and some missing error codes to X509 error -> SSL alert switch.
(Imported from upstream's 5553a12735e11bc9aa28727afe721e7236788aab.)
Change-Id: I3231a6b2e72a3914cb9316b8e90ebaee009a1c5f
Reviewed-on: https://boringssl-review.googlesource.com/8170
Reviewed-by: David Benjamin <davidben@google.com>
This callback is used by BoringSSL tests in order to simulate the time,
so that the tests have repeatable results. This API will allow consumers
of BoringSSL to write the same sort of tests.
Change-Id: I79d72bce5510bbd83c307915cd2cc937579ce948
Reviewed-on: https://boringssl-review.googlesource.com/8200
Reviewed-by: David Benjamin <davidben@google.com>
Match the actual name of the type.
Change-Id: I0ad27196ee2876ce0690d13068fa95f68b05b0da
Reviewed-on: https://boringssl-review.googlesource.com/8187
Reviewed-by: David Benjamin <davidben@google.com>
There's only one thing under "SNI Extension".
Change-Id: I8d8c54c286cb5775a20c4e2623896eb9be2f0009
Reviewed-on: https://boringssl-review.googlesource.com/8181
Reviewed-by: David Benjamin <davidben@google.com>
The business with ssl_record_prefix_len is rather a hassle. Instead, have
tls_open_record always decrypt in-place and give back a CBS to where the body
is.
This way the caller doesn't need to do an extra check all to avoid creating an
invalid pointer and underflow in subtraction.
Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee
Reviewed-on: https://boringssl-review.googlesource.com/8173
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The two modes are quite different. One of them requires the BIO honor an
extra BIO_ctrl. Also add an explanation at the top of
addDTLSRetransmitTests for how these tests work. The description is
scattered across many different places.
BUG=63
Change-Id: Iff4cdd1fbf4f4439ae0c293f565eb6780c7c84f9
Reviewed-on: https://boringssl-review.googlesource.com/8121
Reviewed-by: David Benjamin <davidben@google.com>
More spring-cleaning of unnecessary incompatibilities. Since
OpenSSL_add_all_algorithms_conf doesn't specify a configuration file, it's
perfectly sound to have such a function.
Dear BoringSSL, please add all algorithms.
Uh, sure. They were already all there, but I have added them!
PS: Could you also load all your configuration files while you're at it.
...I don't have any. Fine. I have loaded all configuration files which I
recognize. *mutters under breath* why does everyone ask all these strange
questions...
Change-Id: I57f956933d9e519445bf22f89853bd5f56904172
Reviewed-on: https://boringssl-review.googlesource.com/8160
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The fake numbers collide with other numbers defined below. Also PUSH and POP
are actually used. DUP legitimately isn't though.
Change-Id: Iaa15a065d846b89b9b7958b78068393cfee2bd6f
Reviewed-on: https://boringssl-review.googlesource.com/8143
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Some OpenSSL consumers use them, so provide no-op versions to make porting code
easier.
Change-Id: I4348568c1cb08d2b2c0a9ec9a17e2c0449260965
Reviewed-on: https://boringssl-review.googlesource.com/8142
Reviewed-by: David Benjamin <davidben@google.com>
Make building against software that expects OpenSSL easier.
Change-Id: I1af090ae8208218d6e226ee0baf51053699d85cc
Reviewed-on: https://boringssl-review.googlesource.com/8141
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
doc.go is still a little unhappy.
Change-Id: I5a8f3da91dabb45d29d0e08f13b7dabdcd521c38
Reviewed-on: https://boringssl-review.googlesource.com/8145
Reviewed-by: David Benjamin <davidben@google.com>
One of these tests the distribution of noise polynomials; the other
tests that that agreed-upon keys (prior to whitening) have roughly equal
numbers of 0s and 1s.
Along the way, expose a few more API bits.
Change-Id: I6b04708d41590de45d82ea95bae1033cfccd5d67
Reviewed-on: https://boringssl-review.googlesource.com/8130
Reviewed-by: Adam Langley <agl@google.com>
This commit adds coverage of the "offer" (first) step, as well as
testing all outputs of the "accept" (second) step, not just the shared
key.
Change-Id: Id11fe24029abc302442484a6c01fa496a1578b3a
Reviewed-on: https://boringssl-review.googlesource.com/8100
Reviewed-by: Adam Langley <agl@google.com>
This keeps the naming convention in line with the actual spec.
Change-Id: I34673f78dbc29c1659b4da0e49677ebe9b79636b
Reviewed-on: https://boringssl-review.googlesource.com/8090
Reviewed-by: David Benjamin <davidben@google.com>
The test vectors are taken from the reference implementation, modified
to output the results of its random-number generator, and the results of
key generation prior to SHA3. This allows the interoperability of the
two implementations to be tested somewhat.
To accomplish the testing, this commit creates a new, lower-level API
that leaves the generation of random numbers and all wire encoding and
decoding up to the caller.
Change-Id: Ifae3517696dde4be4a0b7c1998bdefb789bac599
Reviewed-on: https://boringssl-review.googlesource.com/8070
Reviewed-by: Adam Langley <agl@google.com>
Now that we no longer support Windows XP, this is available.
Unfortunately, the public header version of CRYPTO_MUTEX means we
still can't easily merge CRYPTO_MUTEX and CRYPTO_STATIC_MUTEX.
BUG=37
Change-Id: If309de3f06e0854c505083b72fd64d1dbb3f4563
Reviewed-on: https://boringssl-review.googlesource.com/8081
Reviewed-by: Adam Langley <agl@google.com>
This is easier to deploy, and more obvious. This commit reverts a few
pieces of e25775bc, but keeps most of it.
Change-Id: If8d657a4221c665349c06041bb12fffca1527a2c
Reviewed-on: https://boringssl-review.googlesource.com/8061
Reviewed-by: Adam Langley <agl@google.com>
This code has caused a long history of problems. This change rewrites it
completely with something that is, hopefully, much simplier and robust
and adds more testing.
Change-Id: Ibeef51f9386afd95d5b73316e451eb3a2d7ec4e0
Reviewed-on: https://boringssl-review.googlesource.com/8033
Reviewed-by: Adam Langley <agl@google.com>
Constants representing TLS 1.3 are added to allow for future work to be
flagged on TLS1_3_VERSION. To prevent BoringSSL from negotiating the
non-existent TLS 1.3 version, it is explicitly disabled using
SSL_OP_NO_TLSv1_3.
Change-Id: Ie5258a916f4c19ef21646c4073d5b4a7974d6f3f
Reviewed-on: https://boringssl-review.googlesource.com/8041
Reviewed-by: Adam Langley <agl@google.com>
This renames the Channel ID EncryptedExtensions message to allow for
compatibility with TLS 1.3 EncryptedExtensions.
Change-Id: I5b67d00d548518045554becb1b7213fba86731f2
Reviewed-on: https://boringssl-review.googlesource.com/8040
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's bbio logic is kind of crazy. It would be good to eventually do the
buffering in a better way (notably, bbio is fragile, if not outright broken,
for DTLS). In the meantime, this fixes a number of bugs where the existence of
bbio was leaked in the public API and broke things.
- SSL_get_wbio returned the bbio during the handshake. It must always return
the BIO the consumer configured. In doing so, internal accesses of
SSL_get_wbio should be switched to ssl->wbio since those want to see bbio.
For consistency, do the same with rbio.
- The logic in SSL_set_rfd, etc. (which I doubt is quite right since
SSL_set_bio's lifetime is unclear) would get confused once wbio got wrapped.
Those want to compare to SSL_get_wbio.
- If SSL_set_bio was called mid-handshake, bbio would get disconnected and lose
state. It forgets to reattach the bbio afterwards. Unfortunately, Conscrypt
does this a lot. It just never ended up calling it at a point where the bbio
would cause problems.
- Make more explicit the invariant that any bbio's which exist are always
attached. Simplify a few things as part of that.
Change-Id: Ia02d6bdfb9aeb1e3021a8f82dcbd0629f5c7fb8d
Reviewed-on: https://boringssl-review.googlesource.com/8023
Reviewed-by: Kenny Root <kroot@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The 'elliptic_curves' extension is being renamed to 'supported_groups'
in the TLS 1.3 draft, and most of the curve-specific methods are
generalized to groups/group IDs.
Change-Id: Icd1a1cf7365c8a4a64ae601993dc4273802610fb
Reviewed-on: https://boringssl-review.googlesource.com/7955
Reviewed-by: David Benjamin <davidben@google.com>
open_iscsi assumes that it can get |OPENSSL_malloc| after including only
pem.h and err.h. Since pem.h already includes quite a lot, this change
adds crypto.h to that set so that open_iscsi is happy.
Change-Id: I6dc06c27088ce3ca46c1ab53bb29650033cba267
Reviewed-on: https://boringssl-review.googlesource.com/8031
Reviewed-by: David Benjamin <davidben@google.com>
The key schedule in TLS 1.3 requires a separate Extract and Expand phase
for the cryptographic computations.
Change-Id: Ifdac1237bda5212de5d4f7e8db54e202151d45ec
Reviewed-on: https://boringssl-review.googlesource.com/7983
Reviewed-by: David Benjamin <davidben@google.com>
CECPQ1 is a new key exchange that concatenates the results of an X25519
key agreement and a NEWHOPE key agreement.
Change-Id: Ib919bdc2e1f30f28bf80c4c18f6558017ea386bb
Reviewed-on: https://boringssl-review.googlesource.com/7962
Reviewed-by: David Benjamin <davidben@google.com>
This is consistent with the new convention in ssl_ecdh.c.
Along the way, change newhope_test.c to not iterate 1000 times over each
test.
Change-Id: I7a500f45b838eba8f6df96957891aa8e880ba089
Reviewed-on: https://boringssl-review.googlesource.com/8012
Reviewed-by: David Benjamin <davidben@google.com>
We don't have any of these.
Change-Id: I8d12284fbbab0ff35ac32d35a5f2eba326ab79f8
Reviewed-on: https://boringssl-review.googlesource.com/7981
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
They're completely unused now. The handshake message reassembly logic should
not depend on the state machine. This should partially free it up (ugly as it
is) to be shared with a future TLS 1.3 implementation while, in parallel, it
and the layers below, get reworked. This also cuts down on the number of states
significantly.
Partially because I expect we'd want to get ssl_hash_message_t out of there
too. Having it in common code is fine, but it needs to be in the (supposed to
be) protocol-agnostic handshake state machine, not the protocol-specific
handshake message layer.
Change-Id: I12f9dc57bf433ceead0591106ab165d352ef6ee4
Reviewed-on: https://boringssl-review.googlesource.com/7949
Reviewed-by: Adam Langley <agl@google.com>
Rather than this confusing coordination with the handshake state machine and
init_num changing meaning partway through, use the length field already in
BUF_MEM. Like the new record layer parsing, is no need to keep track of whether
we are reading the header or the body. Simply keep extending the handshake
message until it's far enough along.
ssl3_get_message still needs tons of work, but this allows us to disentangle it
from the handshake state.
Change-Id: Ic2b3e7cfe6152a7e28a04980317d3c7c396d9b08
Reviewed-on: https://boringssl-review.googlesource.com/7948
Reviewed-by: Adam Langley <agl@google.com>
BUF_MEM is actually a rather silly API for the SSL stack. There's separate
length and max fields, but init_buf effectively treats length as max and max as
nothing.
We possibly don't want to be using it long-term anyway (if nothing else, the
char*/uint8_t* thing is irritating), but in the meantime, it'll be easier to
separately fix up get_message's book-keeping and state tracking from where the
handshake gets its messages from.
Change-Id: I9e56ea008173991edc8312ec707505ead410a9ee
Reviewed-on: https://boringssl-review.googlesource.com/7947
Reviewed-by: Adam Langley <agl@google.com>
