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

diff --git a/changes/bug33119 b/changes/bug33119
new file mode 100644 (file)
index 0000000..c976654
--- /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 3c62e98df1a0d0907ce2cd5ec496d2c4f4c0ba24..f7792e07a2ca80f02b12facfa21969555e76bc68 100644 (file)
--- 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;
 }