From 94a1b8b66cb0e6d9a996210e8695a07d8ada95ac Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 10:21:35 -0400 Subject: [PATCH 1/4] Add a unit test for decompressing concatenated inputs. --- src/test/test_util.c | 63 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/test/test_util.c b/src/test/test_util.c index 0d9dd974df..7db93324d1 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -2398,6 +2398,59 @@ test_util_compress(void *arg) ; } +static void +test_util_decompress_concatenated_impl(compress_method_t method) +{ + char input[4096]; + char *c1 = NULL, *c2 = NULL, *c3 = NULL; + char *result = NULL; + size_t sz1, sz2, sz3, szr; + int r; + + crypto_rand(input, sizeof(input)); + + /* Compress the input in two chunks. */ + r = tor_compress(&c1, &sz1, input, 2048, method); + tt_int_op(r, OP_EQ, 0); + r = tor_compress(&c2, &sz2, input+2048, 2048, method); + tt_int_op(r, OP_EQ, 0); + + /* concatenate the chunks. */ + sz3 = sz1 + sz2; + c3 = tor_malloc(sz3); + memcpy(c3, c1, sz1); + memcpy(c3+sz1, c2, sz2); + + /* decompress the concatenated result */ + r = tor_uncompress(&result, &szr, c3, sz3, method, 0, LOG_WARN); + tt_int_op(r, OP_EQ, 0); + tt_int_op(szr, OP_EQ, sizeof(input)); + tt_mem_op(result, OP_EQ, input, sizeof(input)); + + done: + tor_free(c1); + tor_free(c2); + tor_free(c3); + tor_free(result); +} + +static void +test_util_decompress_concatenated(void *arg) +{ + const char *methodname = arg; + tt_assert(methodname); + + compress_method_t method = compression_method_get_by_name(methodname); + tt_int_op(method, OP_NE, UNKNOWN_METHOD); + if (! tor_compress_supports_method(method)) { + tt_skip(); + } + + test_util_decompress_concatenated_impl(method); + done: + ; +} + static void test_util_gzip_compression_bomb(void *arg) { @@ -5819,6 +5872,11 @@ test_util_get_unquoted_path(void *arg) { "compress/" #name, test_util_compress, 0, &passthrough_setup, \ (char*)(identifier) } +#define COMPRESS_CONCAT(name, identifier) \ + { "compress_concat/" #name, test_util_decompress_concatenated, 0, \ + &passthrough_setup, \ + (char*)(identifier) } + #ifdef _WIN32 #define UTIL_TEST_NO_WIN(n, f) { #n, NULL, TT_SKIP, NULL, NULL } #define UTIL_TEST_WIN_ONLY(n, f) UTIL_TEST(n, (f)) @@ -5848,6 +5906,11 @@ struct testcase_t util_tests[] = { COMPRESS(lzma, "x-tor-lzma"), COMPRESS(zstd, "x-zstd"), COMPRESS(none, "identity"), + COMPRESS_CONCAT(zlib, "deflate"), + COMPRESS_CONCAT(gzip, "gzip"), + COMPRESS_CONCAT(lzma, "x-tor-lzma"), + COMPRESS_CONCAT(zstd, "x-zstd"), + COMPRESS_CONCAT(none, "identity"), UTIL_TEST(gzip_compression_bomb, TT_FORK), UTIL_LEGACY(datadir), UTIL_LEGACY(memarea), From eb632afb17aad58a23f3c8edce865ddf9a338f20 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 10:24:22 -0400 Subject: [PATCH 2/4] Correct the fix to bug 22629 to permit trailing non-garbage This change makes it so that we can decompress concatenated zstd outputs. --- src/common/compress.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/common/compress.c b/src/common/compress.c index e65894d9d2..c6c3897989 100644 --- a/src/common/compress.c +++ b/src/common/compress.c @@ -150,13 +150,7 @@ tor_compress_impl(int compress, method, compression_level, in_len); goto err; } else { - if (in_len != 0) { - log_fn(protocol_warn_level, LD_PROTOCOL, - "Unexpected extra input while decompressing"); - log_debug(LD_GENERAL, "method: %d level: %d at len: %zd", - method, compression_level, in_len); - goto err; - } else { + if (in_len == 0) { goto done; } } From 782eb02b79363bf8ed609495d516bc59972f4352 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 11:26:51 -0400 Subject: [PATCH 3/4] Send the correct content-encoding when serving cached_dir_t objects A cached_dir_t object (for now) is always compressed with DEFLATE_METHOD, but in handle_get_status_vote() to we were using the general compression-negotiation code decide what compression to claim we were using. This was one of the reasons behind 22502. Fixes bug 22669; bugfix on 0.3.1.1-alpha --- changes/bug22669 | 4 ++++ src/or/directory.c | 25 +++++++++++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 changes/bug22669 diff --git a/changes/bug22669 b/changes/bug22669 new file mode 100644 index 0000000000..804a39e781 --- /dev/null +++ b/changes/bug22669 @@ -0,0 +1,4 @@ + o Minor bugfixes (compression): + - When serving directory votes compressed with zlib, + do not claim to have compressed them with zstd. Fixes bug 22669; + bugfix on 0.3.1.1-alpha. diff --git a/src/or/directory.c b/src/or/directory.c index ecd98119fe..b680b134a4 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -4190,13 +4190,14 @@ static int handle_get_status_vote(dir_connection_t *conn, const get_handler_args_t *args) { const char *url = args->url; - const compress_method_t compress_method = - find_best_compression_method(args->compression_supported, 1); { int current; ssize_t body_len = 0; ssize_t estimated_len = 0; + /* This smartlist holds strings that we can compress on the fly. */ smartlist_t *items = smartlist_new(); + /* This smartlist holds cached_dir_t objects that have a precompressed + * deflated version. */ smartlist_t *dir_items = smartlist_new(); int lifetime = 60; /* XXXX?? should actually use vote intervals. */ url += strlen("/tor/status-vote/"); @@ -4247,6 +4248,26 @@ handle_get_status_vote(dir_connection_t *conn, const get_handler_args_t *args) write_http_status_line(conn, 404, "Not found"); goto vote_done; } + + /* We're sending items from at most one kind of source */ + tor_assert_nonfatal(smartlist_len(items) == 0 || + smartlist_len(dir_items) == 0); + + int streaming; + unsigned mask; + if (smartlist_len(items)) { + /* We're taking strings and compressing them on the fly. */ + streaming = 1; + mask = ~0u; + } else { + /* We're taking cached_dir_t objects. We only have them uncompressed + * or deflated. */ + streaming = 0; + mask = (1u<compression_supported&mask, streaming); + SMARTLIST_FOREACH(dir_items, cached_dir_t *, d, body_len += compress_method != NO_METHOD ? d->dir_compressed_len : d->dir_len); From 9328bd524e38befe240fd729a9febb4dfe8b5be4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 12:26:57 -0400 Subject: [PATCH 4/4] Enforce the rule that COMPRESS_OK means progress was made. If COMPRESS_OK occurs but data is neither consumed nor generated, treat it as a BUG and a COMPRESS_ERROR. This change is meant to prevent infinite loops in the case where we've made a mistake in one of our compression backends. Closes ticket 22672. --- changes/bug22672 | 5 +++++ src/common/compress.c | 36 +++++++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 11 deletions(-) create mode 100644 changes/bug22672 diff --git a/changes/bug22672 b/changes/bug22672 new file mode 100644 index 0000000000..ec6681149d --- /dev/null +++ b/changes/bug22672 @@ -0,0 +1,5 @@ + o Minor features (compression, defensive programming): + - Detect and break out of infinite loops in our compression code. + We don't think that any such loops exist now, but it's best to be + safe. Closes ticket 22672. + diff --git a/src/common/compress.c b/src/common/compress.c index c16d1b51ee..46aa1d52ba 100644 --- a/src/common/compress.c +++ b/src/common/compress.c @@ -548,28 +548,42 @@ tor_compress_process(tor_compress_state_t *state, int finish) { tor_assert(state != NULL); + const size_t in_len_orig = *in_len; + const size_t out_len_orig = *out_len; + tor_compress_output_t rv; switch (state->method) { case GZIP_METHOD: case ZLIB_METHOD: - return tor_zlib_compress_process(state->u.zlib_state, - out, out_len, in, in_len, - finish); + rv = tor_zlib_compress_process(state->u.zlib_state, + out, out_len, in, in_len, + finish); + break; case LZMA_METHOD: - return tor_lzma_compress_process(state->u.lzma_state, - out, out_len, in, in_len, - finish); + rv =tor_lzma_compress_process(state->u.lzma_state, + out, out_len, in, in_len, + finish); + break; case ZSTD_METHOD: - return tor_zstd_compress_process(state->u.zstd_state, - out, out_len, in, in_len, - finish); + rv = tor_zstd_compress_process(state->u.zstd_state, + out, out_len, in, in_len, + finish); + break; case NO_METHOD: - return tor_cnone_compress_process(out, out_len, in, in_len, - finish); + rv = tor_cnone_compress_process(out, out_len, in, in_len, + finish); + break; + default: case UNKNOWN_METHOD: goto err; } + if (BUG((rv == TOR_COMPRESS_OK) && + *in_len == in_len_orig && + *out_len == out_len_orig)) { + return TOR_COMPRESS_ERROR; + } + return rv; err: return TOR_COMPRESS_ERROR; }