Unwind remnants of ASN1_TFLG_NDEF.
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>
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index e30be16..a4bd34e 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -126,12 +126,8 @@
/* ASN1_item_ex_i2d encodes |*pval| as a value of type |it| to |out| under the
* i2d output convention. It returns a non-zero length on success and -1 on
* error. If |tag| is -1. the tag and class come from |it|. Otherwise, the tag
- * number is |tag| and the class is the |ASN1_TFLG_TAG_CLASS| bits of |aclass|.
- * This is used for implicit tagging. This function treats a missing value as an
- * error, not an optional field.
- *
- * TODO(davidben): Historically, |aclass| contained other flags, but we may have
- * removed the last of them. */
+ * number is |tag| and the class is |aclass|. This is used for implicit tagging.
+ * This function treats a missing value as an error, not an optional field. */
int ASN1_item_ex_i2d(ASN1_VALUE **pval, unsigned char **out,
const ASN1_ITEM *it, int tag, int aclass);
diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c
index 4ccacab..9917d2a 100644
--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -75,8 +75,7 @@
static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cont, int *out_omit,
int *putype, const ASN1_ITEM *it);
static int asn1_set_seq_out(STACK_OF(ASN1_VALUE) *sk, unsigned char **out,
- int skcontlen, const ASN1_ITEM *item,
- int do_sort, int iclass);
+ int skcontlen, const ASN1_ITEM *item, int do_sort);
static int asn1_template_ex_i2d(ASN1_VALUE **pval, unsigned char **out,
const ASN1_TEMPLATE *tt, int tag, int aclass);
@@ -132,6 +131,12 @@
const ASN1_TEMPLATE *tt = NULL;
int i, seqcontlen, seqlen;
+ /* Historically, |aclass| was repurposed to pass additional flags into the
+ * encoding process. */
+ assert((aclass & ASN1_TFLG_TAG_CLASS) == aclass);
+ /* If not overridding the tag, |aclass| is ignored and should be zero. */
+ assert(tag != -1 || aclass == 0);
+
/* All fields are pointers, except for boolean |ASN1_ITYPE_PRIMITIVE|s.
* Optional primitives are handled later. */
if ((it->itype != ASN1_ITYPE_PRIMITIVE) && !*pval) {
@@ -163,7 +168,7 @@
OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE);
return -1;
}
- return asn1_i2d_ex_primitive(pval, out, it, -1, aclass, optional);
+ return asn1_i2d_ex_primitive(pval, out, it, -1, 0, optional);
case ASN1_ITYPE_CHOICE: {
/*
@@ -185,7 +190,7 @@
return -1;
}
ASN1_VALUE **pchval = asn1_get_field_ptr(pval, chtt);
- return asn1_template_ex_i2d(pchval, out, chtt, -1, aclass);
+ return asn1_template_ex_i2d(pchval, out, chtt, -1, 0);
}
case ASN1_ITYPE_EXTERN: {
@@ -215,9 +220,7 @@
/* If no IMPLICIT tagging set to SEQUENCE, UNIVERSAL */
if (tag == -1) {
tag = V_ASN1_SEQUENCE;
- /* Retain any other flags in aclass */
- aclass = (aclass & ~ASN1_TFLG_TAG_CLASS)
- | V_ASN1_UNIVERSAL;
+ aclass = V_ASN1_UNIVERSAL;
}
/* First work out sequence content length */
for (i = 0, tt = it->templates; i < it->tcount; tt++, i++) {
@@ -228,7 +231,7 @@
if (!seqtt)
return -1;
pseqval = asn1_get_field_ptr(pval, seqtt);
- tmplen = asn1_template_ex_i2d(pseqval, NULL, seqtt, -1, aclass);
+ tmplen = asn1_template_ex_i2d(pseqval, NULL, seqtt, -1, 0);
if (tmplen == -1 || (tmplen > INT_MAX - seqcontlen))
return -1;
seqcontlen += tmplen;
@@ -246,7 +249,7 @@
if (!seqtt)
return -1;
pseqval = asn1_get_field_ptr(pval, seqtt);
- if (asn1_template_ex_i2d(pseqval, out, seqtt, -1, aclass) < 0) {
+ if (asn1_template_ex_i2d(pseqval, out, seqtt, -1, 0) < 0) {
return -1;
}
}
@@ -269,12 +272,17 @@
int i, ret, flags, ttag, tclass;
size_t j;
flags = tt->flags;
+
+ /* Historically, |iclass| was repurposed to pass additional flags into the
+ * encoding process. */
+ assert((iclass & ASN1_TFLG_TAG_CLASS) == iclass);
+ /* If not overridding the tag, |iclass| is ignored and should be zero. */
+ assert(tag != -1 || iclass == 0);
+
/*
* Work out tag and class to use: tagging may come either from the
* template or the arguments, not both because this would create
- * ambiguity. Additionally the iclass argument may contain some
- * additional flags which should be noted and passed down to other
- * levels.
+ * ambiguity.
*/
if (flags & ASN1_TFLG_TAG_MASK) {
/* Error if argument and template tagging */
@@ -293,16 +301,12 @@
ttag = -1;
tclass = 0;
}
- /*
- * Remove any class mask from iflag.
- */
- iclass &= ~ASN1_TFLG_TAG_CLASS;
const int optional = (flags & ASN1_TFLG_OPTIONAL) != 0;
/*
- * At this point 'ttag' contains the outer tag to use, 'tclass' is the
- * class and iclass is any flags passed to this function.
+ * At this point 'ttag' contains the outer tag to use, and 'tclass' is the
+ * class.
*/
if (flags & ASN1_TFLG_SK_MASK) {
@@ -350,7 +354,7 @@
int tmplen;
skitem = sk_ASN1_VALUE_value(sk, j);
tmplen = ASN1_item_ex_i2d(&skitem, NULL, ASN1_ITEM_ptr(tt->item),
- -1, iclass);
+ -1, 0);
if (tmplen == -1 || (skcontlen > INT_MAX - tmplen))
return -1;
skcontlen += tmplen;
@@ -375,7 +379,7 @@
ASN1_put_object(out, /*constructed=*/1, skcontlen, sktag, skaclass);
/* And the stuff itself */
if (!asn1_set_seq_out(sk, out, skcontlen, ASN1_ITEM_ptr(tt->item),
- isset, iclass)) {
+ isset)) {
return -1;
}
return ret;
@@ -384,8 +388,8 @@
if (flags & ASN1_TFLG_EXPTAG) {
/* EXPLICIT tagging */
/* Find length of tagged item */
- i = asn1_item_ex_i2d_opt(pval, NULL, ASN1_ITEM_ptr(tt->item), -1,
- iclass, optional);
+ i = asn1_item_ex_i2d_opt(pval, NULL, ASN1_ITEM_ptr(tt->item), -1, 0,
+ optional);
if (i <= 0)
return i;
/* Find length of EXPLICIT tag */
@@ -394,16 +398,16 @@
/* Output tag and item */
ASN1_put_object(out, /*constructed=*/1, i, ttag, tclass);
if (ASN1_item_ex_i2d(pval, out, ASN1_ITEM_ptr(tt->item), -1,
- iclass) < 0) {
+ 0) < 0) {
return -1;
}
}
return ret;
}
- /* Either normal or IMPLICIT tagging: combine class and flags */
+ /* Either normal or IMPLICIT tagging */
return asn1_item_ex_i2d_opt(pval, out, ASN1_ITEM_ptr(tt->item),
- ttag, tclass | iclass, optional);
+ ttag, tclass, optional);
}
@@ -428,21 +432,16 @@
/* asn1_set_seq_out writes |sk| to |out| under the i2d output convention,
* excluding the tag and length. It returns one on success and zero on error.
* |skcontlen| must be the total encoded size. If |do_sort| is non-zero, the
- * elements are sorted for a SET OF type. Each element of |sk| has type |item|.
- * |iclass| contains flags for encoding elements of |sk|.
- *
- * TODO(davidben): After |ASN1_TFLG_NDEF| was removed, no more flags are passed
- * into |iclass|. However, due to a bug in x_name.c, we cannot assert |iclass|
- * is zero. Fix that, then unwind the flags. */
+ * elements are sorted for a SET OF type. Each element of |sk| has type
+ * |item|. */
static int asn1_set_seq_out(STACK_OF(ASN1_VALUE) *sk, unsigned char **out,
- int skcontlen, const ASN1_ITEM *item,
- int do_sort, int iclass)
+ int skcontlen, const ASN1_ITEM *item, int do_sort)
{
/* No need to sort if there are fewer than two items. */
if (!do_sort || sk_ASN1_VALUE_num(sk) < 2) {
for (size_t i = 0; i < sk_ASN1_VALUE_num(sk); i++) {
ASN1_VALUE *skitem = sk_ASN1_VALUE_value(sk, i);
- if (ASN1_item_ex_i2d(&skitem, out, item, -1, iclass) < 0) {
+ if (ASN1_item_ex_i2d(&skitem, out, item, -1, 0) < 0) {
return 0;
}
}
@@ -467,7 +466,7 @@
for (size_t i = 0; i < sk_ASN1_VALUE_num(sk); i++) {
ASN1_VALUE *skitem = sk_ASN1_VALUE_value(sk, i);
encoded[i].data = p;
- encoded[i].length = ASN1_item_ex_i2d(&skitem, &p, item, -1, iclass);
+ encoded[i].length = ASN1_item_ex_i2d(&skitem, &p, item, -1, 0);
if (encoded[i].length < 0) {
goto err;
}
diff --git a/crypto/x509/x_name.c b/crypto/x509/x_name.c
index 348d31b..4fea082 100644
--- a/crypto/x509/x_name.c
+++ b/crypto/x509/x_name.c
@@ -303,16 +303,17 @@
goto memerr;
}
ASN1_VALUE *intname_val = (ASN1_VALUE *)intname;
- len = ASN1_item_ex_i2d(&intname_val, NULL,
- ASN1_ITEM_rptr(X509_NAME_INTERNAL), -1, -1);
+ len =
+ ASN1_item_ex_i2d(&intname_val, NULL, ASN1_ITEM_rptr(X509_NAME_INTERNAL),
+ /*tag=*/-1, /*aclass=*/0);
if (len <= 0) {
- goto err;
+ goto err;
}
if (!BUF_MEM_grow(a->bytes, len))
goto memerr;
p = (unsigned char *)a->bytes->data;
- if (ASN1_item_ex_i2d(&intname_val,
- &p, ASN1_ITEM_rptr(X509_NAME_INTERNAL), -1, -1) <= 0) {
+ if (ASN1_item_ex_i2d(&intname_val, &p, ASN1_ITEM_rptr(X509_NAME_INTERNAL),
+ /*tag=*/-1, /*aclass=*/0) <= 0) {
goto err;
}
sk_STACK_OF_X509_NAME_ENTRY_pop_free(intname,
@@ -506,8 +507,8 @@
len = 0;
for (i = 0; i < sk_ASN1_VALUE_num(intname); i++) {
v = sk_ASN1_VALUE_value(intname, i);
- ltmp = ASN1_item_ex_i2d(&v, in,
- ASN1_ITEM_rptr(X509_NAME_ENTRIES), -1, -1);
+ ltmp = ASN1_item_ex_i2d(&v, in, ASN1_ITEM_rptr(X509_NAME_ENTRIES),
+ /*tag=*/-1, /*aclass=*/0);
if (ltmp < 0)
return ltmp;
len += ltmp;
