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 <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/19591)

crypto/punycode.c

@@ -12,6 +12,7 @@
 #include <openssl/e_os2.h>
 #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;

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;
     }

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<xn-->.
 
 =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.

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

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;