refcount.c is now a single, generic file that calls into C11-atomic-like
APIs. Behind the scenes, this selects one of C11 atomics, Windows
interlocked APIs, or unsynchronized reads/writes (in the no-threads
build).
This frees us up to use atomics elsewhere in the library. For now, this
only binds sequentially consistent atomics, but we can add other memory
orders if needed. In particular, I believe up_ref only needs relaxed
atomics. Some of the later change I think only need acquire and release,
but I'm not positive.
Bug: 570
Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan
Change-Id: Ifcd7357611bb7a8cd14b82c23ad080d1a2df1386
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59848
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Saves some duplicated logic.
Change-Id: I202fa92a88101f9ad735648bc414ab05752641da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59685
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Trying to fix all the places where these formats go quadratic isn't a
good use of time. We've already documented that they're not safe for use
with untrusted inputs. Even without such DoS issues, they cannot be
safely used anyway. (E.g. RUSTSEC-2023-0023.)
Just cap the fuzzer input. It'd be nice if we could avoid this more
systematically in the function, but they're not structured to make this
easy to do, and anyone concerned about DoS in this function has worse
problems.
Bug: chromium:1444420, oss-fuzz:56048, 611
Change-Id: I53eeb346f59278ec2db3aac4a92573b927ed8003
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59785
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Envoy needs to have the possible cipher, etc., strings predeclared to
reduce synchronization needs in the steady state. It currently does this
by (1) iterating over SSL_CTX_get_ciphers at SSL_CTX creation time and
(2) hard-coding a lists of known TLS 1.3 ciphers, TLS versions,
NamedGroups, etc.
(1) would work for some applications, but it breaks any applications
that configure ciphers on the SSL on a certificate callback, etc. If the
callback configures a cipher that wasn't configured on the SSL_CTX (e.g.
if the SSL_CTX were left at defaults), Envoy's logging breaks and we hit
an ENVOY_BUG assertion.
(2) breaks whenever BoringSSL adds a new feature. In principle, we could
update Envoy when updating BoringSSL, but this is an unresasonable
development overhead for just one of many BoringSSL consumers to impose.
Such costs are particularly high when considering needing to coordinate
updates to Envoy and BoringSSL across different repositories.
Add APIs to enumerate the possible strings these functions can return.
These string lists are a superset of those that any one application may
care about (e.g. we may have a deprecated cipher that Envoy no longer
needs, or an experimental cipher that's not yet ready for Envoy's
stability goals), but this is fine provided this is just used to
initialize the table. In particular, they are *not* intended to
enumerate supported features.
Bump BORINGSSL_API_VERSION to aid in patching these into Envoy.
Bug: b:280350955
Change-Id: I4d11db980eebed5620d3657778c09dbec004653c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59667
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This TLS 1.2 algorithm is substantially inferior to AES-GCM and should
never be used. It will not be available unless configured by name.
However, in can be used to provide backwards-compatibility with devices
that cannot be updated if so needed.
Change-Id: I1fd78efeb33aceca76ec2e7cb76b70f761ed1af8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59585
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Adam Langley <agl@google.com>
This generalizes the scheme we previously had with
TLS_RSA_WITH_NULL_SHA, in preparation for TLS_RSA_WITH_3DES_EDE_CBC_SHA
(to be deprecated in a later CL) and
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (to regretably be added back in,
but in a deprecated state).
The story is that deprecated ciphers can always be listed by name, but
by default, selectors will not match them when adding ciphers. They will
still match them when removing ciphers. This is so instructions like
"@STRENGTH" or "!RSA" will still sort or disable the deprecated ciphers,
rather than accidentally leaving them at the front or enabled.
Additionally, a selector can mark itself as including deprecated
ciphers. This is specifically for TLS_RSA_WITH_3DES_EDE_CBC_SHA, because
many callers reference it with just "3DES". As an added quirk,
"RSA+3DES" will also match it. (The rule is that, if any selector matches
deprecated ciphers, we'll allow the overall expression to match it. This
is slightly weird, but keeps "RSA+3DES" working.)
Note, in this CL, 3DES is not actually deprecated. This just sets up the
machinery and doesn't do anything with it. But the blockers for
deprecating that should hopefully be resolved soon.
Bug: 599
Change-Id: I7212bdc879b0e49c6742025644f3100026f24228
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59646
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
We cap e in RSA for DoS reasons. draft-amjad-cfrg-partially-blind-rsa
needs to create RSA keys with very large e. To support this, add an API
which disables this check.
Also add some missing checks for negative n and negative e. (Already
rejected by the parser, just not at this layer.)
Change-Id: Ia996bb1b46fc8b73db704f492b3df72b254a15a4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59645
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Fixed: 605
Change-Id: Id2b7d0221c3e43c102ce77c800f7ab65c74e0582
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59625
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
The initial codepoint is called X25519Kyber786Draft00 in the draft, so
align with that name for this version. Also remove the placeholder bits
for the other combinations, which haven't gotten that far yet.
Update-Note: Update references to NID_X25519Kyber768 to
NID_X25519Kyber768Draft00. For now, the old name is available as an
alias.
Change-Id: I2e531947f41e589cec61607944dca844722f0947
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59605
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This was originally removed in
https://boringssl-review.googlesource.com/12477, but restored in
https://boringssl-review.googlesource.com/c/boringssl/+/13122, which
also restored a comment the assembly code secretly relies on the struct
layout.
Our comment references the MOVBE-based assembly, which could mean either
the stitched AES+GCM code, or the GHASH-only code. I don't see an
obvious place where the GHASH-only code does this. The stitched ones
(both x86_64 and aarch64, counter to the comment) did this, but the
preceding CLs fix this. I think we can now remove this comment.
In OpenSSL, this comment dates to
d8d958323bb116bf9f88137ba46948dcb1691a77 in upstream. That commit also
removed a field, so we can assume no assembly prior to that change
relied on that layout.
That commit immediately preceded
1e86318091817459351a19e72f7d12959e9ec570, which was likely the
motivation. At the time, gcm_gmult_neon had some code with a comment
"point at H in GCM128_CTX". At the time, Htable wasn't even filled in,
and the function relied on H being 16 bytes before Htable.
gcm_ghash_neon also relies on them being reachable from Xi.
This code changed in f8cee9d08181f9e966ef01d3b69ba78b6cb7c8a8 and, at a
glance, the file doesn't seem to be making this assumption anymore. I
think, prior to that change, Htable wasn't filled in for the NEON code
at all.
With all this now resolved, remove the comment and unused copy of H in
GCM128_KEY.
Change-Id: I4eb16cfff5dd41a59a0231a998d5502057336215
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59526
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It is now RFC 8452. The final RFC also has a few more test vectors, so
import those too.
Change-Id: Ib7667802973df7733ba981f16ef6a129cb4f62e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59485
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This adds APIs for constructing RSA keys given all the parameters. This
is much less tedious than the set0 functions, and also avoids issues
caused by deferred initialization.
It doesn't quite finish initializing the keys on construction, as that
is tied up in the rest of this mess. But later work, probably after
Conscrypt is moved to these APIs, will do this.
As part of this, add APIs that explicitly create RSA keys with no e.
There is actually no way to do this right now without reaching into
library internals, because RSA_set0_key refuses to accept an e-less
private key. Handle this by adding a flag.
I also had to add a path for Conscrypt to make an RSA_METHOD-backed key
with n but no e. (They need n to compute RSA-PSS padding.) I think that
all wants to be rethought but, for now, just add an API for it.
This bumps BORINGSSL_API_VERSION so Conscrypt can have an easier time
switching to the new APIs.
Bug: 316
Change-Id: I81498a7d0690886842016c3680ea27d1ee0fa490
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59386
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's unwise for organisations to try and define TLS profiles. As in this
case, they usually make security worse. However, since this is already
established and supported by Android, this change raises it to the level
of a supported policy.
Change-Id: Ic66d5eaa33d884e57fc6d8eb922d86882b621e9e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58626
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Also fixes some copy-paste errors in earlier docs.
Bug: 426
Change-Id: I330445477b6feb50f65a868130387804114f23a8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59205
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We have since added an implementation of a subset of the SSL BIO, but we
don't implement all the features, notably some of the BIO_ctrl values.
Remove them, so it doesn't look like they should work.
Update-Note: I found no code using those symbols (that we build). If
anything was, they most likely were broken. Now they'll fail to build
and the brokenness will be more obvious. (If we find something needs it,
we can always go back and implement them.)
Fixed: 420
Change-Id: Iad03fa65f098023dca555a9b2ac0214ba4264546
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59125
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Prior to https://boringssl-review.googlesource.com/c/boringssl/+/58548,
ASN1_item_sign_ctx returned the length of the signature on success. It's
unclear why anyone would ever want this, but some test was sensitive to
it. (I think it was a typo.)
Restore the old behavior.
Change-Id: Ibf3e45331a339226744d51df703634d02b08a7c4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59307
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Per b/31553927, the last use of this is long gone. This cipher has been
deprecated since it was (re)-added in 2015, so it's long past time to
remove it.
Update-Note: TLS_RSA_WITH_NULL_SHA is no longer available. Nothing
should be enabling it anymore. Callers using
SSL_CTX_set_strict_cipher_list instead of SSL_CTX_set_cipher_list will
notice if they're affected very quickly, because the functino will
fail if this cipher is referenced. As a deprecated cipher suite, this
cipher was already unavailable unless explicitly named, so if your
configuration doesn't say "TLS_RSA_WITH_NULL_SHA" or "NULL-SHA", you
were not using this cipher.
Bug: b:31553927
Change-Id: Id560bb4f9b156be3650d63f4ecf7783fad5ae209
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59145
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This allows us to mark functions as deprecated
declarations with OPENSSL_DEPRECATED.
We also add an OPENSSL_BEGIN_ALLOW_DEPRECATED and an
OPENSSL_END_ALLOW_DEPRECATED for testing use to
test deprecated functions.
The purpose of this is to allow us to mark things
people should not be using as deprecated, and force some
inconvenience on the user of such things to notice them
(as opposed to a only a warning to not use it that they
may not see or read without something tripping them up.)
The intent is to still allow use, with some effort,
before removing the function, or moving it to
libdecrepit.
We initially mark X509V3_EXT_add and X509V3_EXT_add_alias
as deprecated.
Update-Note: We are starting to mark some functions in
boringssl as deprecated declarations which will cause the
compiler to emit warnings if they are used. The intention
is both to prevent accidental use in new code, and to to call
attention to call sites in existing code so that the documentation
for the deprecated function can be revisted and appropriate action
taken.
Bug: 584
Change-Id: Ia9ff386f0d22588e8a5999eda1a48b8c28dca2de
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58405
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
These appear to be unused. Some ones of note:
- XN_FLAG_FN_ALIGN breaks with multi-attribute RDNs anyway
- XN_FLAG_FN_NONE is completely pointless
Update-Note: Some seemingly unused XN_FLAG_* values were removed. If
some project fails to build, we can put them back but one shouldn't be
using this function in the first place.
Change-Id: I4d8472e1e31aeec623b4d4e2aea48da7b1ef6798
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58930
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These probably don't need their own section. They're just thin wrappers
over other ASN1_TIME functions.
Bug: 426
Change-Id: I8672feb0ca7ba1cf69b56d02d2559de5b80a3ee7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58928
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
They're not used anywhere, as X509_REQ doesn't expose the underlying
STACK_OF(X509_ATTRIBUTE) anyway. They're also very thin wrappers over
the stack functions, so just delete them and inline them into X509_REQ
functions.
While I'm here, I've tidied up the add1_attr_by_* functions to reduce an
unnecessary copy.
Change-Id: Iec002c83ab7ad7267314e98866d680d12a82e971
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58927
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Also organize the low-level signature verification functions. I missed
those in the first pass.
Bug: 426
Change-Id: I9c93d643d8f0f77a35ee132f31377ba447f2f2f1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58926
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
I had a branch lying around to rewrite X509_NAME_print(_ex) because
those functions are a disaster, but it needs more work and probably
isn't high priority. In the meantime, document what we've got.
Also tidy up X509_print_ex slightly. m was completely unused and
some variable declarations could be moved closer to their definition.
Bug: 426
Change-Id: I24295048c36268c745f579ad66f34736cfe6830f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58925
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
HRSS itself remains in libcrypto because there are some direct users of
it. But this will let it be dropped by the linker in many cases.
Change-Id: I870eda30c9ed1d08693c770e9e7df45a2711b7df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58645
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
These are already unused, though add and add_alias will need more work.
In doing so, simplify the X509V3_EXT_DYNAMIC business. I added some
cleanup calls to https://boringssl-review.googlesource.com/2208, but
that should have been in the error-handling path of
X509V3_EXT_add_alias, the only case that cares about this.
Update-Note: Removed unused API.
Bug: 590
Change-Id: Idd97366d90d7aab0ca2e020c76a7c8065b3dd7ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58765
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
I don't believe these codepaths have ever been run. All the built-in
X509V3_EXT_METHODs are ASN1_ITEM-based, as are all callers I found of
X509V3_EXT_add and X509V3_EXT_add_list.
Also document not to use those APIs because they're pointless and (for
now) not even thread-safe. Making them thread-safe is doable, but it'd
add rwlock contention in certificate verification, unless we first
rework certificate verification to ignore custom registrations, because
it never uses them anyway. But that only proves that this whole feature
was pointless, so this time may be better spent trying to get rid of
this API.
Update-Note: Externally-installed X509V3_EXT_METHODs now must be
ASN1_ITEM-based. I believe all existing ones already are. If there are
any that aren't, let us know. We'll either revert this for now, or
export a way to implement custom ASN1_ITEMs, or, most likely, try to
move the caller off custom X509V3_EXT_METHODs altogether. Like most of
OpenSSL's other global registration APIs, this one is unsafe (two
callers may conflict) and there isn't much reason to register it with
the library because OpenSSL doesn't do much with the registration
anyway.
Bug: 590
Change-Id: Ice0e246d50069e10e6cca8949f60fac474d0846c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58687
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I6e53434246f3fef06d4f88924bfe1cbfad0543e8
Bug: chromium:1414562
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58205
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Also unexport PEM_proc_type and PEM_dek_info. They're never called
externally, just private functions within one file. Also, while I'm
here, fix the include guard on asn1/internal.h.
Bug: 516
Change-Id: I6961a65f638e7b464a8c349663898a954d7826b4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58548
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This was replaced with the upstream-compatible SSL_CIPHER_standard_name
in https://boringssl-review.googlesource.com/17324. It looks like we've
since migrated everything off the old name, so let's just remove it.
Update-Note: SSL_CIPHER_get_rfc_name calls can be replaced with
SSL_CIPHER_standard_name, which is also more efficient as it avoids an
allocation and copy. If anyone's using this function and can't easily
migrate, let us know and we can put this back for a little longer.
Change-Id: I6bce40a8a146671429641a5dbff6f614006a9a1c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58665
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
We support BIO_gets on three BIOs. They're all slightly different. File
BIOs have the NUL truncation bug. fd BIOs swallow the embedded newline.
This CL fixes the second issue and updates the BIO_gets test to cover
all three.
See also upstream's https://github.com/openssl/openssl/pull/3442
Update-Note: BIO_gets on an fd BIO now returns the newline, to align
with memory and file BIOs. BIO_gets is primarily used in the PEM parser,
which tries to tolerate both cases, but this reduces the risk of some
weird bug that only appears in fd BIOs.
Change-Id: Ia8ffb8c67b6981d6ef7144a1522f8605dc01d525
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58552
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This updated value is not in the private use area and can hopefully be
more stable over time.
Change-Id: Ieda34ec0f356cfd03365973f611a6edc23431e29
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58525
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Passes test vectors, and should be constant time, but is currently
not optimized and neither the API nor the standard is stable.
Change-Id: I89b90877e023a43ee7238e11b86065444ab3bdec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57845
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
If we see a critical policy constraints extension, we have two options:
We can either process it, which requires running policy validation, or
reject the certificate. We and OpenSSL do neither by default, which
means we may accept certificate chains that we weren't supposed to.
This fixes it by enabling X.509 policy validation unconditionally and
makes X509_V_FLAG_POLICY_CHECK moot. As a side effect, callers no longer
need to do anything for CVE-2023-0466.
This is the opposite of [0]'s advice, which instead recommends skipping
the feature and rejecting critical policy contraints. That would be a
good move for a new X.509 implementation. Policy validation is
badly-designed, even by X.509's standards. But we have OpenSSL's history
of previously accepting critical policy constraints (even though it
didn't check it). I also found at least one caller that tests a chain
with policy constraints, albeit a non-critical one.
We now have an efficient policy validation implementation, so just
enable it.
Of course, fixing this bug in either direction has compatibility risks:
either we take on the compat risk of being newly incompatible with
policyConstraints-using PKIs, or we take on the compat risk of newly
rejecting certificates that were invalid due to a policy validation
error, but no one noticed. The latter case seems safer because the chain
is unambiguously invalid.
Update-Note: X.509 certificate verification (not parsing) will now
notice policy-validation-related errors in the certificate chain. These
include syntax errors in policy-related extensions, and chains with a
requireExplicitPolicy policy constraint that are valid for no
certificate policies. Such chains are unambiguously invalid. We just did
not check it before by default. This is an obscure corner of X.509 and
not expected to come up in most PKIs.
[0] https://www.ietf.org/archive/id/draft-davidben-x509-policy-graph-01.html#section-3.4.4
Fixed: 557
Change-Id: Icc00c7797bb95fd3b14570eb068543fd83cda7b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58426
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This was not thread-safe and, until the previous CL, egregiously so. No
one uses this API, so remove it.
Update-Note: Various unused functions for registering named
X509_VERIFY_PARAMs were removed. These functions only exist to make
X509_VERIFY_PARAM_lookup return a custom value. Instead, applications
that want a particular X509_VERIFY_PARAM can just configure it directly,
rather than stashing it in library-global state and then looking it back
up with X509_VERIFY_PARAM_lookup.
Change-Id: I8d532a1a137c7abbc131f2cb5d12ba94e5728e2d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58386
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This is a double-pointer and both layers should be const. This matches
OpenSSL 1.1.1, so in addition to being more const-correct, we're more
OpenSSL-compatible.
Update-Note: Anything that defines a comparison function would need to
fix the type signature. I found only one external caller, Envoy, that
defines it. https://github.com/envoyproxy/envoy/pull/25051 fixes it.
(That we hadn't run into the upstream incompatibility suggests this is
just not a feature folks use outside the library much.)
Bumping BORINGSSL_API_VERSION, in case I missed any, and there's some
caller where we can't just use C++14 generic lambdas to smooth it over.
Fixed: 498
Change-Id: I8f07ff42215172aa65ad8819acf69b63d6d8e54c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56190
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This function is probably not what you want. I think the
X509V3_EXT_METHOD and ASN1_STRING_TABLE_add stuff are the only cases
where you actually need an OID. Those, themselves, have problems with
conflicting registrations and aren't really necessary.
ASN1_STRING_TABLE_add has a similar warning on it. X509V3_EXT_METHOD is
not yet documented, but should get a similar warning when we document
it.
Change-Id: I173918a3f7db25059ed078b2f67b23acfb43033d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58365
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
alg was a copy-paste error.
Change-Id: I4c591bb98f7da853c79679727d49a00c7c65ad91
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58305
Reviewed-by: Alex Gaynor <alex.gaynor@gmail.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
I mistakenly thought no one needed X509 as an ASN1_ITEM, but that wasn't
true. wpa_supplicant relies on this. Restore this and add a test for it.
As with the rest of the rewrite, this is currently a little tedious. I'm
hoping that, as the internals are rewritten with CBS and CBB, we can
establish some cleaner patterns and abstractions.
Bug: 547
Change-Id: I761ee058f8ec916b2ec7f4730a764d46d72f1f10
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58285
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Now that the preceding CL has isolated the X.509 signature hack, we can
apply the strictness across the legacy parser. This is particularly
important for the TBSCertificate parser, where it is ambiguous which
value one checks the signature over. (Officially, you're supposed to
re-encode as DER. In practice, people don't do this.)
This change means many of our primitive types are actually parsed as
DER. I've removed the bug references in the comment in the documentation
where I believe they're finally correct.
Update-Note: Non-minimal lengths in certificates are no longer accepted,
as required for standards compliance. The one exception is the signature
field, where we still carry an exception. Some of this was already
enforced by libssl's parser.
Bug: 354
Change-Id: I57cfa7df9e1ec5707390e9b32fe1ec6b5d8172f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58186
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is a cursory conversion and is, currently, very tedious because it
needs to bridge calling conventions. After tasn_*.c and all the
underlying primitives have CBS/CBB-based calling conventions, this
should be a lot cleaner.
This is to break a dependency cycle:
- We'd like to rewrite d2i_X509 with CBS
- To do that, we need to rewrite its underlying types with CBS
- Those parsers are tied up in tasn_dec.c, so we effectively need to
rewrite tasn_dec.c with CBS.
- CBS is designed for DER, not BER, so such a change would most
naturally switch the TLV parser to require DER.
- We've *almost* done that already except
https://boringssl-review.googlesource.com/c/boringssl/+/51626 had to
stop at non-minimal definite lengths, which are allowed in BER but
forbidden in DER. See b/18228011 for a bunch of certificates which
have a non-minimal definite length at *just* the signature field.
- So, to do that, we'd ideally special case just that field, or BIT
STRINGs in general, to tolerate minimal lengths. That's easiest done
when d2i_X509 is CBS, so we can just do what we want in imperative
code. And thus we're back full circle.
So, detach X509 from the templates now. It's a bit tedious because we
need to switch calling conventions for now, but it breaks the cycle.
Later, we can revisit this and get all the benefits of a fully CBS-based
path.
For now, I haven't added an ASN1_ITEM. If it comes up, we can make an
EXTERN ASN1_ITEM.
Update-Note: The ASN1_ITEM removal means custom ASN.1 templates (which
are discouraged in favor of our much simpler CBS and CBB types) using
X509 will fail to compile. We don't believe anyone is relying on this,
but this can be restored if we find something.
Update-Note: Certificate parsing is slightly stricter: the outermost
TLVs, except for the signature field, no longer accept non-minimal
lengths, as mandated by DER. This strictness was broadly already applied
by the libssl parser.
Bug: 547
Change-Id: Ie5ad8ba4bb39f54fdd3dd45c53965b72a3850709
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58185
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL's ASN1_STRING representation has many cases. There's a grab-bag
V_ASN1_OTHER cases that can represent any element. But it is currently
only used for non-universal tags. Unknown universal tags go into the
type field directly.
This has a few problems:
- Certain high values, V_ASN1_NEG_INTEGER and V_ASN1_NEG_ENUMERATED,
are treated special. This was one of the two causes behind
CVE-2016-2108 and had to be worked around with V_ASN1_MAX_UNIVERSAL.
- OpenSSL can never compatibly support a new universal type in a
non-ASN1_STRING form. Otherwise ASN1_TYPE's union changes its
in-memory representation.
- It is a bit ambiguous when OpenSSL does or doesn't know the type.
- This is broadly implemented by having a default in all the
switch/cases, which is a little awkward.
- It's yet another "unknown tag" case when V_ASN1_OTHER covers such
cases just fine.
Remove this representation and use V_ASN1_OTHER. This more unambiguously
resolves CVE-2016-2108. ASN1_STRING's and ASN1_TYPE's respective type
fields are now a closed set. Update the documenthation accordingly.
Formally allowing universal types in ASN1_STRING also opens the door to
clearing the ASN1_PRINTABLE mess (https://crbug.com/boringssl/412).
BoringSSL currently rejects X.509 names that are actually valid, because
the OpenSSL X509_NAME representation cannot represent them. This allows
us to introduce an ASN1_STRING-based ANY representation, which just
represents all non-ASN1_STRING types in an V_ASN1_OTHER.
The implementation is a little clumsy (the way things tasn_dec.c is
written, I had to introduce yet another check), but I'm hoping that,
when the parser is rewritten with CBS, this can be integrated into a
single type dispatch.
Update-Note: This does not change the set of inputs accepted or rejected
by the ASN.1 parser. It does, however, change the in-memory
representation in edge cases. Unless the application was specifically
inspecting the in-memory representation for these unknown types, we
expect this to have no impact.
Fixed: 561
Change-Id: Ibf9550e285ce50b11c7609d28b139354b9dd41dc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58148
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Removing object reuse makes it dramatically simpler. Along the way, lift
the OID validity checker into crypto/bytestring, so we can use it more
generally. (Although the difference between invalid OID and unknown OID
is pretty academic, so this check isn't that important.)
For now I've preserved the existing behavior, where the OID validity
checker accepts arbitrarily large OID components. Though this results in
an oddity where the OID to string functions reject inputs that the
parser accepts. (There we only allow up to 2^64-1.)
Update-Note: When we removed object-reuse from all the d2i functions, we
missed one d2i_ASN1_OBJECT. See
https://boringssl-review.googlesource.com/c/boringssl/+/56647.
Otherwise, this CL is not expected to change behavior.
Change-Id: If4d2d83d9f3c96abfdc268e156f2cf3a9a903b0c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58147
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
David Benjamin
Bob Beck
Adam Langley
Bob Beck
Adam Langley
Adam Langley
Steven Valdez
Sophie Schmieg