July 2020 - tor-commits - lists.torproject.org

[tor/master] Revert "config: Make clients tell dual-stack exits they prefer IPv6"
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 66d5292ee6a173b21e5a54a669e8e584c6781e3a Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Jul 7 12:05:27 2020 -0400 Revert "config: Make clients tell dual-stack exits they prefer IPv6" This reverts commit bf2a399fc0d90df76e091fa3259f7c1b8fb87781. Don't set by default the prefer IPv6 feature on client ports 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 #33796 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket33796 | 7 +++++++ doc/tor.1.txt | 16 +++++++--------- src/app/config/config.c | 2 +- src/test/test_config.c | 2 -- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/changes/ticket33796 b/changes/ticket33796 new file mode 100644 index 000000000..9a98bf2d9 --- /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 7b3150e2a..9d073635a 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 71f8c18ca..ba2cecd9a 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/test/test_config.c b/src/test/test_config.c index 095eb24c4..cb0d9bba2 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);

1 0

[tor/master] Merge remote-tracking branch 'dgoulet/ticket33796_044_01' into maint-0.4.4
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 8dd89e433078245778924ce67403673e9d35a5c3 Merge: 0f39cc10f 66d5292ee Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:59:45 2020 -0400 Merge remote-tracking branch 'dgoulet/ticket33796_044_01' into maint-0.4.4 changes/ticket33796 | 7 +++++++ doc/tor.1.txt | 16 +++++++--------- src/app/config/config.c | 2 +- src/test/test_config.c | 2 -- 4 files changed, 15 insertions(+), 12 deletions(-)

1 0

[dev/master] Change readme
by hiro＠torproject.org 09 Jul '20

09 Jul '20

commit a26dc2a594945d2c4d835ad642c0abf7ea9d54a5 Author: hiro <hiro(a)torproject.org> Date: Thu Jul 9 15:55:26 2020 +0200 Change readme --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 35f6c38..8c148e3 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,6 @@ Check our [wiki pages](https://gitlab.torproject.org/tpo/web/dev/wikis/How-to-de To help us to translate, please join the Tor Project team in [Transifex](https://www.transifex.com/). - ### Getting help If you want to contribute to the Community portal, we will be happy to help you. Join us at #tor-www in [irc.oftc.net](https://www.oftc.net).

1 0

[tor/release-0.4.4] Fold in more changelog entries, hope to finish changelog
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 781dd1bd8ad43a7a3397d1d63dd3bf00c8e6d091 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:46:53 2020 -0400 Fold in more changelog entries, hope to finish changelog --- ChangeLog | 35 ++++++++++++++++++++++++++++++++++- changes/bug33119 | 4 ---- changes/bug34084 | 3 --- changes/bug40028 | 3 --- changes/ticket33873 | 4 ---- changes/ticket40026 | 3 --- 6 files changed, 34 insertions(+), 18 deletions(-) diff --git a/ChangeLog b/ChangeLog index 495f9f83c..4cc0fbd27 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,8 +1,23 @@ -Changes in version 0.4.4.2-alpha - 2020-07-?? +Changes in version 0.4.4.2-alpha - 2020-07-09 This is the second alpha release in the 0.4.4.x series. It fixes a few bugs in the previous release, and solves a few usability, compatibility, and portability issues. + This release also fixes TROVE-2020-001, a medium-severity denial of + service vulnerability affecting all versions of Tor when compiled with + the NSS encryption library. (This is not the default configuration.) + Using this vulnerability, an attacker could cause an affected Tor + instance to crash remotely. This issue is also tracked as CVE-2020- + 15572. Anybody running a version of Tor built with the NSS library + should upgrade to 0.3.5.11, 0.4.2.8, 0.4.3.6, or 0.4.4.2-alpha + or later. + + o Major bugfixes (NSS, security): + - Fix a crash due to an out-of-bound memory access 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 + and CVE-2020-15572. + o Minor features (bootstrap reporting): - Report more detailed reasons for bootstrap failure when the failure happens due to a TLS error. Previously we would just call @@ -29,6 +44,19 @@ Changes in version 0.4.4.2-alpha - 2020-07-?? - Permit the unlinkat() syscall, which some Libc implementations use to implement unlink(). Closes ticket 33346. + o Minor bugfix (CI, Windows): + - Use the correct 64-bit printf format when compiling with MINGW on + Appveyor. Fixes bug 40026; bugfix on 0.3.5.5-alpha. + + o Minor bugfix (SOCKS, onion service client): + - Detect v3 onion service addresses of the wrong length when + returning the F6 ExtendedErrors code. Fixes bug 33873; bugfix + on 0.4.3.1-alpha. + + o Minor bugfixes (compiler warnings): + - Fix a compiler warning on platforms with 32-bit time_t values. + Fixes bug 40028; bugfix on 0.3.2.8-rc. + o Minor bugfixes (control port, onion service): - Consistently use 'address' in "Invalid v3 address" response to ONION_CLIENT_AUTH commands. Previously, we would sometimes say @@ -39,6 +67,11 @@ Changes in version 0.4.4.2-alpha - 2020-07-?? receiving an extrainfo document that we no longer want. Fixes bug 16016; bugfix on 0.2.6.3-alpha. + o Minor bugfixes (onion services v3): + - Avoid a non-fatal assertion failure in certain edge-cases when + opening an intro circuit as a client. Fixes bug 34084; bugfix + on 0.3.2.1-alpha. + o Deprecated features (onion service v2): - Add a deprecation warning for version 2 onion services. Closes ticket 40003. diff --git a/changes/bug33119 b/changes/bug33119 deleted file mode 100644 index c976654b2..000000000 --- a/changes/bug33119 +++ /dev/null @@ -1,4 +0,0 @@ - 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/bug34084 b/changes/bug34084 deleted file mode 100644 index 524c4cf68..000000000 --- a/changes/bug34084 +++ /dev/null @@ -1,3 +0,0 @@ - o Minor bugfixes (onion services v3): - - Avoid a non-fatal assert log in an edge-case of opening an intro circuit - as a client. Fixes bug 34084; bugfix on 0.3.2.1-alpha. diff --git a/changes/bug40028 b/changes/bug40028 deleted file mode 100644 index cfd1ffe51..000000000 --- a/changes/bug40028 +++ /dev/null @@ -1,3 +0,0 @@ - o Minor bugfixes (compiler warnings): - - Fix a compiler warning on platforms with 32-bit time_t values. - Fixes bug 40028; bugfix on 0.3.2.8-rc. diff --git a/changes/ticket33873 b/changes/ticket33873 deleted file mode 100644 index c45191181..000000000 --- a/changes/ticket33873 +++ /dev/null @@ -1,4 +0,0 @@ - o Minor bugfix (SOCKS, onion service client): - - Also detect bad v3 onion service address of the wrong length when - returning the F6 ExtendedErrors code. Fixes bug 33873; bugfix on - 0.4.3.1-alpha. diff --git a/changes/ticket40026 b/changes/ticket40026 deleted file mode 100644 index f87c2964e..000000000 --- a/changes/ticket40026 +++ /dev/null @@ -1,3 +0,0 @@ - o Minor bugfix (CI, Windows): - - Don't use stdio 64 bit printf format when compiling with MINGW on - Appveyor. Fixes bug 40026; bugfix on 0.3.5.5-alpha.

1 0

[tor/release-0.3.5] Add constness to length variables in `tor_tls_cert_matches_key`.
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 06f1e959c218bfbe0b85bbd0acc59b8f408fbc99 Author: Alexander Færøy <ahf(a)torproject.org> Date: Sat May 16 15:34:37 2020 +0000 Add constness to length variables in `tor_tls_cert_matches_key`. We add constness to `peer_info_orig_len` and `cert_info_orig_len` in `tor_tls_cert_matches_key` to ensure that we don't accidentally alter the variables. This patch is part of the fix for TROVE-2020-001. See: https://bugs.torproject.org/33119 --- src/lib/tls/tortls_nss.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index f7792e07a..f1ef3ef27 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -739,8 +739,8 @@ tor_tls_cert_matches_key,(const tor_tls_t *tls, * in seckey.c in the NSS source tree. This function also does the conversion * between bits and bytes. */ - unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; - unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len; + const unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; + const unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len; peer_info->subjectPublicKey.len = (peer_info_orig_len >> 3); cert_info->subjectPublicKey.len = (cert_info_orig_len >> 3);

1 0

[tor/release-0.3.5] Fix out-of-bound memory read in `tor_tls_cert_matches_key()` for NSS.
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit b46984e97ec4064ac8178ea9b3bf6985a4f2f632 Author: Alexander Færøy <ahf(a)torproject.org> Date: Tue Mar 31 02:33:54 2020 +0000 Fix out-of-bound memory read in `tor_tls_cert_matches_key()` for NSS. This patch fixes an out-of-bound memory read in `tor_tls_cert_matches_key()` when Tor is compiled to use Mozilla's NSS instead of OpenSSL. The NSS library stores some length fields in bits instead of bytes, but the comparison function found in `SECITEM_ItemsAreEqual()` needs the length to be encoded in bytes. This means that for a 140-byte, DER-encoded, SubjectPublicKeyInfo struct (with a 1024-bit RSA public key in it), we would ask `SECITEM_ItemsAreEqual()` to compare the first 1120 bytes instead of 140 (140bytes * 8bits = 1120bits). This patch fixes the issue by converting from bits to bytes before calling `SECITEM_ItemsAreEqual()` and convert the `len`-fields back to bits before we leave the function. This patch is part of the fix for TROVE-2020-001. See: https://bugs.torproject.org/33119 --- changes/bug33119 | 4 ++++ src/lib/tls/tortls_nss.c | 38 ++++++++++++++++++++++++++++++++------ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/changes/bug33119 b/changes/bug33119 new file mode 100644 index 000000000..c976654b2 --- /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/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 3c62e98df..f7792e07a 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -713,23 +713,49 @@ 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. + */ + unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; + unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len; + + peer_info->subjectPublicKey.len = (peer_info_orig_len >> 3); + cert_info->subjectPublicKey.len = (cert_info_orig_len >> 3); + rv = SECOID_CompareAlgorithmID(&peer_info->algorithm, &cert_info->algorithm) == 0 && SECITEM_ItemsAreEqual(&peer_info->subjectPublicKey, &cert_info->subjectPublicKey); + 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; }

1 0

[tor/release-0.3.5] Merge branch 'trove_2020_001_035' into maint-0.3.5
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 7142f3e435f99e357c7a7b55d4542ac7f6584f47 Merge: 3e08dd9df 7b2d10700 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:28:36 2020 -0400 Merge branch 'trove_2020_001_035' into maint-0.3.5 changes/bug33119 | 4 +++ src/lib/tls/tortls_nss.c | 47 +++++++++++++++++++++++---- src/test/test_tortls.c | 73 ++++++++++++++++++++++++++++++++++++++++++ src/test/test_tortls_openssl.c | 70 ---------------------------------------- 4 files changed, 118 insertions(+), 76 deletions(-)

1 0

[tor/release-0.3.5] Run `tor_tls_cert_matches_key()` Test Suite with both OpenSSL and NSS.
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 33e1c2e6fd614f8cb42a6d5758d411d3f8d5411c Author: Alexander Færøy <ahf(a)torproject.org> Date: Tue Mar 31 02:28:12 2020 +0000 Run `tor_tls_cert_matches_key()` Test Suite with both OpenSSL and NSS. This patch lifts the `tor_tls_cert_matches_key()` tests out of the OpenSSL specific TLS test suite and moves it into the generic TLS test suite that is executed for both OpenSSL and NSS. This patch is largely a code movement, but we had to rewrite parts of the test to avoid using OpenSSL specific data-types (such as `X509 *`) and replace it with the generic Tor abstraction type (`tor_x509_cert_impl_t *`). This patch is part of the fix for TROVE-2020-001. See: https://bugs.torproject.org/33119 --- src/test/test_tortls.c | 73 ++++++++++++++++++++++++++++++++++++++++++ src/test/test_tortls_openssl.c | 70 ---------------------------------------- 2 files changed, 73 insertions(+), 70 deletions(-) diff --git a/src/test/test_tortls.c b/src/test/test_tortls.c index 11e35be2f..853abc4f9 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 73041a871..f039980a2 100644 --- a/src/test/test_tortls_openssl.c +++ b/src/test/test_tortls_openssl.c @@ -479,75 +479,6 @@ fake_x509_free(X509 *cert) } #endif -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) @@ -2279,7 +2210,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),

1 0

[tor/release-0.3.5] Use ((x + 7) >> 3) instead of (x >> 3) when converting from bits to bytes.
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 7b2d10700fb0844fbe9aa7c030b09467cf173936 Author: Alexander Færøy <ahf(a)torproject.org> Date: Sat May 16 19:18:56 2020 +0000 Use ((x + 7) >> 3) instead of (x >> 3) when converting from bits to bytes. This patch changes our bits-to-bytes conversion logic in the NSS implementation of `tor_tls_cert_matches_key()` from using (x >> 3) to ((x + 7) >> 3) since DER bit-strings are allowed to contain a number of bits that is not a multiple of 8. Additionally, we add a comment on why we cannot use the `DER_ConvertBitString()` macro from NSS, as we would potentially apply the bits-to-bytes conversion logic twice, which would lead to an insignificant amount of bytes being compared in `SECITEM_ItemsAreEqual()` and thus turn the logic into being a prefix match instead of a full match. The `DER_ConvertBitString()` macro is defined in NSS as: /* ** Macro to convert der decoded bit string into a decoded octet ** string. All it needs to do is fiddle with the length code. */ #define DER_ConvertBitString(item) \ { \ (item)->len = ((item)->len + 7) >> 3; \ } Thanks to Taylor Yu for spotting this problem. This patch is part of the fix for TROVE-2020-001. See: https://bugs.torproject.org/33119 --- src/lib/tls/tortls_nss.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index f1ef3ef27..1436442e1 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -742,14 +742,23 @@ tor_tls_cert_matches_key,(const tor_tls_t *tls, const unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; const unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len; - peer_info->subjectPublicKey.len = (peer_info_orig_len >> 3); - cert_info->subjectPublicKey.len = (cert_info_orig_len >> 3); + /* 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;

1 0

[tor/release-0.4.3] Add constness to length variables in `tor_tls_cert_matches_key`.
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 06f1e959c218bfbe0b85bbd0acc59b8f408fbc99 Author: Alexander Færøy <ahf(a)torproject.org> Date: Sat May 16 15:34:37 2020 +0000 Add constness to length variables in `tor_tls_cert_matches_key`. We add constness to `peer_info_orig_len` and `cert_info_orig_len` in `tor_tls_cert_matches_key` to ensure that we don't accidentally alter the variables. This patch is part of the fix for TROVE-2020-001. See: https://bugs.torproject.org/33119 --- src/lib/tls/tortls_nss.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index f7792e07a..f1ef3ef27 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -739,8 +739,8 @@ tor_tls_cert_matches_key,(const tor_tls_t *tls, * in seckey.c in the NSS source tree. This function also does the conversion * between bits and bytes. */ - unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; - unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len; + const unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; + const unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len; peer_info->subjectPublicKey.len = (peer_info_orig_len >> 3); cert_info->subjectPublicKey.len = (cert_info_orig_len >> 3);

1 0