The i2d functions internally take a tag/class pair of parameters. If tag
is not -1, we override the tag with (tag, class). Otherwise, class is
ignored. (class is inconsistently called aclass or iclass.)
Historically, the remaning bits of class were repurposed to pass extra
flags down the structure. These had to be preserved in all recursive
calls, so the functions take apart and reassemble the two halves of
aclass/iclass. The only such flag was ASN1_TFLG_NDEF, which on certain
types, caused OpenSSL to encode indefinite-length encoding. We removed
this in https://boringssl-review.googlesource.com/c/boringssl/+/43889.
Due to these flags, if tag == -1, class should default to zero. However,
X509_NAME's callbacks pass -1, -1, instead of -1, 0, effectively setting
all flags. This wasn't noticed because none of the types below X509_NAME
pay attention to ASN1_TFLG_NDEF.
This CL does two things: First, it unwinds the remainder of the flags
machinery. If we ever need flags, we should pass it as a distinct
argument. Second, it fixes the X509_NAME calls and asserts that -1 is
always paired with 0.
Change-Id: I285a73a06ad16980617fe23d5ea7f260fc5dbf16
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49385
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
These are a little odd with the ASN1_ENCODING paths. And there were some
bugs previously around CHOICE types. Nothing defines them, inside or
outside BoringSSL, so remove them.
Change-Id: Id2954fef8ee9637f36f7511b51dc0adc2557e3ba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49352
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
it->funcs is only an ASN1_AUX for ASN1_ITYPE_SEQUENCE and
ASN1_ITYPE_CHOICE. Fortunately, the other possible types for it->funcs
are larger than ASN1_AUX and we don't touch the result when we
shouldn't, so this is merely a strict aliasing violation.
Change-Id: I29e94249e0b137fe8df0b16254366ae6705c8784
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49351
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See also 006906cddda37e24a66443199444ef4476697477 from OpenSSL, though
this CL uses a different strategy from upstream. Upstream makes
ASN1_item_ex_i2d continue to allow optionals and checks afterwards at
every non-optional call site. This CL pushes down an optional parameter
and says functions cannot omit items unless explicitly allowed.
I think this is a better default, though it is a larger change. Fields
are only optional when they come from an ASN1_TEMPLATE with the OPTIONAL
flag. Upstream's strategy misses top-level calls.
This CL additionally adds checks for optional ASN1_TEMPLATEs in contexts
where it doesn't make sense. Only fields of SEQUENCEs and SETs may be
OPTIONAL, but the ASN1_ITEM/ASN1_TEMPLATE split doesn't quite match
ASN.1 itself. ASN1_TEMPLATE is additionally responsible for
explicit/implicit tagging, and SEQUENCE/SET OF. That means CHOICE arms
and the occasional top-level type (ASN1_ITEM_TEMPLATE) use ASN1_TEMPLATE
but will get confused if marked optional.
As part of this, i2d_FOO(NULL) now returns -1 rather than "successfully"
writing 0 bytes. If we want to allow NULL at the top-level, that's not
too hard to arrange, but our CBB-based i2d functions do not.
Update-Note: Structures with missing mandatory fields can no longer be
encoded. Note that, apart from the cases already handled by preceding
CLs, tasn_new.c will fill in non-NULL empty objects everywhere. The main
downstream impact I've seen of this particular change is in combination
with other bugs. Consider a caller that does:
GENERAL_NAME *name = GENERAL_NAME_new();
name->type = GEN_DNS;
name->d.dNSName = DoSomethingComplicated(...);
Suppose DoSomethingComplicated() was actually fallible and returned
NULL, but the caller forgot to check. They'd now construct a
GENERAL_NAME with a missing field. Previously, this would silently
serialize some garbage (omitted field) or empty string. Now we fail to
encode, but the true error was the uncaught DoSomethingComplicated()
failure. (Which likely was itself a bug.)
Bug: 429
Change-Id: I37fe618761be64a619be9fdc8d416f24ecbb8c46
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49350
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
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