[tor-commits] [tor/maint-0.4.3] Fix out-of-bound memory read in `tor_tls_cert_matches_key()` for NSS.

[nickm at torproject.org]

commit b46984e97ec4064ac8178ea9b3bf6985a4f2f632
Author: Alexander Færøy <ahf at 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;
 }