Historically, sha.h included both SHA-1 and SHA-2 functions. But SHA-1
functions mostly shouldn't be used now, and it's useful to be able to
audit at the level of header names in some contexts.
Therefore move SHA-2 things into a new sha2.h. In order not to break
everything, sha.h now includes sha2.h so no changes are needed in
existing callers.
Change-Id: I68d5e991f58a1c74ca377ba017caaff356acc870
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/80327
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This removes the goofy sizeof/alignof assertion we've been making. That
was a strict aliasing violation. Now CRYPTO_refcount_t is just
CRYPTO_atomic_u32 without any fuss. It also means nothing should include
<openssl/thread.h> because it contains only deprecated symbols.
Bug: 412269080
Change-Id: Icbf98a31d5af2a4dadab3b20a410c10f98061ed7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/79569
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
The one remaining external reference was in an unused parameter where it
was impossible to obtain a non-null value. Remove the last of that
machinery from the public API. This leaves us free to completely rework
LHASH_OF(T) internally, including making it a template that understands
whether it owns its values.
(If we end up needing to revert it, we can still make the real
LHASH_OF(T) into a template. It just won't be called LHASH_OF(T)
anymore.)
Update-Note: Calling code which references LHASH_OF(T) will no longer
compile. We have had no public APIs that allow a caller to usefully
construct an LHASH_OF(T) for some time, so this only ever came up in odd
cases around bindings APIs.
Change-Id: I5a44310b04d8f8f3599f0f356b64786e62db5fbf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/78787
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The distinction between B_ASN1_UNKNOWN and zero never made much sense.
Now that X509_NAME parsing is fixed, this can be removed. In doing so,
implement this function more straightforwardly.
Update-Note: B_ASN1_UNKNOWN is removed. Code search says nothing
external was using it, which makes sense because there was nothing
useful callers could do with it.
Fixed: 42290275
Change-Id: I445bea1b11dfa70abf279530a7338305f28aeb98
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/78330
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is part of a broader issue with OpenSSL's goofy ASN1_PRINTABLE type
(which seems to have no connection to any standard), but this is a
minimal fix intended for easy cherry-picking.
This now leaves ASN1_PRINTABLE unused, but for now I've forked it into
an ASN1_ANY_AS_STRING type. It is still very far from an actual ANY, but
the intent is to capture AttributeValue's status as an ANY type, but
which OpenSSL's API forces us to represent as an ASN1_STRING. (Later
changes will make it actually match the name.)
Update-Note: X.509 name attributes now may be OCTET STRINGs.
Bug: 42290275
Change-Id: I1389533595a972bd0b4aa3c1840dc0f15c0fa645
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/78327
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This reflects some assumptions we have on our headers:
- <openssl/foo.h> pulls in <openssl/base.h>. In particular, the
canonical FOO typedefs for foo_st all live forward declared in
<opessl/base.h>, but including <openssl/foo.h> is sufficient to use
FOO.
- <openssl/base.h> pulls in <stdint.h> and <stddef.h> so we don't have
to keep including it.
Add IWYU exports to reflect this so that clang's include cleaner gets
less upset. It's a bit of a blunt instrument because it also means that
<openssl/foo.h> lets you use the forward-declared BAR typedef, and
downstream projects might still prefer to explicit include <stdint.h>
when they use it (we use it so much that it would be too much), but I
think this is fine.
NB: By adding these, we're essentially promising we won't include those
transitive includes because downstream code might accidentally start
relying on it.
Change-Id: I705fe6d1026fbd302ed0f070a0cf7658a70af8ef
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/77687
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We use the standard Apache 2.0 file header, described in "APPENDIX: How
to apply the Apache License to your work."
This was primarily automated by running:
git ls-tree -r --name-only HEAD | xargs go run ./util/relicense.go
See go/boringssl-relicensing-triage for the results of triaging the
output of the tool.
As part of this, switch from taking fiat-crypto under MIT license to
Apache 2.0. (It is licensed under MIT OR Apache-2.0 OR BSD-1-Clause.)
The copyright_summary tool can also be used to confirm we didn't
accidentally drop any copyright lines:
# Run before the CL
git grep -l Copyright | xargs go run ./util/copyright_summary.go -out /tmp/old.json
# Run after the CL
git grep -l Copyright | xargs go run ./util/copyright_summary.go -compare /tmp/old.json
Bug: 364634028
Change-Id: I17c50e761e9d077a1f92e25969e50ed35e320c59
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/75852
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This applies the OpenSSL "copyright consolidation" process from the
following upstream changes:
* e0a651945c
* 3fb2cf1ad1
* ac3d0e1377
* c2f312f5c2
* 596d6b7e1c
* e18cf66aaf
* 846e33c729
* 440e5d805f
* 21dcbebc6e
* 6286757141
* 4f22f40507
* d2e9e32018
* 2039c421b0
* b1322259d9
* aa6bb1352b
* b6cff313cb
* 9e20068958
* 6aa36e8e5a
* 44c8a5e2b9
This was mostly automated, but partially manual. The automated portion
can be reproduced by checking OpenSSL to commit
44c8a5e2b9af8909844cc002c53049311634b314, and running the following:
git grep -l -E 'Copyright remains Eric Young|Copyright.*The OpenSSL Project\.|Written by.*for the OpenSSL Project' crypto/ decrepit/ include/ ssl/ | grep -v objects.go > files.txt
cat files.txt | xargs -n1 perl -i ./util/copyright.pl
From there, some years were fixed up manually according to
go/openssl-copyright-consolidation-comparison (internal-only).
Three files required additional manual fixing:
- crypto/ecdh_extra/ecdh_extra.cc
- crypto/fipsmodule/ecdh/ecdh.cc.inc
- include/openssl/ecdh.h
These files have an OpenSSL header, but *after* a different header, so
the script does not correctly detect the now redundant OpenSSL header.
They were manually modified to remove it. This matches what seems to
have been done to crypto/ec/ecdh_ossl.c in OpenSSL's
4f22f40507fea3f272637eb8e00cadf1f34b10d9.
Bug: 364634028
Change-Id: I79a559a409ebe2476f2cb8a48a488ac5dd77c90a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74710
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
gRPC is no longer using these, so remove them. They were impossible to
use correctly and are the cause of weird statefulness around
ctx->error_depth.
Once this CL sticks, we can follow up and clean up this a code a bit.
Update-Note: Some unused (and unusable) callbacks were removed.
Bug: 674
Change-Id: I8109dd6555d2ca056447c1b4f0aa28abe7af81b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68387
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This API allocates internally and can leave a corrupted |alg| behind.
Change it to return an int so that callers can check for an error.
Also fix its only caller in rsa_md_to_algor().
This is an ABI change but will not break any callers.
Also add a small regress test for this API.
Change-Id: I7a5d1729dcd4c7726c3d4ead3740d478231f3611
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67187
Commit-Queue: Theo Buehler <theorbuehler@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
tb noticed that our X509_ALGOR_set_md differs from OpenSSL because we
never set EVP_MD_FLAG_DIGALGID_ABSENT. That is, we include an explicit
NULL parameter, while OpenSSL omits it.
RFC 4055, section 2.1 says:
There are two possible encodings for the AlgorithmIdentifier
parameters field associated with these object identifiers. The two
alternatives arise from the loss of the OPTIONAL associated with the
algorithm identifier parameters when the 1988 syntax for
AlgorithmIdentifier was translated into the 1997 syntax. Later the
OPTIONAL was recovered via a defect report, but by then many people
thought that algorithm parameters were mandatory. Because of this
history some implementations encode parameters as a NULL element
while others omit them entirely. The correct encoding is to omit the
parameters field; however, when RSASSA-PSS and RSAES-OAEP were
defined, it was done using the NULL parameters rather than absent
parameters.
...
To be clear, the following algorithm identifiers are used when a NULL
parameter MUST be present:
...
My read of this text is:
1. The correct encoding of, say, SHA-256 as an AlgorithmIdentifer *was*
to omit the parameter. So if you're using it in, I dunno, CMS, you
should omit it.
2. Due to a mishap, RSASSA-PSS originally said otherwise and included
it. Additionally, there are some implementations that only work if
you include it.
3. Once the mistake was discovered, PSS chose to preserve the mistake,
rather than undo it.
This means that the correct encoding of SHA-256 as an AlgorithmIdentifer
is *different* depending on whether you're doing PSS or CMS.
Fortunately, there are only two users of this function, one inside the
library and one in Android. Both are trying to encode PSS, so the
current behavior is correct. Nonetheless, we should document this.
Also, because this is a huge mess, we should also add an API for
specifically encoding RSA-PSS. From there, we can update Android to call
that function and remove X509_ALGOR_set_md.
Amusingly, RSASSA-PKCS1-v1_5 *also* differs from the "correct" encoding.
RFC 8017, Appendix B.1 says:
The parameters field associated with id-sha1, id-sha224, id-sha256,
id-sha384, id-sha512, id-sha512/224, and id-sha512/256 should
generally be omitted, but if present, it shall have a value of type
NULL.
This is to align with the definitions originally promulgated by NIST.
For the SHA algorithms, implementations MUST accept
AlgorithmIdentifier values both without parameters and with NULL
parameters.
Exception: When formatting the DigestInfoValue in EMSA-PKCS1-v1_5
(see Section 9.2), the parameters field associated with id-sha1,
id-sha224, id-sha256, id-sha384, id-sha512, id-sha512/224, and
id-sha512/256 shall have a value of type NULL. This is to maintain
compatibility with existing implementations and with the numeric
information values already published for EMSA-PKCS1-v1_5, which are
also reflected in IEEE 1363a [IEEE1363A].
Finally, there's EVP_marshal_digest_algorithm, used in PKCS#8 and OCSP.
I suspect we're doing that one wrong. I've left a TODO there to dig into
that one.
Bug: 710
Change-Id: I46b11f8c56442a9badd186c7f04bb366147ed98f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67088
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
It took 3.5 years, but this header is now DONE! I opted to add a section
for each extension just because there were so many functions. It's a
little weird because, for example, we don't have a section for key usage
because it's just BIT STRING. But I think this is better than having a
great big "types for built-in extensions" section.
Fixed: 426
Change-Id: Ifc7684cc6ff6a211ea1f5065eff67663adf004b3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66392
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Many of the extension types are not the extensions themselves, but the
interious types used in various subfields. In preparation for when we
rewrite these parsers with <openssl/bytestring.h>, having fewer of these
means fewer compatibility functions to bridge the calling conventions.
We do still need new/free functions, so that callers can construct
extensions themselves. While I'm here, go ahead and expand the macros
and document.
(Top-level extension types need ASN1_ITEMs for X509V3_METHOD, and
d2i/i2d functions for callers that wish to parse and serialize. But
there's no real need to do this for the individual fields.)
Update-Note: Some interior ASN.1 types no longer have d2i and i2d
functions or ASN1_ITEMs. I checked code search and no one was using any
of these. We can restore them as needed.
Bug: 547
Change-Id: I0b2840bf4aea2212a757ce39b4918c8742043725
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66389
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This includes the somewhat odd "defaults" API, which I've currently left
kind of handwavy. We should eventually decide what to do with this, be
it remove it, decide /etc/ssl is a fine default, or do something else
entirely. But I'll leave that to future us.
(If nothing else, we really should make it return an error on Windows
and macOS. It's really just Linux where /etc/ssl is a plausible platform
API.)
Bug: 426
Change-Id: Iacd2bb903f452ffe236a7a0b97e3072b5dcd8516
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66388
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
While I'm here, unexport STACK_OF(X509V3_EXT_METHOD). We use it
internally, but it never appears in any public APIs, and there's no real
reason for any caller to use it.
Bug: 426
Change-Id: I6057834847a37f435d1b687701a3e65b5afb2890
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66387
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Writing these tests revealed that actually this has been broken on
Windows this whole time!
First, the APIs to configure a directory actually configure a
colon-separated list of directories. But file paths contain colons on
Windows, so the actual separator on Windows is semicolon. But that got
lost in the fork at some point. This CL fixes that.
Second, X509_FILETYPE_ASN1 is broken because of a text vs binary mode
mixup. The following CL will fix this.
Some of the behaviors tested here around CRLs are not quite reasonable.
See https://crbug.com/boringssl/690 for details. For now, I've tried to
capture the existing behavior. As BY_DIR actually maintains some shared
mutable state, I've also added TSAn tests.
Another subtlety is that multiple CAs with the same name work, but the
reason they work is pretty messy because OpenSSL's internal interfaces
are incompatible with it. Instead, OpenSSL works around itself with the
X509_STORE cache. These tests do not cover this case, but a subsequent
CL will add tests for it.
Change-Id: Ifd8f2faea164edb0eda771350cd9bf6dc94104e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66008
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
There are still a pile of functions left to document, but we're far
enough now that the doc generation is happy to run on this header. Go
ahead and start generating output.
Bug: 426
Change-Id: I4c807d625df3a4a881936e99b5a3fc6559cda6c9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65793
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It's less bad than I originally wrote because trust properties only
matter if configured on the X509_STORE. Add a test for this.
This is good because lots of functions trigger d2i_X509_AUX, so I think
we have to assume attackers can specify these values. Nonetheless, this
is surprising, so document which functions trigger this.
Change-Id: I73ce44acfa2a373ef3f3ef09c3e46cea98124f33
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65791
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These were mostly already documented, but fit the current style. Add a
couple tests for some interesting cases.
With this, all we have left to document are:
- Built-in and custom extensions
- Filesystem-based X509_STORE bits
- The APIs to query X509_STORE (mildly annoying because the
sort-of-a-cache-sort-of-not thing is exposed)
Bug: 426
Change-Id: I68c16071b8781f560e6601fd65a7fba9b6efe862
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65790
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
OpenSSL's API uses this weird "index" intermediate integer
representation, which is the same as the ID but offset bit. Just use the
IDs throughout. Also document and deprecate the string-based APIs that
rust-openssl uses.
As a bonus, we remove some int/size_t casts.
Change-Id: I3ffd2ab59bf3c9d96014a028b667b0bd3288b16b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65789
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This is only used internally, for X509_PURPOSE_ANY to mark that it has
no corresponding trust value. Countrary to the name, this doesn't mean
to use the default X509_TRUST behavior, but to make it impossible to
configure via X509_STORE_CTX_set_purpose.
Since it's only used in one place, as any value that fails lookup, I've
just put a local define in v3_purp.c.
Change-Id: Id3e44c08528a303132ef09d0a94521af67cc2230
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65212
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This will be needed for https://github.com/python/cpython/pull/114573.
Along the way, document the various functions that expose "query from
X509_STORE". Most of them unfortunately leak the weird caching thing
that hash_dir does, as well as OpenSSL's generally poor handling of
issuers with the same name and CRL lookup, but I don't think it's really
worth trying to unexport these APIs.
Change-Id: I18137bdc4cbaa4bd20ff55116a18f350df386e4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65787
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This is not sufficient to get the correct type on these constants,
because bindgen is broken and does not correctly bind constants. But in
preparation for that bug being fixed, we should at least mark the
headers correctly.
Bug: 636
Change-Id: Ib42a2d4f42a84b385dd7bd286564ccfe457923eb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66227
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is an old predecessor to the extended key usage extension, with no
tests. Other implementations, such as Chromium's certificate verifier or
Go's already do not process it.
This doesn't remove the parser for the extension, or the config-based
machinery. Folks who want to manually process the extension or construct
it can continue to do so.
Update-Note: Certificates with a critical Netscape cert type extension
will now be rejected by the certificate verifier, matching the behavior
of the Chromium verifier. Non-critical extensions will continue to work
fine. They will instead be ignored.
Change-Id: I5889fb48f00d8dc081fe784d5f1d73d1b884ca5c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65209
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
X509_STORE_CTX_purpose_inherit's behavior is even more bizarre than
X509_STORE_CTX_set_purpose and X509_STORE_CTX_set_trust. Remove it and
reimplement X509_STORE_CTX_set_purpose and X509_STORE_CTX_set_trust's
behaviors directly.
Change-Id: Icc6a4a84ee8fa38e2fe70a4cfa06e74dee186d29
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65208
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The trust and purpose is all a bit tied up together, as is the meaning
of the certificates in an X509_STORE at all. (It's hard to discuss
whether a "trusted certificate" is actually a trust anchor without a
description of trust settings to reference.)
Cut the Gordian Knot by documenting all that first. Later CLs will move
other symbols into the sections established here. Also as the behavior
is a little complex, add some tests to cover some of this machinery.
Bug: 426
Change-Id: Idde8bc4e588de92ebabf6ecf640b62a2a6803688
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65207
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
strongswan defines conflicting symbols and has been relying on them only
being defined in <openssl/x509v3.h>. Defining the constants in
<openssl/x509.h> would break strongswan, so move them back for now.
Long term, we would like for new code to only need <openssl/x509.h>, so
I've left a TODO to introduce properly namespaced versions of these
constants and, separately, see if we can fix strongswan to similarly
avoid the conflict. Between OpenSSL, strongswan, and wincrypt.h all
defining these constants, it seems best for everyone to just avoid them
going forward.
Change-Id: I23ce4c5013a80a831e0dc74fda8623027017190c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65387
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These are unused and are the only options that remove the "compat"
self-signed fallback.
X509_TRUST_OCSP_REQUEST was intended for checking signed OCSP requests.
While OpenSSL's OCSP implementation (which we've dropped) does attempt
to configure it, it actually does nothing. They call
X509_STORE_CTX_set_trust after X509_STORE_CTX_set_purpose, but
X509_STORE_CTX_set_purpose already sets the trust parameter and
X509_STORE_CTX_set_trust only acts when trust is not configured.
X509_TRUST_OCSP_SIGN was briefly used in upstream's
30c278aa6bb614f4cfc5a26c7cbe66ad090f6896, by way of
X509_PURPOSE_OCSP_HELPER, but then immediately undone in
e9754726d236b74476cd0be5fa60acfef0c7024f.
Change-Id: I6d2cf9b88a6b013e74fe95cd88f94051111086df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65151
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These tables are small enough that a linear scan is fine. This is one
less thing we need to keep in sync, and means we can remove entries
without renumbering them.
Change-Id: If1a41397aac3917534529e7e704983489e266a0f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65150
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
In particular, deprecate get_crl and check_crl. Only gRPC uses them, and
does so incorrectly. It is impossible to implement those callbacks
correctly.
Also remove X509_STORE_CTX_set_cert. No one uses it, and it's redundant
with X509_STORE_CTX_init. We should remove X509_STORE_CTX_set_chain too,
but there are some callers to cleanup.
Bug: 426
Change-Id: I846f8292a5f5d6cc3b5d2a576bfaf86e9ea84bdc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65147
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
All callers I found seem to be compatible with this. Using the non-const
pointer isn't very useful because you cannot resize the value. Let's try
const-correcting it and we'll revert if it's too annoying to fix.
Update-Note: The above functions are now const-correct. Store the result
in a const pointer to avoid compatibility issues.
Change-Id: Id4a1c7223fbb333716906e20844bf8795118a8ea
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65128
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This CL makes two changes. First, it removes the couple of places where
X509_verify_cert may return -1 and switches to our standard 0/1 return
convention. The only -1 cases were get_issuer returning < 0 and the
caller error cases at the top. It seems implausible that any caller
would care about the latter and the former is actually impossible.
get_issuer never returns < 0.
Second, OpenSSL's original implementation did not follow the usual
error-handling convention. The usual convention is that there's a
cleanup epilog, and a variable (usually called 'ret' or 'ok') that
stores the return value. This variable is initialized in the failure
case and may only be modified immediately before a goto or when falling
through to the epilog. This allows error conditions to simply 'goto err'
and rely on the variable's value.
X509_verify_cert instead overwrite 'ok' throughout the function, which
is tedious and error-prone. Fix this to follow the usual convention.
Also remove uses of this pattern when there isn't anything to cleanup.
As part of this cleanup, we fix a near miss: the three cert_self_signed
call sites did not correctly account for this non-standard pattern.
Fortunately (as demonstrated by existing unit tests), the first call
site is fine. The remainder are only called on "trusted" certificates
from the X509_STORE. An attacker with control over trust anchors already
controls certificate verification, so this is moot. Moreover, all such
certificates first go through get_issuer, which calls X509_check_issued,
which already handles EXFLAG_INVALID, so the error condition was
redundant.
Update-Note: X509_verify_cert no longer returns -1 on some error
conditions, only zero.
Change-Id: I88d5e845cd4cb8f48d5c5df6782bf6730c682642
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65067
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
If a verify_cb consistently returns -1, it would broadly get treated as
success, except the final call would leak into ok and come out of
X509_verify_cert and read as failure. Prevent this ambiguity by
requiring the return value be 0 or 1, and aborting otherwise.
Update-Note: If the verify callback returns anything other than 0 or 1,
X509_verify_cert will now crash in BSSL_CHECK. If this happens, fix the
callback to use the correct return value.
Change-Id: I0394e68febe9f22a42bcd5de8ea4f3a82b07c862
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65107
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
All this flag does is cause verify_cb to be called with ok=2 after
policy validation happens, breaking the otherwise strict 0/1 behavior of
the callback.
We can't quite remove the symbol because a lot of bindings libraries
wrap it without realizing what it does. But no one actually uses it,
because it's pretty useless. Since we now always (other than the
bad_chain thing) check policies and that happens last, this flag really
means "please call the verify callback an extra time at the end with
ok=2".
Update-Note: X509_V_FLAG_NOTIFY_POLICY is now a no-op. This is not
expected to impact anyone.
Change-Id: I892a872181d1c1836ef2533ac616edfb6b3b5836
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65087
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Nothing uses it, OpenSSL does not document it, and it does not seem to
be correctly filled in across all the callback calls anyway. We cannot
*quite* remove the current_issuer field because it is used as a
communication channel between get_crl and check_crl callbacks, and we
can't change the function signatures without first fixing some broken
code in gRPC.
Update-Note: Removed an unused function.
Change-Id: I545e654d6c8f0a7973636217f3da27d05c0ef831
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65068
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
