Merge branch 'maint-0.4.2' into release-0.4.2

4 files changed, 118 insertions, 76 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/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c
index 3c62e98df1..1436442e1c 100644
--- a/src/lib/tls/tortls_nss.c
+++ b/src/lib/tls/tortls_nss.c
@@ -713,23 +713,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_tortls.c b/src/test/test_tortls.c
index 5f87434f6a..d59b8e001d 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 81e2d3aa4f..93135d0b19 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 /* !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)
@@ -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),