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>
Without |EC_POINTs_mul|, there's never more than one variable point
passed to a |EC_METHOD|'s |mul| method. This allows them to be
simplified considerably. In this commit, the p256-x86_64 implementation
has been simplified to eliminate the heap allocation and looping
related that was previously necessary to deal with the possibility of
there being multiple input points. The other implementations were left
mostly as-is; they should be similarly simplified in the future.
Change-Id: I70751d1d5296be2562af0730e7ccefdba7a1acae
Reviewed-on: https://boringssl-review.googlesource.com/6493
Reviewed-by: Adam Langley <agl@google.com>
This moves us closer to having |EC_GROUP| and |EC_KEY| being immutable.
The functions are left as no-ops for backward compatibility.
Change-Id: Ie23921ab0364f0771c03aede37b064804c9f69e0
Reviewed-on: https://boringssl-review.googlesource.com/6485
Reviewed-by: Adam Langley <agl@google.com>
If -mfpu=neon is passed then we don't need to worry about checking for
NEON support at run time. This change allows |CRYPTO_is_NEON_capable| to
statically return 1 in this case. This then allows the compiler to
discard generic code in several cases.
Change-Id: I3b229740ea3d5cb0a304f365c400a0996d0c66ef
Reviewed-on: https://boringssl-review.googlesource.com/6523
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I3072f884be77b8646e90d316154b96448f0cf2a1
Reviewed-on: https://boringssl-review.googlesource.com/6520
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
So long as we're not getting rid of them (the certificate variants may
be useful when we decouple from crypto/x509 anyway), get the types and
bounds checks right.
Also reject trailing data and require the input be a single element.
Note: this is a slight compatibility risk, but we did it for
SSL*_use_RSAPrivateKey_ASN1 previously and I think it's probably worth
seeing if anything breaks here.
Change-Id: I64fa3fc6249021ccf59584d68e56ff424a190082
Reviewed-on: https://boringssl-review.googlesource.com/6490
Reviewed-by: Adam Langley <agl@google.com>
OpenSSH calls |RAND_seed| before jailing in the expectation that that
will be sufficient to ensure that later RAND calls are successful.
See internal bug 25695426.
Change-Id: I9d3f5665249af6610328ac767cb83059bb2953dd
Reviewed-on: https://boringssl-review.googlesource.com/6494
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
dh.c had a 10k-bit limit but it wasn't quite correctly enforced. However,
that's still 1.12s of jank on the IO thread, which is too long. Since the SSL
code consumes DHE groups from the network, it should be responsible for
enforcing what sanity it needs on them.
Costs of various bit lengths on 2013 Macbook Air:
1024 - 1.4ms
2048 - 14ms
3072 - 24ms
4096 - 55ms
5000 - 160ms
10000 - 1.12s
UMA says that DHE groups are 0.2% 4096-bit and otherwise are 5.5% 2048-bit and
94% 1024-bit and some noise. Set the limit to 4096-bit to be conservative,
although that's already quite a lot of jank.
BUG=554295
Change-Id: I8e167748a67e4e1adfb62d73dfff094abfa7d215
Reviewed-on: https://boringssl-review.googlesource.com/6464
Reviewed-by: Adam Langley <agl@google.com>
This exposes the ServerKeyExchange signature hash type used in the most recent
handshake, for histogramming on the client.
BUG=549662
Change-Id: I8a4e00ac735b1ecd2c2df824112c3a0bc62332a7
Reviewed-on: https://boringssl-review.googlesource.com/6413
Reviewed-by: Adam Langley <agl@google.com>
I've used these defines to easy the update of BoringSSL in Android
because Android's external/boringssl is a different git repository from
the rest of Android and thus it's not possible to land changes the
atomically update several things at once.
For this I tended just to add this define in the Android copy of
BoringSSL, but we're starting to see that bleed into other situations
now so it's looking like this will be generally useful.
These defines may be added when useful but shouldn't build up: once the
change has been done, the #if'ed code elsewhere that uses it should be
cleaned up. So far, that's worked ok. (I.e. we've had a BORINGSSL_201509
that correctly disappeared.)
Change-Id: I8cbb4731efe840cc798c970d37bc040b16a4a755
Reviewed-on: https://boringssl-review.googlesource.com/6442
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Not sure if we want to leave bio.h and bytestring.h's instance as-is, but the
evp.h ones are just baffling.
Change-Id: I485c2e355ba93764da0c4c72c48af48b055a8500
Reviewed-on: https://boringssl-review.googlesource.com/6454
Reviewed-by: Adam Langley <agl@google.com>
