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