Make ASN1_EXTERN_FUNCS's parse callback CBS-based
Having to keep flipping back and forth between the calling conventions
when bridging rewritten types is tedious.
This does not rewrite the core of tasn_dec.cc (although at this point
much of it is already CBS-based), but it does suggest a calling
convention for it. Right now, the internal calling convention is the
double-pointer thing from d2i, with an extra twist that functions must
return 0 for error, 1 for success, and -1 for "success but parsed
nothing".
This proposes a CBS in/out param to return how bytes we consumed, and a
plain success/error return value. The -1 case can be modeled as
successfully consuming zero bytes.
For now, we still have to bridge the two inside tasn_dec.cc. Also for
this CL I have not yet rewritten the messy X509_NAME parser, or the
tasn_dec.cc parser.
There's also a messy situation where this object sometimes is called
with an existing object already there, and sometimes without one. I
don't see a good way to avoid that one, but hopefully it'll become less
and as important as we stop using the tasn system.
Bug: 42290418
Change-Id: I3969e19ade81aedf449b4baf139fe5f5b1dd867b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81776
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 74657e0..8eca02c 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -291,8 +291,18 @@
long length);
typedef int ASN1_i2d_func(ASN1_VALUE *a, unsigned char **in);
-typedef int ASN1_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len,
- const ASN1_ITEM *it, int opt, ASN1_TLC *ctx);
+// An ASN1_ex_parse function should parse a value from |cbs| and set |*pval| to
+// the result. It should return one on success and zero on failure. If |opt| is
+// non-zero, the field may be optional. If an optional element is missing, the
+// function should return one and consume zero bytes from |cbs|.
+//
+// If |opt| is non-zero, the function can assume that |*pval| is nullptr on
+// entry. Otherwise, |*pval| may either be nullptr, or the result of
+// |ASN1_ex_new_func|. The function may either write into the existing object,
+// if any, or unconditionally make a new one. (The existing object comes from
+// tasn_new.cc recursively filling in objects before parsing into them.)
+typedef int ASN1_ex_parse(ASN1_VALUE **pval, CBS *cbs, const ASN1_ITEM *it,
+ int opt);
typedef int ASN1_ex_i2d(ASN1_VALUE **pval, unsigned char **out,
const ASN1_ITEM *it);
@@ -302,7 +312,7 @@
typedef struct ASN1_EXTERN_FUNCS_st {
ASN1_ex_new_func *asn1_ex_new;
ASN1_ex_free_func *asn1_ex_free;
- ASN1_ex_d2i *asn1_ex_d2i;
+ ASN1_ex_parse *asn1_ex_parse;
ASN1_ex_i2d *asn1_ex_i2d;
} ASN1_EXTERN_FUNCS;
diff --git a/crypto/asn1/tasn_dec.cc b/crypto/asn1/tasn_dec.cc
index 1d2b34b..2f53a79 100644
--- a/crypto/asn1/tasn_dec.cc
+++ b/crypto/asn1/tasn_dec.cc
@@ -140,6 +140,10 @@
if (!pval) {
return 0;
}
+ if (len < 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_BUFFER_TOO_SMALL);
+ goto err;
+ }
if (buf != NULL) {
assert(CRYPTO_BUFFER_data(buf) <= *in &&
@@ -217,7 +221,18 @@
}
const ASN1_EXTERN_FUNCS *ef =
reinterpret_cast<const ASN1_EXTERN_FUNCS *>(it->funcs);
- return ef->asn1_ex_d2i(pval, in, len, it, opt, NULL);
+ CBS cbs;
+ CBS_init(&cbs, *in, len);
+ CBS copy = cbs;
+ if (!ef->asn1_ex_parse(pval, &cbs, it, opt)) {
+ goto err;
+ }
+ *in = CBS_data(&cbs);
+ // Check whether the function skipped an optional element.
+ //
+ // TODO(crbug.com/42290418): Switch the rest of this function to
+ // |asn1_ex_parse|'s calling convention.
+ return CBS_len(&cbs) == CBS_len(©) ? -1 : 1;
}
case ASN1_ITYPE_CHOICE: {
diff --git a/crypto/x509/x_name.cc b/crypto/x509/x_name.cc
index 8597375..6dad6a2 100644
--- a/crypto/x509/x_name.cc
+++ b/crypto/x509/x_name.cc
@@ -38,15 +38,6 @@
#define X509_NAME_MAX (1024 * 1024)
-static int x509_name_ex_d2i(ASN1_VALUE **val, const unsigned char **in,
- long len, const ASN1_ITEM *it, int opt,
- ASN1_TLC *ctx);
-
-static int x509_name_ex_i2d(ASN1_VALUE **val, unsigned char **out,
- const ASN1_ITEM *it);
-static int x509_name_ex_new(ASN1_VALUE **val, const ASN1_ITEM *it);
-static void x509_name_ex_free(ASN1_VALUE **val, const ASN1_ITEM *it);
-
static int x509_name_encode(X509_NAME *a);
static int x509_name_canon(X509_NAME *a);
static int asn1_string_canon(ASN1_STRING *out, ASN1_STRING *in);
@@ -77,19 +68,8 @@
// representing the ASN1. Unfortunately X509_NAME uses a completely different
// form and caches encodings so we have to process the internal form and
// convert to the external form.
-
-static const ASN1_EXTERN_FUNCS x509_name_ff = {
- x509_name_ex_new,
- x509_name_ex_free,
- x509_name_ex_d2i,
- x509_name_ex_i2d,
-};
-
-IMPLEMENT_EXTERN_ASN1(X509_NAME, x509_name_ff)
-
-IMPLEMENT_ASN1_FUNCTIONS(X509_NAME)
-
-IMPLEMENT_ASN1_DUP_FUNCTION(X509_NAME)
+//
+// TODO(crbug.com/42290417): Rewrite all this with |CBS| and |CBB|.
static int x509_name_ex_new(ASN1_VALUE **val, const ASN1_ITEM *it) {
X509_NAME *ret = NULL;
@@ -143,29 +123,37 @@
sk_X509_NAME_ENTRY_pop_free(ne, X509_NAME_ENTRY_free);
}
-static int x509_name_ex_d2i(ASN1_VALUE **val, const unsigned char **in,
- long len, const ASN1_ITEM *it, int opt,
- ASN1_TLC *ctx) {
- const unsigned char *p = *in, *q;
+static int x509_name_ex_parse(ASN1_VALUE **val, CBS *cbs, const ASN1_ITEM *it,
+ int opt) {
+ if (opt && !CBS_peek_asn1_tag(cbs, CBS_ASN1_SEQUENCE)) {
+ return 1;
+ }
+
+ CBS elem;
+ if (!CBS_get_asn1_element(cbs, &elem, CBS_ASN1_SEQUENCE) ||
+ // Bound the size of an X509_NAME we are willing to parse.
+ CBS_len(&elem) > X509_NAME_MAX) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
+ return 0;
+ }
+
+ // TODO(crbug.com/42290417): Rewrite the parser and canonicalization code with
+ // CBS and CBB. For now this calls into the original two-layer d2i code.
+ long len = static_cast<long>(CBS_len(&elem));
+ const unsigned char *p = CBS_data(&elem), *q;
STACK_OF(STACK_OF_X509_NAME_ENTRY) *intname = NULL;
X509_NAME *nm = NULL;
size_t i, j;
- int ret;
STACK_OF(X509_NAME_ENTRY) *entries;
X509_NAME_ENTRY *entry;
- // Bound the size of an X509_NAME we are willing to parse.
- if (len > X509_NAME_MAX) {
- len = X509_NAME_MAX;
- }
q = p;
// Get internal representation of Name
ASN1_VALUE *intname_val = NULL;
- ret = ASN1_item_ex_d2i(&intname_val, &p, len,
- ASN1_ITEM_rptr(X509_NAME_INTERNAL), /*tag=*/-1,
- /*aclass=*/0, opt, /*buf=*/NULL);
- if (ret <= 0) {
- return ret;
+ if (ASN1_item_ex_d2i(&intname_val, &p, len,
+ ASN1_ITEM_rptr(X509_NAME_INTERNAL), /*tag=*/-1,
+ /*aclass=*/0, /*opt=*/0, /*buf=*/NULL) <= 0) {
+ return 0;
}
intname = (STACK_OF(STACK_OF_X509_NAME_ENTRY) *)intname_val;
@@ -195,15 +183,13 @@
(void)sk_X509_NAME_ENTRY_set(entries, j, NULL);
}
}
- ret = x509_name_canon(nm);
- if (!ret) {
+ if (!x509_name_canon(nm)) {
goto err;
}
sk_STACK_OF_X509_NAME_ENTRY_pop_free(intname, local_sk_X509_NAME_ENTRY_free);
nm->modified = 0;
*val = (ASN1_VALUE *)nm;
- *in = p;
- return ret;
+ return 1;
err:
X509_NAME_free(nm);
sk_STACK_OF_X509_NAME_ENTRY_pop_free(intname,
@@ -226,6 +212,12 @@
return ret;
}
+static const ASN1_EXTERN_FUNCS x509_name_ff = {
+ x509_name_ex_new, x509_name_ex_free, x509_name_ex_parse, x509_name_ex_i2d};
+IMPLEMENT_EXTERN_ASN1(X509_NAME, x509_name_ff)
+IMPLEMENT_ASN1_FUNCTIONS(X509_NAME)
+IMPLEMENT_ASN1_DUP_FUNCTION(X509_NAME)
+
static int x509_name_encode(X509_NAME *a) {
int len;
unsigned char *p;
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc
index a2a5bdc..a110368 100644
--- a/crypto/x509/x_x509.cc
+++ b/crypto/x509/x_x509.cc
@@ -215,25 +215,17 @@
*pval = NULL;
}
-static int x509_d2i_cb(ASN1_VALUE **pval, const unsigned char **in, long len,
- const ASN1_ITEM *it, int opt, ASN1_TLC *ctx) {
- if (len < 0) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_BUFFER_TOO_SMALL);
+static int x509_parse_cb(ASN1_VALUE **pval, CBS *cbs, const ASN1_ITEM *it,
+ int opt) {
+ if (opt && !CBS_peek_asn1_tag(cbs, CBS_ASN1_SEQUENCE)) {
+ return 1;
+ }
+
+ X509 *ret = x509_parse(cbs, nullptr);
+ if (ret == nullptr) {
return 0;
}
- CBS cbs;
- CBS_init(&cbs, *in, len);
- if (opt && !CBS_peek_asn1_tag(&cbs, CBS_ASN1_SEQUENCE)) {
- return -1;
- }
-
- X509 *ret = x509_parse(&cbs, NULL);
- if (ret == NULL) {
- return 0;
- }
-
- *in = CBS_data(&cbs);
X509_free((X509 *)*pval);
*pval = (ASN1_VALUE *)ret;
return 1;
@@ -244,13 +236,8 @@
return i2d_X509((X509 *)*pval, out);
}
-static const ASN1_EXTERN_FUNCS x509_extern_funcs = {
- x509_new_cb,
- x509_free_cb,
- x509_d2i_cb,
- x509_i2d_cb,
-};
-
+static const ASN1_EXTERN_FUNCS x509_extern_funcs = {x509_new_cb, x509_free_cb,
+ x509_parse_cb, x509_i2d_cb};
IMPLEMENT_EXTERN_ASN1(X509, x509_extern_funcs)
X509 *X509_dup(X509 *x509) {
