Closed Bug 1019577 Opened 6 years ago Closed 6 years ago
HTTP/2 HPACK out of sync when implied header is evicted by referencing static header table entry
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36 Steps to reproduce: I have once experienced that Firefox nightly sends corrupted header set (:path is duplicated twice, etc) to HTTP/2 proxy (nghttpx) when I was playing with HTTP/2 secure proxy feature of Firefox. It is hard to reproduce, so I checked current firefox codebase and found that possible cause of this issue. I'll explain the situation when the bug will emerge. I think if you have unit test of HPACK compressor, it is relatively easy to reproduce the situation. This is basically similar case described in Http2Compression.cpp:1440: 1440 // make sure to makeroom() first so that any implied items 1441 // get preserved. 1442 MakeRoom(newSize); But this care is not taken when encoder is referencing static header table entry and copies it into dynamic table. Suppose we have the following header table and implied reference set (both 0-based): header table:  :authority: example.org ...  :path: /bar  :path: /foo implied reference set: ( 15 ) Then we reference static header table entry :  :method: GET And suppose that adding this entry to dynamic header table evicts index=15 (which is :path: /foo). Current codebase first emits the indexed representation of static header table entry, then add its copy to dynamic header table, which checks implied reference set and emits indexed representation of evicted entry to toggle off (which is index=15, in this case). On receiver side, it sees first indexed representation of static header table entry and adds its copy to dynamic header table, which evicts index=15, but no toggle off. Then it sees indexed representation of index=15, which will toggle on the unexpected header entry (this time, it is :path: /bar). This leads to corrupted header set and compression contexts between decoder and encoder becomes out of sync. Actual results: The server receives corrupted request headers because compression context gets out of sync. Expected results: The server receives must receive correct request headers.
Component: Untriaged → Networking: HTTP
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Thanks, Tatsuhiro! Sadly, we don't have unit tests for HPACK (it's somewhere on my TODO list), but just inspecting the code makes me see this is a Real Bug, just as you described (thanks for the great description, by the way!) Patch incoming.
Assignee: nobody → hurley
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8433519 - Flags: review?(mcmanus)
Comment on attachment 8433519 [details] [diff] [review] patch Review of attachment 8433519 [details] [diff] [review]: ----------------------------------------------------------------- Thanks everyone - awesome
Attachment #8433519 - Flags: review?(mcmanus) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.