[UNIVERSAL 0] is reserved by X.680 for the encoding to use. BER uses
this to encode indefinite-length EOCs, but it is possible to encode it
in a definite-length element or in a non-EOC form (non-zero length, or
constructed).
Whether we accept such encodings is normally moot: parsers will reject
the tag as unsuitable for the type. However, the ANY type matches all
tags. Previously, we would allow this, but crypto/asn1 has some ad-hoc
checks for unexpected EOCs, in some contexts, but not others.
Generalize this check to simply rejecting [UNIVERSAL 0] in all forms.
This avoids a weird hole in the abstraction where tags are sometimes
representable in BER and sometimes not. It also means we'll preserve
this check when migrating parsers from crypto/asn1.
Update-Note: There are two kinds of impacts I might expect from this
change. The first is BER parsers might be relying on the CBS DER/BER
element parser to pick up EOCs, as our ber.c does. This should be caught
by the most basic unit test and can be fixed by detecting EOCs
externally.
The second is code might be trying to parse "actual" elements with tag
[UNIVERSAL 0]. No actual types use this tag, so any non-ANY field is
already rejecting such inputs. However, it is possible some input has
this tag in a field with type ANY. This CL will cause us to reject that
input. Note, however, that crypto/asn1 already rejects unexpected EOCs
inside sequences, so many cases were already rejected anyway. Such
inputs are also invalid as the ANY should match some actual, unknown
ASN.1 type, and that type cannot use the reserved tag.
Fixed: 455
Change-Id: If42cacc01840439059baa0e67179d0f198234fc4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Having to check for header_len == len and a last byte of 0x80 is
actually unambiguous, but not obvious. Before we supported multi-byte
tags, a two-byte header was always {tag, 0x80}, but now a three-byte
header could be {tag1, tag2, 0x80}. But a 0x80 suffix could also be
{tag, 0x81, 0x80} for a 128-byte definite-length element.
This is unambiguous because header_len == len implies either zero length
or indefinite-length, and it is not possible to encode a definite length
of zero, in BER or DER, with a header that ends in 0x80. Still, rather
than go through all this, we can just report indefinite lengths to the
caller directly.
Update-Note: This is a breaking change to CBS_get_any_ber_asn1_element.
There is only one external caller of this function, and it should be
possible to fix them atomically with this change, so I haven't bothered
introducing another name, etc. (See cl/429632075 for the fix.)
Change-Id: Ic94dab562724fd0b388bc8d2a7a223f21a8da413
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Update-Note: PKCS#7 and PKCS#12 parsers will now reject BER constructed
BIT STRINGs. We were previously misparsing them, as was OpenSSL. Given
how long the incorrect parse has been out there, without anyone noticing
(other parsers handle it correctly), it is unlikely these exist.
Change-Id: I61d317461cc59480dc9f772f88edc7758206d20d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50289
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>
See also 8393de42498f8be75cf0353f5c9f906a43a748d2 from upstream and
CBS-2021-3712. But rather than do that, I've rewritten it with CBS, so
it's a bit clearer. The previous commit added tests.
Change-Id: Ie52e28f07b9bf805c8730eab7be5d40cb5d558b6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49008
Reviewed-by: Adam Langley <agl@google.com>
We fill in placeholder values of all zeros fairly often in TLS now,
as workarounds for messages being constructed in the wrong order.
draft-12 of ECH adds even more of these. Add a helper so we don't need
to interrupt an || chain with a memset.
Change-Id: Id4f9d988ee67598645a01637cc9515b475c1aec2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48909
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
BER permits lengths to be non-minimal. Previously this was not supported
at all. This change brings greater support, allowing non-minimal lengths
so long as they fit in a uint32_t.
Change-Id: I002ed2375c78fdb326e725eb1c23eca71ef9ba4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44684
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We have several implementations of this internally, so consolidate them.
Chromium also has a copy in net/der/parse_values.cc which could call
into this.
(I'm also hoping we can make c2i_ASN1_INTEGER call this and
further tighten up crypto/asn1's parser, but I see Chromium still has an
allow_invalid_serial_numbers option, so perhaps not quite yet.)
Update-Note: This CL does not change behavior, but I'm leaving a note to
myself to make net/der/parse_values.cc call the new functions.
Change-Id: If2aae6574ba6a30e343e1308da6af543616156ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44051
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley