See https://github.com/openssl/openssl/issues/16538
Update-Note: A default-constructed object with a required ANY or
string-like CHOICE field cannot be encoded until the field is specified.
Note this affects i2d_X509: notBefore and notAfter are string-like
CHOICEs in OpenSSL.
Bug: 429
Change-Id: I97d971fa588ab72be25a4c1eb7310ed330f16c4f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49349
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
asn1_ex_i2c actually does have an error condition, it just wasn't being
handled.
628b3c7f2f, imported from upstream's
f3f8e72f494b36d05e0d04fe418f92b692fbb261, tried to check for OID-less
ASN1_OBJECTs and return an error. But it and the upstream change didn't
actually work. -1 in this function means to omit the object, so OpenSSL
was silently misinterpreting the input structure.
This changes the calling convention for asn1_ex_i2c to support this. It
is, unfortunately, a little messy because:
1. One cannot check for object presense without walking the
ASN1_ITEM/ASN1_TEMPLATE structures. You can *almost* check if *pval
is NULL, but ASN1_BOOLEAN is an int with -1 to indicate an omitted
optional. There are also FBOOLEAN/TBOOLEAN types that omit FALSE/TRUE
for DEFAULT. Thus, without more invasive changes, asn1_ex_i2c must be
able to report an omitted element.
2. While the i2d functions report an omitted element by successfully
writing zero bytes, i2c only writes the contents. It thus must
distinguish between an omitted element and an element with
zero-length contents.
3. i2c_ASN1_INTEGER and i2c_ASN1_BIT_STRING return zero on error rather
than -1. Those error paths are not actually reachable because they
only check for NULL. In fact, OpenSSL has even unexported them. But I
found a few callers. Rather than unwind all this and change the
calling convention, I've just made it handle 0 and map to -1 for now.
It's all a no-op anyway, and hopefully we can redo all this with CBB
later.
I've just added an output parameter for now.
In writing tests, I also noticed that the hand-written i2d_ASN1_OBJECT
and i2d_ASN1_BOOLEAN return the wrong value for errors, so I've fixed
that.
Update-Note: A default-constructed object with a required ASN1_OBJECT
field can no longer be encoded without initializing the ASN1_OBJECT.
Note this affects X509: the signature algorithm is an ASN1_OBJECT. Tests
that try to serialize an X509_new() must fill in all required fields.
(Production code is unlikely to be affected because the output was
unparsable anyway, while tests sometimes wouldn't notice.)
Bug: 429
Change-Id: I04417f5ad6b994cc5ccca540c8a7714b9b3af33d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49348
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This handles normal CHOICE types. A follow-up CL will handle MSTRING and
ANY types.
Update-Note: An invalid CHOICE object (e.g. GENERAL_NAME) will now fail
when encoded, rather than be silently omitted. In particular, CHOICE
objects are default-initialized by tasn_new.c in an empty -1 state.
Structures containing a required CHOICE field can no longer be encoded
without filling in the CHOICE.
Bug: 429
Change-Id: I7011deadf518ddc344a56b07a0e268ceaae17fe0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49347
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
tasn_enc.c was missing lots of error checks and mixed up 0 and -1
returns. Document all the internal calling conventions, as best as I can
tell, and fix things up.
There are also error cases it forgets to check (it generally does not
notice missing non-OPTIONAL fields). This CL only addresses errors it
already tries to report. Subsequent CLs will add in the missing error
cases. And then if it all sticks, I'm hoping we can rewrite this with
CBB. Rewriting tsan_dec.c to CBS would also be good, but that will be
more difficult as we need to clear out BER first.
Update-Note: Some error cases which were silently misinterpreted as
missing OPTIONAL elements will now cause encoding to fail.
Bug: 429
Change-Id: Ibbb3eba08eb8f8f878930c9456edc8c74479aade
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49345
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
I noticed this while I was reading through the encoder. OpenSSL's ASN.1
library is very sloppy when it comes to reusing enums. It has...
- Universal tag numbers. These are just tag numbers from ASN.1
- utype. These are used in the ASN1_TYPE type field, as well as the
ASN1_ITEM utype fields They are the same as universal tag numbers,
except non-universal types map to V_ASN1_OTHER. I believe ASN1_TYPE
types and ASN1_ITEM utypes are the same, but I am not positive.
- ASN1_STRING types. These are the same as utypes, except V_ASN1_OTHER
appears to only be possible when embedded inside ASN1_TYPE, and
negative INTEGER and ENUMERATED values get mapped to
V_ASN1_NEG_INTEGER and V_ASN1_NEG_ENUMERATED. Additionally, some
values like V_ASN1_OBJECT are possible in a utype but not possible in
an ASN1_STRING (and will cause lots of problems if ever placed in
one).
- Sometimes one of these enums is augmented with V_ASN1_UNDEF and/or
V_ASN1_APP_CHOOSE for extra behaviors.
- Probably others I'm missing.
These get mixed up all the time. asn1_ex_i2c's MSTRING path converts
from ASN1_STRING type to utype and forgets to normalize V_ASN1_NEG_*.
This means that negative INTEGERs and ENUMERATEDs in MSTRINGs do not get
encoded right.
The negative INTEGER case is unreachable (unless the caller passes
the wrong ASN1_STRING to an MSTRING i2d function, but mismatching i2d
functions generally does wrong things), but the negative ENUMERATED case
is reachable. Fix this and add a test.
Change-Id: I762d482e72ebf03fd64bba291e751ab0b51af2a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48805
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
ASN1_TFLG_SET_ORDER was used in OpenSSL's CMS and PKCS#7
implementations, which we've removed. Fields that use it not only get
the DER SET sorting but, when serialized, go back and mutate the
original object to match.
This is unused, so remove it. This removes one of the sources of
non-const behavior in i2d functions.
Bug: 407
Change-Id: I6b2bf8d11c30a41b53d14ad475c26a1a30dfd31f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48786
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The ASN1_BOOLEAN representation is a mess. ASN1_BOOLEAN is an int
and if non-negative (negative values mean omitted or default), gets cast
to uint8_t and encoded as the value. This means callers are simply
expected to know true is 0xff, not 1. Fix this by only encoding 0 or
0xff.
This also fixes a bug where values like 0x100 are interpreted as true
(e.g. in the tasn_enc.c logic to handle default values), but encoded as
false because the cast only looks at the least significant byte.
This CL does not change the parsing behavior, which is to allow any BER
encoding and preserve the value in the in-memory representation (though
we should tighten that). However the BER encode will no longer be
preserved when re-encoding.
Update-Note: Callers setting ASN1_BOOLEANs to a positive value other
than 0xff will now encode 0xff. This probably fixes a bug, but if anyone
was attaching significance to incorrectly-encoded booleans, that will
break.
Change-Id: I5bb53e068d5900daca07299a27c0551e78ffa91d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46924
Commit-Queue: David Benjamin <davidben@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>
A few can be made static. Move the rest of asn1_locl.h.
Update-Note: Code search says these are unused. If someone's using them,
we can reexport them.
Change-Id: Ib41fd15792b59e7a1a41fa6b7ef5297dc19f3021
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43893
Reviewed-by: Adam Langley <agl@google.com>
This was used to register custom primitive types, namely some INTEGER
variations. We have since removed them all.
Change-Id: Id3f5b15058bc3be1cef5e0f989d2e7e6db392712
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43891
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 is a remnant of an older incarnation of OpenSSL's ASN.1 code.
Update-Note: Types using IMPLEMENT_COMPAT_ASN1 from openssl/asn1t.h will
fail to compile. This CL should not affect certificate parsing.
Change-Id: I59e04f7ec219ae478119b77ce3f851a16b6c038f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43888
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley