From 58a5e045a77eb0f9a6cf164946d9c037e4f8cb18 Mon Sep 17 00:00:00 2001 From: Menno de Gier Date: Wed, 4 May 2022 18:43:35 +0100 Subject: [PATCH 1/4] Check for leaf node after resetting byte counter --- proxy/hdrs/HuffmanCodec.cc | 15 ++++++------ proxy/hdrs/unit_tests/test_XPACK.cc | 38 +++++++++++++++++------------ 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/proxy/hdrs/HuffmanCodec.cc b/proxy/hdrs/HuffmanCodec.cc index 546269042c6..38f4249e11d 100644 --- a/proxy/hdrs/HuffmanCodec.cc +++ b/proxy/hdrs/HuffmanCodec.cc @@ -165,14 +165,6 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) current = current->left; includes_zero = true; } - - if (current->leaf_node == true) { - *dst_end = current->ascii_code; - ++dst_end; - current = HUFFMAN_TREE_ROOT; - byte_boundary_crossed = 0; - includes_zero = false; - } if (shift) { --shift; } else { @@ -181,6 +173,13 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) --src_len; ++byte_boundary_crossed; } + if (current->leaf_node == true) { + *dst_end = current->ascii_code; + ++dst_end; + current = HUFFMAN_TREE_ROOT; + byte_boundary_crossed = 0; + includes_zero = false; + } if (byte_boundary_crossed > 3) { return -1; } diff --git a/proxy/hdrs/unit_tests/test_XPACK.cc b/proxy/hdrs/unit_tests/test_XPACK.cc index 91bd8b31161..3404e6b6786 100644 --- a/proxy/hdrs/unit_tests/test_XPACK.cc +++ b/proxy/hdrs/unit_tests/test_XPACK.cc @@ -73,22 +73,28 @@ TEST_CASE("XPACK_String", "[xpack]") uint32_t raw_string_len; uint8_t *encoded_field; int encoded_field_len; - } string_test_case[] = {{(char *)"", 0, - (uint8_t *)"\x0" - "", - 1}, - {(char *)"custom-key", 10, - (uint8_t *)"\xA" - "custom-key", - 11}, - {(char *)"", 0, - (uint8_t *)"\x80" - "", - 1}, - {(char *)"custom-key", 10, - (uint8_t *)"\x88" - "\x25\xa8\x49\xe9\x5b\xa9\x7d\x7f", - 9}}; + } string_test_case[] = { + {(char *)"", 0, + (uint8_t *)"\x0" + "", + 1}, + {(char *)"custom-key", 10, + (uint8_t *)"\xA" + "custom-key", + 11}, + {(char *)"", 0, + (uint8_t *)"\x80" + "", + 1}, + {(char *)"custom-key", 10, + (uint8_t *)"\x88" + "\x25\xa8\x49\xe9\x5b\xa9\x7d\x7f", + 9}, + {(char *)"cw Times New Roman_σ=1", 23, + (uint8_t *)"\x95" + "\x27\x85\x37\x9a\x92\xa1\x4d\x25\xf0\xa6\xd3\xd2\x3a\xa2\xff\xff\xf6\xff\xff\x44\x01", + 22}, + }; SECTION("Encoding") { From 5bd994d1b740c53fd406480937ffa2815061543d Mon Sep 17 00:00:00 2001 From: Menno de Gier Date: Fri, 6 May 2022 09:46:55 +0100 Subject: [PATCH 2/4] Improve invalid decoding length --- proxy/hdrs/HuffmanCodec.cc | 37 +++++++++++++++++------------ proxy/hdrs/unit_tests/test_XPACK.cc | 26 ++++++++++++++++++++ 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/proxy/hdrs/HuffmanCodec.cc b/proxy/hdrs/HuffmanCodec.cc index 38f4249e11d..c50cf9d13fe 100644 --- a/proxy/hdrs/HuffmanCodec.cc +++ b/proxy/hdrs/HuffmanCodec.cc @@ -149,14 +149,16 @@ hpack_huffman_fin() } } +#define MAX_HUFFMAN_CODE_LEN 30 + int64_t huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) { - char *dst_end = dst_start; - uint8_t shift = 7; - Node *current = HUFFMAN_TREE_ROOT; - int byte_boundary_crossed = 0; - bool includes_zero = false; + char *dst_end = dst_start; + uint8_t shift = 7; + Node *current = HUFFMAN_TREE_ROOT; + bool includes_zero = false; + int nbits = 0; while (src_len) { if (*src & (1 << shift)) { @@ -165,28 +167,33 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) current = current->left; includes_zero = true; } + ++nbits; + + if (current->leaf_node == true) { + nbits = 0; + *dst_end = current->ascii_code; + ++dst_end; + current = HUFFMAN_TREE_ROOT; + includes_zero = false; + } + if (shift) { --shift; } else { shift = 7; ++src; --src_len; - ++byte_boundary_crossed; } - if (current->leaf_node == true) { - *dst_end = current->ascii_code; - ++dst_end; - current = HUFFMAN_TREE_ROOT; - byte_boundary_crossed = 0; - includes_zero = false; - } - if (byte_boundary_crossed > 3) { + + if (nbits > MAX_HUFFMAN_CODE_LEN) { return -1; } } - if (byte_boundary_crossed > 1) { + + if (nbits > 7) { return -1; } + if (includes_zero) { return -1; } diff --git a/proxy/hdrs/unit_tests/test_XPACK.cc b/proxy/hdrs/unit_tests/test_XPACK.cc index 3404e6b6786..ed2deadb1b6 100644 --- a/proxy/hdrs/unit_tests/test_XPACK.cc +++ b/proxy/hdrs/unit_tests/test_XPACK.cc @@ -65,6 +65,32 @@ TEST_CASE("XPACK_Integer", "[xpack]") } } +TEST_CASE("huffman_decode", "[decoding_errors]") +{ + SECTION("Decoding") + { + hpack_huffman_init(); + Arena arena; + + const static struct { + char *input; + int input_len; + } test_cases[] = { + {(char *)"\x00", 1}, + {(char *)"\xff", 1}, + {(char *)"\x1f\xff", 2}, + {(char *)"\xff\x9f\xff\xff\xff", 5}, + }; + + for (unsigned int i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) { + int dst_len = 2 * test_cases[i].input_len; + char *dst = arena.str_alloc(dst_len); + int len = huffman_decode(dst, (uint8_t *)test_cases[i].input, test_cases[i].input_len); + REQUIRE(len == -1); + } + } +} + TEST_CASE("XPACK_String", "[xpack]") { // Example: custom-key: custom-header From 3071ace8b93de2c53c93166f1db4e3b5b9fa8452 Mon Sep 17 00:00:00 2001 From: Menno de Gier Date: Thu, 12 May 2022 13:11:43 +0100 Subject: [PATCH 3/4] Move error tests to test_Huffmancode --- proxy/hdrs/test_Huffmancode.cc | 23 +++++++++++++++++++++++ proxy/hdrs/unit_tests/test_XPACK.cc | 26 -------------------------- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/proxy/hdrs/test_Huffmancode.cc b/proxy/hdrs/test_Huffmancode.cc index 24344d7b8c1..968f38839d7 100644 --- a/proxy/hdrs/test_Huffmancode.cc +++ b/proxy/hdrs/test_Huffmancode.cc @@ -171,6 +171,28 @@ encode_test() } } +void +decode_errors_test() +{ + const static struct { + char *input; + int input_len; + } test_cases[] = { + {(char *)"\x00", 1}, + {(char *)"\xff", 1}, + {(char *)"\x1f\xff", 2}, + {(char *)"\xff\x9f\xff\xff\xff", 5}, + }; + + for (unsigned int i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) { + int dst_len = 2 * test_cases[i].input_len; + char *dst = (char *)malloc(dst_len); + int len = huffman_decode(dst, (uint8_t *)test_cases[i].input, test_cases[i].input_len); + assert(len == -1); + free(dst); + } +} + int main() { @@ -180,6 +202,7 @@ main() random_test(); } values_test(); + decode_errors_test(); hpack_huffman_fin(); diff --git a/proxy/hdrs/unit_tests/test_XPACK.cc b/proxy/hdrs/unit_tests/test_XPACK.cc index ed2deadb1b6..3404e6b6786 100644 --- a/proxy/hdrs/unit_tests/test_XPACK.cc +++ b/proxy/hdrs/unit_tests/test_XPACK.cc @@ -65,32 +65,6 @@ TEST_CASE("XPACK_Integer", "[xpack]") } } -TEST_CASE("huffman_decode", "[decoding_errors]") -{ - SECTION("Decoding") - { - hpack_huffman_init(); - Arena arena; - - const static struct { - char *input; - int input_len; - } test_cases[] = { - {(char *)"\x00", 1}, - {(char *)"\xff", 1}, - {(char *)"\x1f\xff", 2}, - {(char *)"\xff\x9f\xff\xff\xff", 5}, - }; - - for (unsigned int i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) { - int dst_len = 2 * test_cases[i].input_len; - char *dst = arena.str_alloc(dst_len); - int len = huffman_decode(dst, (uint8_t *)test_cases[i].input, test_cases[i].input_len); - REQUIRE(len == -1); - } - } -} - TEST_CASE("XPACK_String", "[xpack]") { // Example: custom-key: custom-header From 054194db0c5332ebb77be677d6877cba82a48952 Mon Sep 17 00:00:00 2001 From: Menno de Gier Date: Thu, 12 May 2022 13:12:12 +0100 Subject: [PATCH 4/4] Reject EOS symbol --- proxy/hdrs/HuffmanCodec.cc | 25 +++++++++++++++++-------- proxy/hdrs/test_Huffmancode.cc | 11 +++++++---- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/proxy/hdrs/HuffmanCodec.cc b/proxy/hdrs/HuffmanCodec.cc index c50cf9d13fe..a99e30306c8 100644 --- a/proxy/hdrs/HuffmanCodec.cc +++ b/proxy/hdrs/HuffmanCodec.cc @@ -150,6 +150,7 @@ hpack_huffman_fin() } #define MAX_HUFFMAN_CODE_LEN 30 +#define EOS 0x3fffffff int64_t huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) @@ -157,24 +158,30 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) char *dst_end = dst_start; uint8_t shift = 7; Node *current = HUFFMAN_TREE_ROOT; - bool includes_zero = false; int nbits = 0; + uint32_t curr_bits = 0; while (src_len) { + if (nbits > 0) { + curr_bits <<= 1; + } if (*src & (1 << shift)) { + curr_bits |= 1; current = current->right; } else { - current = current->left; - includes_zero = true; + current = current->left; } ++nbits; if (current->leaf_node == true) { - nbits = 0; - *dst_end = current->ascii_code; + if (curr_bits == EOS) { + return -1; + } + nbits = 0; + curr_bits = 0; + *dst_end = current->ascii_code; ++dst_end; - current = HUFFMAN_TREE_ROOT; - includes_zero = false; + current = HUFFMAN_TREE_ROOT; } if (shift) { @@ -194,7 +201,9 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) return -1; } - if (includes_zero) { + // Padding bits must be a prefix of EOS + uint8_t mask = (1 << nbits) - 1; + if ((mask & curr_bits) != mask) { return -1; } diff --git a/proxy/hdrs/test_Huffmancode.cc b/proxy/hdrs/test_Huffmancode.cc index 968f38839d7..6f5bf7b915c 100644 --- a/proxy/hdrs/test_Huffmancode.cc +++ b/proxy/hdrs/test_Huffmancode.cc @@ -136,6 +136,12 @@ values_test() int bytes = huffman_decode(dst_start, encoded_mapped.y, encoded_size); char ascii_value = i / 2; + + // EOS is treated as invalid so check for an error + if (value == 0x3fffffff) { + assert(bytes == -1); + continue; + } assert(dst_start[0] == ascii_value); assert(bytes == 1); } @@ -178,10 +184,7 @@ decode_errors_test() char *input; int input_len; } test_cases[] = { - {(char *)"\x00", 1}, - {(char *)"\xff", 1}, - {(char *)"\x1f\xff", 2}, - {(char *)"\xff\x9f\xff\xff\xff", 5}, + {(char *)"\x00", 1}, {(char *)"\xff", 1}, {(char *)"\x1f\xff", 2}, {(char *)"\xff\xae", 2}, {(char *)"\xff\x9f\xff\xff\xff", 5}, }; for (unsigned int i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) {