Bug 1713562 - Validate ECH public names, r=bbeurdouche

This validates that they are LDH (with underscore because we don't hate
freedom), but that they are not IP addresses.  This invokes the horrible WhatWG
IP parsing routines, so that it recognizes a vast array of crazy address formats
(thanks 1980s design).

Differential Revision: https://phabricator.services.mozilla.com/D116343

9 files changed, 342 insertions, 22 deletions

diff --git a/gtests/ssl_gtest/libssl_internals.c b/gtests/ssl_gtest/libssl_internals.c
index b6a2bd3be..963299659 100644
--- a/gtests/ssl_gtest/libssl_internals.c
+++ b/gtests/ssl_gtest/libssl_internals.c
@@ -497,3 +497,7 @@ SECStatus SSLInt_SetRawEchConfigForRetry(PRFileDesc *fd, const uint8_t *buf,
   PORT_Memcpy(cfg->raw.data, buf, len);
   return SECSuccess;
 }
+
+PRBool SSLInt_IsIp(PRUint8 *s, unsigned int len) {
+  return tls13_IsIp(s, len);
+}
diff --git a/gtests/ssl_gtest/libssl_internals.h b/gtests/ssl_gtest/libssl_internals.h
index c21c4a7da..70c6520c5 100644
--- a/gtests/ssl_gtest/libssl_internals.h
+++ b/gtests/ssl_gtest/libssl_internals.h
@@ -51,4 +51,6 @@ SECStatus SSLInt_SetDCAdvertisedSigSchemes(PRFileDesc *fd,
 SECStatus SSLInt_RemoveServerCertificates(PRFileDesc *fd);
 SECStatus SSLInt_SetRawEchConfigForRetry(PRFileDesc *fd, const uint8_t *buf,
                                          size_t len);
+PRBool SSLInt_IsIp(PRUint8 *s, unsigned int len);
+
 #endif  // ifndef libssl_internals_h_
diff --git a/gtests/ssl_gtest/tls_ech_unittest.cc b/gtests/ssl_gtest/tls_ech_unittest.cc
index 9d082c6f5..dbfb06dec 100644
--- a/gtests/ssl_gtest/tls_ech_unittest.cc
+++ b/gtests/ssl_gtest/tls_ech_unittest.cc
@@ -177,6 +177,45 @@ class TlsConnectStreamTls13Ech : public TlsConnectTestBase {
                                       echconfig.len()));
   }
 
+  void ValidatePublicNames(const std::vector<std::string>& names,
+                           SECStatus expected) {
+    static const std::vector<HpkeSymmetricSuite> kSuites = {
+        {HpkeKdfHkdfSha256, HpkeAeadAes128Gcm}};
+
+    SECKEYECParams ecParams = {siBuffer, NULL, 0};
+    MakeEcKeyParams(&ecParams, ssl_grp_ec_curve25519);
+
+    ScopedSECKEYPublicKey pub;
+    ScopedSECKEYPrivateKey priv;
+    SECKEYPublicKey* pub_p = nullptr;
+    SECKEYPrivateKey* priv_p =
+        SECKEY_CreateECPrivateKey(&ecParams, &pub_p, nullptr);
+    pub.reset(pub_p);
+    priv.reset(priv_p);
+    ASSERT_TRUE(!!pub);
+    ASSERT_TRUE(!!priv);
+
+    EnsureTlsSetup();
+
+    DataBuffer cfg;
+    SECStatus rv;
+    for (auto name : names) {
+      if (g_ssl_gtest_verbose) {
+        std::cout << ((expected == SECFailure) ? "in" : "")
+                  << "valid public_name: " << name << std::endl;
+      }
+      GenerateEchConfig(HpkeDhKemX25519Sha256, kSuites, name, 100, cfg, pub,
+                        priv);
+
+      rv = SSL_SetServerEchConfigs(server_->ssl_fd(), pub.get(), priv.get(),
+                                   cfg.data(), cfg.len());
+      EXPECT_EQ(expected, rv);
+
+      rv = SSL_SetClientEchConfigs(client_->ssl_fd(), cfg.data(), cfg.len());
+      EXPECT_EQ(expected, rv);
+    }
+  }
+
  private:
   // Testing certan invalid CHInner configurations is tricky, particularly
   // since the CHOuter forms AAD and isn't available in filters. Instead of
@@ -1774,6 +1813,108 @@ TEST_F(TlsConnectStreamTls13, EchBackendAcceptance) {
   server_->ExpectReceiveAlert(kTlsAlertCloseNotify, kTlsAlertWarning);
 }
 
+// A public_name that includes an IP address has to be rejected.
+TEST_F(TlsConnectStreamTls13Ech, EchPublicNameIp) {
+  static const std::vector<std::string> kIps = {
+      "0.0.0.0",
+      "1.1.1.1",
+      "255.255.255.255",
+      "255.255.65535",
+      "255.16777215",
+      "4294967295",
+      "0377.0377.0377.0377",
+      "0377.0377.0177777",
+      "0377.077777777",
+      "037777777777",
+      "00377.00377.00377.00377",
+      "00377.00377.00177777",
+      "00377.0077777777",
+      "0037777777777",
+      "0xff.0xff.0xff.0xff",
+      "0xff.0xff.0xffff",
+      "0xff.0xffffff",
+      "0xffffffff",
+      "0XFF.0XFF.0XFF.0XFF",
+      "0XFF.0XFF.0XFFFF",
+      "0XFF.0XFFFFFF",
+      "0XFFFFFFFF",
+      "0x0ff.0x0ff.0x0ff.0x0ff",
+      "0x0ff.0x0ff.0x0ffff",
+      "0x0ff.0x0ffffff",
+      "0x0ffffffff",
+      "00000000000000000000000000000000000000000",
+      "00000000000000000000000000000000000000001",
+      "127.0.0.1",
+      "127.0.1",
+      "127.1",
+      "2130706433",
+      "017700000001",
+  };
+  ValidatePublicNames(kIps, SECFailure);
+}
+
+// These are nearly IP addresses.
+TEST_F(TlsConnectStreamTls13Ech, EchPublicNameNotIp) {
+  static const std::vector<std::string> kNotIps = {
+      "0.0.0.0.0",
+      "1.2.3.4.5",
+      "999999999999999999999999999999999",
+      "07777777777777777777777777777777777777777",
+      "111111111100000000001111111111000000000011111111110000000000123",
+      "256.255.255.255",
+      "255.256.255.255",
+      "255.255.256.255",
+      "255.255.255.256",
+      "255.255.65536",
+      "255.16777216",
+      "4294967296",
+      "0400.0377.0377.0377",
+      "0377.0400.0377.0377",
+      "0377.0377.0400.0377",
+      "0377.0377.0377.0400",
+      "0377.0377.0200000",
+      "0377.0100000000",
+      "040000000000",
+      "0x100.0xff.0xff.0xff",
+      "0xff.0x100.0xff.0xff",
+      "0xff.0xff.0x100.0xff",
+      "0xff.0xff.0xff.0x100",
+      "0xff.0xff.0x10000",
+      "0xff.0x1000000",
+      "0x100000000",
+      "08",
+      "09",
+      "a",
+      "0xg",
+      "0XG",
+      "0x",
+      "0x.1.2.3",
+      "test-name",
+      "test-name.test",
+      "TEST-NAME",
+      "under_score",
+      "_under_score",
+      "under_score_",
+  };
+  ValidatePublicNames(kNotIps, SECSuccess);
+}
+
+TEST_F(TlsConnectStreamTls13Ech, EchPublicNameNotLdh) {
+  static const std::vector<std::string> kNotLdh = {
+      ".",
+      "name.",
+      ".name",
+      "test..name",
+      "1111111111000000000011111111110000000000111111111100000000001234",
+      "-name",
+      "name-",
+      "test-.name",
+      "!",
+      u8"\u2077",
+  };
+  ValidatePublicNames(kNotLdh, SECFailure);
+}
+
 INSTANTIATE_TEST_SUITE_P(EchAgentTest, TlsAgentEchTest,
                          ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
                                             TlsConnectTestBase::kTlsV13));
diff --git a/lib/ssl/manifest.mn b/lib/ssl/manifest.mn
index d3e38b85b..fedc42b4e 100644
--- a/lib/ssl/manifest.mn
+++ b/lib/ssl/manifest.mn
@@ -21,24 +21,32 @@ EXPORTS = \
 MODULE = nss
 
 CSRCS = \
-        dtlscon.c \
+        authcert.c \
+        cmpcert.c \
         dtls13con.c \
+        dtlscon.c \
         prelib.c \
+        selfencrypt.c \
         ssl3con.c \
+        ssl3ecc.c \
+        ssl3ext.c \
+        ssl3exthandle.c \
         ssl3gthr.c \
         sslauth.c \
         sslbloom.c \
+        sslcert.c \
         sslcon.c \
         ssldef.c \
         sslencode.c \
         sslenum.c \
         sslerr.c \
         sslerrstrs.c \
+        sslgrp.c \
+        sslinfo.c \
         sslinit.c \
-        ssl3ext.c \
-        ssl3exthandle.c \
         sslmutex.c \
         sslnonce.c \
+        sslprimitive.c \
         sslreveal.c \
         sslsecur.c \
         sslsnce.c \
@@ -46,21 +54,14 @@ CSRCS = \
         sslspec.c \
         ssltrace.c \
         sslver.c \
-        authcert.c \
-        cmpcert.c \
-        selfencrypt.c \
-        sslinfo.c \
-        ssl3ecc.c \
         tls13con.c \
+        tls13ech.c \
+        tls13echv.c \
         tls13exthandle.c \
         tls13hashstate.c \
         tls13hkdf.c \
         tls13psk.c \
         tls13replay.c \
-        sslcert.c \
-        sslgrp.c \
-        sslprimitive.c \
-        tls13ech.c \
         tls13subcerts.c \
         $(NULL)
 
diff --git a/lib/ssl/ssl.gyp b/lib/ssl/ssl.gyp
index c3c792805..2aa35cc96 100644
--- a/lib/ssl/ssl.gyp
+++ b/lib/ssl/ssl.gyp
@@ -45,6 +45,7 @@
         'sslver.c',
         'tls13con.c',
         'tls13ech.c',
+        'tls13echv.c',
         'tls13exthandle.c',
         'tls13hashstate.c',
         'tls13hkdf.c',
diff --git a/lib/ssl/tls13ech.c b/lib/ssl/tls13ech.c
index 1891caf39..ff6a634d8 100644
--- a/lib/ssl/tls13ech.c
+++ b/lib/ssl/tls13ech.c
@@ -243,11 +243,10 @@ tls13_DecodeEchConfigContents(const sslReadBuffer *rawConfig,
         PORT_SetError(SSL_ERROR_RX_MALFORMED_ECH_CONFIG);
         goto loser;
     }
-    for (tmpn = 0; tmpn < tmpBuf.len; tmpn++) {
-        if (tmpBuf.buf[tmpn] == '\0') {
-            PORT_SetError(SSL_ERROR_RX_MALFORMED_ECH_CONFIG);
-            goto loser;
-        }
+    if (!tls13_IsLDH(tmpBuf.buf, tmpBuf.len) ||
+        tls13_IsIp(tmpBuf.buf, tmpBuf.len)) {
+        PORT_SetError(SSL_ERROR_RX_MALFORMED_ECH_CONFIG);
+        goto loser;
     }
 
     contents.publicName = PORT_ZAlloc(tmpBuf.len + 1);
@@ -603,9 +602,11 @@ SSLExp_RemoveEchConfigs(PRFileDesc *fd)
         return SECFailure;
     }
 
-    if (!PR_CLIST_IS_EMPTY(&ss->echConfigs)) {
-        tls13_DestroyEchConfigs(&ss->echConfigs);
-    }
+    SECKEY_DestroyPrivateKey(ss->echPrivKey);
+    ss->echPrivKey = NULL;
+    SECKEY_DestroyPublicKey(ss->echPubKey);
+    ss->echPubKey = NULL;
+    tls13_DestroyEchConfigs(&ss->echConfigs);
 
     /* Also remove any retry_configs and handshake context. */
     if (ss->xtnData.ech && ss->xtnData.ech->retryConfigs.len) {
diff --git a/lib/ssl/tls13ech.h b/lib/ssl/tls13ech.h
index 55abf76ea..83f92a452 100644
--- a/lib/ssl/tls13ech.h
+++ b/lib/ssl/tls13ech.h
@@ -90,4 +90,7 @@ SECStatus tls13_MaybeAcceptEch(sslSocket *ss, const SECItem *sidBytes, const PRU
 SECStatus tls13_MaybeGreaseEch(sslSocket *ss, unsigned int prefixLen, sslBuffer *buf);
 SECStatus tls13_WriteServerEchSignal(sslSocket *ss, PRUint8 *sh, unsigned int shLen);
 
+PRBool tls13_IsIp(const PRUint8 *str, unsigned int len);
+PRBool tls13_IsLDH(const PRUint8 *str, unsigned int len);
+
 #endif
diff --git a/lib/ssl/tls13echv.c b/lib/ssl/tls13echv.c
new file mode 100644
index 000000000..ae9792a91
--- /dev/null
+++ b/lib/ssl/tls13echv.c
@@ -0,0 +1,167 @@
+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/* Validation functions for ECH public names. */
+
+#include "seccomon.h"
+
+/* Convert a single character `c` into a number `*d` with the given radix.
+ * Fails if the character isn't valid for the radix.
+ */
+static SECStatus
+tls13_IpDigit(PRUint8 c, PRUint8 radix, PRUint8 *d)
+{
+    PRUint8 v = 0xff;
+    if (c >= '0' && c <= '9') {
+        v = c - '0';
+    } else if (radix > 10) {
+        if (c >= 'a' && c <= 'f') {
+            v = c - 'a';
+        } else if (c >= 'A' && c <= 'F') {
+            v = c - 'A';
+        }
+    }
+    if (v >= radix) {
+        return SECFailure;
+    }
+    *d = v;
+    return SECSuccess;
+}
+
+/* This function takes the first couple of characters from `str`, starting at offset
+ * `*i` and calculates a radix.  If it starts with "0x" or "0X", then `*i` is moved up
+ * by two and `*radix` is set to 16 (hexadecimal).  If it starts with "0", then `*i` is
+ * moved up by one and `*radix` is set to 8 (octal).  Otherwise, `*i` is left alone and
+ * `*radix` is set to 10 (decimal).
+ * Fails if there are no characters remaining or the next character is '.', either at
+ * the start or after "0x".
+ */
+static SECStatus
+tls13_IpRadix(const PRUint8 *str, unsigned int len, unsigned int *i, PRUint8 *radix)
+{
+    if (*i == len || str[*i] == '.') {
+        return SECFailure;
+    }
+    if (str[*i] == '0') {
+        (*i)++;
+        if (*i < len && (str[*i] == 'x' || str[*i] == 'X')) {
+            (*i)++;
+            if (*i == len || str[*i] == '.') {
+                return SECFailure;
+            }
+            *radix = 16;
+        } else {
+            *radix = 8;
+        }
+    } else {
+        *radix = 10;
+    }
+    return SECSuccess;
+}
+
+/* Take a number from `str` from offset `*i` and put the value in `*v`.
+ * This calculates the radix and returns a value between 0 and 2^32-1, using all
+ * of the digits up to the end of the string (determined by `len`) or a period ('.').
+ * Fails if there is no value, if there a non-digit characters, or if the value is
+ * too large.
+ */
+static SECStatus
+tls13_IpValue(const PRUint8 *str, unsigned int len, unsigned int *i, PRUint32 *v)
+{
+    PRUint8 radix;
+    SECStatus rv = tls13_IpRadix(str, len, i, &radix);
+    if (rv != SECSuccess) {
+        return SECFailure;
+    }
+    PRUint64 part = 0;
+    while (*i < len) {
+        PRUint8 d;
+        rv = tls13_IpDigit(str[*i], radix, &d);
+        if (rv != SECSuccess) {
+            if (str[*i] != '.') {
+                return SECFailure;
+            }
+            break;
+        }
+        part = part * radix + d;
+        if (part > PR_UINT32_MAX) {
+            return SECFailure;
+        }
+        (*i)++;
+    }
+    *v = part;
+    return SECSuccess;
+}
+
+/* Returns true if `end` is true and `v` is within the `limit`. Used to validate the
+ * last part of an IPv4 address, which can hold larger numbers if there are fewer then
+ * four parts. */
+static PRBool
+tls13_IpLastPart(PRBool end, PRUint32 v, PRUint32 limit)
+{
+    if (!end) {
+        return PR_FALSE;
+    }
+    return v <= limit;
+}
+
+/* Returns true if `str` contains an IPv4 address. */
+PRBool
+tls13_IsIp(const PRUint8 *str, unsigned int len)
+{
+    PRUint32 part;
+    PRUint32 v;
+    unsigned int i = 0;
+    for (part = 0; part < 4; part++) {
+        SECStatus rv = tls13_IpValue(str, len, &i, &v);
+        if (rv != SECSuccess) {
+            return PR_FALSE;
+        }
+        if (v > 0xff || i == len) {
+            return tls13_IpLastPart(i == len, v, PR_UINT32_MAX >> (part * 8));
+        }
+        PORT_Assert(str[i] == '.');
+        i++;
+    }
+
+    return tls13_IpLastPart(i == len, v, 0xff);
+}
+
+static PRBool
+tls13_IsLD(PRUint8 c)
+{
+    return (c >= 'a' && c <= 'z') ||
+           (c >= 'A' && c <= 'Z') ||
+           (c >= '0' && c <= '9') ||
+           c == '_'; /* not in spec, but in the world; bug 1136616 */
+}
+
+/* Is this a valid dotted LDH string (that is, an A-Label domain name)?
+ * This does not tolerate a trailing '.', where the DNS generally does.
+ */
+PRBool
+tls13_IsLDH(const PRUint8 *str, unsigned int len)
+{
+    unsigned int i = 0;
+    while (i < len && tls13_IsLD(str[i])) {
+        unsigned int labelEnd = PR_MIN(len, i + 63);
+        i++;
+        while (i < labelEnd && (tls13_IsLD(str[i]) || str[i] == '-')) {
+            i++;
+        }
+        if (str[i - 1] == '-') {
+            /* labels cannot end in a hyphen */
+            return PR_FALSE;
+        }
+        if (i == len) {
+            return PR_TRUE;
+        }
+        if (str[i] != '.') {
+            return PR_FALSE;
+        }
+        i++;
+    }
+    return PR_FALSE;
+}
diff --git a/lib/util/seccomon.h b/lib/util/seccomon.h
index edd5c5329..2c6be3502 100644
--- a/lib/util/seccomon.h
+++ b/lib/util/seccomon.h
@@ -61,7 +61,7 @@ struct SECItemArrayStr {
 };
 
 /*
-** A status code. Status's are used by procedures that return status
+** A status code. Statuses are used by procedures that return status
 ** values. Again the motivation is so that a compiler can generate
 ** warnings when return values are wrong. Correct testing of status codes:
 **
@@ -78,7 +78,7 @@ typedef enum _SECStatus {
 } SECStatus;
 
 /*
-** A comparison code. Used for procedures that return comparision
+** A comparison code. Used for procedures that return comparison
 ** values. Again the motivation is so that a compiler can generate
 ** warnings when return values are wrong.
 */