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>
Along the way, add ASN1_INTEGER_get_uint64 from upstream, which has much
better error-handling. Also fold the IntegerSetting test into the main
integer test as the test data is largely redundant.
Change-Id: I7ec84629264ebf405bea4bce59a13c4495d81ed7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51634
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This fixes several issues around ASN1_INTEGER handling. First, invalid
INTEGERs (not allowed in BER or DER) will no longer be accepted by
d2i_ASN1_INTEGER. This aligns with upstream OpenSSL, which became strict
in 6c5b6cb035666d46495ccbe4a4f3d5e3a659cd40, part of OpenSSL 1.1.0.
In addition to matching the standard, this is needed to avoid
round-tripping issues: ASN1_INTEGER uses a sign-and-magnitude
representation, different from the DER two's complement representation.
That means we cannot represent invalid DER INTEGERs. Attempting to do so
messes up some invariants and causes values to not round-trip correctly
when re-encoded. Thanks to Tavis Ormandy for catching this.
Next, this CL tidies the story around invalid ASN1_INTEGERs (non-minimal
and negative zero). Although we will never produce them in parsing, it
is still possible to manually construct them with ASN1_STRING APIs.
Historically (CVE-2016-2108), it was possible to get them out of the
parser, due to a different bug, *and* i2d_ASN1_INTEGER had a memory
error in doing so. That different bug has since been fixed, but we
should still handle them correctly and test this. (To that end, this CL
adds a test we ought to have added importing upstream's
3661bb4e7934668bd99ca777ea8b30eedfafa871 back in
c4eec0c16b02c97a62a95b6a08656c3a9ddb6baa.)
As the two's complement invariants are subtle as it is, I've opted to
just fix the invalid values before encoding. However, invalid
ASN1_INTEGERs still do not quite work right because ASN1_INTEGER_get,
ASN1_INTEGER_cmp, and ASN1_STRING_cmp will all return surprising values
with them. I've left those alone.
Finally, that leads to the zero value. Almost every function believes
the representation of 0 is a "\0" rather than "". However, a
default-constructed INTEGER, like any other string type, is "". Those do
not compare as equal. crypto/asn1 treats ASN1_INTEGER generically as
ASN1_STRING enough that I think changing the other functions to match is
cleaner than changing default-constructed ASN1_INTEGERs. Thus this CL
removes all the special cases around zero.
Update-Note: Invalid INTEGERs will no longer parse, but they already
would not have parsed in OpenSSL. Additionally, zero is now internally
represented as "" rather than "\0".
Bug: 354
Change-Id: Id4d51a18f32afe90fd4df7455b21e0c8bdbc5389
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51632
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These functions need some work, but first avoid the duplicate versions.
See also upstream's 6c5b6cb035666d46495ccbe4a4f3d5e3a659cd40.
Update-Note: ASN1_INTEGER_to_BN and ASN1_ENUMERATED_to_BN will now fail
when called on an ASN1_STRING/ASN1_INTEGER/ASN1_ENUMERATED (they're all
the same type) with the wrong runtime type value. Previously, callers
that mixed them up would get the right answer on positive values and
silently misinterpret the input on negative values. This change matches
OpenSSL's 1.1.0's behavior.
Change-Id: Ie01366003f7b2e49477cb73eaf7eaac26d86675d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51631
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@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>
One down, two more to go! As part of this, I've added it to doc.config,
revised the note at the top, and moved the sample i2d/d2i functions
here.
Bug: 426
Change-Id: I7bb9d56bf9ba58c921cfcf9626bf3647c6e5c7df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50107
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
ASN1_ENCODING can be unexported because all types using it are now
hidden. This does mean external uses of <openssl/asn1t.h> can no longer
use ASN1_SEQUENCE_enc, but there do not seem to be any such uses.
ASN1_TLC and ASN1_TEMPLATE typedefs are only necessary for users of
asn1t.h. I'm hopeful we can do away with ASN1_TLC once I get to
reworking tasn_dec.c. ASN1_TEMPLATE is somewhat stuck, though all
references should be hidden behind macros.
ASN1_generate_* appear to only referenced within the library. Remove the
unused one and move the other to x509/internal.h. (asn1_gen.c is
currently in crypto/x509 rather than crypto/asn1, so I put it in
x509/internal.h to match. I'll leave figuring out that file to later.)
Annoyingly, asn1/internal.h now pulls in asn1t.h, but so it goes.
Change-Id: I8b43de3fa9647883103006e27907730d5531fd7d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50106
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
While I'm here, add missing parentheses around the B_ASN1_* bitmasks.
I've tossed ASN1_PRINTABLE into the deprecated bucket, though X509_NAME
relies on it, because it is a mess.
Bug: 407, 426
Change-Id: I287f60e98d6c9f237908011e1a816f4b4fb4433e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50105
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Outside the library, this function is practically useless. It creates an
empty ASN1_OBJECT, which can never be filled in because the struct is
private and there are no mutating setters.
(See https://boringssl-review.googlesource.com/c/boringssl/+/46164 and
https://boringssl-review.googlesource.com/c/boringssl/+/48326 for a
discussion on why it's important ASN1_OBJECTs are immutable.)
Update-Note: ASN1_OBJECT_new is no longer exported. While this function
does remain in OpenSSL, it is extremely unlikely anyone has found a use
for this function.
Bug: 452
Change-Id: I111a9a1ce3ca4d7aa717a3c3a03d34c05af8fdbd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50025
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
If the header is valid, but the body is truncated, ASN1_get_object
intentionally preserves the indefinite-length and constructed output
bits. This means callers who check for error with == 0x80 may read off
the end of the buffer on accident.
This is unlikely to break callers: 0x80 was already a possible error
value, so callers already needed to handle it. The original function's
aim in returning more information is unlikely to matter because callers
cannot distinguish 0x80 (could not parse header) and 0x80 (header was
valid, definite-length, and primitive, but length was too long).
Update-Note: ASN1_get_object's calling convention is slightly
simplified.
Bug: 451
Change-Id: If2b45c47e6b8864aef9fd5e04f313219639991ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50005
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is what crypto/asn1 uses instead of CBS and CBB. ASN1_get_object is
particularly impressive.
Bug: 426
Change-Id: Ib11ace5448d596ec134ed77e60139c0e2d7e9d07
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49985
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Also fill in docs for some easy ASN1_STRING wrappers while I'm here.
(Not sure why they exist, but removing them is probably more trouble
than is worth it.)
Bug: 407, 426
Change-Id: Id12c5fbc84982728435d105d66a3b63e5f3a1d15
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49945
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The comparison should notice differences in bit count.
Update-Note: ASN1_STRING_cmp no longer incorrectly treats BIT STRINGs
with different padding bits as equal.
Bug: 446
Change-Id: I22b3fcc5d369540d029ca234e9b3b02402cec4c3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49928
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
In doing so, fix ASN1_item_pack to not use the ASN1_OCTET_STRING
typedef. The function makes an untyped ASN1_STRING.
With all these caveats, one might think that ASN1_BOOLEAN ASN1_ITEMs are
pretty useless. This is about right. They're really only usable embedded
as a field in another struct.
Bug: 426
Change-Id: Id7830b91b2d011038ce79ec848e17ad6241423e1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49926
Reviewed-by: Adam Langley <agl@google.com>
There are potentially more than three ASN1_BOOLEAN ASN1_ITEMs.
ASN1_BOOLEAN may be wrapped by explicit or implicit tagging into another
ASN1_ITEM. (I also suspect SEQUENCE OF BOOLEAN is just unrepresentable
in this library, but I will leave that rabbithole alone.)
Bug: 426
Change-Id: I3e58bfb63ee5c7a6d112b4a16e0f13fbacaea93a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49925
Reviewed-by: Adam Langley <agl@google.com>
Not quite ready to add it to doc.config, but this fixes up the different
C++ guard styles, and a few mistakes in the comments.
Bug: 426
Change-Id: I027f14b2f79861e510bfa7a958604f47ae78dda1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49911
Reviewed-by: Adam Langley <agl@google.com>
This is a bit verbose, since it expands out the macros, but I think this
is more understandable in the long run than figuring out which of the
three name parameters here goes in which spot:
DECLARE_ASN1_FUNCTIONS_fname(ASN1_TYPE, ASN1_ANY, ASN1_TYPE)
This CL leaves ASN1_TYPE and mstrings for later.
Bug: 426
Change-Id: I942eb4f9fd5fbb6d30106eac2c667e28615f5199
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49910
Reviewed-by: Adam Langley <agl@google.com>
This starts expanding out the DECLARE_* macros in asn1.h. It also
documents some ways in which ASN1_NULL is odd.
Bug: 426
Change-Id: Ie166861d91ce78901c76b85de79dcc683e480275
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49909
Reviewed-by: Adam Langley <agl@google.com>
This is completely unchecked for now, as it all goes through tasn_enc.c.
But the only non-const encoders now are X509_NAME, and the functions
that call into it, so we can fix up the ones at the bottom.
I haven't done the macros that use the "name" or "fname" variants. The
set of macros for const are a little weird. But before expanding the
header macros out, I wanted to change the signatures on the macro side
once, so the compiler checks they're expanded correctly.
Update-Note: The type signature of some i2d functions, such as
i2d_ASN1_OCTET_STRING, is now const-correct.
Bug: 407
Change-Id: I03988f5591191b41ab4e7f014bd8d41cb071b39a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49908
Reviewed-by: Adam Langley <agl@google.com>
I've intentionally not discussed defining ASN1_ITEM, because I'm hoping
we can limit that to libdecrepit and users of asn1t.h. I suspect we
can't avoid ASN1_ITEM itself, but we may be able to replace it with an
internal new/free/d2i/i2d vtable someday.
Bug: 426
Change-Id: Iebd5a8f5ab7078d14131f869b98cdb79b56884ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49907
Reviewed-by: Adam Langley <agl@google.com>
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>
David Benjamin