From 516c2f591aa95f746c7bcd5d7de1650853930eed Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 12 May 2020 04:43:57 -0700 Subject: [PATCH 1/4] Fail decompression when the gzip trailer is missing --- src/core/lib/compression/message_compress.cc | 4 +++ .../core/compression/message_compress_test.cc | 29 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/core/lib/compression/message_compress.cc b/src/core/lib/compression/message_compress.cc index 7faa1dcd828..bea29798b6c 100644 --- a/src/core/lib/compression/message_compress.cc +++ b/src/core/lib/compression/message_compress.cc @@ -68,6 +68,10 @@ static int zlib_body(z_stream* zs, grpc_slice_buffer* input, goto error; } } + if (r != Z_STREAM_END) { + gpr_log(GPR_INFO, "zlib: Data error"); + goto error; + } GPR_ASSERT(outbuf.refcount); outbuf.data.refcounted.length -= zs->avail_out; diff --git a/test/core/compression/message_compress_test.cc b/test/core/compression/message_compress_test.cc index e2abf2d3215..5c833f33194 100644 --- a/test/core/compression/message_compress_test.cc +++ b/test/core/compression/message_compress_test.cc @@ -196,6 +196,34 @@ static void test_bad_decompression_data_crc(void) { grpc_slice_buffer_destroy(&output); } +static void test_bad_decompression_data_missing_trailer(void) { + grpc_slice_buffer input; + grpc_slice_buffer corrupted; + grpc_slice_buffer output; + size_t idx; + + grpc_slice_buffer_init(&input); + grpc_slice_buffer_init(&corrupted); + grpc_slice_buffer_init(&output); + grpc_slice_buffer_add(&input, create_test_value(ONE_MB_A)); + + grpc_core::ExecCtx exec_ctx; + /* compress it */ + grpc_msg_compress(GRPC_MESSAGE_COMPRESS_GZIP, &input, &corrupted); + /* corrupt the output by smashing the CRC */ + GPR_ASSERT(corrupted.count > 1); + GPR_ASSERT(GRPC_SLICE_LENGTH(corrupted.slices[1]) > 8); + corrupted.slices[1].data.refcounted.length -= 8; + + /* try (and fail) to decompress the corrupted compresed buffer */ + GPR_ASSERT(0 == grpc_msg_decompress(GRPC_MESSAGE_COMPRESS_GZIP, &corrupted, + &output)); + + grpc_slice_buffer_destroy(&input); + grpc_slice_buffer_destroy(&corrupted); + grpc_slice_buffer_destroy(&output); +} + static void test_bad_decompression_data_trailing_garbage(void) { grpc_slice_buffer input; grpc_slice_buffer output; @@ -315,6 +343,7 @@ int main(int argc, char** argv) { test_tiny_data_compress(); test_bad_decompression_data_crc(); + test_bad_decompression_data_missing_trailer(); test_bad_decompression_data_stream(); test_bad_decompression_data_trailing_garbage(); test_bad_compression_algorithm(); From 17b6be1ed8f5276a08c8eebb89d2d2fc9595f8ef Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 12 May 2020 11:13:38 -0700 Subject: [PATCH 2/4] Reviewer comments --- src/core/lib/compression/message_compress.cc | 2 +- test/core/compression/message_compress_test.cc | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/core/lib/compression/message_compress.cc b/src/core/lib/compression/message_compress.cc index bea29798b6c..f7c7614d81e 100644 --- a/src/core/lib/compression/message_compress.cc +++ b/src/core/lib/compression/message_compress.cc @@ -34,7 +34,7 @@ static int zlib_body(z_stream* zs, grpc_slice_buffer* input, grpc_slice_buffer* output, int (*flate)(z_stream* zs, int flush)) { - int r; + int r = Z_OK; int flush; size_t i; grpc_slice outbuf = GRPC_SLICE_MALLOC(OUTPUT_BLOCK_SIZE); diff --git a/test/core/compression/message_compress_test.cc b/test/core/compression/message_compress_test.cc index 5c833f33194..6a2d46602ab 100644 --- a/test/core/compression/message_compress_test.cc +++ b/test/core/compression/message_compress_test.cc @@ -210,12 +210,10 @@ static void test_bad_decompression_data_missing_trailer(void) { grpc_core::ExecCtx exec_ctx; /* compress it */ grpc_msg_compress(GRPC_MESSAGE_COMPRESS_GZIP, &input, &corrupted); - /* corrupt the output by smashing the CRC */ - GPR_ASSERT(corrupted.count > 1); - GPR_ASSERT(GRPC_SLICE_LENGTH(corrupted.slices[1]) > 8); + GPR_ASSERT(GRPC_SLICE_LENGTH(corrupted.slices[corrupted.count - 1]) > 8); + /* Remove the footer by manipulating the slice length */ corrupted.slices[1].data.refcounted.length -= 8; - - /* try (and fail) to decompress the corrupted compresed buffer */ + /* try (and fail) to decompress the compressed buffer without the footer */ GPR_ASSERT(0 == grpc_msg_decompress(GRPC_MESSAGE_COMPRESS_GZIP, &corrupted, &output)); From 8f11aba81825b3307c3de211857ae857acd5d5e5 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 12 May 2020 11:26:11 -0700 Subject: [PATCH 3/4] Reviewer comments --- .../core/compression/message_compress_test.cc | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/test/core/compression/message_compress_test.cc b/test/core/compression/message_compress_test.cc index 6a2d46602ab..9615582f78d 100644 --- a/test/core/compression/message_compress_test.cc +++ b/test/core/compression/message_compress_test.cc @@ -198,27 +198,29 @@ static void test_bad_decompression_data_crc(void) { static void test_bad_decompression_data_missing_trailer(void) { grpc_slice_buffer input; - grpc_slice_buffer corrupted; + grpc_slice_buffer decompressed; + grpc_slice_buffer garbage; grpc_slice_buffer output; - size_t idx; grpc_slice_buffer_init(&input); - grpc_slice_buffer_init(&corrupted); + grpc_slice_buffer_init(&decompressed); + grpc_slice_buffer_init(&garbage); grpc_slice_buffer_init(&output); grpc_slice_buffer_add(&input, create_test_value(ONE_MB_A)); grpc_core::ExecCtx exec_ctx; /* compress it */ - grpc_msg_compress(GRPC_MESSAGE_COMPRESS_GZIP, &input, &corrupted); - GPR_ASSERT(GRPC_SLICE_LENGTH(corrupted.slices[corrupted.count - 1]) > 8); - /* Remove the footer by manipulating the slice length */ - corrupted.slices[1].data.refcounted.length -= 8; + grpc_msg_compress(GRPC_MESSAGE_COMPRESS_GZIP, &input, &decompressed); + GPR_ASSERT(decompressed.length > 8); + /* Remove the footer from the decompressed message */ + grpc_slice_buffer_trim_end(&decompressed, 8, &garbage); /* try (and fail) to decompress the compressed buffer without the footer */ - GPR_ASSERT(0 == grpc_msg_decompress(GRPC_MESSAGE_COMPRESS_GZIP, &corrupted, + GPR_ASSERT(0 == grpc_msg_decompress(GRPC_MESSAGE_COMPRESS_GZIP, &decompressed, &output)); grpc_slice_buffer_destroy(&input); - grpc_slice_buffer_destroy(&corrupted); + grpc_slice_buffer_destroy(&decompressed); + grpc_slice_buffer_destroy(&garbage); grpc_slice_buffer_destroy(&output); } From 1d7f3b130b439c876db56461920dbadfd0f4c7ba Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 12 May 2020 11:31:36 -0700 Subject: [PATCH 4/4] Reviewer comments --- src/core/lib/compression/message_compress.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lib/compression/message_compress.cc b/src/core/lib/compression/message_compress.cc index f7c7614d81e..7c2a70bad4e 100644 --- a/src/core/lib/compression/message_compress.cc +++ b/src/core/lib/compression/message_compress.cc @@ -34,7 +34,7 @@ static int zlib_body(z_stream* zs, grpc_slice_buffer* input, grpc_slice_buffer* output, int (*flate)(z_stream* zs, int flush)) { - int r = Z_OK; + int r = Z_STREAM_END; /* Do not fail on an empty input. */ int flush; size_t i; grpc_slice outbuf = GRPC_SLICE_MALLOC(OUTPUT_BLOCK_SIZE);