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>
