From c0e9698fca7cfc5d736f1df6b997c7d0bc5246b0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 11:43:37 -0400 Subject: [PATCH 1/5] Extract "decompress" portion of connection_dir_client_reached_eof() --- src/or/directory.c | 190 +++++++++++++++++++++++++-------------------- 1 file changed, 106 insertions(+), 84 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index ecd98119fe..52a24e08ef 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2191,6 +2191,108 @@ static int handle_response_fetch_renddesc_v2(dir_connection_t *, static int handle_response_upload_renddesc_v2(dir_connection_t *, const response_handler_args_t *); +static int +dir_client_decompress_response_body(char **bodyp, size_t *bodylenp, + dir_connection_t *conn, + compress_method_t compression, + int anonymized_connection) +{ + int rv = 0; + const char *body = *bodyp; + size_t body_len = *bodylenp; + int allow_partial = (conn->base_.purpose == DIR_PURPOSE_FETCH_SERVERDESC || + conn->base_.purpose == DIR_PURPOSE_FETCH_EXTRAINFO || + conn->base_.purpose == DIR_PURPOSE_FETCH_MICRODESC); + + int plausible = body_is_plausible(body, body_len, conn->base_.purpose); + if (compression != NO_METHOD || !plausible) { + int severity = LOG_DEBUG; + char *new_body = NULL; + size_t new_len = 0; + const char *description1, *description2; + int want_to_try_both = 0; + int tried_both = 0; + compress_method_t guessed = detect_compression_method(body, body_len); + + description1 = compression_method_get_human_name(compression); + + if (BUG(description1 == NULL)) + description1 = compression_method_get_human_name(UNKNOWN_METHOD); + + if (guessed == UNKNOWN_METHOD && !plausible) + description2 = "confusing binary junk"; + else + description2 = compression_method_get_human_name(guessed); + + /* Tell the user if we don't believe what we're told about compression.*/ + want_to_try_both = (compression == UNKNOWN_METHOD || + guessed != compression); + if (want_to_try_both) { + severity = LOG_INFO; + } + + tor_log(severity, LD_HTTP, + "HTTP body from server '%s:%d' was labeled as %s, " + "%s it seems to be %s.%s", + conn->base_.address, conn->base_.port, description1, + guessed != compression?"but":"and", + description2, + (compression>0 && guessed>0 && want_to_try_both)? + " Trying both.":""); + + /* Try declared compression first if we can. + * tor_compress_supports_method() also returns true for NO_METHOD. + * Ensure that the server is not sending us data compressed using a + * compression method that is not allowed for anonymous connections. */ + if (anonymized_connection && + ! allowed_anonymous_connection_compression_method(compression)) { + warn_disallowed_anonymous_compression_method(compression); + rv = -1; + goto done; + } + + if (tor_compress_supports_method(compression)) + tor_uncompress(&new_body, &new_len, body, body_len, compression, + !allow_partial, LOG_PROTOCOL_WARN); + + /* Okay, if that didn't work, and we think that it was compressed + * differently, try that. */ + if (anonymized_connection && + ! allowed_anonymous_connection_compression_method(guessed)) { + warn_disallowed_anonymous_compression_method(guessed); + rv = -1; + goto done; + } + + if (!new_body && tor_compress_supports_method(guessed) && + compression != guessed) { + tor_uncompress(&new_body, &new_len, body, body_len, guessed, + !allow_partial, LOG_PROTOCOL_WARN); + tried_both = 1; + } + /* If we're pretty sure that we have a compressed directory, and + * we didn't manage to uncompress it, then warn and bail. */ + if (!plausible && !new_body) { + log_fn(LOG_PROTOCOL_WARN, LD_HTTP, + "Unable to decompress HTTP body (tried %s%s%s, server '%s:%d').", + description1, + tried_both?" and ":"", + tried_both?description2:"", + conn->base_.address, conn->base_.port); + rv = -1; + goto done; + } + if (new_body) { + tor_free(*bodyp); + *bodyp = new_body; + *bodylenp = new_len; + } + } + + done: + return rv; +} + /** We are a client, and we've finished reading the server's * response. Parse it and act appropriately. * @@ -2211,7 +2313,6 @@ connection_dir_client_reached_eof(dir_connection_t *conn) time_t date_header = 0; long apparent_skew; compress_method_t compression; - int plausible; int skewed = 0; int rv; int allow_partial = (conn->base_.purpose == DIR_PURPOSE_FETCH_SERVERDESC || @@ -2325,89 +2426,10 @@ connection_dir_client_reached_eof(dir_connection_t *conn) goto done; } - plausible = body_is_plausible(body, body_len, conn->base_.purpose); - if (compression != NO_METHOD || !plausible) { - int severity = LOG_DEBUG; - char *new_body = NULL; - size_t new_len = 0; - const char *description1, *description2; - int want_to_try_both = 0; - int tried_both = 0; - compress_method_t guessed = detect_compression_method(body, body_len); - - description1 = compression_method_get_human_name(compression); - - if (BUG(description1 == NULL)) - description1 = compression_method_get_human_name(UNKNOWN_METHOD); - - if (guessed == UNKNOWN_METHOD && !plausible) - description2 = "confusing binary junk"; - else - description2 = compression_method_get_human_name(guessed); - - /* Tell the user if we don't believe what we're told about compression.*/ - want_to_try_both = (compression == UNKNOWN_METHOD || - guessed != compression); - if (want_to_try_both) { - severity = LOG_INFO; - } - - tor_log(severity, LD_HTTP, - "HTTP body from server '%s:%d' was labeled as %s, " - "%s it seems to be %s.%s", - conn->base_.address, conn->base_.port, description1, - guessed != compression?"but":"and", - description2, - (compression>0 && guessed>0 && want_to_try_both)? - " Trying both.":""); - - /* Try declared compression first if we can. - * tor_compress_supports_method() also returns true for NO_METHOD. - * Ensure that the server is not sending us data compressed using a - * compression method that is not allowed for anonymous connections. */ - if (anonymized_connection && - ! allowed_anonymous_connection_compression_method(compression)) { - warn_disallowed_anonymous_compression_method(compression); - rv = -1; - goto done; - } - - if (tor_compress_supports_method(compression)) - tor_uncompress(&new_body, &new_len, body, body_len, compression, - !allow_partial, LOG_PROTOCOL_WARN); - - /* Okay, if that didn't work, and we think that it was compressed - * differently, try that. */ - if (anonymized_connection && - ! allowed_anonymous_connection_compression_method(guessed)) { - warn_disallowed_anonymous_compression_method(guessed); - rv = -1; - goto done; - } - - if (!new_body && tor_compress_supports_method(guessed) && - compression != guessed) { - tor_uncompress(&new_body, &new_len, body, body_len, guessed, - !allow_partial, LOG_PROTOCOL_WARN); - tried_both = 1; - } - /* If we're pretty sure that we have a compressed directory, and - * we didn't manage to uncompress it, then warn and bail. */ - if (!plausible && !new_body) { - log_fn(LOG_PROTOCOL_WARN, LD_HTTP, - "Unable to decompress HTTP body (tried %s%s%s, server '%s:%d').", - description1, - tried_both?" and ":"", - tried_both?description2:"", - conn->base_.address, conn->base_.port); - rv = -1; - goto done; - } - if (new_body) { - tor_free(body); - body = new_body; - body_len = new_len; - } + if (dir_client_decompress_response_body(&body, &body_len, + conn, compression, anonymized_connection) < 0) { + rv = -1; + goto done; } response_handler_args_t args; From 9018da06c783dbc8a6d1e10b0c3ae2790d052981 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 11:46:54 -0400 Subject: [PATCH 2/5] Short-circuit the no-decompression-needed case, for clarity This commit is mostly just deindentation. --- src/or/directory.c | 149 +++++++++++++++++++++++---------------------- 1 file changed, 76 insertions(+), 73 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 52a24e08ef..89e4bbc492 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2205,88 +2205,91 @@ dir_client_decompress_response_body(char **bodyp, size_t *bodylenp, conn->base_.purpose == DIR_PURPOSE_FETCH_MICRODESC); int plausible = body_is_plausible(body, body_len, conn->base_.purpose); - if (compression != NO_METHOD || !plausible) { - int severity = LOG_DEBUG; - char *new_body = NULL; - size_t new_len = 0; - const char *description1, *description2; - int want_to_try_both = 0; - int tried_both = 0; - compress_method_t guessed = detect_compression_method(body, body_len); - description1 = compression_method_get_human_name(compression); + if (plausible && compression == NO_METHOD) { + return 0; + } - if (BUG(description1 == NULL)) - description1 = compression_method_get_human_name(UNKNOWN_METHOD); + int severity = LOG_DEBUG; + char *new_body = NULL; + size_t new_len = 0; + const char *description1, *description2; + int want_to_try_both = 0; + int tried_both = 0; + compress_method_t guessed = detect_compression_method(body, body_len); - if (guessed == UNKNOWN_METHOD && !plausible) - description2 = "confusing binary junk"; - else - description2 = compression_method_get_human_name(guessed); + description1 = compression_method_get_human_name(compression); - /* Tell the user if we don't believe what we're told about compression.*/ - want_to_try_both = (compression == UNKNOWN_METHOD || - guessed != compression); - if (want_to_try_both) { - severity = LOG_INFO; - } + if (BUG(description1 == NULL)) + description1 = compression_method_get_human_name(UNKNOWN_METHOD); - tor_log(severity, LD_HTTP, - "HTTP body from server '%s:%d' was labeled as %s, " - "%s it seems to be %s.%s", - conn->base_.address, conn->base_.port, description1, - guessed != compression?"but":"and", - description2, - (compression>0 && guessed>0 && want_to_try_both)? - " Trying both.":""); + if (guessed == UNKNOWN_METHOD && !plausible) + description2 = "confusing binary junk"; + else + description2 = compression_method_get_human_name(guessed); - /* Try declared compression first if we can. - * tor_compress_supports_method() also returns true for NO_METHOD. - * Ensure that the server is not sending us data compressed using a - * compression method that is not allowed for anonymous connections. */ - if (anonymized_connection && - ! allowed_anonymous_connection_compression_method(compression)) { - warn_disallowed_anonymous_compression_method(compression); - rv = -1; - goto done; - } + /* Tell the user if we don't believe what we're told about compression.*/ + want_to_try_both = (compression == UNKNOWN_METHOD || + guessed != compression); + if (want_to_try_both) { + severity = LOG_INFO; + } - if (tor_compress_supports_method(compression)) - tor_uncompress(&new_body, &new_len, body, body_len, compression, - !allow_partial, LOG_PROTOCOL_WARN); + tor_log(severity, LD_HTTP, + "HTTP body from server '%s:%d' was labeled as %s, " + "%s it seems to be %s.%s", + conn->base_.address, conn->base_.port, description1, + guessed != compression?"but":"and", + description2, + (compression>0 && guessed>0 && want_to_try_both)? + " Trying both.":""); - /* Okay, if that didn't work, and we think that it was compressed - * differently, try that. */ - if (anonymized_connection && - ! allowed_anonymous_connection_compression_method(guessed)) { - warn_disallowed_anonymous_compression_method(guessed); - rv = -1; - goto done; - } + /* Try declared compression first if we can. + * tor_compress_supports_method() also returns true for NO_METHOD. + * Ensure that the server is not sending us data compressed using a + * compression method that is not allowed for anonymous connections. */ + if (anonymized_connection && + ! allowed_anonymous_connection_compression_method(compression)) { + warn_disallowed_anonymous_compression_method(compression); + rv = -1; + goto done; + } - if (!new_body && tor_compress_supports_method(guessed) && - compression != guessed) { - tor_uncompress(&new_body, &new_len, body, body_len, guessed, - !allow_partial, LOG_PROTOCOL_WARN); - tried_both = 1; - } - /* If we're pretty sure that we have a compressed directory, and - * we didn't manage to uncompress it, then warn and bail. */ - if (!plausible && !new_body) { - log_fn(LOG_PROTOCOL_WARN, LD_HTTP, - "Unable to decompress HTTP body (tried %s%s%s, server '%s:%d').", - description1, - tried_both?" and ":"", - tried_both?description2:"", - conn->base_.address, conn->base_.port); - rv = -1; - goto done; - } - if (new_body) { - tor_free(*bodyp); - *bodyp = new_body; - *bodylenp = new_len; - } + if (tor_compress_supports_method(compression)) + tor_uncompress(&new_body, &new_len, body, body_len, compression, + !allow_partial, LOG_PROTOCOL_WARN); + + /* Okay, if that didn't work, and we think that it was compressed + * differently, try that. */ + if (anonymized_connection && + ! allowed_anonymous_connection_compression_method(guessed)) { + warn_disallowed_anonymous_compression_method(guessed); + rv = -1; + goto done; + } + + if (!new_body && tor_compress_supports_method(guessed) && + compression != guessed) { + tor_uncompress(&new_body, &new_len, body, body_len, guessed, + !allow_partial, LOG_PROTOCOL_WARN); + tried_both = 1; + } + /* If we're pretty sure that we have a compressed directory, and + * we didn't manage to uncompress it, then warn and bail. */ + if (!plausible && !new_body) { + log_fn(LOG_PROTOCOL_WARN, LD_HTTP, + "Unable to decompress HTTP body (tried %s%s%s, server '%s:%d').", + description1, + tried_both?" and ":"", + tried_both?description2:"", + conn->base_.address, conn->base_.port); + rv = -1; + goto done; + } + if (new_body) { + tor_free(*bodyp); + *bodyp = new_body; + *bodylenp = new_len; } done: From 7b3161f0082a626c721acc1100cce0acb40efa85 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 11:48:15 -0400 Subject: [PATCH 3/5] It should be a PROTOCOL_WARN when we have an incorrect content-encoding. Rationale: The server did not obey the protocol, and its content-encoding got munged. That's what PROTOCOL_WARN is for. --- changes/bug22670_02 | 4 ++++ src/or/directory.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changes/bug22670_02 diff --git a/changes/bug22670_02 b/changes/bug22670_02 new file mode 100644 index 0000000000..3e7a428faf --- /dev/null +++ b/changes/bug22670_02 @@ -0,0 +1,4 @@ + o Minor bugfixes (logging, compression): + - When decompressing, treat mismatch between content-encoding and + actual compression type as a protocol warning. Fixes part of bug + 22670; bugfix on 0.1.1.9-alpha. diff --git a/src/or/directory.c b/src/or/directory.c index 89e4bbc492..1d2f8986aa 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2232,7 +2232,7 @@ dir_client_decompress_response_body(char **bodyp, size_t *bodylenp, want_to_try_both = (compression == UNKNOWN_METHOD || guessed != compression); if (want_to_try_both) { - severity = LOG_INFO; + severity = LOG_PROTOCOL_WARN; } tor_log(severity, LD_HTTP, From d8cd68caf1edb24b9daff2eb58f2b5f517b02c52 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 11:49:54 -0400 Subject: [PATCH 4/5] If a _guessed_ compression method fails, it is never PROTOCOL_WARN. Rationale: When use a guessed compression method, we already gave a PROTOCOL_WARN when our guess differed from the declared method, AND we gave a PROTOCOL_WARN when the declared method failed. It is not a protocol problem that the guessed method failed too; it's just a recovery attempt that failed. --- changes/bug22670 | 4 ++++ src/or/directory.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changes/bug22670 diff --git a/changes/bug22670 b/changes/bug22670 new file mode 100644 index 0000000000..47403277d2 --- /dev/null +++ b/changes/bug22670 @@ -0,0 +1,4 @@ + o Minor bugfixes (logging, compression): + - When decompressing, do not warn if we fail to decompress using a + compression method that we merely guessed. Fixes part of + bug 22670; bugfix on 0.1.1.14-alpha. diff --git a/src/or/directory.c b/src/or/directory.c index 1d2f8986aa..b6121d80d8 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2271,7 +2271,7 @@ dir_client_decompress_response_body(char **bodyp, size_t *bodylenp, if (!new_body && tor_compress_supports_method(guessed) && compression != guessed) { tor_uncompress(&new_body, &new_len, body, body_len, guessed, - !allow_partial, LOG_PROTOCOL_WARN); + !allow_partial, LOG_INFO); tried_both = 1; } /* If we're pretty sure that we have a compressed directory, and From 5537e1fc45b683f10b3702f00e0b1280d4a5c87a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 20 Jun 2017 11:55:18 -0400 Subject: [PATCH 5/5] If we successfully decompress an HTTP body, return immediately. This prevents us from calling allowed_anonymous_connection_compression_method() on the unused guessed method (if any), and rejecting something that was already safe to use. --- changes/bug22670_03 | 6 ++++++ src/or/directory.c | 26 +++++++++++++++++++------- 2 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 changes/bug22670_03 diff --git a/changes/bug22670_03 b/changes/bug22670_03 new file mode 100644 index 0000000000..8a7aa49bcd --- /dev/null +++ b/changes/bug22670_03 @@ -0,0 +1,6 @@ + o Minor bugfixes (compression): + - When decompressing an object received over an anonymous directory + connection, if we have already successfully decompressed it using an + acceptable compression method, do not reject it for looking like an + unacceptable compression method. Fixes part of bug 22670; bugfix on + 0.3.1.1-alpha. diff --git a/src/or/directory.c b/src/or/directory.c index b6121d80d8..ac40e54ceb 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2255,9 +2255,15 @@ dir_client_decompress_response_body(char **bodyp, size_t *bodylenp, goto done; } - if (tor_compress_supports_method(compression)) + if (tor_compress_supports_method(compression)) { tor_uncompress(&new_body, &new_len, body, body_len, compression, !allow_partial, LOG_PROTOCOL_WARN); + if (new_body) { + /* We succeeded with the declared compression method. Great! */ + rv = 0; + goto done; + } + } /* Okay, if that didn't work, and we think that it was compressed * differently, try that. */ @@ -2268,7 +2274,7 @@ dir_client_decompress_response_body(char **bodyp, size_t *bodylenp, goto done; } - if (!new_body && tor_compress_supports_method(guessed) && + if (tor_compress_supports_method(guessed) && compression != guessed) { tor_uncompress(&new_body, &new_len, body, body_len, guessed, !allow_partial, LOG_INFO); @@ -2286,13 +2292,19 @@ dir_client_decompress_response_body(char **bodyp, size_t *bodylenp, rv = -1; goto done; } - if (new_body) { - tor_free(*bodyp); - *bodyp = new_body; - *bodylenp = new_len; - } done: + if (new_body) { + if (rv == 0) { + /* success! */ + tor_free(*bodyp); + *bodyp = new_body; + *bodylenp = new_len; + } else { + tor_free(new_body); + } + } + return rv; }