GitLab is used only for code review, issue tracking and project management. Canonical locations for source code are still https://gitweb.torproject.org/ https://git.torproject.org/ and git-rw.torproject.org.

Commit b46984e9 authored by Alexander Færøy's avatar Alexander Færøy 🍍 Committed by Nick Mathewson

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
parent 33e1c2e6
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.
......@@ -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;
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment