Most code already dereferences it directly.
Change-Id: I227fa91ecbf25a19077f7cfba21b0abd2bc2bd1d
Reviewed-on: https://boringssl-review.googlesource.com/7422
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I6267c9bfb66940d0b6fe5368514210a058ebd3cc
Reviewed-on: https://boringssl-review.googlesource.com/7494
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
EC point format negotiation is dead and gone.
Change-Id: If13ed7c5f31b64df2bbe90c018b2683b6371a980
Reviewed-on: https://boringssl-review.googlesource.com/7293
Reviewed-by: Adam Langley <agl@google.com>
C has implicit conversion of |void *| to other pointer types so these
casts are unnecessary. Clean them up to make the code easier to read
and to make it easier to find dangerous casts.
Change-Id: I26988a672e8ed4d69c75cfbb284413999b475464
Reviewed-on: https://boringssl-review.googlesource.com/7102
Reviewed-by: David Benjamin <davidben@google.com>
Take the mappings for MD5 and SHA-224 values out of the code altogether. This
aligns with the current TLS 1.3 draft.
For MD5, this is a no-op. It is not currently possible to configure accepted
signature algorithms, MD5 wasn't in the hardcoded list, and we already had a
test ensuring we enforced our preferences correctly. MD5 also wasn't in the
default list of hashes our keys could sign and no one overrides it with a
different hash.
For SHA-224, this is not quite a no-op. The hardcoded accepted signature
algorithms list included SHA-224, so this will break servers relying on that.
However, Chrome's metrics have zero data points of servers picking SHA-224 and
no other major browser includes it. Thus that should be safe.
SHA-224 was also in the default list of hashes we are willing to sign. For
client certificates, Chromium's abstractions already did not allow signing
SHA-224, so this is a no-op there. For servers, this will break any clients
which only accept SHA-224. But no major browsers do this and I am not aware of
any client implementation which does such ridiculous thing.
(SHA-1's still in there. Getting rid of that one is going to take more effort.)
Change-Id: I6a765fdeea9e19348e409d58a0eac770b318e599
Reviewed-on: https://boringssl-review.googlesource.com/7020
Reviewed-by: Adam Langley <agl@google.com>
This removes the various non-PRF checks from SSL3_ENC_METHOD so that can
have a clearer purpose. It also makes TLS 1.0 through 1.2's
SSL3_ENC_METHOD tables identical and gives us an assert to ensure
nothing accesses the version bits before version negotiation.
Accordingly, ssl_needs_record_splitting was reordered slightly so we
don't rely on enc_method being initialized to TLS 1.2
pre-version-negotiation.
This leaves alert_value as the only part of SSL3_ENC_METHOD which may be
accessed before version negotiation.
Change-Id: If9e299e2ef5511b5fa442b2af654eed054c3e675
Reviewed-on: https://boringssl-review.googlesource.com/6842
Reviewed-by: Adam Langley <alangley@gmail.com>
node.js is, effectively, another bindings library. However, it's better
written than most and, with these changes, only a couple of tiny fixes
are needed in node.js. Some of these changes are a little depressing
however so we'll need to push node.js to use APIs where possible.
Changes:
∙ Support verify_recover. This is very obscure and the motivation
appears to be https://github.com/nodejs/node/issues/477 – where it's
not clear that anyone understands what it means :(
∙ Add a few, no-op #defines
∙ Add some members to |SSL_CTX| and |SSL| – node.js needs to not
reach into these structs in the future.
∙ Add EC_get_builtin_curves.
∙ Add EVP_[CIPHER|MD]_do_all_sorted – these functions are limited to
decrepit.
Change-Id: I9a3566054260d6c4db9d430beb7c46cc970a9d46
Reviewed-on: https://boringssl-review.googlesource.com/6952
Reviewed-by: Adam Langley <agl@google.com>
I got that from the TLS 1.3 draft, but it's kind of silly-looking. X25519
already refers to a Diffie-Hellman primitive.
Also hopefully the WG will split NamedGroups and SignatureAlgorithms per the
recent proposal, so it won't be needed anyway. (Most chatter is about what
hashes should be allowed with what NIST curves, so it seems like people like
the split itself? We'll see.)
Change-Id: I7bb713190001199a3ebd30b67df2c00d29132431
Reviewed-on: https://boringssl-review.googlesource.com/6912
Reviewed-by: Adam Langley <agl@google.com>
The various SSL3_ENC_METHODs ought to be defined in the same file their
functions are defined in, so they can be static.
Change-Id: I34a1d3437e8e61d4d50f2be70312e4630ea89c19
Reviewed-on: https://boringssl-review.googlesource.com/6840
Reviewed-by: Adam Langley <alangley@gmail.com>
There's not much point in putting those in the interface as the
final_finished_mac implementation is itself different between SSL 3.0
and TLS.
Change-Id: I76528a88d255c451ae008f1a34e51c3cb57d3073
Reviewed-on: https://boringssl-review.googlesource.com/6838
Reviewed-by: Adam Langley <alangley@gmail.com>
As things stand now, they don't actually do anything.
Change-Id: I9f8b4cbf38a0dffabfc5265805c52bb8d7a8fb0d
Reviewed-on: https://boringssl-review.googlesource.com/6837
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>
It's the same between TLS and SSL 3.0. There's also no need for the
do_change_cipher_spec wrapper (it no longer needs checks to ensure it
isn't called at a bad place). Finally fold the setup_key_block call into
change_cipher_spec.
Change-Id: I7917f48e1a322f5fbafcf1dfb8ad53f66565c314
Reviewed-on: https://boringssl-review.googlesource.com/6834
Reviewed-by: Adam Langley <alangley@gmail.com>
This is a minor regression from
https://boringssl-review.googlesource.com/5235.
If the client, for whatever reason, had an ID-based session but also
supports tickets, it will send non-empty ID + empty ticket extension.
If the ticket extension is non-empty, then the ID is not an ID but a
dummy signaling value, so 5235 avoided looking it up. But if it is
present and empty, the ID is still an ID and should be looked up.
This shouldn't have any practical consequences, except if a server
switched from not supporting tickets and then started supporting it,
while keeping the session cache fixed.
Add a test for this case, and tighten up existing ID vs ticket tests so
they fail if we resume with the wrong type.
Change-Id: Id4d08cd809af00af30a2b67fe3a971078e404c75
Reviewed-on: https://boringssl-review.googlesource.com/6554
Reviewed-by: Adam Langley <alangley@gmail.com>
That we're half and half is really confusing.
Change-Id: I1c2632682e8a3e63d01dada8e0eb3b735ff709ce
Reviewed-on: https://boringssl-review.googlesource.com/6785
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>
It already wasn't in the default list and no one enables it. Remove it
altogether. (It's also gone from the current TLS 1.3 draft.)
Change-Id: I143d07d390d186252204df6bdb8ffd22649f80e3
Reviewed-on: https://boringssl-review.googlesource.com/6775
Reviewed-by: Adam Langley <agl@google.com>
In doing so, make the asynchronous portion look more like
ssl3_send_server_key_exchange. This is a considerably simpler structure,
so the save/resume doesn't need any state.
Mostly this means writing out the signature algorithm can now go through
CBB rather than a uint8_t* without bounds check.
Change-Id: If99fcffd0d41a84514c3d23034062c582f1bccb2
Reviewed-on: https://boringssl-review.googlesource.com/6771
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>
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>
RFC 5746 forbids a server from downgrading or upgrading
renegotiation_info support. Even with SSL_OP_LEGACY_SERVER_CONNECT set
(the default), we can still enforce a few things.
I do not believe this has practical consequences. The attack variant
where the server half is prefixed does not involve a renegotiation on
the client. The converse where the client sees the renegotiation and
prefix does, but we only support renego for the mid-stream HTTP/1.1
client auth hack, which doesn't do this. (And with triple-handshake,
HTTPS clients should be requiring the certificate be unchanged across
renego which makes this moot.)
Ultimately, an application which makes the mistake of using
renegotiation needs to be aware of what exactly that means and how to
handle connection state changing mid-stream. We make renego opt-in now,
so this is a tenable requirement.
(Also the legacy -> secure direction would have been caught by the
server anyway since we send a non-empty RI extension.)
Change-Id: I915965c342f8a9cf3a4b6b32f0a87a00c3df3559
Reviewed-on: https://boringssl-review.googlesource.com/6559
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 current check has two problems:
- It only runs on the server, where there isn't a curve list at all. This was a
mistake in https://boringssl-review.googlesource.com/1843 which flipped it
from client-only to server-only.
- It only runs in TLS 1.2, so one could bypass it by just negotiating TLS 1.1.
Upstream added it as part of their Suite B mode, which requires 1.2.
Move it elsewhere. Though we do not check the entire chain, leaving that to the
certificate verifier, signatures made by the leaf certificate are made by the
SSL/TLS stack, so it's reasonable to check the curve as part of checking
suitability of a leaf.
Change-Id: I7c12f2a32ba946a20e9ba6c70eff23bebcb60bb2
Reviewed-on: https://boringssl-review.googlesource.com/6414
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>
This is completely a no-op as currently tls12_get_psigalgs always returns a
hardcoded list which always includes SHA-1. But if this were to be made
configurable in the future, we should reject SHA-1 when configured to do so.
Change-Id: I7ab188eeff850d1e5f70b9522304812bab2d941a
Reviewed-on: https://boringssl-review.googlesource.com/6411
Reviewed-by: Adam Langley <agl@google.com>
Later when TLS 1.3 comes around, we'll need SSL_CIPHER_get_max_version too. In
the meantime, hide the SSL_TLSV1_2 messiness behind a reasonable API.
Change-Id: Ibcc17cccf48dd99e364d6defdfa5a87d031ecf0a
Reviewed-on: https://boringssl-review.googlesource.com/6452
Reviewed-by: Adam Langley <agl@google.com>
Right whether NPN is advertised can only be configured globally on the SSL_CTX.
Rather than adding two pointers to each SSL*, add an options bit to disable it
so we may plumb in a field trial to disable NPN.
Chromium wants to be able to route a bit in to disable NPN, but it uses SSL_CTX
incorrectly and has a global one, so it can't disconnect the callback. (That
really needs to get fixed. Although it's not clear this necessarily wants to be
lifted up to SSL_CTX as far as Chromium's SSLClientSocket is concerned since
NPN doesn't interact with the session cache.)
BUG=526713
Change-Id: I49c86828b963eb341c6ea6a442557b7dfa190ed3
Reviewed-on: https://boringssl-review.googlesource.com/6351
Reviewed-by: Adam Langley <alangley@gmail.com>
Also added a SSL_CTX_set_select_certificate_cb setter for
select_certificate_cb so code needn't access SSL_CTX directly. Plus it
serves as a convenient anchor for the documentation.
Change-Id: I23755b910e1d77d4bea7bb9103961181dd3c5efe
Reviewed-on: https://boringssl-review.googlesource.com/6291
Reviewed-by: Adam Langley <alangley@gmail.com>
Start converting the ones we can right now. Some of the messier ones
resize init_buf rather than assume the initial size is sufficient, so
those will probably wait until init_buf is gone and the handshake's
undergone some more invasive surgery. The async ones will also require
some thought. But some can be incrementally converted now.
BUG=468889
Change-Id: I0bc22e4dca37d9d671a488c42eba864c51933638
Reviewed-on: https://boringssl-review.googlesource.com/6190
Reviewed-by: Adam Langley <alangley@gmail.com>
SSL 3.0 used to have a nice and simple rule around extensions. They don't
exist. And then RFC 5746 came along and made this all extremely confusing.
In an SSL 3.0 server, rather than blocking ServerHello extension
emission when renegotiation_info is missing, ignore all ClientHello
extensions but renegotiation_info. This avoids a mismatch between local
state and the extensions with emit.
Notably if, for some reason, a ClientHello includes the session_ticket
extension, does NOT include renegotiation_info or the SCSV, and yet the
client or server are decrepit enough to negotiate SSL 3.0, the
connection will fail due to unexpected NewSessionTicket message.
See https://crbug.com/425979#c9 for a discussion of something similar
that came up in diagnosing https://poodle.io/'s buggy POODLE check.
This is analogous to upstream's
5a3d8eebb7667b32af0ccc3f12f314df6809d32d.
(Not supporting renego as a server in any form anyway, we may as well
completely ignore extensions, but then our extensions callbacks can't
assume the parse hooks are always called. This way the various NULL
handlers still function.)
Change-Id: Ie689a0e9ffb0369ef7a20ab4231005e87f32d5f8
Reviewed-on: https://boringssl-review.googlesource.com/6180
Reviewed-by: Adam Langley <agl@google.com>
The Android system BoringSSL has a couple of changes:
∙ ChaCha20-Poly1305 is disabled because it's not an offical
cipher suite.
∙ P-521 is offered in the ClientHello.
These changes were carried in the Android BoringSSL repo directly. This
change upstreams them when BORINGSSL_ANDROID_SYSTEM is defined.
Change-Id: If3e787c6694655b56e7701118aca661e97a5f26c
Allow configuring digest preferences for the private key. Some
smartcards have limited support for signing digests, notably Windows
CAPI keys and old Estonian smartcards. Chromium used the supports_digest
hook in SSL_PRIVATE_KEY_METHOD to limit such keys to SHA1. However,
detecting those keys was a heuristic, so some SHA256-capable keys
authenticating to SHA256-only servers regressed in the switch to
BoringSSL. Replace this mechanism with an API to configure digest
preference order. This way heuristically-detected SHA1-only keys may be
configured by Chromium as SHA1-preferring rather than SHA1-requiring.
In doing so, clean up the shared_sigalgs machinery somewhat.
BUG=468076
Change-Id: I996a2df213ae4d8b4062f0ab85b15262ca26f3c6
Reviewed-on: https://boringssl-review.googlesource.com/5755
Reviewed-by: Adam Langley <agl@google.com>
SCT and OCSP are part of the session data and as such shouldn't be sent
again to the client when resuming.
Change-Id: Iaee3a3c4c167ea34b91504929e38aadee37da572
Reviewed-on: https://boringssl-review.googlesource.com/5900
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
ssl.h should be first. Also two lines after includes and the rest of the
file.
Change-Id: Icb7586e00a3e64170082c96cf3f8bfbb2b7e1611
Reviewed-on: https://boringssl-review.googlesource.com/5892
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifa44fef160fc9d67771eed165f8fc277f28a0222
Reviewed-on: https://boringssl-review.googlesource.com/5840
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
If the two extensions select different next protocols (quite possible since one
is server-selected and the other is client-selected), things will break. This
matches the behavior of NSS (Firefox) and Go.
Change-Id: Ie1da97bf062b91a370c85c12bc61423220a22f36
Reviewed-on: https://boringssl-review.googlesource.com/5780
Reviewed-by: Adam Langley <agl@google.com>
This is a simpler implementation than OpenSSL's, lacking responder IDs
and request extensions support. This mirrors the client implementation
already present.
Change-Id: I54592b60e0a708bfb003d491c9250401403c9e69
Reviewed-on: https://boringssl-review.googlesource.com/5700
Reviewed-by: Adam Langley <agl@google.com>
They're not called (new in 1.0.2). We actually may well need to
configure these later to strike ECDSA from the list on Chrome/XP
depending on what TLS 1.3 does, but for now striking it from the cipher
suite list is both necessary and sufficient. I think we're better off
removing these for now and adding new APIs later if we need them.
(This API is weird. You pass in an array of NIDs that must be even
length and alternating between hash and signature NID. We'd also need a
way to query the configured set of sigalgs to filter away. Those used to
exist but were removed in
https://boringssl-review.googlesource.com/#/c/5347/. SSL_get_sigalgs is
an even uglier API and doesn't act on the SSL_CTX.)
And with that, SSL_ctrl and SSL_CTX_ctrl can *finally* be dropped. Don't
leave no-op wrappers; anything calling SSL_ctrl and SSL_CTX_ctrl should
instead switch to the wrapper macros.
BUG=404754
Change-Id: I5d465cd27eef30d108eeb6de075330c9ef5c05e8
Reviewed-on: https://boringssl-review.googlesource.com/5675
Reviewed-by: Adam Langley <agl@google.com>
This change stores the size of the group/modulus (for RSA/DHE) or curve
ID (for ECDHE) in the |SSL_SESSION|. This makes it available for UIs
where desired.
Change-Id: I354141da432a08f71704c9683f298b361362483d
Reviewed-on: https://boringssl-review.googlesource.com/5280
Reviewed-by: Adam Langley <agl@google.com>
Rather than iterate over handshake_dgsts itself, it can just call
tls1_handshake_digest.
Change-Id: Ia518da540e47e65b13367eb1af184c0885908488
Reviewed-on: https://boringssl-review.googlesource.com/5617
Reviewed-by: Adam Langley <agl@google.com>
The handshake hash is initialized from the buffer as soon as the cipher
is known. When adding a message to the transcript, independently update
the buffer and rolling hash, whichever is active. This avoids the
complications around dont_free_handshake_buffer and EMS.
BUG=492371
Change-Id: I3b1065796a50fd1be5d42ead7210c2f253ef0aca
Reviewed-on: https://boringssl-review.googlesource.com/5615
Reviewed-by: Adam Langley <agl@google.com>
The only point format that we ever support is uncompressed, which the
RFC says implementations MUST support. The TLS 1.3 and Curve25519
forecast is that point format negotiation is gone. Each curve has just
one point format and it's labeled, for historial reasons, as
"uncompressed".
Change-Id: I8ffc8556bed1127cf288d2a29671abe3c9b3c585
Reviewed-on: https://boringssl-review.googlesource.com/5542
Reviewed-by: Adam Langley <agl@google.com>
This change mirrors upstream's custom extension API because we have some
internal users that depend on it.
Change-Id: I408e442de0a55df7b05c872c953ff048cd406513
Reviewed-on: https://boringssl-review.googlesource.com/5471
Reviewed-by: Adam Langley <agl@google.com>
With the fastradio stuff gone, the padding computation is slightly more
straight-forward.
Change-Id: I67ede92fdf5f34c265c7a44e4cdc1a5ce5416df2
Reviewed-on: https://boringssl-review.googlesource.com/5482
Reviewed-by: Adam Langley <agl@google.com>
Fastradio was a trick where the ClientHello was padding to at least 1024
bytes in order to trick some mobile radios into entering high-power mode
immediately. After experimentation, the feature is being dropped.
This change also tidies up a bit of the extensions code now that
everything is using the new system.
Change-Id: Icf7892e0ac1fbe5d66a5d7b405ec455c6850a41c
Reviewed-on: https://boringssl-review.googlesource.com/5466
Reviewed-by: Adam Langley <agl@google.com>
This also removes support for the “old” Channel ID extension.
Change-Id: I1168efb9365c274db6b9d7e32013336e4404ff54
Reviewed-on: https://boringssl-review.googlesource.com/5462
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I5777b73f485da6534b407e6c531f8293898b9c06
Reviewed-on: https://boringssl-review.googlesource.com/5461
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I3b085cd13295e83c578c549763f0de82f39499a2
Reviewed-on: https://boringssl-review.googlesource.com/5460
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Chromium's NaCl build has _POSIX_SOURCE already defined, so #undef it first.
The compiler used also dislikes static asserts with the same name.
Change-Id: I0283fbad1a2ccf98cdb0ca2a7965b15441806308
Reviewed-on: https://boringssl-review.googlesource.com/5430
Reviewed-by: Adam Langley <agl@google.com>
It switched from CBB_remaining to CBB_len partway through review, but
the semantics are still CBB_remaining. Using CBB_len allows the
len_before/len_after logic to continue working even if, in the future,
handshake messages are built on a non-fixed CBB.
Change-Id: Id466bb341a14dbbafcdb26e4c940a04181f2787d
Reviewed-on: https://boringssl-review.googlesource.com/5371
Reviewed-by: Adam Langley <agl@google.com>
This allows us to remove the confusing EVP_PKEY argument to the
SSL_PRIVATE_KEY_METHOD wrapper functions. It also simplifies some of the
book-keeping around the CERT structure, as well as the API for
configuring certificates themselves. The current one is a little odd as
some functions automatically route to the slot while others affect the
most recently touched slot. Others still (extra_certs) apply to all
slots, making them not terribly useful.
Consumers with complex needs should use cert_cb or the early callback
(select_certificate_cb) to configure whatever they like based on the
ClientHello.
BUG=486295
Change-Id: Ice29ffeb867fa4959898b70dfc50fc00137f01f3
Reviewed-on: https://boringssl-review.googlesource.com/5351
Reviewed-by: Adam Langley <agl@google.com>
There's no need to store more than the TLS values.
Change-Id: I1a93c7c6aa3254caf7cc09969da52713e6f8acf4
Reviewed-on: https://boringssl-review.googlesource.com/5348
Reviewed-by: Adam Langley <agl@google.com>
These are new as of 1.0.2, not terribly useful of APIs, and are the only
reason we have to retain so many NIDs in the TLS_SIGALGS structure.
Change-Id: I7237becca09acc2ec2be441ca17364f062253893
Reviewed-on: https://boringssl-review.googlesource.com/5347
Reviewed-by: Adam Langley <agl@google.com>
Use more sensible variable names. Also move some work between the helpers and
s3_srvr.c a little; the session lookup functions now only return a new session.
Whether to send a ticket is now an additional output to avoid the enum
explosion around renewal. The actual SSL state is not modified.
This is somewhat cleaner as s3_srvr.c may still reject a session for other
reasons, so we avoid setting ssl->session and ssl->verify_result to a session
that wouldn't be used. (They get fixed up in ssl_get_new_session, so it didn't
actually matter.)
Change-Id: Ib52fabbe993b5e2b7408395a02cdea3dee66df7b
Reviewed-on: https://boringssl-review.googlesource.com/5235
Reviewed-by: Adam Langley <agl@google.com>
This change also switches the behaviour of the client. Previously the
client would send the SCSV rather than the extension, but now it'll only
do that for SSLv3 connections.
Change-Id: I67a04b8abbef2234747c0dac450458deb6b0cd0a
Reviewed-on: https://boringssl-review.googlesource.com/5143
Reviewed-by: Adam Langley <agl@google.com>
It's still the case that we have many old compilers that can't cope with
anything else ☹.
Change-Id: Ie5a1987cd5164bdbde0c17effaa62aecb7d12352
Reviewed-on: https://boringssl-review.googlesource.com/5320
Reviewed-by: Adam Langley <agl@google.com>
Rather than four massive functions that handle every extension,
organise the code by extension with four smaller functions for each.
Change-Id: I876b31dacb05aca9884ed3ae7c48462e6ffe3b49
Reviewed-on: https://boringssl-review.googlesource.com/5142
Reviewed-by: Adam Langley <agl@google.com>
This adds a new API, SSL_set_private_key_method, which allows the consumer to
customize private key operations. For simplicity, it is incompatible with the
multiple slots feature (which will hopefully go away) but does not, for now,
break it.
The new method is only routed up for the client for now. The server will
require a decrypt hook as well for the plain RSA key exchange.
BUG=347404
Change-Id: I35d69095c29134c34c2af88c613ad557d6957614
Reviewed-on: https://boringssl-review.googlesource.com/5049
Reviewed-by: Adam Langley <agl@google.com>
Mirrors SSL_SESSION_to_bytes. It avoids having to deal with object-reuse, the
non-size_t length parameter, and trailing data. Both it and the object-reuse
variant back onto an unexposed SSL_SESSION_parse which reads a CBS.
Note that this changes the object reuse story slightly. It's now merely an
optional output pointer that frees its old contents. No d2i_SSL_SESSION
consumer in Google that's built does reuse, much less reuse with the assumption
that the top-level object won't be overridden.
Change-Id: I5cb8522f96909bb222cab0f342423f2dd7814282
Reviewed-on: https://boringssl-review.googlesource.com/5121
Reviewed-by: Adam Langley <agl@google.com>
ssl_cipher_list_to_bytes is client-only, so s->renegotiate worked, but
the only reason the other two worked is because s->renegotiate isn't a
lie on the server before ServerHello.
BUG=429450
Change-Id: If68a986c6ec4a0f16e57a6187238e05b50ecedfc
Reviewed-on: https://boringssl-review.googlesource.com/4822
Reviewed-by: Adam Langley <agl@google.com>
tls1_enc is now SSL_AEAD_CTX_{open,seal}. This starts tidying up a bit
of the record-layer logic. This removes rr->input, as encrypting and
decrypting records no longer refers to various globals. It also removes
wrec altogether. SSL3_RECORD is now only used to maintain state about
the current incoming record. Outgoing records go straight to the write
buffer.
This also removes the outgoing alignment memcpy and simply calls
SSL_AEAD_CTX_seal with the parameters as appropriate. From bssl speed
tests, this seems to be faster on non-ARM and a bit of a wash on ARM.
Later it may be worth recasting these open/seal functions to write into
a CBB (tweaked so it can be malloc-averse), but for now they take an
out/out_len/max_out trio like their EVP_AEAD counterparts.
BUG=468889
Change-Id: Ie9266a818cc053f695d35ef611fd74c5d4def6c3
Reviewed-on: https://boringssl-review.googlesource.com/4792
Reviewed-by: Adam Langley <agl@google.com>
This cuts down on one config knob as well as one case in the renego
combinatorial explosion. Since the only case we care about with renego
is the client auth hack, there's no reason to ever do resumption.
Especially since, no matter what's in the session cache:
- OpenSSL will only ever offer the session it just established,
whether or not a newer one with client auth was since established.
- Chrome will never cache sessions created on a renegotiation, so
such a session would never make it to the session cache.
- The new_session + SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
logic had a bug where it would unconditionally never offer tickets
(but would advertise support) on renego, so any server doing renego
resumption against an OpenSSL-derived client must not support
session tickets.
This also gets rid of s->new_session which is now pointless.
BUG=429450
Change-Id: I884bdcdc80bff45935b2c429b4bbc9c16b2288f8
Reviewed-on: https://boringssl-review.googlesource.com/4732
Reviewed-by: Adam Langley <agl@google.com>
There's multiple different versions of this check, between
s->s3->have_version (only works at some points), s->new_session (really
weird and not actually right), s->renegotiate (fails on the server
because it's always 2 after ClientHello), and s->s3->tmp.finish_md_len
(super confusing). Add an explicit bit with clear meaning. We'll prune
some of the others later; notably s->renegotiate can go away when
initiating renegotiation is removed.
This also tidies up the extensions to be consistent about whether
they're allowed during renego:
- ALPN failed to condition when accepting from the server, so even
if the client didn't advertise, the server could.
- SCTs now *are* allowed during renego. I think forbidding it was a
stray copy-paste. It wasn't consistently enforced in both ClientHello
and ServerHello, so the server could still supply it. Moreover, SCTs
are part of the certificate, so we should accept it wherever we accept
certificates, otherwise that session's state becomes incomplete. This
matches OCSP stapling. (NB: Chrome will never insert a session created
on renego into the session cache and won't accept a certificate
change, so this is moot anyway.)
Change-Id: Ic9bd1ebe2a2dbe75930ed0213bf3c8ed8170e251
Reviewed-on: https://boringssl-review.googlesource.com/4730
Reviewed-by: Adam Langley <agl@google.com>
When tlsext_ticket_key_cb is used, the full bounds aren't known until
after the callback has returned.
Change-Id: I9e89ffae6944c74c4ca04e6aa28afd3ec80aa1d4
Reviewed-on: https://boringssl-review.googlesource.com/4552
Reviewed-by: Adam Langley <agl@google.com>
It's unused, but for some old #ifdef branch in wpa_supplicant's EAP-FAST
hack, before SSL_set_session_ticket_ext_cb existed.
Change-Id: Ifc11fea2f6434354f756e04e5fc3ed5f1692025e
Reviewed-on: https://boringssl-review.googlesource.com/4550
Reviewed-by: Adam Langley <agl@google.com>
This is a really dumb API wart. Now that we have a limited set of curves that
are all reasonable, the automatic logic should just always kick in. This makes
set_ecdh_auto a no-op and, instead of making it the first choice, uses it as
the fallback behavior should none of the older curve selection APIs be used.
Currently, by default, server sockets can only use the plain RSA key exchange.
BUG=481139
Change-Id: Iaabc82de766cd00968844a71aaac29bd59841cd4
Reviewed-on: https://boringssl-review.googlesource.com/4531
Reviewed-by: Adam Langley <agl@google.com>
See upstream's bd891f098bdfcaa285c073ce556d0f5e27ec3a10. It honestly seems
kinda dumb for a client to do this, but apparently the spec allows this.
Judging by code inspection, OpenSSL 1.0.1 also allowed this, so this avoids a
behavior change when switching from 1.0.1 to BoringSSL.
Add a test for this, which revealed that, unlike upstream's version, this
actually works with ecdh_auto since tls1_get_shared_curve also needs updating.
(To be mentioned in newsletter.)
Change-Id: Ie622700f17835965457034393b90f346740cfca8
Reviewed-on: https://boringssl-review.googlesource.com/4464
Reviewed-by: Adam Langley <agl@google.com>
These are all masks of some sort (except id which is a combined version and
cipher), so they should use fixed-size unsigned integers.
Change-Id: I058dd8ad231ee747df4b4fb17d9c1e2cbee21918
Reviewed-on: https://boringssl-review.googlesource.com/4283
Reviewed-by: Adam Langley <agl@google.com>
The rest of ssl/ still includes things everywhere, but this at least fixes the
includes that were implicit from ssl/internal.h.
Change-Id: I7ed22590aca0fe78af84fd99a3e557f4b05f6782
Reviewed-on: https://boringssl-review.googlesource.com/4281
Reviewed-by: Adam Langley <agl@google.com>
Match the other internal headers.
Change-Id: Iff7e2dd06a1a7bf993053d0464cc15638ace3aaa
Reviewed-on: https://boringssl-review.googlesource.com/4280
Reviewed-by: Adam Langley <agl@google.com>
There's multiple sets of APIs for selecting the curve. Fold away
SSL_OP_SINGLE_ECDH_USE as failing to set it is either a no-op or a bug. With
that gone, the consumer only needs to control the selection of a curve, with
key generation from then on being uniform. Also clean up the interaction
between the three API modes in s3_srvr.c; they were already mutually exclusive
due to tls1_check_ec_tmp_key.
This also removes all callers of EC_KEY_dup (and thus CRYPTO_dup_ex_data)
within the library.
Change-Id: I477b13bd9e77eb03d944ef631dd521639968dc8c
Reviewed-on: https://boringssl-review.googlesource.com/4200
Reviewed-by: Adam Langley <agl@google.com>
Quite a few functions reported wrong function names when pushing
to the error stack.
Change-Id: I84d89dbefd2ecdc89ffb09799e673bae17be0e0f
Reviewed-on: https://boringssl-review.googlesource.com/4080
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Align with upstream's renames from a while ago. These names are considerably
more standard. This also aligns with upstream in that both "ECDHE" and "EECDH"
are now accepted in the various cipher string parsing bits.
Change-Id: I84c3daeacf806f79f12bc661c314941828656b04
Reviewed-on: https://boringssl-review.googlesource.com/4053
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 34e3edbf3a10953cb407288101fd56a629af22f9. This fixes
CVE-2015-0291. Also bubble up malloc failures in tls1_set_shared_sigalgs. Tidy
up style a bit and remove unnecessary check (it actually is unnecessary; see
https://boringssl-review.googlesource.com/4042).
Change-Id: Idfb31a90fb3e56ef6fe7701464748a5c1603f064
Reviewed-on: https://boringssl-review.googlesource.com/4047
Reviewed-by: Adam Langley <agl@google.com>
Some things were misindented in the reformatting.
Change-Id: I97642000452ce4d5b4c8a39b794cec13097d8760
Reviewed-on: https://boringssl-review.googlesource.com/3870
Reviewed-by: Adam Langley <agl@google.com>
None of these are version-specific. SSL_PROTOCOL_METHOD's interface will change
later, but this gets us closer to folding away SSL3_ENC_METHOD.
Change-Id: Ib427cdff32d0701a18fe42a52cdbf798f82ba956
Reviewed-on: https://boringssl-review.googlesource.com/3769
Reviewed-by: Adam Langley <agl@google.com>
Found while diagnosing some crashes and hangs in the malloc tests. This (and
the follow-up) get us further but does not quite let the malloc tests pass
quietly, even without valgrind. DTLS silently ignores some malloc failures
(confusion with silently dropping bad packets) which then translate to hangs.
Change-Id: Ief06a671e0973d09d2883432b89a86259e346653
Reviewed-on: https://boringssl-review.googlesource.com/3482
Reviewed-by: Adam Langley <agl@google.com>
