From 0a78b6201304962cac977a9cc29d4cc591c35a6a Mon Sep 17 00:00:00 2001 From: jsing Date: Sun, 13 Mar 2022 14:58:14 +0000 Subject: [PATCH] Remove free_cont from asn1_d2i_ex_primitive()/asn1_ex_c2i(). The constructed ASN.1 handling in asn1_d2i_ex_primitive() and asn1_ex_c2i() currently has code to potentially avoid a malloc/memcpy - this is a less common code path and it introduces a bunch of complexity for minimal gain. In particular, we're manually adding a trailing NUL when ASN1_STRING_set() would already do that for us, plus we currently manually free() the data on an ASN1_STRING, rather than using freezero(). ok inoguchi@ tb@ --- lib/libcrypto/asn1/asn1_locl.h | 4 +- lib/libcrypto/asn1/tasn_dec.c | 77 +++++++++++++--------------------- 2 files changed, 31 insertions(+), 50 deletions(-) diff --git a/lib/libcrypto/asn1/asn1_locl.h b/lib/libcrypto/asn1/asn1_locl.h index bb6a9fc91ae..9a29a2b13fa 100644 --- a/lib/libcrypto/asn1/asn1_locl.h +++ b/lib/libcrypto/asn1/asn1_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: asn1_locl.h,v 1.21 2022/03/02 11:28:00 jsing Exp $ */ +/* $OpenBSD: asn1_locl.h,v 1.22 2022/03/13 14:58:14 jsing Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2006. */ @@ -161,8 +161,6 @@ struct x509_crl_method_st { int (*crl_verify)(X509_CRL *crl, EVP_PKEY *pk); }; -int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, char *free_cont, const ASN1_ITEM *it); - int asn1_get_choice_selector(ASN1_VALUE **pval, const ASN1_ITEM *it); int asn1_set_choice_selector(ASN1_VALUE **pval, int value, const ASN1_ITEM *it); diff --git a/lib/libcrypto/asn1/tasn_dec.c b/lib/libcrypto/asn1/tasn_dec.c index 8f41deef8db..d475c996761 100644 --- a/lib/libcrypto/asn1/tasn_dec.c +++ b/lib/libcrypto/asn1/tasn_dec.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tasn_dec.c,v 1.48 2022/01/07 12:24:17 tb Exp $ */ +/* $OpenBSD: tasn_dec.c,v 1.49 2022/03/13 14:58:14 jsing Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2000. */ @@ -95,6 +95,8 @@ static int asn1_template_noexp_d2i(ASN1_VALUE **val, const unsigned char **in, static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in, long len, const ASN1_ITEM *it, int tag, int aclass, char opt, ASN1_TLC *ctx); +static int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *content, int len, + int utype, const ASN1_ITEM *it); static void asn1_tlc_invalidate(ASN1_TLC *ctx) @@ -662,9 +664,9 @@ asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in, long inlen, { int ret = 0, utype; long plen; - char cst, inf, free_cont = 0; + char cst, inf; const unsigned char *p; - const unsigned char *cont = NULL; + const unsigned char *content = NULL; uint8_t *data = NULL; size_t data_len = 0; CBB cbb; @@ -732,14 +734,14 @@ asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in, long inlen, return 0; } - cont = *in; + content = *in; /* If indefinite length constructed find the real end */ if (inf) { if (!asn1_find_end(&p, plen, inf)) goto err; - len = p - cont; + len = p - content; } else { - len = p - cont + plen; + len = p - content + plen; p += plen; } } else if (cst) { @@ -754,29 +756,22 @@ asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in, long inlen, goto err; if (!asn1_collect(&cbb, &p, plen, inf, -1, V_ASN1_UNIVERSAL, 0)) goto err; - - /* Append a final NUL to string. */ - if (!CBB_add_u8(&cbb, 0)) - goto err; - if (!CBB_finish(&cbb, &data, &data_len)) goto err; - free_cont = 1; - - if (data_len < 1 || data_len > LONG_MAX) + if (data_len > LONG_MAX) goto err; - cont = data; - len = data_len - 1; + content = data; + len = data_len; } else { - cont = p; + content = p; len = plen; p += plen; } /* We now have content length and type: translate into a structure */ - if (!asn1_ex_c2i(pval, cont, len, utype, &free_cont, it)) + if (!asn1_ex_c2i(pval, content, len, utype, it)) goto err; *in = p; @@ -784,18 +779,16 @@ asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in, long inlen, err: CBB_cleanup(&cbb); - - if (free_cont) - freezero(data, data_len); + freezero(data, data_len); return ret; } /* Translate ASN1 content octets into a structure */ -int -asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, - char *free_cont, const ASN1_ITEM *it) +static int +asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *content, int len, int utype, + const ASN1_ITEM *it) { ASN1_VALUE **opval = NULL; ASN1_STRING *stmp; @@ -805,10 +798,11 @@ asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, if (it->funcs != NULL) { const ASN1_PRIMITIVE_FUNCS *pf = it->funcs; + char free_content = 0; if (pf->prim_c2i == NULL) return 0; - return pf->prim_c2i(pval, cont, len, utype, free_cont, it); + return pf->prim_c2i(pval, content, len, utype, &free_content, it); } /* If ANY type clear type and set pointer to internal value */ @@ -828,7 +822,7 @@ asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, } switch (utype) { case V_ASN1_OBJECT: - if (!c2i_ASN1_OBJECT((ASN1_OBJECT **)pval, &cont, len)) + if (!c2i_ASN1_OBJECT((ASN1_OBJECT **)pval, &content, len)) goto err; break; @@ -847,19 +841,19 @@ asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, } else { ASN1_BOOLEAN *tbool; tbool = (ASN1_BOOLEAN *)pval; - *tbool = *cont; + *tbool = *content; } break; case V_ASN1_BIT_STRING: - if (!c2i_ASN1_BIT_STRING((ASN1_BIT_STRING **)pval, &cont, len)) + if (!c2i_ASN1_BIT_STRING((ASN1_BIT_STRING **)pval, &content, len)) goto err; break; case V_ASN1_INTEGER: case V_ASN1_ENUMERATED: tint = (ASN1_INTEGER **)pval; - if (!c2i_ASN1_INTEGER(tint, &cont, len)) + if (!c2i_ASN1_INTEGER(tint, &content, len)) goto err; /* Fixup type to match the expected form */ (*tint)->type = utype | ((*tint)->type & V_ASN1_NEG); @@ -891,10 +885,9 @@ asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, ASN1error(ASN1_R_UNIVERSALSTRING_IS_WRONG_LENGTH); goto err; } - /* All based on ASN1_STRING and handled the same */ - if (!*pval) { - stmp = ASN1_STRING_type_new(utype); - if (!stmp) { + /* All based on ASN1_STRING and handled the same way. */ + if (*pval == NULL) { + if ((stmp = ASN1_STRING_type_new(utype)) == NULL) { ASN1error(ERR_R_MALLOC_FAILURE); goto err; } @@ -903,19 +896,10 @@ asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, stmp = (ASN1_STRING *)*pval; stmp->type = utype; } - /* If we've already allocated a buffer use it */ - if (*free_cont) { - free(stmp->data); - stmp->data = (unsigned char *)cont; /* UGLY CAST! RL */ - stmp->length = len; - *free_cont = 0; - } else { - if (!ASN1_STRING_set(stmp, cont, len)) { - ASN1error(ERR_R_MALLOC_FAILURE); - ASN1_STRING_free(stmp); - *pval = NULL; - goto err; - } + if (!ASN1_STRING_set(stmp, content, len)) { + ASN1_STRING_free(stmp); + *pval = NULL; + goto err; } break; } @@ -934,7 +918,6 @@ asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, int utype, return ret; } - /* This function finds the end of an ASN1 structure when passed its maximum * length, whether it is indefinite length and a pointer to the content. * This is more efficient than calling asn1_collect because it does not -- 2.20.1