Skip to content

Commit

Permalink
Bug 1713562 - Validate ECH public names, r=bbeurdouche
Browse files Browse the repository at this point in the history 
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

--HG--
extra : source : 27c19f19a885b7381fb263b1a56f88025b24e488
extra : amend_source : 8d6852c85e9d5e06e48228723edb144ffc64cc8f
  • Loading branch information
@martinthomson
martinthomson committed May 31, 2021
1 parent 35c3550 commit d96a53e
Show file tree
Hide file tree
Showing 9 changed files with 342 additions and 22 deletions.
4 changes: 4 additions & 0 deletions gtests/ssl_gtest/libssl_internals.c
View file
Expand Up @@ -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);
}
2 changes: 2 additions & 0 deletions gtests/ssl_gtest/libssl_internals.h
View file
Expand Up @@ -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_
141 changes: 141 additions & 0 deletions gtests/ssl_gtest/tls_ech_unittest.cc
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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));
Expand Down
25 changes: 13 additions & 12 deletions lib/ssl/manifest.mn
View file
Expand Up @@ -21,46 +21,47 @@ 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 \
sslsock.c \
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)

Expand Down
1 change: 1 addition & 0 deletions lib/ssl/ssl.gyp
View file
Expand Up @@ -45,6 +45,7 @@
'sslver.c',
'tls13con.c',
'tls13ech.c',
'tls13echv.c',
'tls13exthandle.c',
'tls13hashstate.c',
'tls13hkdf.c',
Expand Down
17 changes: 9 additions & 8 deletions lib/ssl/tls13ech.c
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions lib/ssl/tls13ech.h
View file
Expand Up @@ -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

0 comments on commit d96a53e

Please sign in to comment.