Some of these were non-const because dup functions weren't
const-correct, but they are now. Once nuisance is the accessors. Ideally
they'd return non-const pointers, but that'll break OpenSSL consumers.
Bug: 407
Change-Id: I52b939a846b726d1d84dd2d5fdf71a7a7284d49e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53336
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Although it contains an X509_NAME (by way of GENERAL_NAME), the
GENERAL_NAME field does not participate in serialization, so it is
actually const.
Bug: 407
Change-Id: I299815789744597e2b355cb0b996ba90c3b6a72f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53334
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
There's a lot of these, so toss them into a corner somewhere.
I've added a note to note to discourage using them with memory BIOs.
Memory BIOs are, by far, the most common use I've seen for i2d_*_bio and
d2i_*_bio. It works, but it's unnecessary.
Bug: 426
Change-Id: I8645207a4ac9f1223d0739b5351c99a55400195f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53331
Commit-Queue: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Update-Note: Suite B flags in the X.509 stack are no longer supported.
This isn't expected to affect anything but bindings wrapping unused
options.
Change-Id: Ia0770e545d34e041ab995e80ea11b4dd4a5e47ef
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53329
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This was originally added for the openssl command-line utility and
otherwise is not very useful. I found no callers, so drop it.
Update-Note: An unused function was removed.
Change-Id: I12aa314fd3d8f1dad79eb5a07e0dea662dd9b4a8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53328
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL has a large exported API surface for exporting the policy tree
out of an X509_STORE_CTX. As far as I can tell, no one uses any of these
APIs. Remove them.
Update-Note: It is no longer possibly to see the policy tree after an
X.509 verification. As far as we can tell, this feature is unused.
Change-Id: Ieab374774805e90106555ce4e4155f8451ceb5b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53327
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Unfortunately, these functions are not const-correct, even the
accessors. Document what they should have been, especially since
mutating an X509_NAME_ENTRY directly won't even update the 'modified'
bit correctly.
Do a pass at adding consts to our code internally, but since the
functions return non-const pointers, this isn't checked anywhere. And
since serializing an X509_NAME is not always thread-safe, there's a
limit to how much we can correctly mark things as const.
Bug: 426
Change-Id: Ifa3d8bafb5396fbe7b91416f234de4585284c705
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53326
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The X509_NAME representation is a little odd because it flattens the
RDN sequence, but maintains RDN indices (X509_NAME_ENTRY_set) to recover
the information. add_entry and delete_entry both have logic to maintain
invariants. Test that they work.
Change-Id: I7d4a033db0a82a1f13888bbfca29076b589cc214
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53325
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I found no uses of this. If we have to put it back, it's no big deal,
but one less group of functions to have to document. (SEQUENCE OF
AlgorithmIdentifier is mostly a weird PKCS#7 thing.)
Update-Note: X509_ALGORS is removed. If someone was relying on it, we
can re-export it.
Change-Id: I2b8e8d0d1d56d2debf99687023bc3621e92f6b08
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53307
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We have no X509_REQ_up_ref function, and neither does OpenSSL. That
means this reference count is unreachable, so remove it.
Change-Id: I6a0899b8fcb0716d47c89174b561e90d99a9b54b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53306
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Types which do not contain an X509_NAME can be const. X509_NAME still
requires some thought. (i2d_X509_NAME can mutate the object and isn't
even thread-safe when modified.)
Bug: 407
Change-Id: Iceafa2b4ea9c4194cfcc3044d90393b5d91f7c11
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53305
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
It seems no one has ever exercised this mode to notice it doesn't work.
Add a test for this, which catches the memory leak. See also
https://github.com/openssl/openssl/issues/18677
Change-Id: I8890febc71decd553a040bdda8739f68ed616c39
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53285
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Our free functions all tolerate NULL.
Change-Id: Ifcb3185c8d2f34afb83f7286c3463136edc926fa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53125
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
No sense in keeping two around. This does cause the functions to reject
some previously accepted invalid inputs. These were intentionally
accepted by
https://boringssl-review.googlesource.com/c/boringssl/+/13082 for
an old version of M2Crypto, but I belive we no longer need to be
compatible with that.
Update-Note: ASN1_TIME_print, ASN1_UTCTIME_print, and
ASN1_GENERALIZEDTIME_print will no longer accept various invalid inputs.
Change-Id: I4606d0b39585a19eb4b984ac809706e497a3f799
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53090
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This is a mechanical change generated from the following command:
find crypto/{asn1,pem,x509,x509v3} -name '*.c' -o -name '*.h' | xargs sed -i -e 's/return (\([^;()]*\));/return \1;/'
Change-Id: I957295af96c4aa08d6006e27093fd3a07fb6fe75
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53089
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Some things that I noticed paging through.
Change-Id: I41e5e12b743fd996f5ff91d19f6edc074169f629
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53088
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This CL is the result of the following commands:
for d in asn1 x509 x509v3 pem; do
go run util/convert_comments.go crypto/$d/*.h
go run util/convert_comments.go crypto/$d/*.c
done
Change-Id: If78433f68cb2f913b0de06ded744a5a65540e1cf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53087
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This CL runs the same command as in the preceding CL, but with
'IncludeBraces: true' added to .clang-format. I've split this out
separately because the documentation says:
> Setting this option to true could lead to incorrect code formatting
> due to clang-format’s lack of complete semantic information. As such,
> extra care should be taken to review code changes made by this option.
I've also kept InsertBraces out of .clang-format for now because it's a
fairly recent option, and clang-format fails when it sees unrecognized
options.
Change-Id: I305ea7bb2633704053a1f8de1e11b037b9fc8a76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53086
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Previously, we did not clang-format a few directories because we had
left them largely untouched. clang-format them now so we're finally more
uniform.
This CL is the result of the following commands:
for d in asn1 x509 x509v3 pem; do
clang-format -i crypto/$d/*.h
clang-format -i crypto/$d/*.c
done
(Written in this funny way because crypto/pem/*.h doesn't match
anything.)
Change-Id: I7f4ca9b3a9c8f07d6556e00e9e84b3c0880ee12e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53085
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Adding the ending macros to StatementMacros tells clang-format the macro
terminates a statement. Adding trailing commas in the middle keeps it
from trying to bundle the curly brace with the next statement.
Also add a few other trailing commas that clang-format otherwise indents
awkwardly.
Change-Id: I0b2ba9cf07bc775649fa1e92de3e5bb2e2b0b20b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52728
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This has no callers, and seems to be practically unusable. The only way
to set an X509_CRL_METHOD is X509_CRL_set_default_method, which is not
thread-safe and globally affects the CRL implementation across the
application.
The comment says it's to handle large CRLs, so lots of processes don't
have to store the same CRL in memory. As far as I can tell,
X509_CRL_METHOD cannot be used to help with this. It doesn't swap out
storage of the CRL, just signature verification and lookup into it. But
by the time we call into X509_CRL_METHOD, the CRL has already been
downloaded and the data stored on the X509_CRL structure. (Perhaps this
made more sense before the structure was made opaque?)
Update-Note: APIs relating to X509_CRL_METHOD are removed.
Change-Id: Ia5befa2a0e4f4416c2fb2febecad99fa31c1c6ac
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52687
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/51635 switched
the serial number check to ASN1_INTEGER_get_uint64, but as that function
actually treats overflow as an error, it adds to the error queue and we
need to clear it.
See also b/231880827, though whether that is a red herring or the cause,
I'm not sure.
Change-Id: Ibd7e9369c3455898fa3411b7a079ce21b37c586c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52648
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This aligns X509_REQ's and X509_CRL's parsers to the changes already
made with X509; we reject invalid versions and check that extensions are
only with the corresponding version. For now, we still allow X509v1 CRLs
with an explicit version, matching certificates. (The DEFAULT question
is moot for X509_REQ because CSRs always encode their version, see RFC
2986.)
In addition to rejecting garbage, this allows for a more efficient
representation once we stop using the table-based parser: X509 and
X509_CRL can just store a small enum. X509_REQ doesn't need to store
anything because the single version is information-less.
Update-Note: Invalid CRL and CSR versions will no longer be accepted.
X509_set_version, etc., no longer allow invalid versions.
Fixed: 467
Change-Id: I33f3aec747d8060ab80e0cbb8ddf97672e07642c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52605
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The manual construction of the version integer is odd. The default is
already zero, and as of
https://boringssl-review.googlesource.com/c/boringssl/+/51632, we've
settled on the empty string as the ASN1_INTEGER representation of zero.
But there don't seem to be any uses of this function, so just remove it.
Update-Note: Removed seemingly unused public API.
Change-Id: I75f8bcdadb8ffefb0b2da0fcb0a87a8cb6398f70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52585
Reviewed-by: Adam Langley <agl@google.com>
For now, the tests assert the existing behavior of X509_NAME_print, but
there are several bugs in it.
Change-Id: I9bc211a880ea48f7f756650dbe1f982bc1ec689d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52366
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We no longer use a weak hash for certificate comparisons. There
is no need to do extra work when certificates are the same.
Change-Id: I3b4b295122b289ae389bce2245b8348562700855
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52346
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
directly. This removes some temporary variables that would only be
used to hold ctx->verify_cb.
Change-Id: Id8f61c30bc74bb49757085c68baa4e8eec06b1d0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52285
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is causing -Werror failures with recent Clang builds.
Change-Id: I0f82d99ae51d02037f5f614a8fadfb3efc0da2da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52145
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
It appears to be unused. It has global effects and is not thread-safe.
Rather than try to make the double-function-pointer declaration
readable, remove it.
Change-Id: If58ecd0c9367bbb27cf8c5e27ac9997fe4c1225d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51965
Reviewed-by: Alex Gaynor <alex.gaynor@gmail.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifdb2fe5952930c33dfa9ea5bbdb9d1ce699952a4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52027
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This type is opaque, with no accessors or setters, and there is no way
to get a hold of one except by parsing it. It's only used indirectly via
X509 functions.
The 'other' field is unused and appears to be impossible to set or
query, in either us or upstream.
Change-Id: I4aca665872792f75e9d92e5af68da597b849d4b6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51746
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The getters would leave the length uninitialized when empty, which is
dangerous if the caller does not check. Instead, always fill it in.
This opens a can of worms around whether empty alias and missing alias
are meaningfully different. Treating {NULL, 0} differently from
{non-NULL, 0} has typically caused problems. At the PKCS#12 level,
neither friendlyName, nor localKeyId are allowed to be empty, which
suggests we should not distinguish. However, X509_CERT_AUX, which is
serialized in i2d_X509_AUX, does distinguish the two states. The getters
try to, but an empty ASN1_STRING can have NULL data pointer. (Although,
when parsed, they usually do not because OpenSSL helpfully
NUL-terminates it for you.)
For now, I've just written the documentation to suggest the empty string
is the same as the missing state. I'm thinking we can make the PKCS#12
functions not bother distinguishing the two and see how it goes. I've
also gone ahead and grouped them with d2i_X509_AUX, although the rest of
the headers has not yet been grouped into sections.
Bug: 426, 481
Change-Id: Ic9c21bc2b5ef3b012c2f812b0474f04d5232db06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51745
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This gives clearer, less platform-dependent behavior.
Change-Id: Ib935bf861108ec010d8d409d840f94b52a3b3ae0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51635
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Now we only have one BER/DER TLV parser. Annoyingly, this uses the CBS
BER function, not the DER one. This is because Android sometimes needs
allow a non-minimal length in certificate signature fields (see
b/18228011).
For now, this CL calls CBS_get_any_ber_asn1_element. This is still an
improvement over the old parser because we'll reject non-minimal tags
(which are actually even forbidden in BER). Later, we should move the
special case to just the signature field, and ultimately to a
preprocessing step specific to that part of Android.
Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with incorrectly-encoded tags will now be rejected.
Bug: 354
Change-Id: I56a7faa1ffd51ee38cc315ebaddaef98079fd90e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This fix isn't ideal, given the current space of possible version
values. But rather than make the printing code complicated, we should
make invalid versions impossible. I've left TODOs where that would be
needed.
Bug: 467, 450
Change-Id: I6c9ae97b8454182b0c1ab6ba2e070dc6d7d8b3f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50767
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
X509_print_ex tries to print negative serial numbers in decimal. In
doing so, it ends up passing a signed long to %lx and trips
-Wformat-signed.
A minimal fix would be to cast to unsigned long, but this unsigned long
is the absolute value of a signed long (l = -l). This is tricky because
-LONG_MIN does not fit in long. It all works because the length check
only allows one bit short of sizeof(long)*8 bits (ASN1_INTEGER is
sign-and-magnitude).
Still, this is a whole lot of subtlety to account for an invalid case.
Instead, send negative serial numbers down the generic path.
Bug: 450
Change-Id: Ib215fd23863de27e01f7ededf95578f9c800da37
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50766
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Update-Note: ".example.com" as an input DNS name will no longer match
"www.example.com" in a certificate. (Note this does not impact wildcard
certificates. Rather, it removes a non-standard "reverse wildcard" that
OpenSSL implemented.)
Fixed: 463
Change-Id: I627e1bd00b8e4b810e9bb756f424f6230a99496e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50726
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This replaces v3name_test.cc which is rather difficult to follow.
v3name_test.cc ran all pairs of names against each other, used a
default case-insensitivity rule, and then had a list of string
exceptions to that rule. This is hopefully easier for us to adjust
later. It also fixes a testing bug we wouldn't notice if an expected
"exception" didn't fire.
Sadly, we cannot use designated initializers in C++ yet. MSVC does not
support them until MSVC 2019.
Change-Id: Ia8e3bf5f57d33a9bf1fc929ba1e8cd2a270a8a24
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50725
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
I believe, with this, we have aligned with OpenSSL 1.1.x on the
crypto/x509 and crypto/asn1 types that are now opaque. Strangely,
OpenSSL kept X509_ALGOR public. We may wish to hide that one too later,
but we can leave it for now.
Update-Note: Use X509_REVOKED accessors rather than reaching into the
struct.
Bug: 425
Change-Id: Ib47944648a8693ed7078ffe94f7b557022debe30
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50685
Reviewed-by: Adam Langley <agl@google.com>
This simplifies the ASN1_get_object calling convention and removes
another significant source of tasn_dec.c complexity. This change does
not affect our PKCS#7 and PKCS#12 parsers.
Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with BER indefinite lengths will now be rejected.
Bug: 354
Change-Id: I723036798fc3254d0a289c77b105fcbdcda309b2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50287
Reviewed-by: Adam Langley <agl@google.com>
Constructed strings are a BER mechanism where a string type can be
represented as a tree of constructed nodes and primitive leaves, that
then have to be concatenated by the parser. This is prohibited in DER
and a significant source of complexity in our parser.
Note this change does not affect our PKCS#7 and PKCS#12 parsers (where
BER is sadly necessary for interop) because those use CBS.
Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with BER constructed strings will now be rejected.
Bug: 354
Change-Id: I5a8ee028ec89ed4f2d5c099a0588f2029b864580
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50286
Reviewed-by: Adam Langley <agl@google.com>
X509V3_EXT_d2i should notice if an extension has extra data at the end.
Update-Note: Some previously accepted invalid certicates may be
rejected, either in certificate verification or in X509_get_ext_d2i.
Bug: 352
Change-Id: Iacbb74a52d15bf3318b4cb8271d44b0f0a2df137
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50285
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Bob Beck
Kent Ross
Adam Langley