From 905ba924398f474e647de70345b4ae4089fedba7 Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 3 Nov 2022 11:55:13 +1100 Subject: [PATCH] punycode: update to use WPACKET instead of using custom range checking Add test for `.' overflows, remove the output size argument from ossl_a2ulabel() since it was never used and greatly complicated the code. Convert ossl_a2ulabel() to use WPACKET for building the output string. Update the documentation to match the new definition of ossl_a2ulabel(). x509: let punycode handle the '\0' string termination. Saves a memset(3) and some size fiddling. Also update to deal with the modified parameters. Reviewed-by: Tomas Mraz Reviewed-by: Dmitry Belyavskiy (Merged from https://github.com/openssl/openssl/pull/19591) --- crypto/punycode.c | 65 ++++++++++---------- crypto/x509/v3_ncons.c | 10 ++-- doc/internal/man3/ossl_punycode_decode.pod | 11 ++-- include/crypto/punycode.h | 3 +- test/punycode_test.c | 69 +++++++++++++++++----- 5 files changed, 96 insertions(+), 62 deletions(-) diff --git a/crypto/punycode.c b/crypto/punycode.c index f55e351f7a..ad88495f6c 100644 --- a/crypto/punycode.c +++ b/crypto/punycode.c @@ -12,6 +12,7 @@ #include #include "crypto/punycode.h" #include "internal/common.h" /* for HAS_PREFIX */ +#include "internal/packet.h" /* for WPACKET */ static const unsigned int base = 36; static const unsigned int tmin = 1; @@ -239,12 +240,12 @@ static int codepoint2utf8(unsigned char *out, unsigned long utf) /*- * Return values: - * 1 - ok, *outlen contains valid buf length - * 0 - ok but buf was too short, *outlen contains valid buf length - * -1 - bad string passed + * 1 - ok + * 0 - ok but buf was too short + * -1 - bad string passed or other error */ -int ossl_a2ulabel(const char *in, char *out, size_t *outlen) +int ossl_a2ulabel(const char *in, char *out, size_t outlen) { /*- * Domain name has some parts consisting of ASCII chars joined with dot. @@ -252,63 +253,61 @@ int ossl_a2ulabel(const char *in, char *out, size_t *outlen) * If it does not start with xn--, it becomes U-label as is. * Otherwise we try to decode it. */ - char *outptr = out; const char *inptr = in; - size_t size = 0, maxsize; int result = 1; - unsigned int i, j; + unsigned int i; unsigned int buf[LABEL_BUF_SIZE]; /* It's a hostname */ + WPACKET pkt; - if (out == NULL) { - result = 0; - maxsize = 0; - } else { - maxsize = *outlen; - } + /* Internal API, so should not fail */ + if (!ossl_assert(out != NULL)) + return -1; -#define PUSHC(c) \ - do \ - if (size++ < maxsize) \ - *outptr++ = c; \ - else \ - result = 0; \ - while (0) + if (!WPACKET_init_static_len(&pkt, (unsigned char *)out, outlen, 0)) + return -1; while (1) { char *tmpptr = strchr(inptr, '.'); size_t delta = tmpptr != NULL ? (size_t)(tmpptr - inptr) : strlen(inptr); if (!HAS_PREFIX(inptr, "xn--")) { - for (i = 0; i < delta + 1; i++) - PUSHC(inptr[i]); + if (!WPACKET_memcpy(&pkt, inptr, delta)) + result = 0; } else { unsigned int bufsize = LABEL_BUF_SIZE; - if (ossl_punycode_decode(inptr + 4, delta - 4, buf, &bufsize) <= 0) - return -1; + if (ossl_punycode_decode(inptr + 4, delta - 4, buf, &bufsize) <= 0) { + result = -1; + goto end; + } for (i = 0; i < bufsize; i++) { unsigned char seed[6]; size_t utfsize = codepoint2utf8(seed, buf[i]); - if (utfsize == 0) - return -1; + if (utfsize == 0) { + result = -1; + goto end; + } - for (j = 0; j < utfsize; j++) - PUSHC(seed[j]); + if (!WPACKET_memcpy(&pkt, seed, utfsize)) + result = 0; } - - PUSHC(tmpptr != NULL ? '.' : '\0'); } if (tmpptr == NULL) break; + if (!WPACKET_put_bytes_u8(&pkt, '.')) + result = 0; + inptr = tmpptr + 1; } -#undef PUSHC - *outlen = size; + if (!WPACKET_put_bytes_u8(&pkt, '\0')) + result = 0; + end: + WPACKET_cleanup(&pkt); return result; } @@ -325,7 +324,7 @@ int ossl_a2ucompare(const char *a, const char *u) char a_ulabel[LABEL_BUF_SIZE + 1]; size_t a_size = sizeof(a_ulabel); - if (ossl_a2ulabel(a, a_ulabel, &a_size) <= 0) + if (ossl_a2ulabel(a, a_ulabel, a_size) <= 0) return -1; return strcmp(a_ulabel, u) != 0; diff --git a/crypto/x509/v3_ncons.c b/crypto/x509/v3_ncons.c index 52675530e3..8c7aee32a2 100644 --- a/crypto/x509/v3_ncons.c +++ b/crypto/x509/v3_ncons.c @@ -632,7 +632,7 @@ static int nc_email_eai(ASN1_TYPE *emltype, ASN1_IA5STRING *base) const char *emlptr; const char *emlat; char ulabel[256]; - size_t size = sizeof(ulabel) - 1; + size_t size = sizeof(ulabel); int ret = X509_V_OK; size_t emlhostlen; @@ -659,18 +659,16 @@ static int nc_email_eai(ASN1_TYPE *emltype, ASN1_IA5STRING *base) goto end; } - memset(ulabel, 0, sizeof(ulabel)); /* Special case: initial '.' is RHS match */ if (*baseptr == '.') { ulabel[0] = '.'; - size -= 1; - if (ossl_a2ulabel(baseptr, ulabel + 1, &size) <= 0) { + if (ossl_a2ulabel(baseptr, ulabel + 1, size - 1) <= 0) { ret = X509_V_ERR_UNSPECIFIED; goto end; } if ((size_t)eml->length > strlen(ulabel)) { - emlptr += eml->length - (strlen(ulabel)); + emlptr += eml->length - strlen(ulabel); /* X509_V_OK */ if (ia5ncasecmp(ulabel, emlptr, strlen(ulabel)) == 0) goto end; @@ -679,7 +677,7 @@ static int nc_email_eai(ASN1_TYPE *emltype, ASN1_IA5STRING *base) goto end; } - if (ossl_a2ulabel(baseptr, ulabel, &size) <= 0) { + if (ossl_a2ulabel(baseptr, ulabel, size) <= 0) { ret = X509_V_ERR_UNSPECIFIED; goto end; } diff --git a/doc/internal/man3/ossl_punycode_decode.pod b/doc/internal/man3/ossl_punycode_decode.pod index bf6b56edfc..8c9484889b 100644 --- a/doc/internal/man3/ossl_punycode_decode.pod +++ b/doc/internal/man3/ossl_punycode_decode.pod @@ -12,7 +12,7 @@ ossl_punycode_decode, ossl_a2ulabel, ossl_a2ucompare int ossl_punycode_decode(const char *pEncoded, const size_t enc_len, unsigned int *pDecoded, unsigned int *pout_length); - int ossl_a2ulabel(const char *in, char *out, size_t *outlen); + int ossl_a2ulabel(const char *in, char *out, size_t outlen); int ossl_a2ucompare(const char *a, const char *u); @@ -23,7 +23,7 @@ representation of hostnames in ASCII-only format. Some specifications, such as RFC 8398, require comparison of hostnames encoded in UTF-8 charset. ossl_a2ulabel() decodes NUL-terminated hostname from PUNYCODE to UTF-8, -using a provided buffer for output. +using a provided buffer for output. The output buffer is NUL-terminated. ossl_a2ucompare() accepts two NUL-terminated hostnames, decodes the 1st from PUNYCODE to UTF-8 and compares it with the 2nd one as is. @@ -33,12 +33,11 @@ a hostname, with stripped PUNYCODE marker I. =head1 RETURN VALUES -ossl_a2ulabel() returns 1 on success, 0 on not enough buf passed, --1 on invalid PUNYCODE string passed. When valid string is provided, it sets the -I<*outlen> to the length of required buffer to perform correct decoding. +ossl_a2ulabel() returns 1 on success, 0 if the output buffer is too small and +-1 if an invalid PUNYCODE string is passed or another error occurs. ossl_a2ucompare() returns 1 on non-equal strings, 0 on equal strings, --1 when invalid PUNYCODE string passed. +-1 when an invalid PUNYCODE string is passed or another error occurs. ossl_punycode_decode() returns 1 on success, 0 on error. On success, *pout_length contains the number of codepoints decoded. diff --git a/include/crypto/punycode.h b/include/crypto/punycode.h index 133826d87e..1cc52c544a 100644 --- a/include/crypto/punycode.h +++ b/include/crypto/punycode.h @@ -18,7 +18,8 @@ int ossl_punycode_decode ( unsigned int *pout_length ); -int ossl_a2ulabel(const char *in, char *out, size_t *outlen); +int ossl_a2ulabel(const char *in, char *out, size_t outlen); int ossl_a2ucompare(const char *a, const char *u); + #endif diff --git a/test/punycode_test.c b/test/punycode_test.c index 9d8171346c..5b8b0bd272 100644 --- a/test/punycode_test.c +++ b/test/punycode_test.c @@ -12,6 +12,7 @@ #include "crypto/punycode.h" #include "internal/nelem.h" +#include "internal/packet.h" #include "testutil.h" @@ -166,29 +167,17 @@ static int test_punycode(int n) static int test_a2ulabel(void) { char out[50]; - size_t outlen; /* - * Test that no buffer correctly returns the true length. * The punycode being passed in and parsed is malformed but we're not * verifying that behaviour here. */ - if (!TEST_int_eq(ossl_a2ulabel("xn--a.b.c", NULL, &outlen), 0) - || !TEST_size_t_eq(outlen, 7) - || !TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, &outlen), 1)) - return 0; - /* Test that a short input length returns the true length */ - outlen = 1; - if (!TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, &outlen), 0) - || !TEST_size_t_eq(outlen, 7) - || !TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, &outlen), 1) - || !TEST_str_eq(out,"\xc2\x80.b.c")) + if (!TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, 1), 0) + || !TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, 7), 1)) return 0; /* Test for an off by one on the buffer size works */ - outlen = 6; - if (!TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, &outlen), 0) - || !TEST_size_t_eq(outlen, 7) - || !TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, &outlen), 1) + if (!TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, 6), 0) + || !TEST_int_eq(ossl_a2ulabel("xn--a.b.c", out, 7), 1) || !TEST_str_eq(out,"\xc2\x80.b.c")) return 0; return 1; @@ -211,9 +200,57 @@ static int test_puny_overrun(void) return 1; } +static int test_dotted_overflow(void) +{ + static const char string[] = "a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a"; + const size_t num_reps = OSSL_NELEM(string) / 2; + WPACKET p; + BUF_MEM *in; + char *out = NULL; + size_t i; + int res = 0; + + /* Create out input punycode string */ + if (!TEST_ptr(in = BUF_MEM_new())) + return 0; + if (!TEST_true(WPACKET_init_len(&p, in, 0))) { + BUF_MEM_free(in); + return 0; + } + for (i = 0; i < num_reps; i++) { + if (i > 1 && !TEST_true(WPACKET_put_bytes_u8(&p, '.'))) + goto err; + if (!TEST_true(WPACKET_memcpy(&p, "xn--a", sizeof("xn--a") - 1))) + goto err; + } + if (!TEST_true(WPACKET_put_bytes_u8(&p, '\0'))) + goto err; + if (!TEST_ptr(out = OPENSSL_malloc(in->length))) + goto err; + + /* Test the decode into an undersized buffer */ + memset(out, 0x7f, in->length - 1); + if (!TEST_int_le(ossl_a2ulabel(in->data, out, num_reps), 0) + || !TEST_int_eq(out[num_reps], 0x7f)) + goto err; + + /* Test the decode works into a full size buffer */ + if (!TEST_int_gt(ossl_a2ulabel(in->data, out, in->length), 0) + || !TEST_size_t_eq(strlen(out), num_reps * 3)) + goto err; + + res = 1; + err: + WPACKET_cleanup(&p); + BUF_MEM_free(in); + OPENSSL_free(out); + return res; +} + int setup_tests(void) { ADD_ALL_TESTS(test_punycode, OSSL_NELEM(puny_cases)); + ADD_TEST(test_dotted_overflow); ADD_TEST(test_a2ulabel); ADD_TEST(test_puny_overrun); return 1;