From: doug Date: Tue, 16 Jun 2015 06:37:58 +0000 (+0000) Subject: Be more strict about BER and DER terminology. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=50933c7b621fc99209f85811ec1c37a70d870ffa;p=openbsd Be more strict about BER and DER terminology. bs_ber.c does not convert BER to DER. It's a hack to convert a DER-like encoding with one violation (indefinite form) to strict DER. Rename the functions to reflect this. ok miod@ jsing@ --- diff --git a/lib/libssl/bs_ber.c b/lib/libssl/bs_ber.c index 3d39def111b..1310d4a94ce 100644 --- a/lib/libssl/bs_ber.c +++ b/lib/libssl/bs_ber.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bs_ber.c,v 1.5 2015/06/15 07:35:49 doug Exp $ */ +/* $OpenBSD: bs_ber.c,v 1.6 2015/06/16 06:37:58 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -29,7 +29,7 @@ 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, +cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned int *out_tag, size_t *out_header_len) { return cbs_get_any_asn1_element_internal(cbs, out, @@ -37,13 +37,15 @@ cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, } /* - * 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 - * |in| is not changed. It returns one on success (i.e. |*ber_found| was set) - * and zero on error. + * cbs_find_indefinite walks an ASN.1 structure in |orig_in| and sets + * |*indefinite_found| depending on whether an indefinite length element was + * found. The value of |orig_in| is not modified. + * + * Returns one on success (i.e. |*indefinite_found| was set) and zero on error. */ static int -cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) +cbs_find_indefinite(const CBS *orig_in, char *indefinite_found, + unsigned int depth) { CBS in; @@ -51,7 +53,6 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) return 0; CBS_init(&in, CBS_data(orig_in), CBS_len(orig_in)); - *ber_found = 0; while (CBS_len(&in) > 0) { CBS contents; @@ -65,16 +66,18 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) /* 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; + *indefinite_found = 1; return 1; } if (tag & CBS_ASN1_CONSTRUCTED) { if (!CBS_skip(&contents, header_len) || - !cbs_find_ber(&contents, ber_found, depth + 1)) + !cbs_find_indefinite(&contents, indefinite_found, + depth + 1)) return 0; } } + *indefinite_found = 0; return 1; } @@ -104,7 +107,8 @@ is_eoc(size_t header_len, CBS *contents) } /* - * cbs_convert_ber reads BER data from |in| and writes DER data to |out|. If + * cbs_convert_indefinite reads data with DER encoding (but relaxed to allow + * indefinite form) from |in| and writes definite form DER data to |out|. If * |squash_header| is set then the top-level of elements from |in| will not * have their headers written. This is used when concatenating the fragments of * an indefinite length, primitive value. If |looking_for_eoc| is set then any @@ -112,8 +116,8 @@ is_eoc(size_t header_len, CBS *contents) * It returns one on success and zero on error. */ static int -cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, - unsigned depth) +cbs_convert_indefinite(CBS *in, CBB *out, char squash_header, + char looking_for_eoc, unsigned depth) { if (depth > kMaxDepth) return 0; @@ -143,7 +147,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, * with a concrete length prefix. * * If it's a something else then the contents - * will be a series of BER elements of the same + * will be a series of DER elements of the same * type which need to be concatenated. */ const char context_specific = (tag & 0xc0) @@ -193,7 +197,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, out_contents = &out_contents_storage; } - if (!cbs_convert_ber(in, out_contents, + if (!cbs_convert_indefinite(in, out_contents, squash_child_headers, 1 /* looking for eoc */, depth + 1)) return 0; @@ -216,7 +220,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, return 0; if (tag & CBS_ASN1_CONSTRUCTED) { - if (!cbs_convert_ber(&contents, out_contents, + if (!cbs_convert_indefinite(&contents, out_contents, 0 /* don't squash header */, 0 /* not looking for eoc */, depth + 1)) return 0; @@ -234,7 +238,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, } int -CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) +CBS_asn1_indefinite_to_definite(CBS *in, uint8_t **out, size_t *out_len) { CBB cbb; @@ -244,7 +248,7 @@ CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) * return. */ char conversion_needed; - if (!cbs_find_ber(in, &conversion_needed, 0)) + if (!cbs_find_indefinite(in, &conversion_needed, 0)) return 0; if (!conversion_needed) { @@ -254,7 +258,7 @@ CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) } CBB_init(&cbb, CBS_len(in)); - if (!cbs_convert_ber(in, &cbb, 0, 0, 0)) { + if (!cbs_convert_indefinite(in, &cbb, 0, 0, 0)) { CBB_cleanup(&cbb); return 0; } diff --git a/lib/libssl/bytestring.h b/lib/libssl/bytestring.h index ef824a0cead..07be6ddd50e 100644 --- a/lib/libssl/bytestring.h +++ b/lib/libssl/bytestring.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bytestring.h,v 1.8 2015/06/16 06:11:39 doug Exp $ */ +/* $OpenBSD: bytestring.h,v 1.9 2015/06/16 06:37:58 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -464,22 +464,23 @@ 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 - * and sets |*out| and |*out_length| to describe a malloced buffer containing - * the DER data. Additionally, |*in| will be advanced over the ASN.1 data. + * CBS_asn1_indefinite_to_definite reads an ASN.1 structure from |in|. If it + * finds indefinite-length elements that otherwise appear to be valid DER, it + * attempts to convert the DER-like data to DER and sets |*out| and + * |*out_length| to describe a malloced buffer containing the DER data. + * Additionally, |*in| will be advanced over the ASN.1 data. * * If it doesn't find any indefinite-length elements then it sets |*out| to * NULL and |*in| is unmodified. * - * A sufficiently complex ASN.1 structure will break this function because it's - * not possible to generically convert BER to DER without knowledge of the - * structure itself. However, this sufficies to handle the PKCS#7 and #12 output + * This is NOT a conversion from BER to DER. There are many restrictions when + * dealing with DER data. This is only concerned with one: indefinite vs. + * definite form. However, this suffices to handle the PKCS#7 and PKCS#12 output * from NSS. * * It returns one on success and zero otherwise. */ -int CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len); +int CBS_asn1_indefinite_to_definite(CBS *in, uint8_t **out, size_t *out_len); #endif /* LIBRESSL_INTERNAL */ #if defined(__cplusplus) diff --git a/lib/libssl/src/ssl/bs_ber.c b/lib/libssl/src/ssl/bs_ber.c index 3d39def111b..1310d4a94ce 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.5 2015/06/15 07:35:49 doug Exp $ */ +/* $OpenBSD: bs_ber.c,v 1.6 2015/06/16 06:37:58 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -29,7 +29,7 @@ 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, +cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned int *out_tag, size_t *out_header_len) { return cbs_get_any_asn1_element_internal(cbs, out, @@ -37,13 +37,15 @@ cbs_nonstrict_get_any_asn1_element(CBS *cbs, CBS *out, unsigned *out_tag, } /* - * 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 - * |in| is not changed. It returns one on success (i.e. |*ber_found| was set) - * and zero on error. + * cbs_find_indefinite walks an ASN.1 structure in |orig_in| and sets + * |*indefinite_found| depending on whether an indefinite length element was + * found. The value of |orig_in| is not modified. + * + * Returns one on success (i.e. |*indefinite_found| was set) and zero on error. */ static int -cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) +cbs_find_indefinite(const CBS *orig_in, char *indefinite_found, + unsigned int depth) { CBS in; @@ -51,7 +53,6 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) return 0; CBS_init(&in, CBS_data(orig_in), CBS_len(orig_in)); - *ber_found = 0; while (CBS_len(&in) > 0) { CBS contents; @@ -65,16 +66,18 @@ cbs_find_ber(CBS *orig_in, char *ber_found, unsigned depth) /* 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; + *indefinite_found = 1; return 1; } if (tag & CBS_ASN1_CONSTRUCTED) { if (!CBS_skip(&contents, header_len) || - !cbs_find_ber(&contents, ber_found, depth + 1)) + !cbs_find_indefinite(&contents, indefinite_found, + depth + 1)) return 0; } } + *indefinite_found = 0; return 1; } @@ -104,7 +107,8 @@ is_eoc(size_t header_len, CBS *contents) } /* - * cbs_convert_ber reads BER data from |in| and writes DER data to |out|. If + * cbs_convert_indefinite reads data with DER encoding (but relaxed to allow + * indefinite form) from |in| and writes definite form DER data to |out|. If * |squash_header| is set then the top-level of elements from |in| will not * have their headers written. This is used when concatenating the fragments of * an indefinite length, primitive value. If |looking_for_eoc| is set then any @@ -112,8 +116,8 @@ is_eoc(size_t header_len, CBS *contents) * It returns one on success and zero on error. */ static int -cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, - unsigned depth) +cbs_convert_indefinite(CBS *in, CBB *out, char squash_header, + char looking_for_eoc, unsigned depth) { if (depth > kMaxDepth) return 0; @@ -143,7 +147,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, * with a concrete length prefix. * * If it's a something else then the contents - * will be a series of BER elements of the same + * will be a series of DER elements of the same * type which need to be concatenated. */ const char context_specific = (tag & 0xc0) @@ -193,7 +197,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, out_contents = &out_contents_storage; } - if (!cbs_convert_ber(in, out_contents, + if (!cbs_convert_indefinite(in, out_contents, squash_child_headers, 1 /* looking for eoc */, depth + 1)) return 0; @@ -216,7 +220,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, return 0; if (tag & CBS_ASN1_CONSTRUCTED) { - if (!cbs_convert_ber(&contents, out_contents, + if (!cbs_convert_indefinite(&contents, out_contents, 0 /* don't squash header */, 0 /* not looking for eoc */, depth + 1)) return 0; @@ -234,7 +238,7 @@ cbs_convert_ber(CBS *in, CBB *out, char squash_header, char looking_for_eoc, } int -CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) +CBS_asn1_indefinite_to_definite(CBS *in, uint8_t **out, size_t *out_len) { CBB cbb; @@ -244,7 +248,7 @@ CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) * return. */ char conversion_needed; - if (!cbs_find_ber(in, &conversion_needed, 0)) + if (!cbs_find_indefinite(in, &conversion_needed, 0)) return 0; if (!conversion_needed) { @@ -254,7 +258,7 @@ CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len) } CBB_init(&cbb, CBS_len(in)); - if (!cbs_convert_ber(in, &cbb, 0, 0, 0)) { + if (!cbs_convert_indefinite(in, &cbb, 0, 0, 0)) { CBB_cleanup(&cbb); return 0; } diff --git a/lib/libssl/src/ssl/bytestring.h b/lib/libssl/src/ssl/bytestring.h index ef824a0cead..07be6ddd50e 100644 --- a/lib/libssl/src/ssl/bytestring.h +++ b/lib/libssl/src/ssl/bytestring.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bytestring.h,v 1.8 2015/06/16 06:11:39 doug Exp $ */ +/* $OpenBSD: bytestring.h,v 1.9 2015/06/16 06:37:58 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -464,22 +464,23 @@ 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 - * and sets |*out| and |*out_length| to describe a malloced buffer containing - * the DER data. Additionally, |*in| will be advanced over the ASN.1 data. + * CBS_asn1_indefinite_to_definite reads an ASN.1 structure from |in|. If it + * finds indefinite-length elements that otherwise appear to be valid DER, it + * attempts to convert the DER-like data to DER and sets |*out| and + * |*out_length| to describe a malloced buffer containing the DER data. + * Additionally, |*in| will be advanced over the ASN.1 data. * * If it doesn't find any indefinite-length elements then it sets |*out| to * NULL and |*in| is unmodified. * - * A sufficiently complex ASN.1 structure will break this function because it's - * not possible to generically convert BER to DER without knowledge of the - * structure itself. However, this sufficies to handle the PKCS#7 and #12 output + * This is NOT a conversion from BER to DER. There are many restrictions when + * dealing with DER data. This is only concerned with one: indefinite vs. + * definite form. However, this suffices to handle the PKCS#7 and PKCS#12 output * from NSS. * * It returns one on success and zero otherwise. */ -int CBS_asn1_ber_to_der(CBS *in, uint8_t **out, size_t *out_len); +int CBS_asn1_indefinite_to_definite(CBS *in, uint8_t **out, size_t *out_len); #endif /* LIBRESSL_INTERNAL */ #if defined(__cplusplus) diff --git a/regress/lib/libssl/bytestring/bytestringtest.c b/regress/lib/libssl/bytestring/bytestringtest.c index 7ae9397a352..05ca27e8b51 100644 --- a/regress/lib/libssl/bytestring/bytestringtest.c +++ b/regress/lib/libssl/bytestring/bytestringtest.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bytestringtest.c,v 1.4 2015/04/25 15:28:47 doug Exp $ */ +/* $OpenBSD: bytestringtest.c,v 1.5 2015/06/16 06:37:58 doug Exp $ */ /* * Copyright (c) 2014, Google Inc. * @@ -484,23 +484,25 @@ err: } static int -do_ber_convert(const char *name, const uint8_t *der_expected, size_t der_len, - const uint8_t *ber, size_t ber_len) +do_indefinite_convert(const char *name, const uint8_t *definite_expected, + size_t definite_len, const uint8_t *indefinite, size_t indefinite_len) { CBS in; uint8_t *out = NULL; size_t out_len; int ret = 0; - CBS_init(&in, ber, ber_len); - if (!CBS_asn1_ber_to_der(&in, &out, &out_len)) { - fprintf(stderr, "%s: CBS_asn1_ber_to_der failed.\n", name); + CBS_init(&in, indefinite, indefinite_len); + if (!CBS_asn1_indefinite_to_definite(&in, &out, &out_len)) { + fprintf(stderr, "%s: CBS_asn1_indefinite_to_definite failed.\n", + name); goto end; } if (out == NULL) { - if (ber_len != der_len || - memcmp(der_expected, ber, ber_len) != 0) { + if (indefinite_len != definite_len || + memcmp(definite_expected, indefinite, indefinite_len) + != 0) { fprintf(stderr, "%s: incorrect unconverted result.\n", name); return 0; @@ -509,7 +511,8 @@ do_ber_convert(const char *name, const uint8_t *der_expected, size_t der_len, return 1; } - if (out_len != der_len || memcmp(out, der_expected, der_len) != 0) { + if (out_len != definite_len || memcmp(out, definite_expected, + definite_len) != 0) { fprintf(stderr, "%s: incorrect converted result.\n", name); goto end; } @@ -522,7 +525,7 @@ end: } static int -test_ber_convert(void) +test_indefinite_convert(void) { static const uint8_t kSimpleBER[] = {0x01, 0x01, 0x00}; @@ -566,14 +569,14 @@ test_ber_convert(void) 0x6e, 0x10, 0x9b, 0xb8, 0x02, 0x02, 0x07, 0xd0, }; - return do_ber_convert("kSimpleBER", kSimpleBER, sizeof(kSimpleBER), + return do_indefinite_convert("kSimpleBER", kSimpleBER, sizeof(kSimpleBER), kSimpleBER, sizeof(kSimpleBER)) && - do_ber_convert("kIndefBER", kIndefDER, sizeof(kIndefDER), kIndefBER, + do_indefinite_convert("kIndefBER", kIndefDER, sizeof(kIndefDER), kIndefBER, sizeof(kIndefBER)) && - do_ber_convert("kOctetStringBER", kOctetStringDER, + do_indefinite_convert("kOctetStringBER", kOctetStringDER, sizeof(kOctetStringDER), kOctetStringBER, sizeof(kOctetStringBER)) && - do_ber_convert("kNSSBER", kNSSDER, sizeof(kNSSDER), kNSSBER, + do_indefinite_convert("kNSSBER", kNSSDER, sizeof(kNSSDER), kNSSBER, sizeof(kNSSBER)); } @@ -682,7 +685,7 @@ main(void) !test_cbb_misuse() || !test_cbb_prefixed() || !test_cbb_asn1() || - !test_ber_convert() || + !test_indefinite_convert() || !test_asn1_uint64() || !test_get_optional_asn1_bool()) return 1;