From 3dc89c15a9feb71837a6d6bdd48dc5ed6fd05388 Mon Sep 17 00:00:00 2001 From: Masakazu Kitajo Date: Wed, 21 Jun 2017 13:59:03 +0900 Subject: [PATCH 1/2] Check padding length of Huffman code strictly HPACK Huffman decoder was trying to decode whole Huffman code block that includes extra padding. This is prohibited by the spec (Sec 5.2), and it must be treated as a decoding error. (cherry picked from commit ea9efb1a9bec884793cfda363a4a3c7631b6679a) --- proxy/http2/HPACK.cc | 2 +- proxy/http2/HuffmanCodec.cc | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/proxy/http2/HPACK.cc b/proxy/http2/HPACK.cc index 37815f5ccfb..9ce3e5f6065 100644 --- a/proxy/http2/HPACK.cc +++ b/proxy/http2/HPACK.cc @@ -708,7 +708,7 @@ decode_string(Arena &arena, char **str, uint32_t &str_length, const uint8_t *buf *str = arena.str_alloc(encoded_string_len * 2); len = huffman_decode(*str, p, encoded_string_len); - if (len == HPACK_ERROR_COMPRESSION_ERROR) { + if (len < 0) { return HPACK_ERROR_COMPRESSION_ERROR; } str_length = len; diff --git a/proxy/http2/HuffmanCodec.cc b/proxy/http2/HuffmanCodec.cc index 45ec21441a7..fe976ac5581 100644 --- a/proxy/http2/HuffmanCodec.cc +++ b/proxy/http2/HuffmanCodec.cc @@ -154,6 +154,7 @@ 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; while (src_len) { if (*src & (1 << shift)) { @@ -166,6 +167,7 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) *dst_end = current->ascii_code; ++dst_end; current = HUFFMAN_TREE_ROOT; + byte_boundary_crossed = 0; } if (shift) { --shift; @@ -173,7 +175,14 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) shift = 7; ++src; --src_len; + ++byte_boundary_crossed; } + if (byte_boundary_crossed > 3) { + return -1; + } + } + if (byte_boundary_crossed > 1) { + return -1; } return dst_end - dst_start; From 5e69e2f839719d9e44a0f408792628dfedd2050a Mon Sep 17 00:00:00 2001 From: Masakazu Kitajo Date: Wed, 21 Jun 2017 14:15:07 +0900 Subject: [PATCH 2/2] Check padding bits of Huffman code strictly HPACK Huffman code didn't care whether all padding bits are one. This is prohibited by the spec(Sec 5.2), and it must be treated as a decode error. (cherry picked from commit b228c13d9a2744fa8b87513417c61ec18a437ae2) --- proxy/http2/HuffmanCodec.cc | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/proxy/http2/HuffmanCodec.cc b/proxy/http2/HuffmanCodec.cc index fe976ac5581..34ec166a7d8 100644 --- a/proxy/http2/HuffmanCodec.cc +++ b/proxy/http2/HuffmanCodec.cc @@ -151,23 +151,26 @@ hpack_huffman_fin() 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; + char *dst_end = dst_start; + uint8_t shift = 7; + Node *current = HUFFMAN_TREE_ROOT; int byte_boundary_crossed = 0; + bool includes_zero = false; while (src_len) { if (*src & (1 << shift)) { current = current->right; } else { - current = current->left; + current = current->left; + includes_zero = true; } if (current->leaf_node == true) { *dst_end = current->ascii_code; ++dst_end; - current = HUFFMAN_TREE_ROOT; + current = HUFFMAN_TREE_ROOT; byte_boundary_crossed = 0; + includes_zero = false; } if (shift) { --shift; @@ -184,6 +187,9 @@ huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len) if (byte_boundary_crossed > 1) { return -1; } + if (includes_zero) { + return -1; + } return dst_end - dst_start; }