From 64798dab4f4fa9404c92d98cdb10d312b1f6e556 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 3 Jan 2011 15:54:23 -0500 Subject: [PATCH 1/3] Detect and disallow compression bombs --- changes/bug2324_uncompress | 5 ++++ src/common/torgzip.c | 51 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 changes/bug2324_uncompress diff --git a/changes/bug2324_uncompress b/changes/bug2324_uncompress new file mode 100644 index 0000000000..223a3ce35b --- /dev/null +++ b/changes/bug2324_uncompress @@ -0,0 +1,5 @@ + o Major bugfixes (security): + - Prevent a DoS attack by disallowing any zlib-compressed data + whose compression factor is implausibly high. Fixes the + second part of bug2324; found by doors. + diff --git a/src/common/torgzip.c b/src/common/torgzip.c index 618b8b0300..b44b159a46 100644 --- a/src/common/torgzip.c +++ b/src/common/torgzip.c @@ -57,6 +57,24 @@ method_bits(compress_method_t method) return method == GZIP_METHOD ? 15+16 : 15; } +/* These macros define the maximum allowable compression factor. Anything of + * size greater than check_for_compression_bomb_after is not allowed to + * have an uncompression factor (uncompressed size:compressed size ratio) of + * any greater than MAX_UNCOMPRESSION_FACTOR. */ +#define MAX_UNCOMPRESSION_FACTOR 25 +#define CHECK_FOR_COMPRESSION_BOMB_AFTER (1024*64) + +/** Return true if uncompressing an input of size in_size to an input + * of size at least size_out looks like a compression bomb. */ +static int +is_compression_bomb(size_t size_in, size_t size_out) +{ + if (size_in == 0 || size_out < CHECK_FOR_COMPRESSION_BOMB_AFTER) + return 0; + + return (size_out / size_in > MAX_UNCOMPRESSION_FACTOR); +} + /** Given in_len bytes at in, compress them into a newly * allocated buffer, using the method described in method. Store the * compressed string in *out, and its length in *out_len. @@ -159,6 +177,12 @@ tor_gzip_compress(char **out, size_t *out_len, } tor_free(stream); + if (is_compression_bomb(*out_len, in_len)) { + log_warn(LD_BUG, "We compressed something and got an insanely high " + "compression factor; other Tors would think this was a zlib bomb."); + goto err; + } + return 0; err: if (stream) { @@ -223,7 +247,7 @@ tor_gzip_uncompress(char **out, size_t *out_len, out_size = in_len * 2; /* guess 50% compression. */ if (out_size < 1024) out_size = 1024; - if (out_size > UINT_MAX) + if (out_size > SIZE_T_CEILING || out_size > UINT_MAX) goto err; *out = tor_malloc(out_size); @@ -263,7 +287,16 @@ tor_gzip_uncompress(char **out, size_t *out_len, old_size = out_size; out_size *= 2; if (out_size < old_size) { - log_warn(LD_GENERAL, "Size overflow in compression."); + log_warn(LD_GENERAL, "Size overflow in uncompression."); + goto err; + } + if (is_compression_bomb(in_len, out_size)) { + log_warn(LD_GENERAL, "Input looks look a possible zlib bomb; " + "not proceeding."); + goto err; + } + if (out_size >= SIZE_T_CEILING) { + log_warn(LD_BUG, "Hit SIZE_T_CEILING limit while uncompressing."); goto err; } *out = tor_realloc(*out, out_size); @@ -329,6 +362,11 @@ detect_compression_method(const char *in, size_t in_len) struct tor_zlib_state_t { struct z_stream_s stream; int compress; + + /* Number of bytes read so far. Used to detect zlib bombs. */ + size_t input_so_far; + /* Number of bytes written so far. Used to detect zlib bombs. */ + size_t output_so_far; }; /** Construct and return a tor_zlib_state_t object using method. If @@ -395,11 +433,20 @@ tor_zlib_process(tor_zlib_state_t *state, err = inflate(&state->stream, finish ? Z_FINISH : Z_SYNC_FLUSH); } + state->input_so_far += state->stream.next_in - ((unsigned char*)*in); + state->output_so_far += state->stream.next_out - ((unsigned char*)*out); + *out = (char*) state->stream.next_out; *out_len = state->stream.avail_out; *in = (const char *) state->stream.next_in; *in_len = state->stream.avail_in; + if (! state->compress && + is_compression_bomb(state->input_so_far, state->output_so_far)) { + log_warn(LD_DIR, "Possible zlib bomb; abandoning stream."); + return TOR_ZLIB_ERR; + } + switch (err) { case Z_STREAM_END: From d14b0d54d2469744266769d7e61135d1aa1c9c11 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 5 Jan 2011 12:42:34 -0500 Subject: [PATCH 2/3] Fix a SIZE_T_CEILING check in torgzip.c; noticed by cypherpunks --- src/common/torgzip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/torgzip.c b/src/common/torgzip.c index b44b159a46..7678668777 100644 --- a/src/common/torgzip.c +++ b/src/common/torgzip.c @@ -247,7 +247,7 @@ tor_gzip_uncompress(char **out, size_t *out_len, out_size = in_len * 2; /* guess 50% compression. */ if (out_size < 1024) out_size = 1024; - if (out_size > SIZE_T_CEILING || out_size > UINT_MAX) + if (out_size >= SIZE_T_CEILING || out_size > UINT_MAX) goto err; *out = tor_malloc(out_size); From 1fcfc186284a375bab2595162564f0dd6c1d19f0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 15 Jan 2011 12:12:10 -0500 Subject: [PATCH 3/3] clean up message; explain a magic number in a comment --- src/common/torgzip.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/common/torgzip.c b/src/common/torgzip.c index 7678668777..249151cc9b 100644 --- a/src/common/torgzip.c +++ b/src/common/torgzip.c @@ -58,9 +58,18 @@ method_bits(compress_method_t method) } /* These macros define the maximum allowable compression factor. Anything of - * size greater than check_for_compression_bomb_after is not allowed to + * size greater than CHECK_FOR_COMPRESSION_BOMB_AFTER is not allowed to * have an uncompression factor (uncompressed size:compressed size ratio) of - * any greater than MAX_UNCOMPRESSION_FACTOR. */ + * any greater than MAX_UNCOMPRESSION_FACTOR. + * + * Picking a value for MAX_UNCOMPRESSION_FACTOR is a trade-off: we want it to + * be small to limit the attack multiplier, but we also want it to be large + * enough so that no legitimate document --even ones we might invent in the + * future -- ever compresses by a factor of greater than + * MAX_UNCOMPRESSION_FACTOR. Within those parameters, there's a reasonably + * large range of possible values. IMO, anything over 8 is probably safe; IMO + * anything under 50 is probably sufficient. + */ #define MAX_UNCOMPRESSION_FACTOR 25 #define CHECK_FOR_COMPRESSION_BOMB_AFTER (1024*64) @@ -291,7 +300,7 @@ tor_gzip_uncompress(char **out, size_t *out_len, goto err; } if (is_compression_bomb(in_len, out_size)) { - log_warn(LD_GENERAL, "Input looks look a possible zlib bomb; " + log_warn(LD_GENERAL, "Input looks like a possible zlib bomb; " "not proceeding."); goto err; }