Now that we don't depend on external CRYPTO_library_init calls or the
static initializer to initialize CPU capabilities, we can drop a ton of
code.
This makes CRYPTO_library_init, and all its wrappers, into no-ops and
drops the (non-FIPS) static initializer. I've added an internal
OPENSSL_init_cpuid function for the places where the library actually
needs to initialize the CPU vector.
Note this slightly changes the default, previously
static-initializer-full build: previously, CRYPTO_library_init was a
no-op and we relied on the static initializer. Now we uniformly use
CRYPTO_once. This should be an atomic read in the steady state and
essentially free. We can restore the static initializer by default if
this ends up being a problem, but having only one mode is more
straightforward. This also avoids problems if an application calls into
BoringSSL during its own static initializer. Static initializers are not
coherently ordered.
Update-Note: The BORINGSSL_NO_STATIC_INITIALIZER build option and
CRYPTO_library_init are now unnecessary. Once updating past this
revision, those options can now be cleaned up from downstream projects.
Fixed: 40644931
Change-Id: Idc2e6ea7a73d6352e0360fd886c46d88dba3568c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69508
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
See draft-ietf-tls13-pkcs1-00. The code point is disabled by default
and must be configured in SSL_set_verify_algorithm_prefs and
SSL_set_signing_algorithm_prefs. It is also only defined for TLS 1.3
client certificates and otherwise ignored.
This required reworking the tests a bit since this is the first
signature algorithm that's disabled by default, and the first algorithm
that behaves differently between client and server.
Bug: 347047841
Change-Id: If4f653a456799ed9f0173159da291a9b6b6556fb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69368
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I noticed that runner tests had a very weird test expectation on the
alerts sent around sigalg failures. I think this was an (unimportant)
bug on our end.
If the peer picks a sigalg that we didn't advertise, we send
illegal_parameter. However, it if picks an advertised sigalg that is
invalid in context (protocol version, public key), we end up catching it
very late in ssl_public_key_verify (by way of setup_ctx) and sending
decrypt_error.
Instead, have tls12_check_peer_sigalg check this so we consistently send
illegal_parameter. (Probably this should all fold into
ssl_public_key_verify with an alert out_param, but so it goes.)
Change-Id: I09fb84e9c1ee39b2683fa0b67dd6135d31f51c97
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69367
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
NPN is a little odd, owing to being a three-step process. The client
offers NPN, then the server accepts NPN and offers a list of protocols,
then the client picks a protocol.
The server is permitted to accept NPN but then offer zero supported
protocols. This worked, but was not tested or clearly documented.
In the last step, the client *must* pick a protocol, but it is permitted
to pick the empty string. The semantics of this are not explicitly
stated in the draft, but one can infer it means we aren't picking a
protocol. This also worked but was not tested or clearly documented.
Change-Id: I26d7089f4902834ff68a97467fc826e957d5fdf8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69027
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Update-Note: By default, BoringSSL will now disable (D)TLS 1.0 and 1.1,
aligning with RFC 8996 and the behavior of modern browsers. Callers
which leave the minimum version at the default will now change to a
higher minimum version.
Callers which use SSL_CTX_set_proto_version and SSL_set_proto_version
are not affected. If this breaks something and the application needed
the legacy versions after all, call one of those functions with
TLS1_VERSION (or DTLS1_VERSION for DTLS) to restore the old behavior.
Bug: 599
Change-Id: Ic6e2d7a2ac7c190e81bd715b736c20e942f580b5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68487
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
In the rejection tests, make sure the certificate served matches the
public name in the ECH config, so shims which verify the returned
certificate can make sure they are using the right name.
Previously there was a mismatch where the name to be verified was
public.example, but the certificate returned was valid for "test".
Change-Id: I511415431e01c9a83766c633cf11c34f0fa93058
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68727
Auto-Submit: Roland Shoemaker <bracewell@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Useful for third-party bogo users.
Change-Id: I8f456c714ca689bed8b6a44e4adb5f9103b994c7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68747
Commit-Queue: Roland Shoemaker <bracewell@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: Id9801b1cd65c543115b0daddd170c386bf027706
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68667
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
If the test specifies -host-name, provide the correct credential for
that name. This is now uniforml done for the ECH client tests.
Also switch the P256 key for a RSA 2048 key, since the P256 key caused
issues for at least one 1.3 -> 1.2 session resumption test.
Change-Id: Ib1ac5a77f21625205fd2349406e443b9dd248ee4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67967
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Roland Shoemaker <bracewell@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Add certificates to a couple of tests which expect
a specific host name, but were using the default
certificate which had a different host name.
Change-Id: I03e3be21e1e47313e5dbd5b8bba937b6968533c6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67947
Commit-Queue: Roland Shoemaker <bracewell@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Roland Shoemaker <bracewell@google.com>
To a definitely not real one. Otherwise tests fail if you do actually
support DHKEM(P-256, HKDF-SHA256).
Change-Id: Icb60458bba94e8df1954f154f5f6c9965543af45
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67927
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Roland Shoemaker <bracewell@google.com>
Bug: chromium:40925630
Change-Id: Ide72960600747f5ce9a9213a9103510fee3e3806
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67527
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Make google3 common typos stop complaining in the future.
Change-Id: Ib5156335afa691427dbe618c8b29797665cef35f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66947
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.2 PSK ciphers don't use the credentials system and won't have a
credential set. The OCSP and SCT extension callbacks didn't handle this
case correctly.
Since we were already checking for ssl_cipher_uses_certificate_auth in
the OCSP one, which implies there's a credential, I opted to just fix
the order of the conditions, as well as align the SCT one with it.
I thought we had test coverage for this, but runner automatically
configures a certificate even when it doesn't need one, so we never
actually exercised this path. Refine the automatic behavior a bit.
Change-Id: Idf7f06688fc51a2f5d23fd83c23f6da7035e27a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66927
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Pete Bentley <prb@google.com>
This adds a notion of "credentials" to BoringSSL's API, to support
certificate selection by key type (typically ECDSA vs RSA), though the
aim is for it to be generalizable to other certificate types and other
kinds of selection criteria, such as Trust Expressions, or Merkle Tree
Certificates. Since we already had some nascent delegated credentials
I've reworked that feature with SSL_CREDENTIALs as well.
The model is that you create an SSL_CREDENTIAL object containing all the
configuration for what you are authenticating as. An X.509
SSL_CREDENTIAL has a certificate chain, private key, optionally an OCSP
response and SCT list. Delegated credentials are similar. In the future,
we might use this for raw public keys, other certificate types, etc.
Once you set those up, you configure those on the SSL or SSL_CTX in
preference order, and BoringSSL will internally pick the first one that
is usable.
The current implementation ends up redundantly selecting the signature
algorithm a couple of times. This works but is a little goofy. A
follow-up change will remove this redundancy. The protocol between the
runner and shim for tests is also a little weird, but it was the easiest
way I could think of for injecting that. Long-term, I think we should
just replace that protocol with a JSON structure. (See
https://crbug.com/boringssl/704.)
As split handshakes are in the process of being replaced with handshake
hints, this won't work with split handshakes. It works with handshake
hints without any extra work.
Update-Note: The delegated credentials API has been revamped.
Previously, it worked by configuring an optional delegated credential
and key with your normal certificate chain. This has the side effect of
forcing your DC issuer and your fallback certificate to be the same. The
SSL_CREDENTIAL API lifts this restriction.
A delegated credential is now just a different kind of credential. It
may use the same certificate chain as an X.509 credential or be
completely separate. All the SSL_CREDENTIAL APIs take CRYPTO_BUFFERs,
so, if common, the buffers may be shared to reduce memory.
The SSL_delegated_credential_used API is also removed, in favor of the
more general SSL_get0_selected_credential API. Callers can use ex_data
or pointer equality to identify the credential.
Bug: 249
Change-Id: Ibc290df3b7b95f148df12625e41cf55c50566602
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66690
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
In TLS 1.2 and below, the supported_curves list simultaneously contrains
ECDH and ECDSA. Since BoringSSL, previously, did not handle ECDSA
certificate selection in the library, we ignored the latter and left it
to the callers. If configured with an ECDSA certificate that didn't
match the peer's curve list, we proceeded anyway, and left it to the
client to reject the connection.
This contradicts RFC 8422, which says:
The server constructs an appropriate certificate chain and conveys it
to the client in the Certificate message. If the client has used a
Supported Elliptic Curves Extension, the public key in the server's
certificate MUST respect the client's choice of elliptic curves. A
server that cannot satisfy this requirement MUST NOT choose an ECC
cipher suite in its ServerHello message.)
As with the previous client certificate change, once we move certificate
selection into the library, we'll need to evaluate this ourselves. A
natural implementation of it will, as a side effect, cause us to enforce
this match, even when only a single certificate is configured. This CL
lands that behavior change ahead of time and, in case there are
compatibility impats, leaves a flag, SSL_set_check_ecdsa_curve, to
restore the old behavior. If the change goes through fine, we can retire
the flag after a few months.
If this does cause a problem, we can opt to turn it off for the default
certificate, or only enable it when multiple certificates are
configured, but these all result in some slightly suboptimal behavior,
so I think we should treat them as contingency plans.
To help debugging, I gave this a dedicated error, though doing so is a
little tricky because of the PSK fallback. (See the
CheckECDSACurve-PSK-TLS12 test.)
Update-Note: A TLS 1.2 (or below) server, using an ECDSA certificate,
connecting to a client which doesn't advertise its ECDSA curve will now
fail the connection slightly earlier, rather than sending the
certificate and waiting for the client to reject it. The connection
should fail either way, but now it will fail earlier with
SSL_R_WRONG_CURVE. If the client was buggy and did not correctly
advertise its own capabilities, this may cause a connection to fail
despite previously succeeding. We have included a temporary API,
SSL_set_check_ecdsa_curve, to disable this behavior in the event this
has any impact, but please contact the BoringSSL team if you need it,
as it will interfere with improvements down the line.
TLS 1.3 is not impacted by this change, neither are clients, or RSA
certificiates. Additionally, if your server was already looking at the
curve list before configuring an ECDSA certificate in TLS 1.2, this
will also have no impact.
Bug: 249
Change-Id: I2f1d4e2627641319556847cbbbcdddf347bbc8a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66688
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
TLS <= 1.2 servers indicate supported client certificate key types with
a certificate_types field in the CertificateRequest. Historically, we've
just ignored this field, because we've always outsourced certificate
selection to the caller anyway. This meant that, if you configured an
RSA client certificate in response to a server that requested only ECDSA
certificates, we would happily send the certificate and leave it to the
server to decide if it was happy.
Strictly speaking, this was in violation of RFC 5246:
- The end-entity certificate provided by the client MUST contain a
key that is compatible with certificate_types. [...]
Although prior TLS versions didn't say anything useful about this either
way.
Once we move certificate selection into the library, we'll want to start
evaluating supported algorithms ourselves. A natural implementation of
it will, as a side effect, cause us to enforce this match, even when
only a single certificate is configured. Since this is unlikely to have
any real compatibility impact (every TLS server I've seen just hardcodes
this list), let's just try turning it on. On the off chance it does
break someone, I've left a flag, SSL_set_check_client_certificate_type,
for folks to turn this check off. The flag will most likely be
unnecessary, in which case we can retire it after a few months.
If this does cause a problem, we can opt to turn it off for the default
certificate, or only enable it when multiple certificates are
configured, or lean on the sigalgs list (doesn't work for 1.0/1.1), but
these all result in some slightly suboptimal behavior, so I think we
should treat them as contingency plans.
Update-Note: A TLS 1.2 (or below) client, using client certificates,
connecting to a TLS server which doesn't support its certificate type
will now fail the connection slightly earlier, rather than sending the
certificate and waiting for the server to reject it. The connection
should fail either way, but now it will fail earlier with
SSL_R_UNKNOWN_CERTIFICATE_TYPE. If the server was buggy and did not
correctly advertise its own capabilities (very very unlikely), this may
cause a connection to fail despite previously succeeding. We have
included a temporary API, SSL_set_check_client_certificate_type, to
disable this behavior in the unlikely event this has any impact, but
please contact the BoringSSL team if you need it, as it will interfere
with improvements down the line.
This change does not affect servers requesting client certificates, only
clients sending them.
Bug: 249
Change-Id: I159bc444c4ee79fbe5c476d4253b48d58d2538be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66687
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
The implementation itself does check for a match, but we never tested
it. I suspect I omitted it because, in TLS 1.3, that check is actually
redundant. However, in TLS 1.2, it's load-bearing.
Change-Id: I51ed0b2b554ff2090b360db2c60e22ca0afe0c89
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66669
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is in preparation for configuring multiple Credential objects on
the shim in tests. As part of this, I've removed the OCSP response and
SCT list from the default credentials and made the tests that need them
explicitly configure credentials with them.
Bug: 249
Change-Id: I4623a55e184af18fbd9ac16261eaa6c4e41c69c1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66668
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
After https://boringssl-review.googlesource.com/c/boringssl/+/62565 just
renamed it. :-)
This is in preparation for the type being used to test the
SSL_CREDENTIAL machinery. Since we'll be using this to specify a
credential on the shim, let's just make the name match.
Bug: 249
Change-Id: I03e95d120266ccef2111f9bff4c97cef30deb7d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66667
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Trying to handle malloc failures here is a bit tedious. Just suppress
malloc failures because nothing useful can progress when we can't even
allocate ex_data.
Change-Id: Ieaf417bcf9285783a76097319782282ce74e4734
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66648
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
RFC 9345 has this bizarre special case forbiding the rsaEncryption OID
for delegated credentials. This doesn't make much sense as DCs already
constrain to a single signature algorithm. In fact, they didn't need to
use SPKIs at all and could have just encoded the type-specific values.
Nonetheless, this is where the spec went up. We have long rejected the
RSASSA-PSS OID as being unusably complex, so this effectively means we
will never permit RSA delegated credentials.
This was another oversight in
https://boringssl-review.googlesource.com/c/34884. Fix it separately
before everything is reworked to SSL_CREDENTIAL.
Bug: 249
Change-Id: I7eae1e8da9da8052b8d985e78388ef8f2b235942
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66567
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
https://boringssl-review.googlesource.com/c/34884 tried to update to the
newer DC draft, but didn't quite do so. In that update, DCs
overcomplicated the signature algorithm negotiation so that there are
two different signature algorithm lists, used in different contexts.
The existing signature_algorithms extension is used to verify the
signature *on* the DC, made by the end-entity certificate. On the server
side, we should be using that to decide whether to use the DC, and we
weren't.
The new delegated_credentials extension contains another sigalg list.
That is used to verify the signature *by* the DC, in the
CertificateVerify message. (This means DC changes the operative sigalg
list for the CertificateVerify message, which is quite a mess.) On the
server side, the above CL mixed things up. When deciding whether to use
DCs, it checked the correct list. When actually using DCs, it checked
the wrong one. As a result, any time the DC list wasn't a subset of the
main list, the connection would just break!
Fix both of these, in preparation for redoing DCs over the upcoming
SSL_CREDENTIAL mechanism.
Thankfully we don't support one direction of DC usage (authenticating in
C++ and verifying in Go), so there are fewer places to worry about
mixing this up. Given this overcomplication, I'm now much, much less
inclined to ever support DCs as a client, without an rfc9345bis to redo
this.
Bug: 249
Change-Id: Id5257e89a6c8daf1635757be473c45029492d420
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66550
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is passing in a different TLS version, but the TLS version is both
nonsense and doesn't figure into the delegated credential anyway. All
this test is doing is generating a different keypair and mixing them up.
Probably we should move it to ssl_test, as it's not really testing
anything about the protocol, but I've just left it alone and fixed the
test.
Also fix another issue in the test: the getSigner / signMessage chord
should just be a plain signMessage call. There were a few other issues
of that shape, but they'll be fixed in a follow-up change because they
reveal a deeper problem with
https://boringssl-review.googlesource.com/c/34884
Change-Id: I090b41a081f694b4ff8d97f3895645d6a620904d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66549
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Minor cleanup afforded by Go 1.21
Change-Id: I49a0f257f2585e54159014f1f442497a816e6589
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66548
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
While we do want to add this to the shim and real TLS stack, we have no
need to configure this on the runner. This is a remnant of runner being
a production TLS stack. We only ever configure one certificate because
we know what we expect the peer to send.
Change-Id: Idaafc66a2a7f83a929a6a61c19b97cdef158c534
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66647
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This removes the need to ship the PEM files with the built runner.
Instead we can use go:embed to pick up the key files. We do, annoyingly,
need to write the Channel ID file to a temporary, but it's not a huge
deal. When/if we rework all this to JSON, we can avoid this.
Change-Id: Ie0d187a5396546dc157906430639c26b3cc59ca2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66627
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Rather than using the pre-generated certificates, generate them on the
fly. This allows TLS stacks for which certificate validation and
verification are coupled to work as expected. Certificates and keys are
written to temporary files which are then passed to the shim, and
cleaned up on exit. This requires reworking how testCase passes
certs/keys by adding a new field, sendCertificate, rather than manually
setting the -cert-file and -key-file flags. Incidentally the
rsaChainCertificate is removed, since it was essentially unused, and all
tests that used it also work with rsaCertificate. Finally, include a
single SAN ("test") in all certificates, which fixes some TLS stacks
which require this to operate (such as rustls, which currently
regenerates all the certificates currently in the tree to add a SAN).
Additionally, add a new flag, -trust-cert, which tells the the shim
which certificates it should trust. Shims for TLS stacks which
can completely decouple validation and verification of X509 certificates
(like BoringSSL) can ignore this flag, but for stacks where this
functionality is somewhat more intertwined (like Go), this allows the
shim to properly process the sent certificates.
Change-Id: Ic5c63e18fb2b852cc693aacb3b06cfe7993bc90c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62565
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
In TLS 1.2, cipher suite negotiation is tied up with a ton of other
decisions. Even though both sides have an ECDHE_RSA cipher in common, it
may not work because there isn't a common ECDHE curve or sigalg. In that
case, ideally we would consider a non-ECDHE_RSA cipher suite, notably
one of the legacy RSA key exchange ciphers.
If there is no ECDHE curve common, we already did this. However, if
there was no sigalg in common, we would fail the connection rather than
consider RSA key exchange.
Giving this case a lifetime would normally be unimportant as RSA key
exchange is thoroughly deprecated, but the SSL_CREDENTIAL work will need
to consider signature algorithm matches, at which point we'll pick up
behavior like this anyway. So I'm implementing it separately here just
to get the behavior change out of the way.
Update-Note: TLS 1.2 servers will now consider RSA key exchange when the
signature algorithm portion of ECDHE_RSA fails. Previously, the
connection would just fail. This change will not impact any connections
that previously succeeded, only make some previously failing connections
start to succeed. It also changes the error returned in some cases from
NO_COMMON_SIGNATURE_ALGORITHMS to NO_SHARED_CIPHER.
Bug: 249
Change-Id: I4a70036756ea998f38ea155f208e8122bf9a5b44
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66368
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Although the comments say draft-03, we're currently on draft-06.
dcd6e447eb forgot to update all the
comments.
The final RFC is identical to draft-06, except
expected_cert_verify_algorithm was renamed to dc_cert_verify_algorithm,
so this is just changing comment and renaming something.
While I'm here, write the codepoint in decimal instead of hex, to match
the document and how the other IANA codepoints are written out.
Change-Id: I6d1f362a21eecafeef5bba5879f4158e31c8def4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66367
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
OpenSSL added a similar helper function. It's very, very common for us
to malloc something an then zero it. This saves some effort. Also
replace some more malloc + memcpy pairs with memdup.
Change-Id: I1e765c8774a0d15742827c39a1f16df9748ef247
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63345
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Old version Chrome with the existing ALPS codepoint can potentially cause network error due to an arithmetic overflow bug in Chrome ALPS decoder (We already fixed the issues starting from M100 in Chrome).
This CL add a new codepoint for ALPS extension in a way that can be enabled on individual connections., To support multiple versions of Chrome, we need to support both codepoints in BoringSSL.
For details: https://docs.google.com/document/d/16pysbV_ym_qAau_DBYnrw2A4h5ve2212wfcoYASt52U
Change-Id: Iea7822e757d23009648febc8eaff1c91b0f06e18
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61125
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We have a corresponding check on the ServerHello, but not
HelloRetryRequest. See also https://github.com/rustls/rustls/pull/1374,
where rustls forgot to apply the compatibility logic to
HelloRetryRequest.
(From the perspective of a TLS-1.2-expecting observer, HelloRetryRequest
is the ServerHello, so encoding hacks need to apply to both.)
Change-Id: I9b711ea45c54770a76ecfbca8bc992a4eaef6fcd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62906
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Newer glibc have an attribute((nonnull(1))) on fclose. Attributes aren't
part of the language, so decltype(fclose) lose the attribute. It seems
this causes std::unique_ptr<FILE, decltype(fclose)> to trip
-Wignored-attributes in GCC.
This is a bit aggressive of a warning, but work around this with a
custom deleter, which makes the unique_ptr object smaller anyway.
(Though the compiler can, I hope, dissolve all of this anyway.)
Fixed: 642
Change-Id: I9a0206a8c5675f856e80c5266c90be42d66a5606
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62465
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We'd leak the socket if Connect failed in the middle. (This doesn't
especially matter. The test process would just exit anyway.)
Change-Id: I8e1f252781810b1d8ef3c41bd707dfebb0371e60
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61665
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
We've required C++14 for a while now. As we're mostly C with a little
C++, this is less helpful, but may as well avoid bare new where
possible.
Change-Id: Icf3386e3f3b6f2092bb0089ed874cc50985f1a40
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61429
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This was, I assume, needed for cross-version tests, but we're long past
2020 now. While I'm here, switch a unique_ptr(new T) into make_unique as
we can rely on C++14 now.
Change-Id: I3f2fde90f124522c21868c15217f607b7f6d75db
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61428
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
While I'm here, update x/crypto and x/net to their latest versions.
byteReader is a straightforward port, except there doesn't seem to be a
convenient way to read length-prefixed bytes without manually casting
from cryptobyte.String to []byte, so I've done that.
byteBuilder is a bit more involved because it's based on closures, but
still a mechanical change.
As part of this, I switched runner's ticket format to use u24 length
prefixes instead of u32, because cryptobyte.String doesn't have u32
length prefixes. (Although, oddly, cryptobyte.Builder does.)
Fixed: 374
Change-Id: If9bea0b41fe2b8bc48f040a667753b160da469bb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61186
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
The shim is now passed a shim ID which allows us to dispatch.
Previously, we dispatched with a distinct TCP server port per test, but
this seems to exhaust local ports.
As with https://boringssl-review.googlesource.com/c/boringssl/+/60885,
this changes the runner/shim protocol, but not the shim/handshaker
protocol. See that CL for discussion.
Change-Id: Ia22d1bc0bcda1b1869bce4630138538c30f02668
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60888
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
That we pull a value out of a channel and put it back is pretty weird.
Also we don't need a select in accept(). It's enough to just close the
listener when we learn the child is gone. (That will cancel the
Accept call.)
Change-Id: If520d9f405fa0b1ad6e3cd23e9ba8a35ff39ba75
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60887
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's a bit more verbose to set up, but makes the error paths in
Connect() tidier. While I'm here, stick to Windows' actual SOCKET
type until we have to cross into BIO. It doesn't really matter
(Windows cannot use the upper half of that type without badly
breaking backwards compatibility), but it silences some 64/32
truncation warnings.
Change-Id: I7be7c2b543373a7a9fc50711131e5345d84ebb8b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60886
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The runner currently tries to listen on IPv6 and then falls back to IPv4
on error. The shim does the same. If they pick different ones, this
breaks down.
Normally, fallback happens because the system doesn't have IPv6, and
both sides will make the same decision. But if binding to IPv6 fails for
other reasons, they may mismatch. We're observing them fail due to what
seems to port exhaustion. When this happens, shim and runner don't
agree on the same address family.
Instead, just tell the shim which address to connect to. This doesn't
fix the underlying port exhaustion problem, but it does seem to fix the
flakes. Although given we are still exhausting ports and falling back to
IPv4, it doesn't truly fix it. Later CLs will address port exhaustion by
using a single server port.
This changes the runner <-> shim protocol, but hopefully in a fairly
obvious way that others using BoGo can easily follow. It shouldn't break
our cross-version tests because we keep runner and shim at the same
versio there.
To avoid needing to make an incompatible change to the shim <->
handshaker protocol, which would impact our cross-version tests, this
introduces a mechanism for the shim omit flags when talking to the
handshaker. The handshaker doesn't need to know how to connect to the
runner.
Also print the error string on Windows. Sadly this is a bit tedious.
Change-Id: Ic8bda9a854a115c206c05a659a2e34f544b844a6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60885
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This would have made debugging some cross-version test easier.
Change-Id: I7b1bc160b5acf40ec02b9ed5ac2d836e3203cf9a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60805
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Right now we use NIDs to configure the group list, but group IDs (the
TLS codepoints) to return the negotiated group. The NIDs come from
OpenSSL, while the group ID was original our API. OpenSSL has since
added SSL_get_negotiated_group, but we don't implement it.
To add Kyber to QUIC, we'll need to add an API for configuring groups to
QUICHE. Carrying over our inconsistency into QUICHE's public API would
be unfortunate, so let's use this as the time to align things.
We could either align with OpenSSL and say NIDs are now the group
representation at the public API, or we could add a parallel group ID
API. (Or we could make a whole new SSL_NAMED_GROUP object to pattern
after SSL_CIPHER, which isn't wrong, but is even more new APIs.)
Aligning with OpenSSL would be fewer APIs, but NIDs aren't a great
representation. The numbers are ad-hoc and even diverge a bit between
OpenSSL and BoringSSL. The TLS codepoints are better to export out to
callers. Also QUICHE has exported the negotiated group using the
codepoints, so the natural solution would be to use codepoints on input
too.
Thus, this CL adds SSL_CTX_set1_group_ids and SSL_set1_group_ids. It
also rearranges the API docs slightly to put the group ID ones first,
and leaves a little note about the NID representation before introducing
those.
While I'm here, I've added SSL_get_negotiated_group. NGINX seems to use
it when available, so we may as well fill in that unnecessary
compatibility hole.
Bug: chromium:1442377
Change-Id: I47ca8ae52c274133f28da9893aed7fc70f942bf8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60208
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This adds "group" versions of our codepoint-based APIs. This is mostly
because it looks weird to switch terminology randomly in the
implementation. See I7a356793d36419fc668364c912ca7b4f5c6c23a2 for
additional context.
I've not bothered renaming the bssl_shim flags. That seems a waste of
time.
Change-Id: I566ad132f5a33d99efd8cb2bb8b76d9a6038c825
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60207
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We're this awkward mix of "group" and "curve" right now. On the spec
side, this is because they used to be "curves", but then RFC 7919
renamed to "group" in an attempt to generalize FFDH and ECDH. The
negotiated FFDH stuff never really went anywhere (the way it used cipher
suite values in TLS 1.2 made it unusable), but the name change stuck.
In our implementation and API, we originally called it "curve". In
preparation for TLS 1.3, we renamed the internals to "group" to match
the spec in
https://boringssl-review.googlesource.com/c/boringssl/+/7955, but the
public API was still "curve".
Then we exported a few more things in
https://boringssl-review.googlesource.com/c/boringssl/+/8565, but I left
it at "curve" to keep the public API self-consistent.
Then we added OpenSSL's new "group" APIs in
https://boringssl-review.googlesource.com/c/boringssl/+/54306, but
didn't go as far to deprecate the old ones yet.
Now I'd like to add new APIs to clear up the weird mix of TLS codepoints
and NIDs that appear in our APIs. But our naming is a mess, so either
choice of "group" or "curve" for the new API looks weird.
In hindsight, we probably should have left it at "curve". Both terms are
equally useless for the future post-quantum KEMs, but at least "curve"
is more unique of a name than "group". But at this point, I think we're
too far through the "group" rename to really backtrack:
- Chromium says "group" in its internals
- QUICHE says "group" in its internals and public API
- Our internals say "group"
- OpenSSL has switched to "group" and deprecated "curve", so new APIs
will be based on "group"
So align with all this and say "group". This CL handles set1_curves and
set1_curves_list APIs, which already have "group" replacements from
OpenSSL. A follow-up CL will handle our APIs. This is a soft deprecation
because I don't think updating things is particularly worth the churn,
but get the old names out of the way, so new code can have a simpler API
to target.
Also rewrite the documentation for that section accordingly. I don't
think we need to talk about how it's always enabled now. That's a
reference to some very, very old OpenSSL behavior where ECDH negotiation
needed to be separately enabled.
Change-Id: I7a356793d36419fc668364c912ca7b4f5c6c23a2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60206
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
David Benjamin
Nick Harper
Roland Shoemaker
Filippo Valsorda
Bob Beck
Victor Tan