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) {