Break out a few more helper functions for supported crypto/asn1 types It's all currently the same, but sometimes we're 8-bit-clean because all byte strings are valid, and sometimes it's because we don't check things we should. Also this way we always handle the type field in the helper function. Bug: 42290418 Change-Id: Ia9ebc9bfa978db6fa0dda2f21b06214faa788a4d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/97157 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/crypto/asn1/a_type.cc b/crypto/asn1/a_type.cc index c0f4746..3c58812 100644 --- a/crypto/asn1/a_type.cc +++ b/crypto/asn1/a_type.cc
@@ -313,7 +313,9 @@ case CBS_ASN1_GENERALIZEDTIME: return asn1_parse_generalized_time(&elem, out, tag); case CBS_ASN1_OCTETSTRING: + return asn1_parse_octet_string(&elem, out, tag); case CBS_ASN1_T61STRING: + return asn1_parse_t61_string(&elem, out, tag); case CBS_ASN1_IA5STRING: case CBS_ASN1_NUMERICSTRING: case CBS_ASN1_PRINTABLESTRING: @@ -321,16 +323,11 @@ case CBS_ASN1_GRAPHICSTRING: case CBS_ASN1_VISIBLESTRING: case CBS_ASN1_GENERALSTRING: - // T61String is parsed as Latin-1, so all byte strings are valid. The - // others we currently do not enforce. - // - // TODO(crbug.com/42290290): Enforce the encoding of the other string - // types. - if (!asn1_parse_octet_string(&elem, out, tag)) { - return 0; - } - out->type = static_cast<int>(tag); - return 1; + // |CBS_ASN1_*| and |V_ASN1_*| constants match for universal types. + static_assert(CBS_ASN1_IA5STRING == V_ASN1_IA5STRING); + // TODO(crbug.com/42290290): Enforce the encoding of these string types. + return asn1_parse_string_unchecked(&elem, out, static_cast<int>(tag), + tag); default: // All unrecognized types, or types that cannot be represented as // `ASN1_STRING`, are represented as the whole element.
diff --git a/crypto/asn1/asn1_lib.cc b/crypto/asn1/asn1_lib.cc index 0a05db4e..22c52d1 100644 --- a/crypto/asn1/asn1_lib.cc +++ b/crypto/asn1/asn1_lib.cc
@@ -14,6 +14,7 @@ #include <openssl/asn1.h> +#include <assert.h> #include <limits.h> #include <string.h> @@ -368,9 +369,9 @@ return str->data; } -int bssl::asn1_parse_octet_string(CBS *cbs, ASN1_STRING *out, - CBS_ASN1_TAG tag) { - tag = tag == 0 ? CBS_ASN1_OCTETSTRING : tag; +int bssl::asn1_parse_string_unchecked(CBS *cbs, ASN1_STRING *out, int str_type, + CBS_ASN1_TAG tag) { + assert(tag != 0); CBS child; if (!CBS_get_asn1(cbs, &child, tag)) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); @@ -379,10 +380,24 @@ if (!ASN1_STRING_set(out, CBS_data(&child), CBS_len(&child))) { return 0; } - out->type = V_ASN1_OCTET_STRING; + out->type = str_type; return 1; } +int bssl::asn1_parse_octet_string(CBS *cbs, ASN1_STRING *out, + CBS_ASN1_TAG tag) { + // Nothing to check for OCTET STRING. + tag = tag == 0 ? CBS_ASN1_OCTETSTRING : tag; + return asn1_parse_string_unchecked(cbs, out, V_ASN1_OCTET_STRING, tag); +} + +int bssl::asn1_parse_t61_string(CBS *cbs, ASN1_STRING *out, CBS_ASN1_TAG tag) { + // Nothing to check for T61String. We parse it as Latin-1 and all byte strings + // are valid Latin-1. + tag = tag == 0 ? CBS_ASN1_T61STRING : tag; + return asn1_parse_string_unchecked(cbs, out, V_ASN1_T61STRING, tag); +} + int bssl::asn1_marshal_octet_string(CBB *out, const ASN1_STRING *in, CBS_ASN1_TAG tag) { tag = tag == 0 ? CBS_ASN1_OCTETSTRING : tag;
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h index d774c9a..ee40a75 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h
@@ -104,6 +104,11 @@ // freeing `str` itself. void asn1_string_cleanup(ASN1_STRING *str); +// asn1_parse_string_unchecked parses a DER-encoded string of type `str_type`, +// tagged with `tag`. It does not check the contents. +int asn1_parse_string_unchecked(CBS *cbs, ASN1_STRING *out, int str_type, + CBS_ASN1_TAG tag); + // The following functions parse a DER-encoded ASN.1 value of the specified // type from `cbs` and write the result to `*out`. If `tag` is non-zero, the // value is implicitly tagged with `tag`. On success, they return one and @@ -113,6 +118,7 @@ int asn1_parse_integer(CBS *cbs, ASN1_INTEGER *out, CBS_ASN1_TAG tag); int asn1_parse_enumerated(CBS *cbs, ASN1_ENUMERATED *out, CBS_ASN1_TAG tag); int asn1_parse_octet_string(CBS *cbs, ASN1_STRING *out, CBS_ASN1_TAG tag); +int asn1_parse_t61_string(CBS *cbs, ASN1_STRING *out, CBS_ASN1_TAG tag); int asn1_parse_bmp_string(CBS *cbs, ASN1_BMPSTRING *out, CBS_ASN1_TAG tag); int asn1_parse_universal_string(CBS *cbs, ASN1_UNIVERSALSTRING *out, CBS_ASN1_TAG tag);
diff --git a/crypto/asn1/tasn_dec.cc b/crypto/asn1/tasn_dec.cc index 9107e2e..d2821ff 100644 --- a/crypto/asn1/tasn_dec.cc +++ b/crypto/asn1/tasn_dec.cc
@@ -768,24 +768,19 @@ case V_ASN1_GENERALIZEDTIME: return asn1_parse_generalized_time(cbs, str, cbs_tag); case V_ASN1_OCTET_STRING: + return asn1_parse_octet_string(cbs, str, cbs_tag); + case V_ASN1_T61STRING: + return asn1_parse_t61_string(cbs, str, cbs_tag); case V_ASN1_NUMERICSTRING: case V_ASN1_PRINTABLESTRING: - case V_ASN1_T61STRING: case V_ASN1_VIDEOTEXSTRING: case V_ASN1_IA5STRING: case V_ASN1_GRAPHICSTRING: case V_ASN1_VISIBLESTRING: case V_ASN1_GENERALSTRING: - // T61String is parsed as Latin-1, so all byte strings are valid. The - // others we currently do not enforce. - // // TODO(crbug.com/42290290): Enforce the encoding of the other string // types. - if (!asn1_parse_octet_string(cbs, str, cbs_tag)) { - return 0; - } - str->type = utype; - return 1; + return asn1_parse_string_unchecked(cbs, str, utype, cbs_tag); case V_ASN1_SEQUENCE: { // Save the entire element in the string. CBS elem;