Add an explicit indefinite-length output to CBS_get_any_ber_asn1_element.
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>
diff --git a/crypto/bytestring/ber.c b/crypto/bytestring/ber.c
index e7f67dd..d9b780f 100644
--- a/crypto/bytestring/ber.c
+++ b/crypto/bytestring/ber.c
@@ -69,9 +69,9 @@
CBS contents;
unsigned tag;
size_t header_len;
-
+ int indefinite;
if (!CBS_get_any_ber_asn1_element(&in, &contents, &tag, &header_len,
- ber_found)) {
+ ber_found, &indefinite)) {
return 0;
}
if (*ber_found) {
@@ -119,11 +119,11 @@
CBS contents;
unsigned tag, child_string_tag = string_tag;
size_t header_len;
- int ber_found;
+ int indefinite;
CBB *out_contents, out_contents_storage;
if (!CBS_get_any_ber_asn1_element(in, &contents, &tag, &header_len,
- &ber_found)) {
+ /*out_ber_found=*/NULL, &indefinite)) {
return 0;
}
@@ -153,11 +153,9 @@
out_contents = &out_contents_storage;
}
- if (CBS_len(&contents) == header_len && header_len > 0 &&
- CBS_data(&contents)[header_len - 1] == 0x80) {
- // This is an indefinite length element.
+ if (indefinite) {
if (!cbs_convert_ber(in, out_contents, child_string_tag,
- 1 /* looking for eoc */, depth + 1) ||
+ /*looking_for_eoc=*/1, depth + 1) ||
!CBB_flush(out)) {
return 0;
}
@@ -171,7 +169,7 @@
if (tag & CBS_ASN1_CONSTRUCTED) {
// Recurse into children.
if (!cbs_convert_ber(&contents, out_contents, child_string_tag,
- 0 /* not looking for eoc */, depth + 1)) {
+ /*looking_for_eoc=*/0, depth + 1)) {
return 0;
}
} else {
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 985e38c..77261a3 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -693,29 +693,38 @@
const char *in_hex;
bool ok;
bool ber_found;
+ bool indefinite;
unsigned tag;
};
static const BERTest kBERTests[] = {
- // Trivial cases, also valid DER.
- {"0000", true, false, 0},
- {"0100", true, false, 1},
- {"020101", true, false, 2},
+ // Trivial cases, also valid DER.
+ {"0000", true, false, false, 0},
+ {"0100", true, false, false, 1},
+ {"020101", true, false, false, 2},
- // Non-minimally encoded lengths.
- {"02810101", true, true, 2},
- {"0282000101", true, true, 2},
- {"028300000101", true, true, 2},
- {"02840000000101", true, true, 2},
- // Technically valid BER, but not handled.
- {"02850000000101", false, false, 0},
+ // Non-minimally encoded lengths.
+ {"02810101", true, true, false, 2},
+ {"0282000101", true, true, false, 2},
+ {"028300000101", true, true, false, 2},
+ {"02840000000101", true, true, false, 2},
+ // Technically valid BER, but not handled.
+ {"02850000000101", false, false, false, 0},
- {"0280", false, false, 0}, // Indefinite length, but not constructed.
- {"2280", true, true, CBS_ASN1_CONSTRUCTED | 2}, // Indefinite length.
- {"3f0000", false, false, 0}, // Invalid extended tag zero (X.690 8.1.2.4.2.c)
- {"1f0100", false, false, 0}, // Should be a low-number tag form, even in BER.
- {"1f4000", true, false, 0x40},
- {"1f804000", false, false, 0}, // Non-minimal tags are invalid, even in BER.
+ // Indefinite length, but not constructed.
+ {"0280", false, false, false, 0},
+ // Indefinite length.
+ {"2280", true, true, true, CBS_ASN1_CONSTRUCTED | 2},
+ // Indefinite length with multi-byte tag.
+ {"bf1f80", true, true, true,
+ CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 31},
+ // Invalid extended tag zero (X.690 8.1.2.4.2.c)
+ {"3f0000", false, false, false, 0},
+ // Should be a low-number tag form, even in BER.
+ {"1f0100", false, false, false, 0},
+ {"1f4000", true, false, false, 0x40},
+ // Non-minimal tags are invalid, even in BER.
+ {"1f804000", false, false, false, 0},
};
TEST(CBSTest, BERElementTest) {
@@ -729,14 +738,16 @@
unsigned tag;
size_t header_len;
int ber_found;
- int ok =
- CBS_get_any_ber_asn1_element(&in, &out, &tag, &header_len, &ber_found);
+ int indefinite;
+ int ok = CBS_get_any_ber_asn1_element(&in, &out, &tag, &header_len,
+ &ber_found, &indefinite);
ASSERT_TRUE((ok == 1) == test.ok);
if (!test.ok) {
continue;
}
- EXPECT_TRUE((ber_found == 1) == test.ber_found);
+ EXPECT_EQ(test.ber_found ? 1 : 0, ber_found);
+ EXPECT_EQ(test.indefinite ? 1 : 0, indefinite);
EXPECT_LE(header_len, in_bytes.size());
EXPECT_EQ(CBS_len(&out), in_bytes.size());
EXPECT_EQ(CBS_len(&in), 0u);
diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c
index 803c97a..293e66c 100644
--- a/crypto/bytestring/cbs.c
+++ b/crypto/bytestring/cbs.c
@@ -285,7 +285,7 @@
static int cbs_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
size_t *out_header_len, int *out_ber_found,
- int ber_ok) {
+ int *out_indefinite, int ber_ok) {
CBS header = *cbs;
CBS throwaway;
@@ -294,6 +294,10 @@
}
if (ber_ok) {
*out_ber_found = 0;
+ *out_indefinite = 0;
+ } else {
+ assert(out_ber_found == NULL);
+ assert(out_indefinite == NULL);
}
unsigned tag;
@@ -333,6 +337,7 @@
*out_header_len = header_len;
}
*out_ber_found = 1;
+ *out_indefinite = 1;
return CBS_get_bytes(cbs, out, header_len);
}
@@ -395,16 +400,18 @@
int CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
size_t *out_header_len) {
- return cbs_get_any_asn1_element(cbs, out, out_tag, out_header_len,
- NULL, 0 /* DER only */);
+ return cbs_get_any_asn1_element(cbs, out, out_tag, out_header_len, NULL, NULL,
+ /*ber_ok=*/0);
}
int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag,
- size_t *out_header_len, int *out_ber_found) {
+ size_t *out_header_len, int *out_ber_found,
+ int *out_indefinite) {
int ber_found_temp;
return cbs_get_any_asn1_element(
cbs, out, out_tag, out_header_len,
- out_ber_found ? out_ber_found : &ber_found_temp, 1 /* BER allowed */);
+ out_ber_found ? out_ber_found : &ber_found_temp, out_indefinite,
+ /*ber_ok=*/1);
}
static int cbs_get_asn1(CBS *cbs, CBS *out, unsigned tag_value,
diff --git a/include/openssl/base.h b/include/openssl/base.h
index 983eadc..e8d7994 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -195,7 +195,7 @@
// A consumer may use this symbol in the preprocessor to temporarily build
// against multiple revisions of BoringSSL at the same time. It is not
// recommended to do so for longer than is necessary.
-#define BORINGSSL_API_VERSION 16
+#define BORINGSSL_API_VERSION 17
#if defined(BORINGSSL_SHARED_LIBRARY)
diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h
index 5ef3742..199d89c 100644
--- a/include/openssl/bytestring.h
+++ b/include/openssl/bytestring.h
@@ -259,15 +259,17 @@
// CBS_get_any_ber_asn1_element acts the same as |CBS_get_any_asn1_element| but
// also allows indefinite-length elements to be returned and does not enforce
-// that lengths are minimal. For indefinite-lengths, |*out_header_len| and
+// that lengths are minimal. It sets |*out_indefinite| to one if the length was
+// indefinite and zero otherwise. If indefinite, |*out_header_len| and
// |CBS_len(out)| will be equal as only the header is returned (although this is
-// also true for empty elements so the length must be checked too). If
+// also true for empty elements so |*out_indefinite| should be checked). If
// |out_ber_found| is not NULL then it is set to one if any case of invalid DER
// but valid BER is found, and to zero otherwise.
OPENSSL_EXPORT int CBS_get_any_ber_asn1_element(CBS *cbs, CBS *out,
unsigned *out_tag,
size_t *out_header_len,
- int *out_ber_found);
+ int *out_ber_found,
+ int *out_indefinite);
// CBS_get_asn1_uint64 gets an ASN.1 INTEGER from |cbs| using |CBS_get_asn1|
// and sets |*out| to its value. It returns one on success and zero on error,
