[tor-commits] [tor/master] prop224: Fix length check when purging hidserv requests.

[nickm at torproject.org]

commit 93a0a4a4221b5c60b3c4fe22d38c81914176dedb
Author: George Kadianakis <desnacked at riseup.net>
Date:   Mon Aug 28 16:30:51 2017 +0300

    prop224: Fix length check when purging hidserv requests.
    
    That check was wrong:
    
    a) We should be making sure that the size of `key` is big enough before
       proceeding, since that's the buffer that we would overread with the
       tor_memeq() below.
    
       The old check used to check that `req_key_str` is big enough which is
       not right, since we won't read deep into that buffer.
    
       The new check makes sure that `key` has enough size to survive the
       tor_memeq(), and if not it moves to the next element of the strmap.
    
    b) That check shouldn't be a BUG since that strmap contains
       variable-sized elements and we should not be bugging out if we happen
       to compare a small sized element (v2) to a bigger one (v3).
---
 src/or/hs_common.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/or/hs_common.c b/src/or/hs_common.c
index 03dd07f6c..a4b8ba3e8 100644
--- a/src/or/hs_common.c
+++ b/src/or/hs_common.c
@@ -1453,14 +1453,12 @@ hs_purge_hid_serv_from_last_hid_serv_requests(const char *req_key_str)
     /* XXX: The use of REND_DESC_ID_V2_LEN_BASE32 is very wrong in terms of
      * semantic, see #23305. */
 
-    /* Length check on the strings we are about to compare. The "key" contains
-     * both the base32 HSDir identity digest and the requested key at the
+    /* This strmap contains variable-sized elements so this is a basic length
+     * check on the strings we are about to compare. The "key" contains both
+     * the base32 HSDir identity digest and the requested key at the
      * directory. The "req_key_str" can either be a base32 descriptor ID or a
-     * base64 blinded key which should be the second part of "key". BUG on
-     * this check because both strings are internally controlled so this
-     * should never happen. */
-    if (BUG((strlen(req_key_str) + REND_DESC_ID_V2_LEN_BASE32) <
-            strlen(key))) {
+     * base64 blinded key which should be the second part of "key". */
+    if (strlen(key) < REND_DESC_ID_V2_LEN_BASE32 + strlen(req_key_str)) {
       iter = strmap_iter_next(last_hid_serv_requests, iter);
       continue;
     }