From b46984e97ec4064ac8178ea9b3bf6985a4f2f632 Mon Sep 17 00:00:00 2001 From: Alexander Færøy Date: Tue, 31 Mar 2020 02:33:54 +0000 Subject: 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 --- src/lib/tls/tortls_nss.c | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) (limited to 'src/lib/tls/tortls_nss.c') diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 3c62e98df1..f7792e07a2 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; } -- cgit v1.2.3-54-g00ecf From 06f1e959c218bfbe0b85bbd0acc59b8f408fbc99 Mon Sep 17 00:00:00 2001 From: Alexander Færøy Date: Sat, 16 May 2020 15:34:37 +0000 Subject: Add constness to length variables in `tor_tls_cert_matches_key`. We add constness to `peer_info_orig_len` and `cert_info_orig_len` in `tor_tls_cert_matches_key` to ensure that we don't accidentally alter the variables. This patch is part of the fix for TROVE-2020-001. See: https://bugs.torproject.org/33119 --- src/lib/tls/tortls_nss.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/lib/tls/tortls_nss.c') diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index f7792e07a2..f1ef3ef277 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -739,8 +739,8 @@ tor_tls_cert_matches_key,(const tor_tls_t *tls, * 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; + const unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; + const 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); -- cgit v1.2.3-54-g00ecf From 7b2d10700fb0844fbe9aa7c030b09467cf173936 Mon Sep 17 00:00:00 2001 From: Alexander Færøy Date: Sat, 16 May 2020 19:18:56 +0000 Subject: Use ((x + 7) >> 3) instead of (x >> 3) when converting from bits to bytes. This patch changes our bits-to-bytes conversion logic in the NSS implementation of `tor_tls_cert_matches_key()` from using (x >> 3) to ((x + 7) >> 3) since DER bit-strings are allowed to contain a number of bits that is not a multiple of 8. Additionally, we add a comment on why we cannot use the `DER_ConvertBitString()` macro from NSS, as we would potentially apply the bits-to-bytes conversion logic twice, which would lead to an insignificant amount of bytes being compared in `SECITEM_ItemsAreEqual()` and thus turn the logic into being a prefix match instead of a full match. The `DER_ConvertBitString()` macro is defined in NSS as: /* ** Macro to convert der decoded bit string into a decoded octet ** string. All it needs to do is fiddle with the length code. */ #define DER_ConvertBitString(item) \ { \ (item)->len = ((item)->len + 7) >> 3; \ } Thanks to Taylor Yu for spotting this problem. This patch is part of the fix for TROVE-2020-001. See: https://bugs.torproject.org/33119 --- src/lib/tls/tortls_nss.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 'src/lib/tls/tortls_nss.c') diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index f1ef3ef277..1436442e1c 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -742,14 +742,23 @@ tor_tls_cert_matches_key,(const tor_tls_t *tls, const unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len; const 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); + /* 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; -- cgit v1.2.3-54-g00ecf