nihilist/tor
1
0
Fork 0
mirror of https://gitlab.torproject.org/tpo/core/tor.git synced 2024-11-10 05:03:43 +01:00
Code Issues Packages Projects Releases Wiki Activity

Use ((x + 7) >> 3) instead of (x >> 3) when converting from bits to bytes.

Browse Source 
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
This commit is contained in:
Alexander Færøy 2020-05-16 19:18:56 +00:00 committed by Nick Mathewson
parent 06f1e959c2
commit 7b2d10700f
1 changed files with 11 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
src/lib/tls/tortls_nss.c
View File

@ -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;