From: doug Date: Mon, 15 Jun 2015 07:35:49 +0000 (+0000) Subject: Make CBS_get_any_asn1_element() more compliant with DER encoding. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=0870ee343aa39b66a4b36e75c03c2379b807e74d;p=openbsd Make CBS_get_any_asn1_element() more compliant with DER encoding. CBS_get_any_asn1_element violates DER encoding by allowing indefinite form. All callers except bs_ber.c expect DER encoding. The callers must check to see if it was indefinite or not. Rather than exposing all callers to this behavior, cbs_get_any_asn1_element_internal() allows specifying whether you want to allow the normally forbidden indefinite form. This is used by CBS_get_any_asn1_element() for strict DER encoding and by a new static function in bs_ber.c for the relaxed version. While I was here, I added comments to differentiate between ASN.1 restrictions and CBS limitations. ok miod@ --- diff --git a/lib/libssl/bs_ber.c b/lib/libssl/bs_ber.c index 2ec91fc8008..3d39def111b 100644 --- a/lib/libssl/bs_ber.c +++ b/lib/libssl/bs_ber.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bs_ber.c,v 1.4 2015/04/29 02:11:09 doug Exp $ */ +/* $OpenBSD: bs_ber.c,v 1.5 2015/06/15 07:35:49 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -27,6 +27,15 @@ */ static const unsigned kMaxDepth = 2048; +/* Non-strict version that allows a relaxed DER with indefinite form. */ +static int +cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, + size_t *out_header_len) +{ + return cbs_get_any_asn1_element_internal(cbs, out, + out_tag, out_header_len, 0); +} + /* * cbs_find_ber walks an ASN.1 structure in |orig_in| and sets |*ber_found| * depending on whether an indefinite length element was found. The value of @@ -49,10 +58,11 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) unsigned tag; size_t header_len; - if (!CBS_get_any_asn1_element(&in, &contents, &tag, + if (!cbs_nonstrict_get_any_asn1_element(&in, &contents, &tag, &header_len)) return 0; + /* Indefinite form not allowed by DER. */ if (CBS_len(&contents) == header_len && header_len > 0 && CBS_data(&contents)[header_len - 1] == 0x80) { *ber_found = 1; @@ -84,7 +94,8 @@ is_primitive_type(unsigned tag) /* * is_eoc returns true if |header_len| and |contents|, as returned by - * |CBS_get_any_asn1_element|, indicate an "end of contents" (EOC) value. + * |cbs_nonstrict_get_any_asn1_element|, indicate an "end of contents" (EOC) + * value. */ static char is_eoc(size_t header_len, CBS *contents) @@ -113,7 +124,8 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, size_t header_len; CBB *out_contents, out_contents_storage; - if (!CBS_get_any_asn1_element(in, &contents, &tag, &header_len)) + if (!cbs_nonstrict_get_any_asn1_element(in, &contents, &tag, + &header_len)) return 0; out_contents = out; @@ -156,9 +168,9 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, CBS_init(&in_copy, CBS_data(in), CBS_len(in)); - if (!CBS_get_any_asn1_element(&in_copy, - &inner_contents, &inner_tag, - &inner_header_len)) + if (!cbs_nonstrict_get_any_asn1_element( + &in_copy, &inner_contents, + &inner_tag, &inner_header_len)) return 0; if (CBS_len(&inner_contents) > diff --git a/lib/libssl/bs_cbs.c b/lib/libssl/bs_cbs.c index c37f81dd60f..ba38303c18a 100644 --- a/lib/libssl/bs_cbs.c +++ b/lib/libssl/bs_cbs.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bs_cbs.c,v 1.8 2015/06/13 08:46:00 doug Exp $ */ +/* $OpenBSD: bs_cbs.c,v 1.9 2015/06/15 07:35:49 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -204,25 +204,46 @@ CBS_get_u24_length_prefixed(CBS *cbs, CBS *out) int CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, size_t *out_header_len) +{ + return cbs_get_any_asn1_element_internal(cbs, out, out_tag, + out_header_len, 1); +} + +/* + * Review X.690 for details on ASN.1 DER encoding. + * + * If non-strict mode is enabled, then DER rules are relaxed + * for indefinite constructs (violates DER but a little closer to BER). + * Non-strict mode should only be used by bs_ber.c + * + * Sections 8, 10 and 11 for DER encoding + */ +int +cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag, + size_t *out_header_len, int strict) { uint8_t tag, length_byte; CBS header = *cbs; CBS throwaway; + size_t len; if (out == NULL) out = &throwaway; + /* + * Get identifier octet and length octet. Only 1 octet for each + * is a CBS limitation. + */ if (!CBS_get_u8(&header, &tag) || !CBS_get_u8(&header, &length_byte)) return 0; + /* CBS limitation: long form tags are not supported. */ if ((tag & 0x1f) == 0x1f) - /* Long form tags are not supported. */ return 0; if (out_tag != NULL) *out_tag = tag; - size_t len; if ((length_byte & 0x80) == 0) { /* Short form length. */ len = ((size_t) length_byte) + 2; @@ -234,21 +255,40 @@ CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, const size_t num_bytes = length_byte & 0x7f; uint32_t len32; - if ((tag & CBS_ASN1_CONSTRUCTED) != 0 && num_bytes == 0) { - /* indefinite length */ - if (out_header_len != NULL) - *out_header_len = 2; - return CBS_get_bytes(cbs, out, 2); + /* ASN.1 reserved value for future extensions */ + if (num_bytes == 0x7f) + return 0; + + /* Handle indefinite form length */ + if (num_bytes == 0) { + /* DER encoding doesn't allow for indefinite form. */ + if (strict) { + return 0; + + } else { + if ((tag & CBS_ASN1_CONSTRUCTED) != 0 && + num_bytes == 0) { + /* indefinite length */ + if (out_header_len != NULL) + *out_header_len = 2; + return CBS_get_bytes(cbs, out, 2); + } else { + /* Primitive cannot use indefinite. */ + return 0; + } + } } - if (num_bytes == 0 || num_bytes > 4) + /* CBS limitation. */ + if (num_bytes > 4) return 0; if (!cbs_get_u(&header, &len32, num_bytes)) return 0; + /* DER has a minimum length octet requirements. */ if (len32 < 128) - /* Length should have used short-form encoding. */ + /* Should have used short form instead */ return 0; if ((len32 >> ((num_bytes - 1) * 8)) == 0) @@ -279,13 +319,7 @@ cbs_get_asn1(CBS *cbs, CBS *out, unsigned tag_value, int skip_header) out = &throwaway; if (!CBS_get_any_asn1_element(cbs, out, &tag, &header_len) || - tag != tag_value || (header_len > 0 && - /* - * This ensures that the tag is either zero length or - * indefinite-length. - */ - CBS_len(out) == header_len && - CBS_data(out)[header_len - 1] == 0x80)) + tag != tag_value) return 0; if (skip_header && !CBS_skip(out, header_len)) { diff --git a/lib/libssl/bytestring.h b/lib/libssl/bytestring.h index b98c930da56..d66ab65b919 100644 --- a/lib/libssl/bytestring.h +++ b/lib/libssl/bytestring.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bytestring.h,v 1.6 2015/06/13 09:02:45 doug Exp $ */ +/* $OpenBSD: bytestring.h,v 1.7 2015/06/15 07:35:49 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -223,9 +223,8 @@ int CBS_peek_asn1_tag(const CBS *cbs, unsigned tag_value); /* * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to - * the tag number and |*out_header_len| to the length of the ASN.1 header. If - * the element has indefinite length then |*out| will only contain the - * header. Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore + * the tag number and |*out_header_len| to the length of the ASN.1 header. + * Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore * the value. * * Tag numbers greater than 30 are not supported (i.e. short form only). @@ -451,6 +450,19 @@ int CBB_add_u24(CBB *cbb, uint32_t value); int CBB_add_asn1_uint64(CBB *cbb, uint64_t value); #ifdef LIBRESSL_INTERNAL +/* + * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from + * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to + * the tag number and |*out_header_len| to the length of the ASN.1 header. If + * strict mode is disabled and the element has indefinite length then |*out| + * will only contain the header. Each of |out|, |out_tag|, and + * |out_header_len| may be NULL to ignore the value. + * + * Tag numbers greater than 30 are not supported (i.e. short form only). + */ +int cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag, + size_t *out_header_len, int strict); + /* * CBS_asn1_ber_to_der reads an ASN.1 structure from |in|. If it finds * indefinite-length elements then it attempts to convert the BER data to DER diff --git a/lib/libssl/src/ssl/bs_ber.c b/lib/libssl/src/ssl/bs_ber.c index 2ec91fc8008..3d39def111b 100644 --- a/lib/libssl/src/ssl/bs_ber.c +++ b/lib/libssl/src/ssl/bs_ber.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bs_ber.c,v 1.4 2015/04/29 02:11:09 doug Exp $ */ +/* $OpenBSD: bs_ber.c,v 1.5 2015/06/15 07:35:49 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -27,6 +27,15 @@ */ static const unsigned kMaxDepth = 2048; +/* Non-strict version that allows a relaxed DER with indefinite form. */ +static int +cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, + size_t *out_header_len) +{ + return cbs_get_any_asn1_element_internal(cbs, out, + out_tag, out_header_len, 0); +} + /* * cbs_find_ber walks an ASN.1 structure in |orig_in| and sets |*ber_found| * depending on whether an indefinite length element was found. The value of @@ -49,10 +58,11 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) unsigned tag; size_t header_len; - if (!CBS_get_any_asn1_element(&in, &contents, &tag, + if (!cbs_nonstrict_get_any_asn1_element(&in, &contents, &tag, &header_len)) return 0; + /* Indefinite form not allowed by DER. */ if (CBS_len(&contents) == header_len && header_len > 0 && CBS_data(&contents)[header_len - 1] == 0x80) { *ber_found = 1; @@ -84,7 +94,8 @@ is_primitive_type(unsigned tag) /* * is_eoc returns true if |header_len| and |contents|, as returned by - * |CBS_get_any_asn1_element|, indicate an "end of contents" (EOC) value. + * |cbs_nonstrict_get_any_asn1_element|, indicate an "end of contents" (EOC) + * value. */ static char is_eoc(size_t header_len, CBS *contents) @@ -113,7 +124,8 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, size_t header_len; CBB *out_contents, out_contents_storage; - if (!CBS_get_any_asn1_element(in, &contents, &tag, &header_len)) + if (!cbs_nonstrict_get_any_asn1_element(in, &contents, &tag, + &header_len)) return 0; out_contents = out; @@ -156,9 +168,9 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, CBS_init(&in_copy, CBS_data(in), CBS_len(in)); - if (!CBS_get_any_asn1_element(&in_copy, - &inner_contents, &inner_tag, - &inner_header_len)) + if (!cbs_nonstrict_get_any_asn1_element( + &in_copy, &inner_contents, + &inner_tag, &inner_header_len)) return 0; if (CBS_len(&inner_contents) > diff --git a/lib/libssl/src/ssl/bs_cbs.c b/lib/libssl/src/ssl/bs_cbs.c index c37f81dd60f..ba38303c18a 100644 --- a/lib/libssl/src/ssl/bs_cbs.c +++ b/lib/libssl/src/ssl/bs_cbs.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bs_cbs.c,v 1.8 2015/06/13 08:46:00 doug Exp $ */ +/* $OpenBSD: bs_cbs.c,v 1.9 2015/06/15 07:35:49 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -204,25 +204,46 @@ CBS_get_u24_length_prefixed(CBS *cbs, CBS *out) int CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, size_t *out_header_len) +{ + return cbs_get_any_asn1_element_internal(cbs, out, out_tag, + out_header_len, 1); +} + +/* + * Review X.690 for details on ASN.1 DER encoding. + * + * If non-strict mode is enabled, then DER rules are relaxed + * for indefinite constructs (violates DER but a little closer to BER). + * Non-strict mode should only be used by bs_ber.c + * + * Sections 8, 10 and 11 for DER encoding + */ +int +cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag, + size_t *out_header_len, int strict) { uint8_t tag, length_byte; CBS header = *cbs; CBS throwaway; + size_t len; if (out == NULL) out = &throwaway; + /* + * Get identifier octet and length octet. Only 1 octet for each + * is a CBS limitation. + */ if (!CBS_get_u8(&header, &tag) || !CBS_get_u8(&header, &length_byte)) return 0; + /* CBS limitation: long form tags are not supported. */ if ((tag & 0x1f) == 0x1f) - /* Long form tags are not supported. */ return 0; if (out_tag != NULL) *out_tag = tag; - size_t len; if ((length_byte & 0x80) == 0) { /* Short form length. */ len = ((size_t) length_byte) + 2; @@ -234,21 +255,40 @@ CBS_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, const size_t num_bytes = length_byte & 0x7f; uint32_t len32; - if ((tag & CBS_ASN1_CONSTRUCTED) != 0 && num_bytes == 0) { - /* indefinite length */ - if (out_header_len != NULL) - *out_header_len = 2; - return CBS_get_bytes(cbs, out, 2); + /* ASN.1 reserved value for future extensions */ + if (num_bytes == 0x7f) + return 0; + + /* Handle indefinite form length */ + if (num_bytes == 0) { + /* DER encoding doesn't allow for indefinite form. */ + if (strict) { + return 0; + + } else { + if ((tag & CBS_ASN1_CONSTRUCTED) != 0 && + num_bytes == 0) { + /* indefinite length */ + if (out_header_len != NULL) + *out_header_len = 2; + return CBS_get_bytes(cbs, out, 2); + } else { + /* Primitive cannot use indefinite. */ + return 0; + } + } } - if (num_bytes == 0 || num_bytes > 4) + /* CBS limitation. */ + if (num_bytes > 4) return 0; if (!cbs_get_u(&header, &len32, num_bytes)) return 0; + /* DER has a minimum length octet requirements. */ if (len32 < 128) - /* Length should have used short-form encoding. */ + /* Should have used short form instead */ return 0; if ((len32 >> ((num_bytes - 1) * 8)) == 0) @@ -279,13 +319,7 @@ cbs_get_asn1(CBS *cbs, CBS *out, unsigned tag_value, int skip_header) out = &throwaway; if (!CBS_get_any_asn1_element(cbs, out, &tag, &header_len) || - tag != tag_value || (header_len > 0 && - /* - * This ensures that the tag is either zero length or - * indefinite-length. - */ - CBS_len(out) == header_len && - CBS_data(out)[header_len - 1] == 0x80)) + tag != tag_value) return 0; if (skip_header && !CBS_skip(out, header_len)) { diff --git a/lib/libssl/src/ssl/bytestring.h b/lib/libssl/src/ssl/bytestring.h index b98c930da56..d66ab65b919 100644 --- a/lib/libssl/src/ssl/bytestring.h +++ b/lib/libssl/src/ssl/bytestring.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bytestring.h,v 1.6 2015/06/13 09:02:45 doug Exp $ */ +/* $OpenBSD: bytestring.h,v 1.7 2015/06/15 07:35:49 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -223,9 +223,8 @@ int CBS_peek_asn1_tag(const CBS *cbs, unsigned tag_value); /* * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to - * the tag number and |*out_header_len| to the length of the ASN.1 header. If - * the element has indefinite length then |*out| will only contain the - * header. Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore + * the tag number and |*out_header_len| to the length of the ASN.1 header. + * Each of |out|, |out_tag|, and |out_header_len| may be NULL to ignore * the value. * * Tag numbers greater than 30 are not supported (i.e. short form only). @@ -451,6 +450,19 @@ int CBB_add_u24(CBB *cbb, uint32_t value); int CBB_add_asn1_uint64(CBB *cbb, uint64_t value); #ifdef LIBRESSL_INTERNAL +/* + * CBS_get_any_asn1_element sets |*out| to contain the next ASN.1 element from + * |*cbs| (including header bytes) and advances |*cbs|. It sets |*out_tag| to + * the tag number and |*out_header_len| to the length of the ASN.1 header. If + * strict mode is disabled and the element has indefinite length then |*out| + * will only contain the header. Each of |out|, |out_tag|, and + * |out_header_len| may be NULL to ignore the value. + * + * Tag numbers greater than 30 are not supported (i.e. short form only). + */ +int cbs_get_any_asn1_element_internal(CBS *cbs, CBS *out, unsigned *out_tag, + size_t *out_header_len, int strict); + /* * CBS_asn1_ber_to_der reads an ASN.1 structure from |in|. If it finds * indefinite-length elements then it attempts to convert the BER data to DER