Merge branch 'maint-0.4.2' into maint-0.4.3

1 files changed, 41 insertions, 6 deletions

diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c
index 62e8262115..38c7efe107 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;
 }