Check HPACK Huffman code strictly#2172
Conversation
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.
|
@zwoop This is nice to have because it's not a bug fix. Marked as a 7.1 candidate but there's no necessity. |
| ++byte_boundary_crossed; | ||
| } | ||
| if (byte_boundary_crossed > 3) { | ||
| return -1; |
There was a problem hiding this comment.
Why not use HPACK_ERROR_COMPRESSION_ERROR?
There was a problem hiding this comment.
Because that's in HPACK.h, so we shouldn't use it here.
There was a problem hiding this comment.
Ah, here is HuffmanCodec. Got it.
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.
| --src_len; | ||
| ++byte_boundary_crossed; | ||
| } | ||
| if (byte_boundary_crossed > 3) { |
There was a problem hiding this comment.
This is checking length of encoded data, right? It looks like some Huffman code has 3 byte boundaries. Is this should be 4? ( e.g. |11111111|11111111|11111110|0010 fffffe2 [28])
There was a problem hiding this comment.
Never mind, I thought >= 3 :p
|
I've put this on Docs / CI for testing. |
|
Should we land this? |
|
@maskit This does not cherry-pick cleanly to 7.1.x branch, can either of you make a PR against 7.1.x please? |
3 participants
This fixes issues checked with these h2spec tests below.
5.2. String Literal Representation
1: Sends a Huffman-encoded string literal representation with padding longer than 7 bits
2: Sends a Huffman-encoded string literal representation padded by zero