This makes it slightly clearer which ints are lengths and which are
substituting for T*. (ASN1_BOOLEAN is weird. It is the one non-pointer
representation in crypto/asn1.)
Change-Id: I93ff87264835e64c9f8613edae63e93731e77548
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49865
Reviewed-by: Adam Langley <agl@google.com>
This function is a little awkward. It mutates global data, so if two
libraries in the address space both attempt to define a custom OID, they
will conflict. But some existing code uses it so, as long as it does so,
we should make it thread-safe.
Along the way, I've switched it to a hash table and removed the ability
to overwrite existing entries. Previously, overwriting a built-in table
would crash (on platforms where const structures are write-protected).
Overwriting a dynamic table implemented this weird merging algorithm.
The one caller I've seen does not appear to need this feature.
I've also switched ASN1_STRING_TABLE_cleanup to a no-op, matching our
other global cleanup functions. This function is not safe to call
without global knowledge of all other uses of the library.
Update-Note: ASN1_STRING_TABLE_add no longer allows overwrite existing
entries. In most cases, this would crash or trigger a race condition
anyway.
Bug: 426
Change-Id: Ie024cca87feaef3ff10064b452f3a860844544da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49769
Reviewed-by: Adam Langley <agl@google.com>
V_ASN1_APP_CHOOSE has been discouraged by OpenSSL since 2000:
https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=CHANGES;h=824f421b8d331ba2a2009dbda333a57493bedb1e;hb=fb047ebc87b18bdc4cf9ddee9ee1f5ed93e56aff#l10848
Instead, upstream recommends an MBSTRING_* constant.
https://www.openssl.org/docs/man1.1.1/man3/X509_NAME_add_entry_by_NID.html
This function is a bit overloaded:
MBSTRING_* means "Decode my input from this format and then re-encode it
using whatever string type best suits the NID (usually UTF8String, but
some NIDs require PrintableString)".
V_ASN1_APP_CHOOSE means "This is a Latin-1 string. Without looking at
the NID, pick one of PrintableString, IA5String, or T61String".
The latter is almost certainly not what callers want. If they want a
particular type, they can always force it by passing a particular
V_ASN1_* constant. This removes the only use of ASN1_PRINTABLE_type
within the library, though there is one external use still.
Update-Note: V_ASN1_APP_CHOOSE is removed. I only found one use, which
has been fixed.
Change-Id: Id36376dd0ec68559bbbb366e2305d42be5ddac67
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49067
Reviewed-by: Adam Langley <agl@google.com>
The old loop read one byte past the length. It also stopped the loop
too early on interior NUL. See also upstream's
https://github.com/openssl/openssl/pull/16433, though I've opted to
rewrite the function entirely rather than use their fix.
Also deduplicate the PrintableString check.
Change-Id: Ia8bd282047c2a2ed1d5e71a68a3947c7c108df95
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49066
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
In writing the tests, I noticed that the documentation was wrong. First,
the maximum lengths are measured in codepoints, not bytes.
Second, the TODO was wrong. We actually do handle this correctly,
*almost*. Rather, the bug is that the function assumes |mask| contains
no extraneous bits. If it does, all extraneous bits are interpreted as
B_ASN1_UTF8STRING. This seems like a bug, so I've gone ahead and fixed
that, with a test.
Change-Id: I7ba8fa700a8e21e6d25cb7ce879dace685eecf7e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48825
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
ASN1_STRING_print_ex is extremely complex and attempting to implement
RFC2253, so write some tests for it. Along the way, unexport
CHARTYPE_*, which are internal book-keeping used in
ASN1_STRING_print_ex.
Change-Id: Idb27cd40fb66dc099d1fd6d039a00404608c2063
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48776
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We've never tested this and plenty of files depend on FILE* APIs without
ifdefs.
Change-Id: I8c51c043e068b30bdde1723c3810d3e890eabfca
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48771
Reviewed-by: Adam Langley <agl@google.com>
This matches OpenSSL and the name. Also accessors like X509_ALGOR_get0
are in x509.h.
Change-Id: Ic7583edcf04627cbfae822df11e75eebdd9ad7aa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48770
Reviewed-by: Adam Langley <agl@google.com>
These constants aren't suitably namespaced and, moreover, are redefined
in a_strnid.c. (The constants aren't especially useful because an
X509_NAME doesn't check the upper bound.)
Update-Note: Removed some unnamespaced constants.
Change-Id: I7d15ae731628d3665119081289947600e7f38065
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48768
Reviewed-by: Adam Langley <agl@google.com>
ASN1_STRING_set_by_NID is very complex and depends on a "global mask"
for most NIDs. (Some NIDs use a single type and use STABLE_NO_MASK to
disable the global mask.) Historically, it defaulted to allowing all
types, but it switched to UTF8String in OpenSSL 1.0.2.
Updating the global mask is not thread-safe, and it's 2021. Let's just
always use UTF-8. The only callers I found set it to UTF-8 anyway (with
the exception of some test script we don't use, and some code that
wasn't compiled). No-op writes in the C/C++ memory model are still race
conditions, so this CL fixes some bugs in those callers.
Update-Note: The global mask for ASN1_STRING_set_by_NID is now always
UTF-8. Callers that want another type should reconsider and, if UTF-8 is
still unsuitable, just pass the actual desired type into
ASN1_mbstring_copy, X509_NAME_ENTRY_set_data, etc
Change-Id: I679e99c57da9a48c805460abcb3af5b2f938c93f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48766
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This flag is set when an ASN1_STRING is created from a codepath that is
aware it is an "mstring" (CHOICE of multiple string or string-like
types). With setters like X509_set_notBefore, it is very easy to
accidentally lose the flag on some field that normally has it.
The only place the flag is checked is X509_time_adj_ex. X509_time_adj_ex
usually transparently picks UTCTime vs GeneralizedTime, as in the X.509
CHOICE type. But if writing to an existing object AND if the object
lacks the flag, it will lock to whichever type the object was
previously. It is likely any caller hitting this codepath is doing so
unintentionally and has a latent bug that won't trip until 2050.
In fact, one of the ways callers might accidentally lose the
ASN1_STRING_FLAG_MSTRING flag is by using X509_time_adj_ex!
X509_time_adj_ex(NULL) does not use an mstring-aware constructor. This
CL avoids needing such a notion in the first place.
Looking through callers, the one place that wants the old behavior is a
call site within OpenSSL, to set the producedAt field in OCSP. That
field is a GeneralizedTime, rather than a UTCTime/GeneralizedTime
CHOICE. We dropped that code, but I'm making a note of it to remember
when filing upstream.
Update-Note: ASN1_STRING_FLAG_MSTRING is no longer defined and
X509_time_adj_ex now behaves more predictably. Callers that actually
wanted to lock to a specific type should call ASN1_UTCTIME_adj or
ASN1_GENERALIZEDTIME_adj instead.
Change-Id: Ib9e1c9dbd0c694e1e69f938da3992d1ffc9bd060
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48668
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This covers most of the ASN.1 time functions and a handful more of
x509.h. Also remove some code under #if 0.
I'm running out of a easy ones to do, which is probably a good thing.
Change-Id: I085b1e2a54d191a7a5f18c801b3c135cfda7bd88
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48665
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This cleans up the story with
https://boringssl-review.googlesource.com/c/boringssl/+/46164. None of
our exported functions mutate ASN1_OBJECTS, with the exception of
ASN1_OBJECT_free, the object reuse mode of c2i_ASN1_OBJECT, and their
callers. Those functions check flags to correctly handle static
ASN1_OBJECTs.
For now, I've kept the struct definition in crypto/asn1 even though
ASN1_OBJECT is partially in crypto/obj. Since we prefer to cut
dependencies to crypto/asn1, we probably should rearrange this later.
I've also, for now, kept crypto/asn1/internal.h at C-style comments,
though our style story here is weird. (Maybe it's time to clang-format
crypto/asn1 and crypto/x509? Patches from upstream rarely directly apply
anyway, since we're a mix of 1.0.2 and 1.1.1 in crypto/x509.)
Update-Note: ASN1_OBJECT is now opaque. Callers should use accessors.
Change-Id: I655e6bd8afda98a2d1e676c3abeb873aa8de6691
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48326
Reviewed-by: Adam Langley <agl@google.com>
These macros aren't consumed by anything anymore.
Change-Id: Id9616fa0962ae0dbf27bc884c6883dcad9755eb2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48229
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We already had a test, but move it to asn1_test.cc since it's part of
the ASN.1 library. Also, since it's easy, test it using public APIs
rather than stack-allocating an ASN1_STRING.
Change-Id: Ic77494e6c8f74584d159a600e334416197761475
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48227
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's BIT STRING representation has two modes, one where it
implicitly trims trailing zeros and the other where the number of unused
bits is explicitly set. This means logic in ASN1_item_verify, or
elsewhere in callers, that checks flags and ASN1_STRING_length is
inconsistent with i2c_ASN1_BIT_STRING.
Add ASN1_BIT_STRING_num_bytes for code that needs to deal with X.509
using BIT STRING for some fields instead of OCTET STRING. Switch
ASN1_item_verify to it. Some external code does this too, so export it
as public API.
This is mostly a theoretical issue. All parsed BIT STRINGS use explicit
byte strings, and there are no APIs (apart from not-yet-opaquified
structs) to specify the ASN1_STRING in X509, etc., structures. We
intentionally made X509_set1_signature_value, etc., internally construct
the ASN1_STRING. Still having an API is more consistent and helps nudge
callers towards rejecting excess bits when they want bytes.
It may also be worth a public API for consistently accessing the bit
count. I've left it alone for now because I've not seen callers that
need it, and it saves worrying about bytes-to-bits overflows.
This also fixes a bug in the original version of the truncating logic
when the entire string was all zeros, and const-corrects a few
parameters.
Change-Id: I9d29842a3d3264b0cde61ca8cfea07d02177dbc2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48225
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The implementation is a little goofy, but OBJ_dup internally makes a
copy of all the data.
Change-Id: I58e6804ede00100211ac112f03e26a34a2d29b5a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47125
Reviewed-by: Adam Langley <agl@google.com>
I stopped short of documenting the add1_attr_by functions because the
type parameter is a bit of a mess. It appears to be several enums put
together. To that end, I've updated the documentation on
V_ASN1_MAX_UNIVERSAL to note that we also need to avoid MBSTRING_FLAG.
As a preview of what I'm putting off to later, see
X509_ATTRIBUTE_set1_data for how the type parameter is used. set1_data
is extra fun because PKCS#10 attributes are set-valued. Plus there's
upstream's e20b57270dece66ce2c68aeb5d14dd6d9f3c5d68, which we should
import first.
Change-Id: I3453a0b224e42c6e22828c7d332ee133e09e6173
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46945
Reviewed-by: Adam Langley <agl@google.com>
This fixes a bug in ASN1_TYPE_get. Partly imported from upstream's
261ec72d58af64327214a78ca1c54b169ad93c28, though I don't believe
ASN1_TYPE_set was broken per se. There's also a lot more than in that
commit.
I've added a test to ensure we maintain the unused bits invariant
anyway, in case external code relies on it. (The invariant comes from
the pointer being NULL-initialized and from ASN1_primitive_free zeroing
*pval on free.)
Change-Id: I4c0c57519a7628041d81c26cd850317e01409556
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46324
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The representation here is a bit more messy than necessary. In doing so,
clean up the variable names and smooth away two rough edges:
- X509_ALGOR_get0 would leave *out_param_value uninitialized if
*out_param_type is V_ASN1_UNDEF. Instead, set it to NULL, so callers
do not accidentally use an uninitialized pointer.
- X509_PUBKEY_set0_param, if key is NULL, would leave the key alone. No
one calls this function externally and none of the (since removed)
callers in OpenSSL rely on this behavior. A NULL check here adds a
discontinuity at the empty string that seems unnecessary here:
changing the algorithm without changing the key isn't useful.
(Note the API doesn't support changing the key without the algorithm.)
Note for reviewing: the representation of ASN1_TYPE is specified
somewhat indirectly. ASN1_TYPE uses the ASN1_ANY ASN1_ITEM, which has
utype V_ASN1_ANY. Then you look at asn1_d2i_ex_primitive and asn1_ex_c2i
which peel off the ASN1_TYPE layer and parse directly into the value
field, with a fixup for NULL. Hopefully we can rework this someday...
Change-Id: I628c4e20f8ea2fd036132242337f4dcac5ba5015
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46165
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
ASN1_OBJECTs are awkward. Sometimes they are static, when returned from
OBJ_nid2obj, and sometimes they are dynamic, when parsed from
crypto/asn1.
Most structures in crypto/asn1 need to support unknown OIDs and thus
must own their ASN1_OBJECTs. But they also may be initialized with
static ones in various APIs, such as X509_ALGOR_set0. To make that work,
ASN1_OBJECT_free detects static ASN1_OBJECTs and is a no-op.
Functions like X509_ALGOR_set0 take ownership, so OpenSSL has them take
a non-const ASN1_OBJECT*. To match, OBJ_nid2obj then returns a non-const
ASN1_OBJECT*, to signal that it is freeable.
However, this means OBJ_nid2obj's mutability doesn't match its return
type. In the fork, we switched OBJ_nid2obj to return const. But, in
doing so, we had to make X509_ALGOR_set0 and X509_PUBKEY_set0_param take
const ASN1_OBJECT, even though they would actually take ownership of
dynamic ASN1_OBJECTs. There are also a few internal casts with a TODO to
be const-correct.
Neither situation is ideal. (Perhaps a more sound model would be to copy
static ASN1_OBJECTs before putting them in most structs. But that would
not match current usage.) But I think aligning with OpenSSL is the
lesser evil here, since it avoids misleading set0 functions. Managing
ownership of ASN1_OBJECTs is much more common than mutating them. To
that end, I've added a note that ASN1_OBJECTs you didn't create must be
assumed immutable[*].
Update-Note: The change to OBJ_nid2obj should be compatible. The changes
to X509_PUBKEY_set0_param and X509_ALGOR_set0 may require fixing some
pointer types.
[*] This is *almost* honored by all of our functions. The exception is
c2i_ASN1_OBJECT, which instead checks the DYNAMIC flag as part of the
object reuse business. This would come up if we ever embedded
ASN1_OBJECTs directly in structs.
Change-Id: I1e6c700645c12b43323dd3887adb74e795c285b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46164
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This imports 1ecc76f6746cefd502c7e9000bdfa4e5d7911386 and
41d62636fd996c031c0c7cef746476278583dc9e from upstream. These would have
rejected the mistake in OpenSSL's EDIPartyName sturcture.
Change-Id: I4eb218f9372bea0f7ff302321b9dc1992ef0c13a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44424
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Almost everything in <openssl/asn1.h> uses ASN1_STRING, and there are a
lot of unspoken assumptions in the library about the type field, so it
needs quite a bit of text.
Change-Id: Ied56c9428069477da8ecb17a174da4320e573fa1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44184
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This still needs some overall documentation describing ASN1_STRING's
relationship to all the other types, but start with the easy bits.
Change-Id: I968d4b1b3d57a9b543b3db489d14cf0789e30eb3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44049
Reviewed-by: Adam Langley <agl@google.com>
The free and dup macros are fine and can be replaced with their function
counterparts, but the new macros call ASN1_STRING_type_new with a
representative type in the CHOICE. This does not match what the
corresponding functions (e.g. ASN1_TIME_new) do.
The functions go through tasn_new.c and end up at ASN1_primitive_new.
That ends up creating an ASN1_STRING with type -1 and the
ASN1_STRING_FLAG_MSTRING flag set. X509_time_adj_ex uses the flag to
determine whether to trigger X.509's UTCTime vs GeneralizedTime
switching.
Confusingly, ASN1_TIME_adj, ASN1_UTCTIME_adj, and
ASN1_GENERALIZEDTIME_adj trigger this behavior based on the function
itself. That seems more robust (X509_set1_notBefore might accidentally
lose the flag), so maybe we can remove this flag. In the meantime, at
least remove the old macros so we don't create the wrong type.
Update-Note: Some M_ASN1 macros were removed. Code search says there
were no uses, and OpenSSL upstream removed all of them.
Change-Id: Iffa63f2624c38e64679207720c5ebd5241da644c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44047
Reviewed-by: Adam Langley <agl@google.com>
At one point in the SSLeay days, all the ASN1_STRING typedefs were
separate structs (but only in debug builds) and the M_ASN1_* macros
included type casts to handle this.
This is long gone, but we still have the M_ASN1_* macros. Remove the
casts and switch code within the library to call the macros. Some
subtleties:
- The "MSTRING" types (what OpenSSL calls its built-in CHOICEs
containing some set of string types) are weird because the M_FOO_new()
macro and the tasn_new.c FOO_new() function behave differently. I've
split those into a separate CL.
- ASN1_STRING_type, etc., call into the macro, which accesses the field
directly. This CL inverts the dependency.
- ASN1_INTEGER_new and ASN1_INTEGER_free, etc., are generated via
IMPLEMENT_ASN1_STRING_FUNCTIONS in tasn_typ.c. I've pointed
M_ASN1_INTEGER_new and M_ASN1_INTEGER_free to these fields. (The free
function is a no-op, but consistent.)
- The other macros like M_ASN1_BIT_STRING_dup largely do not have
corresponding functions. I've aligned with OpenSSL in just using the
generic ASN1_STRING_dup function. But some others, like
M_ASN1_OCTET_STRING_dup have a corresponding ASN1_OCTET_STRING_dup
function. OpenSSL retained these, so I have too.
Update-Note: Some external code uses the M_ASN1_* macros. This should
remain compatible, but some type errors may have gotten through
unnoticed. This CL restores type-checking.
Change-Id: I8656abc7d0f179192e05a852c97483c021ad9b20
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44045
Reviewed-by: Adam Langley <agl@google.com>
Some of the X509 functions are hard to document without first
documenting the ASN.1 types themselves. (ASN1_TYPE's goofy
representation is leaked everywhere.)
Change-Id: I0adcf055414925f9e39c8293cbd42d29f0db3143
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44044
Reviewed-by: Adam Langley <agl@google.com>
Sadly we need to keep ASN1_put_eoc. Ruby uses it.
OpenSSL's PKCS#7 implementation generated an "ndef" variant of the
encoding functions, to request indefinite-length encoding. Remove the
support code for this.
Update-Note: Types that use one of the NDEF macros in asn1t.h will fail
to compile. This CL should not affect certificate parsing.
Change-Id: I6e03f6927ea4b7a6acd73ac58bf49512b39baab8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43889
Reviewed-by: Adam Langley <agl@google.com>
This function is unused and quite unsafe.
Update-Note: Use ASN1_STRING_set instead, though this function appears
to be unused.
Change-Id: Ie6f4dec4b9e11ebde95b322ef91e1b8d63fbb8af
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42724
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley