Besides being a good idea anyway, this avoids clang warning about using
a non-literal format string when |ERR_add_error_dataf| calls
|BIO_vsnprintf|.
Change-Id: Iebc84d9c9d85e08e93010267d473387b661717a5
Reviewed-on: https://boringssl-review.googlesource.com/6920
Reviewed-by: David Benjamin <davidben@google.com>
This centralizes the conditional logic into openssl/base.h so that it
doesn't have to be repeated. The name |OPENSSL_PRINTF_FORMAT_FUNC| was
chosen in anticipation of eventually defining an
|OPENSSL_PRINTF_FORMAT_ARG| for MSVC-style parameter annotations.
Change-Id: I273e6eddd209e696dc9f82099008c35b6d477cdb
Reviewed-on: https://boringssl-review.googlesource.com/6909
Reviewed-by: David Benjamin <davidben@google.com>
This change imports the following changes from upstream:
6281abc79623419eae6a64768c478272d5d3a426
dfd3322d72a2d49f597b86dab6f37a8cf0f26dbf
f34b095fab1569d093b639bfcc9a77d6020148ff
21376d8ae310cf0455ca2b73c8e9f77cafeb28dd
25efcb44ac88ab34f60047e16a96c9462fad39c1
56353962e7da7e385c3d577581ccc3015ed6d1dc
39c76ceb2d3e51eaff95e04d6e4448f685718f8d
a3d74afcae435c549de8dbaa219fcb30491c1bfb
These contain the “altchains” functionality which allows OpenSSL to
backtrack when chain building.
Change-Id: I8d4bc2ac67b90091f9d46e7355cae878b4ccf37d
Reviewed-on: https://boringssl-review.googlesource.com/6905
Reviewed-by: Adam Langley <agl@google.com>
(Comment-only change; no functional difference.)
Some code was broken by the |d2i_ECDSA_SIG| change in 87897a8c. It was
passing in a pointer to an existing |ECDSA_SIG| as the first argument
and then simply assuming that the structure would be updated in place.
The comments on the function suggested that this was reasonable.
This change updates the comments that use similar wording to either note
that the function will never update in-place, or else to note that
depending on that is a bad idea for the future.
I've also audited all the uses of these functions that I can find and,
in addition to the one case with |d2i_ECDSA_SIG|, there are several
users of |d2i_PrivateKey| that could become a problem in the future.
I'll try to fix them before it does become an issue.
Change-Id: I769f7b2e0b5308d09ea07dd447e02fc161795071
Reviewed-on: https://boringssl-review.googlesource.com/6902
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This is a companion to SSL_get_rc4_state and SSL_get_ivs which doesn't
require poking at internal state. Partly since it aligns with the
current code and partly the off chance we ever need to get
wpa_supplicant's EAP-FAST code working, the API allows one to generate
more key material than is actually in the key block.
Change-Id: I58bc3f2b017482dbb8567dcd0cd754947a95397f
Reviewed-on: https://boringssl-review.googlesource.com/6839
Reviewed-by: Adam Langley <alangley@gmail.com>
Both are connection state rather than configuration state. Notably this
cuts down more of SSL_clear that can't just use ssl_free + ssl_new.
Change-Id: I3c05b3ae86d4db8bd75f1cd21656f57fc5b55ca9
Reviewed-on: https://boringssl-review.googlesource.com/6835
Reviewed-by: Adam Langley <alangley@gmail.com>
Move the actual SSL_AEAD_CTX swap into the record layer. Also revise the
intermediate state we store between setup_key_block and
change_cipher_state. With SSL_AEAD_CTX_new abstracted out, keeping the
EVP_AEAD around doesn't make much sense. Just store enough to partition
the key block.
Change-Id: I773fb46a2cb78fa570f00c0a89339c15bbb1d719
Reviewed-on: https://boringssl-review.googlesource.com/6832
Reviewed-by: Adam Langley <alangley@gmail.com>
wpa_supplicant needs to get at the client and server random. OpenSSL
1.1.0 added these APIs, so match their semantics.
Change-Id: I2b71ba850ac63e574c9ea79012d1d0efec5a979a
Reviewed-on: https://boringssl-review.googlesource.com/6830
Reviewed-by: Adam Langley <alangley@gmail.com>
EVP_MAX_MD_SIZE is large enough to fit a MD5/SHA1 concatenation, and
necessarily is because EVP_md5_sha1 exists. This shaves 128 bytes of
per-connection state.
Change-Id: I848a8563dfcbac14735bb7b302263a638528f98e
Reviewed-on: https://boringssl-review.googlesource.com/6804
Reviewed-by: Adam Langley <agl@google.com>
This unifies the ClientKeyExchange code rather nicely. ServerKeyExchange
is still pretty specialized. For simplicity, I've extended the yaSSL bug
workaround for clients as well as servers rather than route in a
boolean.
Chrome's already banished DHE to a fallback with intention to remove
altogether later, and the spec doesn't say anything useful about
ClientDiffieHellmanPublic encoding, so this is unlikely to cause
problems.
Change-Id: I0355cd1fd0fab5729e8812e4427dd689124f53a2
Reviewed-on: https://boringssl-review.googlesource.com/6784
Reviewed-by: Adam Langley <agl@google.com>
We don't actually have an API to let you know if the value is legal to
interpret as a curve ID. (This was kind of a poor API. Oh well.) Also add tests
for key_exchange_info. I've intentionally left server-side plain RSA missing
for now because the SSL_PRIVATE_KEY_METHOD abstraction only gives you bytes and
it's probably better to tweak this API instead.
(key_exchange_info also wasn't populated on the server, though due to a
rebasing error, that fix ended up in the parent CL. Oh well.)
Change-Id: I74a322c8ad03f25b02059da7568c9e1a78419069
Reviewed-on: https://boringssl-review.googlesource.com/6783
Reviewed-by: Adam Langley <agl@google.com>
The new curve is not enabled by default.
As EC_GROUP/EC_POINT is a bit too complex for X25519, this introduces an
SSL_ECDH_METHOD abstraction which wraps just the raw ECDH operation. It
also tidies up some of the curve code which kept converting back and
force between NIDs and curve IDs. Now everything transits as curve IDs
except for API entry points (SSL_set1_curves) which take NIDs. Those
convert immediately and act on curve IDs from then on.
Note that, like the Go implementation, this slightly tweaks the order of
operations. The client sees the server public key before sending its
own. To keep the abstraction simple, SSL_ECDH_METHOD expects to
generate a keypair before consuming the peer's public key. Instead, the
client handshake stashes the serialized peer public value and defers
parsing it until it comes time to send ClientKeyExchange. (This is
analogous to what it was doing before where it stashed the parsed peer
public value instead.)
It still uses TLS 1.2 terminology everywhere, but this abstraction should also
be compatible with TLS 1.3 which unifies (EC)DH-style key exchanges.
(Accordingly, this abstraction intentionally does not handle parsing the
ClientKeyExchange/ServerKeyExchange framing or attempt to handle asynchronous
plain RSA or the authentication bits.)
BUG=571231
Change-Id: Iba09dddee5bcdfeb2b70185308e8ab0632717932
Reviewed-on: https://boringssl-review.googlesource.com/6780
Reviewed-by: Adam Langley <agl@google.com>
There is some messiness around saving and restoring the CBB, but this is
still significantly clearer.
Note that the BUF_MEM_grow line is gone in favor of a fixed CBB like the
other functions ported thus far. This line was never necessary as
init_buf is initialized to 16k and none of our key exchanges get that
large. (The largest one can get is DHE_RSA. Even so, it'd take a roughly
30k-bit DH group with a 30k-bit RSA key.)
Having such limits and tight assumptions on init_buf's initial size is
poor (but on par for the old code which usually just blindly assumed the
message would not get too large) and the size of the certificate chain
is much less obviously bounded, so those BUF_MEM_grows can't easily go.
My current plan is convert everything but those which legitimately need
BUF_MEM_grow to CBB, then atomically convert the rest, remove init_buf,
and switch everything to non-fixed CBBs. This will hopefully also
simplify async resumption. In the meantime, having a story for
resumption means the future atomic change is smaller and, more
importantly, relieves some complexity budget in the ServerKeyExchange
code for adding Curve25519.
Change-Id: I1de6af9856caaed353453d92a502ba461a938fbd
Reviewed-on: https://boringssl-review.googlesource.com/6770
Reviewed-by: Adam Langley <agl@google.com>
This relieves some complexity budget for adding Curve25519 to this
code.
This also adds a BN_bn2cbb_padded helper function since this seems to be a
fairly common need.
Change-Id: Ied0066fdaec9d02659abd6eb1a13f33502c9e198
Reviewed-on: https://boringssl-review.googlesource.com/6767
Reviewed-by: Adam Langley <agl@google.com>
These will be needed when we start writing variable-length things to a
CBB.
Change-Id: Ie7b9b140f5f875b43adedc8203ce9d3f4068dfea
Reviewed-on: https://boringssl-review.googlesource.com/6764
Reviewed-by: Adam Langley <agl@google.com>
A lot of commented-out code we haven't had to put them back, so these
can go now. Also remove the TODO about OAEP having a weird API. The API
is wrong, but upstream's shipped it with the wrong API, so that's what
it is now.
Change-Id: I7da607cf2d877cbede41ccdada31380f812f6dfa
Reviewed-on: https://boringssl-review.googlesource.com/6763
Reviewed-by: Adam Langley <agl@google.com>
There's a few that can't work since the types don't even exist.
Change-Id: Idf860b146439c95d33814d25bbc9b8f61774b569
Reviewed-on: https://boringssl-review.googlesource.com/6762
Reviewed-by: Adam Langley <agl@google.com>
There was a TODO to remove it once asn1_mac.h was trimmed. This has now
happened. Remove it and reset error codes for crypto/asn1.
Change-Id: Iaf2f3e75741914415372939471b135618910f95d
Reviewed-on: https://boringssl-review.googlesource.com/6761
Reviewed-by: Adam Langley <agl@google.com>
This check was fixed a while ago, but it could have been much simpler.
In the RSA key exchange, the expected size of the output is known, making the
padding check much simpler. There isn't any use in exporting the more general
RSA_message_index_PKCS1_type_2. (Without knowing the expected size, any
integrity check or swap to randomness or other mitigation is basically doomed
to fail.)
Verified with the valgrind uninitialized memory trick that we're still
constant-time.
Also update rsa.h to recommend against using the PKCS#1 v1.5 schemes.
Thanks to Ryan Sleevi for the suggestion.
Change-Id: I4328076b1d2e5e06617dd8907cdaa702635c2651
Reviewed-on: https://boringssl-review.googlesource.com/6613
Reviewed-by: Adam Langley <agl@google.com>
Only ECDHE-based ciphers are implemented. To ease the transition, the
pre-standard cipher shares a name with the standard one. The cipher rule parser
is hacked up to match the name to both ciphers. From the perspective of the
cipher suite configuration language, there is only one cipher.
This does mean it is impossible to disable the old variant without a code
change, but this situation will be very short-lived, so this is fine.
Also take this opportunity to make the CK and TXT names align with convention.
Change-Id: Ie819819c55bce8ff58e533f1dbc8bef5af955c21
Reviewed-on: https://boringssl-review.googlesource.com/6686
Reviewed-by: Adam Langley <agl@google.com>
The consumers have all been updated, so we can move EVP_aead_chacha20_poly1305
to its final state. Unfortunately, the _rfc7539-suffixed version will need to
stick around for just a hair longer. Also the tls1.h macros, but the remaining
consumers are okay with that changing underneath them.
Change-Id: Ibbb70ec1860d6ac6a7e1d7b45e70fe692bf5ebe5
Reviewed-on: https://boringssl-review.googlesource.com/6600
Reviewed-by: Adam Langley <agl@google.com>
Rather than the length of the top-level CBB, which is kind of odd when ASN.1
length prefixes are not yet determined, return the number of bytes written to
the CBB so far. This can be computed without increasing the size of CBB at all.
Have offset and pending_*.
This means functions which take in a CBB as argument will not be sensitive to
whether the CBB is a top-level or child CBB. The extensions logic had to be
careful to only ever compare differences of lengths, which was awkward.
The reversal will also allow for the following pattern in the future, once
CBB_add_space is split into, say, CBB_reserve and CBB_did_write and we add a
CBB_data:
uint8_t *signature;
size_t signature_len = 0;
if (!CBB_add_asn1(out, &cert, CBB_ASN1_SEQUENCE) ||
/* Emit the TBSCertificate. */
!CBB_add_asn1(&cert, &tbs_cert, CBS_ASN1_SEQUENCE) ||
!CBB_add_tbs_cert_stuff(&tbs_cert, stuff) ||
!CBB_flush(&cert) ||
/* Feed it into md_ctx. */
!EVP_DigestSignInit(&md_ctx, NULL, EVP_sha256(), NULL, pkey) ||
!EVP_DigestSignUpdate(&md_ctx, CBB_data(&cert), CBB_len(&cert)) ||
/* Emit the signature algorithm. */
!CBB_add_asn1(&cert, &sig_alg, CBS_ASN1_SEQUENCE) ||
!CBB_add_sigalg_stuff(&sig_alg, other_stuff) ||
/* Emit the signature. */
!EVP_DigestSignFinal(&md_ctx, NULL, &signature_len) ||
!CBB_reserve(&cert, &signature, signature_len) ||
!EVP_DigestSignFinal(&md_ctx, signature, &signature_len) ||
!CBB_did_write(&cert, signature_len)) {
goto err;
}
(Were TBSCertificate not the first field, we'd still have to sample
CBB_len(&cert), but at least that's reasonable straight-forward. The
alternative would be if CBB_data and CBB_len somehow worked on
recently-invalidated CBBs, but that would go wrong once the invalidated CBB's
parent flushed and possibly shifts everything.)
And similar for signing ServerKeyExchange.
Change-Id: I7761e492ae472d7632875b5666b6088970261b14
Reviewed-on: https://boringssl-review.googlesource.com/6681
Reviewed-by: Adam Langley <agl@google.com>
The uint32_t likely dates to them using HASH_LONG everywhere. Nothing ever
touches c->data as a uint32_t, only bytes. (Which makes sense seeing as it
stores the partial block.)
Change-Id: I634cb7f2b6306523aa663f8697b7dc92aa491320
Reviewed-on: https://boringssl-review.googlesource.com/6651
Reviewed-by: Adam Langley <agl@google.com>
Manual tweaks and then clang-formatted again.
Change-Id: I809fdb71b2135343e5c1264dd659b464780fc54a
Reviewed-on: https://boringssl-review.googlesource.com/6649
Reviewed-by: Adam Langley <agl@google.com>
It is redundant given the other state in the connection.
Change-Id: I5dc71627132659ab4316a5ea360c9ca480fb7c6c
Reviewed-on: https://boringssl-review.googlesource.com/6646
Reviewed-by: Adam Langley <agl@google.com>
These have been unused since we unified everything on EVP_AEAD. I must
have missed them when clearing out dead state. This shaves 136 bytes of
per-connection state.
Change-Id: I705f8de389fd34ab4524554ee9e4b1d6be198994
Reviewed-on: https://boringssl-review.googlesource.com/6645
Reviewed-by: Adam Langley <agl@google.com>
There's no need to track consumed bytes, so rr->data and rr->off may be
merged together.
Change-Id: I8842d005665ea8b4d4a0cced941f3373872cdac4
Reviewed-on: https://boringssl-review.googlesource.com/6644
Reviewed-by: Adam Langley <agl@google.com>
38 error codes have fallen off the list since the last time we did this.
Change-Id: Id7ee30889a5da2f6ab66957fd8e49e97640c8489
Reviewed-on: https://boringssl-review.googlesource.com/6643
Reviewed-by: Adam Langley <agl@google.com>
This uses ssl3_read_bytes for now. We still need to dismantle that
function and then invert the handshake state machine, but this gets
things closer to the right shape as an intermediate step and is a large
chunk in itself. It simplifies a lot of the CCS/handshake
synchronization as a lot of the invariants much more clearly follow from
the handshake itself.
Tests need to be adjusted since this changes some error codes. Now all
the CCS/Handshake checks fall through to the usual
SSL_R_UNEXPECTED_RECORD codepath. Most of what used to be a special-case
falls out naturally. (If half of Finished was in the same record as the
pre-CCS message, that part of the handshake record would have been left
unconsumed, so read_change_cipher_spec would have noticed, just like
read_app_data would have noticed.)
Change-Id: I15c7501afe523d5062f0e24a3b65f053008d87be
Reviewed-on: https://boringssl-review.googlesource.com/6642
Reviewed-by: Adam Langley <agl@google.com>
With server-side renegotiation gone, handshake_fragment's only purpose
in life is to handle a fragmented HelloRequest (we probably do need to
support those if some server does 1/n-1 record-splitting on handshake
records). The logic to route the data into
ssl3_read_bytes(SSL3_RT_HANDSHAKE) never happens, and the contents are
always a HelloRequest prefix.
This also trims a tiny bit of per-connection state.
Change-Id: Ia1b0dda5b7e79d817c28da1478640977891ebc97
Reviewed-on: https://boringssl-review.googlesource.com/6641
Reviewed-by: Adam Langley <agl@google.com>
Avoids bouncing on the lock, but it doesn't really matter since it's all
taking read locks. If we're declaring that callbacks don't get to see
every object being created, they shouldn't see every object being
destroyed.
CRYPTO_dup_ex_data also already had this optimization, though it wasn't
documented.
BUG=391192
Change-Id: I5b8282335112bca3850a7c0168f8bd7f7d4a2d57
Reviewed-on: https://boringssl-review.googlesource.com/6626
Reviewed-by: Adam Langley <agl@google.com>
This callback is never used. The one caller I've ever seen is in Android
code which isn't built with BoringSSL and it was a no-op.
It also doesn't actually make much sense. A callback cannot reasonably
assume that it sees every, say, SSL_CTX created because the index may be
registered after the first SSL_CTX is created. Nor is there any point in
an EX_DATA consumer in one file knowing about an SSL_CTX created in
completely unrelated code.
Replace all the pointers with a typedef to int*. This will ensure code
which passes NULL or 0 continues to compile while breaking code which
passes an actual function.
This simplifies some object creation functions which now needn't worry
about CRYPTO_new_ex_data failing. (Also avoids bouncing on the lock, but
it's taking a read lock, so this doesn't really matter.)
BUG=391192
Change-Id: I02893883c6fa8693682075b7b130aa538a0a1437
Reviewed-on: https://boringssl-review.googlesource.com/6625
Reviewed-by: Adam Langley <agl@google.com>
Then deprecate the old functions. Thanks to upstream's
6977e8ee4a718a76351ba5275a9f0be4e530eab5 for the idea.
Change-Id: I916abd6fca2a3b2a439ec9902d9779707f7e41eb
Reviewed-on: https://boringssl-review.googlesource.com/6622
Reviewed-by: Adam Langley <agl@google.com>
It has no callers. I prepped for its removal earlier with
c05697c2c5
and then completely forgot.
Thanks to upstream's 6f78b9e824c053d062188578635c575017b587c5 for
the reminder. Quoth them:
> This only gets used to set a specific curve without actually checking
> that the peer supports it or not and can therefor result in handshake
> failures that can be avoided by selecting a different cipher.
It's also a very confusing API since it does NOT pass ownership of the
EC_KEY to the caller.
Change-Id: I6a00643b3a2d6746e9e0e228b47c2bc9694b0084
Reviewed-on: https://boringssl-review.googlesource.com/6621
Reviewed-by: Adam Langley <agl@google.com>
I don't think we're ever going to manage to enforce this, and it doesn't
seem worth the trouble. We don't support application protocols which use
renegotiation outside of the HTTP/1.1 mid-stream client auth hack.
There, it's on the server to reject legacy renegotiations.
This removes the last of SSL_OP_ALL.
Change-Id: I996fdeaabf175b6facb4f687436549c0d3bb0042
Reviewed-on: https://boringssl-review.googlesource.com/6580
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.8.0 (or earlier). The use counter sees virtually
no hits.
Change-Id: Iff4c8899d5cb0ba4afca113c66d15f1d980ffe41
Reviewed-on: https://boringssl-review.googlesource.com/6558
Reviewed-by: Adam Langley <agl@google.com>
This dates to SSLeay 0.9.0. The Internet seems to have completely
forgotten what "D5" is. (I can't find reference to it beyond
documentation of this quirk.) The use counter we added sees virtually no
hits.
Change-Id: I9781d401acb98ce3790b1b165fc257a6f5e9b155
Reviewed-on: https://boringssl-review.googlesource.com/6557
Reviewed-by: Adam Langley <agl@google.com>
|EC_GROUP_get0_order| doesn't require any heap allocations and never
fails, so it is much more convenient and more efficient for callers to
call.
Change-Id: Ic60f768875e7bc8e74362dacdb5cbbc6957b05a6
Reviewed-on: https://boringssl-review.googlesource.com/6532
Reviewed-by: Adam Langley <agl@google.com>
BUG=webrtc:5222
Change-Id: I8399bd595564dedbe5492b8ea6eb915f41367cbf
Reviewed-on: https://boringssl-review.googlesource.com/6690
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Windows does support anonymous unions but warns about it. Since I'm not
sure what warnings we have enabled in Chromium, this change just drops
the union for Windows.
Change-Id: I914f8cd5855eb07153105250c0f026eaedb35365
Reviewed-on: https://boringssl-review.googlesource.com/6631
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
wpa_supplicant needs access to the internals of SHA_CTX. We supported
this only for builds with ANDROID defined previously but that's a pain
for wpa_supplicant to deal with. Thus this change enables it
unconditionally.
Perhaps in the future we'll be able to get a function to do this into
OpenSSL and BoringSSL.
Change-Id: Ib5d088c586fe69249c87404adb45aab5a7d5cf80
Reviewed-on: https://boringssl-review.googlesource.com/6630
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Right now your options are:
- Bounce on a reference and deal with cleanup needlessly.
- Manually check the type tag and peek into the union.
We probably have no hope of opaquifying this struct, but for new code, let's
recommend using this function rather than the more error-prone thing.
Change-Id: I9b39ff95fe4264a3f7d1e0d2894db337aa968f6c
Reviewed-on: https://boringssl-review.googlesource.com/6551
Reviewed-by: Adam Langley <agl@google.com>
The |ri| field was only used in |BN_MONT_CTX_set|, so make it a local
variable of that function.
Change-Id: Id8c3d44ac2e30e3961311a7b1a6731fe2c33a0eb
Reviewed-on: https://boringssl-review.googlesource.com/6526
Reviewed-by: Adam Langley <agl@google.com>
Trim the cipher table further. Those values are entirely determined by
algorithm_enc.
Change-Id: I355c245b0663e41e54e62d15903a4a9a667b4ffe
Reviewed-on: https://boringssl-review.googlesource.com/6516
Reviewed-by: Adam Langley <agl@google.com>
FIPS is the same as HIGH (but for CHACHA20), so those are redundant.
Likewise, MEDIUM vs HIGH was just RC4. Remove those in favor of
redefining those legacy rules to mean this.
One less field to keep track of in each cipher.
Change-Id: I2b2489cffb9e16efb0ac7d7290c173cac061432a
Reviewed-on: https://boringssl-review.googlesource.com/6515
Reviewed-by: Adam Langley <agl@google.com>
It's redundant with other cipher properties. We can express these in code.
Cipher rule matching gets a little bit complicated due to the confusing legacy
protocol version cipher rules, so add some tests for it. (It's really hard to
grep for uses of them, so I've kept them working to be safe.)
Change-Id: Ic6b3fcd55d76d4a51b31bf7ae629a2da50a7450e
Reviewed-on: https://boringssl-review.googlesource.com/6453
Reviewed-by: Adam Langley <agl@google.com>
The keylog BIO is internally synchronized by the SSL_CTX lock, but an
application may wish to log keys from multiple SSL_CTXs. This is in
preparation for switching Chromium to use a separate SSL_CTX per profile
to more naturally split up the session caches.
It will also be useful for routing up SSLKEYLOGFILE in WebRTC. There,
each log line must be converted to an IPC up from the renderer
processes.
This will require changes in Chromium when we roll BoringSSL.
BUG=458365,webrtc:4417
Change-Id: I2945bdb4def0a9c36e751eab3d5b06c330d66b54
Reviewed-on: https://boringssl-review.googlesource.com/6514
Reviewed-by: Adam Langley <agl@google.com>
