From d4f1b566e14941245f101f443a073ca11aa64425 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 28 Aug 2017 16:30:39 +0300 Subject: [PATCH 1/3] prop224: Add test that exposes the #23343 bug. --- src/test/test_hs_common.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index b1e47adf24..b9215ea187 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -703,13 +703,14 @@ test_hid_serv_request_tracker(void *arg) { (void) arg; time_t retval; - routerstatus_t *hsdir = NULL, *hsdir2 = NULL; + routerstatus_t *hsdir = NULL, *hsdir2 = NULL, *hsdir3 = NULL; time_t now = approx_time(); const char *req_key_str_first = "vd4zb6zesaubtrjvdqcr2w7x7lhw2up4Xnw4526ThUNbL5o1go+EdUuEqlKxHkNbnK41pRzizzs"; const char *req_key_str_second = "g53o7iavcd62oihswhr24u6czmqws5kpXnw4526ThUNbL5o1go+EdUuEqlKxHkNbnK41pRzizzs"; + const char *req_key_str_small = "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ"; /*************************** basic test *******************************/ @@ -741,7 +742,7 @@ test_hid_serv_request_tracker(void *arg) /* Let's add another request for the same HS but on a different HSDir. */ hsdir2 = tor_malloc_zero(sizeof(routerstatus_t)); - memset(hsdir->identity_digest, 2, DIGEST_LEN); + memset(hsdir2->identity_digest, 2, DIGEST_LEN); retval = hs_lookup_last_hid_serv_request(hsdir2, req_key_str_first, now+3, 1); tt_int_op(retval, OP_EQ, now+3); @@ -755,16 +756,25 @@ test_hid_serv_request_tracker(void *arg) now+2, 0); tt_int_op(retval, OP_EQ, 0); + /* Now let's add a smaller req key str */ + hsdir3 = tor_malloc_zero(sizeof(routerstatus_t)); + memset(hsdir3->identity_digest, 3, DIGEST_LEN); + retval = hs_lookup_last_hid_serv_request(hsdir3, req_key_str_small, + now+4, 1); + tt_int_op(retval, OP_EQ, now+4); + tt_int_op(strmap_size(request_tracker),OP_EQ, 2); + /*************************** deleting entries **************************/ /* Add another request with very short key */ retval = hs_lookup_last_hid_serv_request(hsdir, "l", now, 1); + tt_int_op(strmap_size(request_tracker),OP_EQ, 3); /* Try deleting entries with a dummy key. Check that our previous requests * are still there */ tor_capture_bugs_(1); hs_purge_hid_serv_from_last_hid_serv_requests("a"); - tt_int_op(strmap_size(request_tracker),OP_EQ, 2); + tt_int_op(strmap_size(request_tracker),OP_EQ, 3); tor_end_capture_bugs_(); /* Try another dummy key. Check that requests are still there */ @@ -773,16 +783,16 @@ test_hid_serv_request_tracker(void *arg) memset(dummy, 'Z', 2000); dummy[1999] = '\x00'; hs_purge_hid_serv_from_last_hid_serv_requests(dummy); - tt_int_op(strmap_size(request_tracker),OP_EQ, 2); + tt_int_op(strmap_size(request_tracker),OP_EQ, 3); } /* Another dummy key! */ hs_purge_hid_serv_from_last_hid_serv_requests(req_key_str_second); - tt_int_op(strmap_size(request_tracker),OP_EQ, 2); + tt_int_op(strmap_size(request_tracker),OP_EQ, 3); /* Now actually delete a request! */ hs_purge_hid_serv_from_last_hid_serv_requests(req_key_str_first); - tt_int_op(strmap_size(request_tracker),OP_EQ, 1); + tt_int_op(strmap_size(request_tracker),OP_EQ, 2); /* Purge it all! */ hs_purge_last_hid_serv_requests(); @@ -792,6 +802,7 @@ test_hid_serv_request_tracker(void *arg) done: tor_free(hsdir); tor_free(hsdir2); + tor_free(hsdir3); } static void From 93a0a4a4221b5c60b3c4fe22d38c81914176dedb Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 28 Aug 2017 16:30:51 +0300 Subject: [PATCH 2/3] 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 03dd07f6ca..a4b8ba3e8d 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; } From e94f68e301caf2d01659e7ba24fb1ba678ad3233 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Mon, 28 Aug 2017 19:55:26 +0300 Subject: [PATCH 3/3] fixup! prop224: Fix length check when purging hidserv requests. Improve doc based on david's comments. --- src/or/hs_common.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/or/hs_common.c b/src/or/hs_common.c index a4b8ba3e8d..d866ab6a8f 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -1357,7 +1357,7 @@ hs_hsdir_requery_period(const or_options_t *options) * * where 'hsdir_identity' is the identity digest of the HSDir node, and * 'hs_identity' is the descriptor ID of the HS in the v2 case, or the ed25519 - * identity public key of the HS in the v3 case. */ + * blinded public key of the HS in the v3 case. */ static strmap_t *last_hid_serv_requests_ = NULL; /** Returns last_hid_serv_requests_, initializing it to a new strmap if @@ -1454,10 +1454,11 @@ hs_purge_hid_serv_from_last_hid_serv_requests(const char *req_key_str) * semantic, see #23305. */ /* 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". */ + * check on the strings we are about to compare. The key is variable sized + * since it's composed as follows: + * key = base32(hsdir_identity) + base32(req_key_str) + * where 'req_key_str' is the descriptor ID of the HS in the v2 case, or + * the ed25519 blinded public key of the HS in the v3 case. */ if (strlen(key) < REND_DESC_ID_V2_LEN_BASE32 + strlen(req_key_str)) { iter = strmap_iter_next(last_hid_serv_requests, iter); continue;