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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: tatsuhiro.t, Assigned: u408661)

Details

(Whiteboard: [spdy][http2release])

Attachments

(1 file)

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:

[0] :authority: example.org
...
[14] :path: /bar
[15] :path: /foo

implied reference set:
( 15 )

Then we reference static header table entry :

[17] :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
Whiteboard: [spdy]
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
Attached patch patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=971dfc78fbdd
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+
Whiteboard: [spdy] → [spdy][http2release]
https://hg.mozilla.org/mozilla-central/rev/68ba56dc7149
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.