From ea9efb1a9bec884793cfda363a4a3c7631b6679a 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. --- 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 2f90f1686a3..0576f1c281f 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 b228c13d9a2744fa8b87513417c61ec18a437ae2 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. --- 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; }