From: beck Date: Mon, 29 May 2023 11:54:50 +0000 (+0000) Subject: Make X509_NAME_get_text_by[NID|OBJ] safer. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=f5d1ae505ed0968cd64052fbccd700808707dbfe;p=openbsd Make X509_NAME_get_text_by[NID|OBJ] safer. This is an un-revert with nits of the previously landed change to do this which broke libtls. libtls has now been changed to not use this function. This change ensures that if something is returned it is "text" (UTF-8) and a C string not containing a NUL byte. Historically callers to this function assume the result is text and a C string however the OpenSSL version simply hands them the bytes from an ASN1_STRING and expects them to know bad things can happen which they almost universally do not check for. Partly inspired by goings on in boringssl. ok jsing@ tb@ --- diff --git a/lib/libcrypto/man/X509_NAME_get_index_by_NID.3 b/lib/libcrypto/man/X509_NAME_get_index_by_NID.3 index 20730fb52a7..a2ceb10eb5d 100644 --- a/lib/libcrypto/man/X509_NAME_get_index_by_NID.3 +++ b/lib/libcrypto/man/X509_NAME_get_index_by_NID.3 @@ -1,4 +1,4 @@ -.\" $OpenBSD: X509_NAME_get_index_by_NID.3,v 1.15 2023/05/03 08:10:23 beck Exp $ +.\" $OpenBSD: X509_NAME_get_index_by_NID.3,v 1.16 2023/05/29 11:54:50 beck Exp $ .\" OpenSSL aebb9aac Jul 19 09:27:53 2016 -0400 .\" .\" This file was written by Dr. Stephen Henson . @@ -49,7 +49,7 @@ .\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED .\" OF THE POSSIBILITY OF SUCH DAMAGE. .\" -.Dd $Mdocdate: May 3 2023 $ +.Dd $Mdocdate: May 29 2023 $ .Dt X509_NAME_GET_INDEX_BY_NID 3 .Os .Sh NAME @@ -136,22 +136,32 @@ run from 0 to .Fn X509_NAME_get_text_by_NID and .Fn X509_NAME_get_text_by_OBJ -retrieve the "text" from the first entry in +retrieve the bytes encoded as UTF-8 from the first entry in .Fa name which matches .Fa nid or .Fa obj . -At most -.Fa len -bytes will be written and the text written to -.Fa buf -will be NUL terminated. If .Fa buf is .Dv NULL , nothing is written, but the return value is calculated as usual. +If +.Fa buf +is not +.Dv NULL , +no more than +.Fa len +bytes will be written and the text written to +.Fa buf +will be NUL terminated. +.Pp +If +.Fa len +is not large enough to hold the NUL byte terminated UTF-8 encoding of +the text, or if the UTF-8 encoding of the text would contains a NUL +byte, no data will be written and the call will return failure. .Pp All relevant .Dv NID_* @@ -189,8 +199,8 @@ if the index is invalid. .Fn X509_NAME_get_text_by_NID and .Fn X509_NAME_get_text_by_OBJ -return the length of the output string written, not counting the -terminating NUL, or -1 if no match is found. +return the length of the output UTF-8 string written, not counting the +terminating NUL, or -1 in the case of an error or no match being found. .Pp In some cases of failure of .Fn X509_NAME_get_index_by_NID diff --git a/lib/libcrypto/x509/x509name.c b/lib/libcrypto/x509/x509name.c index ecdf473ea92..d2df06ccc6a 100644 --- a/lib/libcrypto/x509/x509name.c +++ b/lib/libcrypto/x509/x509name.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509name.c,v 1.34 2023/05/03 08:10:23 beck Exp $ */ +/* $OpenBSD: x509name.c,v 1.35 2023/05/29 11:54:50 beck Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -66,6 +66,7 @@ #include #include +#include "bytestring.h" #include "x509_local.h" int @@ -84,21 +85,38 @@ int X509_NAME_get_text_by_OBJ(X509_NAME *name, const ASN1_OBJECT *obj, char *buf, int len) { - int i; + unsigned char *text = NULL; ASN1_STRING *data; + int i, text_len; + int ret = -1; + CBS cbs; i = X509_NAME_get_index_by_OBJ(name, obj, -1); if (i < 0) - return (-1); + goto err; data = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); - i = (data->length > (len - 1)) ? (len - 1) : data->length; - if (buf == NULL) - return (data->length); - if (i >= 0) { - memcpy(buf, data->data, i); - buf[i] = '\0'; + /* + * Fail if we cannot encode as UTF-8, or if the UTF-8 encoding of the + * string contains a 0 byte, because mortal callers seldom handle the + * length difference correctly. + */ + if ((text_len = ASN1_STRING_to_UTF8(&text, data)) < 0) + goto err; + CBS_init(&cbs, text, text_len); + if (CBS_contains_zero_byte(&cbs)) + goto err; + /* We still support the "pass NULL to find out how much" API */ + if (buf != NULL) { + if (len <= 0 || !CBS_write_bytes(&cbs, buf, len - 1, NULL)) + goto err; + /* It must be a C string */ + buf[text_len] = '\0'; } - return (i); + ret = text_len; + + err: + free(text); + return (ret); } LCRYPTO_ALIAS(X509_NAME_get_text_by_OBJ); diff --git a/regress/lib/libcrypto/x509/x509_asn1.c b/regress/lib/libcrypto/x509/x509_asn1.c index d96a51880e2..4c038c9bbf4 100644 --- a/regress/lib/libcrypto/x509/x509_asn1.c +++ b/regress/lib/libcrypto/x509/x509_asn1.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509_asn1.c,v 1.18 2023/05/03 08:10:23 beck Exp $ */ +/* $OpenBSD: x509_asn1.c,v 1.19 2023/05/29 11:54:50 beck Exp $ */ /* * Copyright (c) 2023 Job Snijders * @@ -512,13 +512,88 @@ test_x509_req_setters(void) return failed; } -int main(void) +static const struct testcase { + char *data; + int len; + int len_to_pass; + int encode_type; + int expected_result; + char *expected_string; +} testCases[] = { + /* should work */ + {"fozzie", 6, 80, MBSTRING_ASC, 6, "fozzie"}, + /* should work */ + {"fozzie", 6, -1, MBSTRING_ASC, 6, ""}, + /* should fail, truncation */ + {"muppet", 6, 5, MBSTRING_ASC, -1, ""}, + /* should fail, contains 0 byte */ + {"g\0nzo", 5, 80, MBSTRING_ASC, -1, ""}, + /* should fail, can't encode as utf-8 */ + {"\x30\x00", 2, 80, V_ASN1_SEQUENCE, -1, ""}, +}; + +#define NUM_TEST_CASES (sizeof(testCases) / sizeof(testCases[0])) + +static int +test_x509_name_get(void) +{ + int failed = 0; + size_t i; + + for (i = 0; i < NUM_TEST_CASES; i++) { + const struct testcase *test = testCases + i; + X509_NAME_ENTRY *entry = NULL; + X509_NAME *name = NULL; + char textbuf[80]; + int result; + + textbuf[0] = '\0'; + if ((name = X509_NAME_new()) == NULL) + err(1, "X509_NAME_new"); + if ((entry = X509_NAME_ENTRY_new()) == NULL) + err(1, "X509_NAME_ENTRY_new"); + if (!X509_NAME_ENTRY_set_object(entry, + OBJ_nid2obj(NID_commonName))) + err(1, "X509_NAME_ENTRY_set_object"); + if (!X509_NAME_ENTRY_set_data(entry, test->encode_type, + test->data, test->len)) + err(1, "X509_NAME_ENTRY_set_data"); + if (!X509_NAME_add_entry(name, entry, -1, 0)) + err(1, "X509_NAME_add_entry"); + if (test->len_to_pass == -1) + result = X509_NAME_get_text_by_NID(name, NID_commonName, + NULL, 0); + else + result = X509_NAME_get_text_by_NID(name, NID_commonName, + textbuf, test->len_to_pass); + if (result != test->expected_result) { + fprintf(stderr, + "Test %zu X509_GET_text_by_NID returned %d," + "expected %d\n", i, result, test->expected_result); + failed++; + } + if (result != -1 && + strcmp(test->expected_string, textbuf) != 0) { + fprintf(stderr, + "Test %zu, X509_GET_text_by_NID returned bytes do" + "not match \n", i); + failed++; + } + X509_NAME_ENTRY_free(entry); + X509_NAME_free(name); + } + return failed; +} + +int +main(void) { int failed = 0; failed |= test_x509_setters(); /* failed |= */ test_x509_crl_setters(); /* failed |= */ test_x509_req_setters(); + failed |= test_x509_name_get(); OPENSSL_cleanup();