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

[tor/master] 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/maint-0.4.4] Merge branch 'maint-0.3.5' into maint-0.4.2
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 283ce30c536568129f0a2b531d4c195c93c7ff52 Merge: b81e24cda 7142f3e43 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:28:53 2020 -0400 Merge branch 'maint-0.3.5' into maint-0.4.2 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(-) diff --cc src/test/test_tortls_openssl.c index 81e2d3aa4,f039980a2..93135d0b1 --- a/src/test/test_tortls_openssl.c +++ b/src/test/test_tortls_openssl.c @@@ -477,77 -477,8 +477,8 @@@ fake_x509_free(X509 *cert tor_free(cert); } } -#endif +#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)

1 0

[tor/master] 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/master] 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/master] 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/master] Merge branch 'maint-0.4.4'
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit b0399c60269647678b873f3ca47cabb3888bbd8f Merge: 32a0bc2a8 0f39cc10f Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:28:53 2020 -0400 Merge branch 'maint-0.4.4' 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/master] Merge branch 'maint-0.4.2' into maint-0.4.3
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit af08dad6d1b00f1f433787d2712568b07f782729 Merge: c364e0e83 283ce30c5 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:28:53 2020 -0400 Merge branch 'maint-0.4.2' into maint-0.4.3 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/master] Merge branch 'maint-0.3.5' into maint-0.4.2
by nickm＠torproject.org 09 Jul '20

09 Jul '20

commit 283ce30c536568129f0a2b531d4c195c93c7ff52 Merge: b81e24cda 7142f3e43 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:28:53 2020 -0400 Merge branch 'maint-0.3.5' into maint-0.4.2 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(-) diff --cc src/test/test_tortls_openssl.c index 81e2d3aa4,f039980a2..93135d0b1 --- a/src/test/test_tortls_openssl.c +++ b/src/test/test_tortls_openssl.c @@@ -477,77 -477,8 +477,8 @@@ fake_x509_free(X509 *cert tor_free(cert); } } -#endif +#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)

1 0

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

09 Jul '20

commit 0f39cc10f65df9fa52123eb607f5f1474c10e58b Merge: 1af7f40da af08dad6d Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jul 9 09:28:53 2020 -0400 Merge branch 'maint-0.4.3' into maint-0.4.4 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/maint-0.4.4] Merge branch 'tor-github/pr/1988' into maint-0.4.4
by dgoulet＠torproject.org 09 Jul '20

09 Jul '20

commit 1af7f40dad505c506f2237262bc7c03fe7681820 Merge: 562957e0d c1598be1e Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Jul 9 07:27:13 2020 -0400 Merge branch 'tor-github/pr/1988' into maint-0.4.4 changes/bug34084 | 3 +++ src/feature/hs/hs_client.c | 36 +++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 15 deletions(-)

1 0