diff options
| -rw-r--r-- | changes/bug33119 | 4 | ||||
| -rw-r--r-- | changes/ticket33796 | 7 | ||||
| -rw-r--r-- | doc/tor.1.txt | 16 | ||||
| -rw-r--r-- | src/app/config/config.c | 2 | ||||
| -rw-r--r-- | src/lib/tls/tortls_nss.c | 47 | ||||
| -rw-r--r-- | src/test/test_config.c | 2 | ||||
| -rw-r--r-- | src/test/test_tortls.c | 73 | ||||
| -rw-r--r-- | src/test/test_tortls_openssl.c | 70 |
8 files changed, 133 insertions, 88 deletions
diff --git a/changes/bug33119 b/changes/bug33119 new file mode 100644 index 0000000000..c976654b26 --- /dev/null +++ b/changes/bug33119 @@ -0,0 +1,4 @@ + o Major bugfixes (NSS): + - Fix out-of-bound memory access in `tor_tls_cert_matches_key()` when Tor is + compiled with NSS support. Fixes bug 33119; bugfix on 0.3.5.1-alpha. This + issue is also tracked as TROVE-2020-001. diff --git a/changes/ticket33796 b/changes/ticket33796 new file mode 100644 index 0000000000..9a98bf2d9a --- /dev/null +++ b/changes/ticket33796 @@ -0,0 +1,7 @@ + o Removed features (IPv6, revert): + - Revert the client port prefer IPv6 feature because it breaks the + torsocks use case. The SOCKS resolve command is lacking a mechanism to + ask for a specific address family (v4 or v6) thus prioritizing IPv6 when + an IPv4 address is asked on the resolve SOCKS interface resulting in a + failure. Tor Browser explicitly set PreferIPv6 so this should not affect + the majority of our users. Closes ticket 33796; bugfix on 0.4.4.1-alpha. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 7b3150e2a4..9d073635af 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1492,16 +1492,14 @@ The following options are useful only for clients (that is, if Other recognized __flags__ for a SocksPort are: **NoIPv4Traffic**;; Tell exits to not connect to IPv4 addresses in response to SOCKS - requests on this connection. (Allowing IPv4 is the default.) - **NoIPv6Traffic**;; - Tell exits to not connect to IPv6 addresses in response to SOCKS - requests on this connection. This option is only relevant when SOCKS5 - is in use, because SOCKS4 can't handle IPv6. (Allowing IPv6 is the - default.) - **NoPreferIPv6**;; + requests on this connection. + **IPv6Traffic**;; + Tell exits to allow IPv6 addresses in response to SOCKS requests on + this connection, so long as SOCKS5 is in use. (SOCKS4 can't handle + IPv6.) + **PreferIPv6**;; Tells exits that, if a host has both an IPv4 and an IPv6 address, - we would prefer to connect to it via IPv4. (IPv6 is the default in - recent versions of Tor.) + we would prefer to connect to it via IPv6. (IPv4 is the default.) **NoDNSRequest**;; Do not ask exits to resolve DNS addresses in SOCKS5 requests. Tor will connect to IPv4 addresses, IPv6 addresses (if IPv6Traffic is set) and diff --git a/src/app/config/config.c b/src/app/config/config.c index 71f8c18ca2..ba2cecd9ab 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -5610,7 +5610,7 @@ port_cfg_new(size_t namelen) /* entry_cfg flags */ cfg->entry_cfg.ipv4_traffic = 1; cfg->entry_cfg.ipv6_traffic = 1; - cfg->entry_cfg.prefer_ipv6 = 1; + cfg->entry_cfg.prefer_ipv6 = 0; cfg->entry_cfg.dns_request = 1; cfg->entry_cfg.onion_traffic = 1; cfg->entry_cfg.prefer_ipv6_virtaddr = 1; diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index a9ec731c0a..adbfcad0a4 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -726,23 +726,58 @@ MOCK_IMPL(int, tor_tls_cert_matches_key,(const tor_tls_t *tls, const struct tor_x509_cert_t *cert)) { - tor_assert(tls); tor_assert(cert); + tor_assert(cert->cert); + int rv = 0; - CERTCertificate *peercert = SSL_PeerCertificate(tls->ssl); - if (!peercert) + tor_x509_cert_t *peercert = tor_tls_get_peer_cert((tor_tls_t *)tls); + + if (!peercert || !peercert->cert) goto done; - CERTSubjectPublicKeyInfo *peer_info = &peercert->subjectPublicKeyInfo; + + CERTSubjectPublicKeyInfo *peer_info = &peercert->cert->subjectPublicKeyInfo; CERTSubjectPublicKeyInfo *cert_info = &cert->cert->subjectPublicKeyInfo; + + /* NSS stores the `len` field in bits, instead of bytes, for the + * `subjectPublicKey` field in CERTSubjectPublicKeyInfo, but + * `SECITEM_ItemsAreEqual()` compares the two bitstrings using a length field + * defined in bytes. + * + * We convert the `len` field from bits to bytes, do our comparison with + * `SECITEM_ItemsAreEqual()`, and reset the length field from bytes to bits + * again. + * + * See also NSS's own implementation of `SECKEY_CopySubjectPublicKeyInfo()` + * in seckey.c in the NSS source tree. This function also does the conversion + * between bits and bytes. + */ + const unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; + const unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len; + + /* We convert the length from bits to bytes, but instead of using NSS's + * `DER_ConvertBitString()` macro on both of peer_info->subjectPublicKey and + * cert_info->subjectPublicKey, we have to do the conversion explicitly since + * both of the two subjectPublicKey fields are allowed to point to the same + * memory address. Otherwise, the bits to bytes conversion would potentially + * be applied twice, which would lead to us comparing too few of the bytes + * when we call SECITEM_ItemsAreEqual(), which would be catastrophic. + */ + peer_info->subjectPublicKey.len = ((peer_info_orig_len + 7) >> 3); + cert_info->subjectPublicKey.len = ((cert_info_orig_len + 7) >> 3); + rv = SECOID_CompareAlgorithmID(&peer_info->algorithm, &cert_info->algorithm) == 0 && SECITEM_ItemsAreEqual(&peer_info->subjectPublicKey, &cert_info->subjectPublicKey); + /* Convert from bytes back to bits. */ + peer_info->subjectPublicKey.len = peer_info_orig_len; + cert_info->subjectPublicKey.len = cert_info_orig_len; + done: - if (peercert) - CERT_DestroyCertificate(peercert); + tor_x509_cert_free(peercert); + return rv; } diff --git a/src/test/test_config.c b/src/test/test_config.c index 095eb24c49..cb0d9bba28 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -4160,8 +4160,6 @@ test_config_parse_port_config__ports__ports_given(void *data) /* Test entry port defaults as initialised in port_parse_config */ tt_int_op(port_cfg->entry_cfg.dns_request, OP_EQ, 1); tt_int_op(port_cfg->entry_cfg.ipv4_traffic, OP_EQ, 1); - tt_int_op(port_cfg->entry_cfg.ipv6_traffic, OP_EQ, 1); - tt_int_op(port_cfg->entry_cfg.prefer_ipv6, OP_EQ, 1); tt_int_op(port_cfg->entry_cfg.onion_traffic, OP_EQ, 1); tt_int_op(port_cfg->entry_cfg.cache_ipv4_answers, OP_EQ, 0); tt_int_op(port_cfg->entry_cfg.prefer_ipv6_virtaddr, OP_EQ, 1); diff --git a/src/test/test_tortls.c b/src/test/test_tortls.c index a822bc5ad8..12ba873650 100644 --- a/src/test/test_tortls.c +++ b/src/test/test_tortls.c @@ -105,6 +105,17 @@ const char* caCertString = "-----BEGIN CERTIFICATE-----\n" "Yy1RT69d0rwYc5u/vnqODz1IjvT90smsrkBumGt791FAFeg=\n" "-----END CERTIFICATE-----\n"; +static tor_x509_cert_t *fixed_x509_cert = NULL; +static tor_x509_cert_t * +get_peer_cert_mock_return_fixed(tor_tls_t *tls) +{ + (void)tls; + if (fixed_x509_cert) + return tor_x509_cert_dup(fixed_x509_cert); + else + return NULL; +} + tor_x509_cert_impl_t * read_cert_from(const char *str) { @@ -513,6 +524,67 @@ test_tortls_verify(void *ignored) crypto_pk_free(k); } +static void +test_tortls_cert_matches_key(void *ignored) +{ + (void)ignored; + + tor_x509_cert_impl_t *cert1 = NULL, + *cert2 = NULL, + *cert3 = NULL, + *cert4 = NULL; + tor_x509_cert_t *c1 = NULL, *c2 = NULL, *c3 = NULL, *c4 = NULL; + crypto_pk_t *k1 = NULL, *k2 = NULL, *k3 = NULL; + + k1 = pk_generate(1); + k2 = pk_generate(2); + k3 = pk_generate(3); + + cert1 = tor_tls_create_certificate(k1, k2, "A", "B", 1000); + cert2 = tor_tls_create_certificate(k1, k3, "C", "D", 1000); + cert3 = tor_tls_create_certificate(k2, k3, "C", "D", 1000); + cert4 = tor_tls_create_certificate(k3, k2, "E", "F", 1000); + + tt_assert(cert1 && cert2 && cert3 && cert4); + + c1 = tor_x509_cert_new(cert1); cert1 = NULL; + c2 = tor_x509_cert_new(cert2); cert2 = NULL; + c3 = tor_x509_cert_new(cert3); cert3 = NULL; + c4 = tor_x509_cert_new(cert4); cert4 = NULL; + + tt_assert(c1 && c2 && c3 && c4); + + MOCK(tor_tls_get_peer_cert, get_peer_cert_mock_return_fixed); + + fixed_x509_cert = NULL; + /* If the peer has no certificate, it shouldn't match anything. */ + tt_assert(! tor_tls_cert_matches_key(NULL, c1)); + tt_assert(! tor_tls_cert_matches_key(NULL, c2)); + tt_assert(! tor_tls_cert_matches_key(NULL, c3)); + tt_assert(! tor_tls_cert_matches_key(NULL, c4)); + fixed_x509_cert = c1; + /* If the peer has a certificate, it should match every cert with the same + * subject key. */ + tt_assert(tor_tls_cert_matches_key(NULL, c1)); + tt_assert(tor_tls_cert_matches_key(NULL, c2)); + tt_assert(! tor_tls_cert_matches_key(NULL, c3)); + tt_assert(! tor_tls_cert_matches_key(NULL, c4)); + + done: + tor_x509_cert_free(c1); + tor_x509_cert_free(c2); + tor_x509_cert_free(c3); + tor_x509_cert_free(c4); + if (cert1) tor_x509_cert_impl_free(cert1); + if (cert2) tor_x509_cert_impl_free(cert2); + if (cert3) tor_x509_cert_impl_free(cert3); + if (cert4) tor_x509_cert_impl_free(cert4); + crypto_pk_free(k1); + crypto_pk_free(k2); + crypto_pk_free(k3); + UNMOCK(tor_tls_get_peer_cert); +} + #define LOCAL_TEST_CASE(name, flags) \ { #name, test_tortls_##name, (flags|TT_FORK), NULL, NULL } @@ -533,5 +605,6 @@ struct testcase_t tortls_tests[] = { LOCAL_TEST_CASE(is_server, 0), LOCAL_TEST_CASE(bridge_init, TT_FORK), LOCAL_TEST_CASE(verify, TT_FORK), + LOCAL_TEST_CASE(cert_matches_key, 0), END_OF_TESTCASES }; diff --git a/src/test/test_tortls_openssl.c b/src/test/test_tortls_openssl.c index 4318f7f1eb..e20b0d1ede 100644 --- a/src/test/test_tortls_openssl.c +++ b/src/test/test_tortls_openssl.c @@ -475,75 +475,6 @@ fake_x509_free(X509 *cert) } #endif /* !defined(OPENSSL_OPAQUE) */ -static tor_x509_cert_t *fixed_x509_cert = NULL; -static tor_x509_cert_t * -get_peer_cert_mock_return_fixed(tor_tls_t *tls) -{ - (void)tls; - if (fixed_x509_cert) - return tor_x509_cert_dup(fixed_x509_cert); - else - return NULL; -} - -static void -test_tortls_cert_matches_key(void *ignored) -{ - (void)ignored; - - X509 *cert1 = NULL, *cert2 = NULL, *cert3 = NULL, *cert4 = NULL; - tor_x509_cert_t *c1 = NULL, *c2 = NULL, *c3 = NULL, *c4 = NULL; - crypto_pk_t *k1 = NULL, *k2 = NULL, *k3 = NULL; - - k1 = pk_generate(1); - k2 = pk_generate(2); - k3 = pk_generate(3); - - cert1 = tor_tls_create_certificate(k1, k2, "A", "B", 1000); - cert2 = tor_tls_create_certificate(k1, k3, "C", "D", 1000); - cert3 = tor_tls_create_certificate(k2, k3, "C", "D", 1000); - cert4 = tor_tls_create_certificate(k3, k2, "E", "F", 1000); - - tt_assert(cert1 && cert2 && cert3 && cert4); - - c1 = tor_x509_cert_new(cert1); cert1 = NULL; - c2 = tor_x509_cert_new(cert2); cert2 = NULL; - c3 = tor_x509_cert_new(cert3); cert3 = NULL; - c4 = tor_x509_cert_new(cert4); cert4 = NULL; - - tt_assert(c1 && c2 && c3 && c4); - - MOCK(tor_tls_get_peer_cert, get_peer_cert_mock_return_fixed); - - fixed_x509_cert = NULL; - /* If the peer has no certificate, it shouldn't match anything. */ - tt_assert(! tor_tls_cert_matches_key(NULL, c1)); - tt_assert(! tor_tls_cert_matches_key(NULL, c2)); - tt_assert(! tor_tls_cert_matches_key(NULL, c3)); - tt_assert(! tor_tls_cert_matches_key(NULL, c4)); - fixed_x509_cert = c1; - /* If the peer has a certificate, it should match every cert with the same - * subject key. */ - tt_assert(tor_tls_cert_matches_key(NULL, c1)); - tt_assert(tor_tls_cert_matches_key(NULL, c2)); - tt_assert(! tor_tls_cert_matches_key(NULL, c3)); - tt_assert(! tor_tls_cert_matches_key(NULL, c4)); - - done: - tor_x509_cert_free(c1); - tor_x509_cert_free(c2); - tor_x509_cert_free(c3); - tor_x509_cert_free(c4); - if (cert1) X509_free(cert1); - if (cert2) X509_free(cert2); - if (cert3) X509_free(cert3); - if (cert4) X509_free(cert4); - crypto_pk_free(k1); - crypto_pk_free(k2); - crypto_pk_free(k3); - UNMOCK(tor_tls_get_peer_cert); -} - #ifndef OPENSSL_OPAQUE static void test_tortls_cert_get_key(void *ignored) @@ -2275,7 +2206,6 @@ struct testcase_t tortls_openssl_tests[] = { INTRUSIVE_TEST_CASE(get_error, TT_FORK), LOCAL_TEST_CASE(always_accept_verify_cb, 0), INTRUSIVE_TEST_CASE(x509_cert_free, 0), - LOCAL_TEST_CASE(cert_matches_key, 0), INTRUSIVE_TEST_CASE(cert_get_key, 0), LOCAL_TEST_CASE(get_my_client_auth_key, TT_FORK), INTRUSIVE_TEST_CASE(get_ciphersuite_name, 0), |