[tor-commits] [tor/release-0.4.3] Use ((x + 7) >> 3) instead of (x >> 3) when converting from bits to bytes.

[nickm@xxxxxxxxxxxxxx]

commit 7b2d10700fb0844fbe9aa7c030b09467cf173936
Author: Alexander Færøy <ahf@xxxxxxxxxxxxxx>
Date:   Sat May 16 19:18:56 2020 +0000

    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(-)

diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c
index f1ef3ef27..1436442e1 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;
 



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits