dont allow line folding in h2 headers

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mcmanus, Assigned: u408661)

Tracking

({dev-doc-complete, site-compat})

32 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [spdy])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
name : part1\n
           part2\n
           part3

(the newlines and spaces were part of the header value encoded in h2). Firefox accepted this because we basically parse values with the h1 parser which accepts line folding,

There are two arguments against this - the first is that line folding is a piece of h1 specific syntax and h2 has different syntax (even though it inherits semantics). 

"Besides which, RFC 7540 says that the HEADERS frame carries headers as described in RFC 7230 section 3.2, which says in 3.2.4:

   Historically, HTTP header field values could be extended over

   multiple lines by preceding each extra line with at least one space

   or horizontal tab (obs-fold).  This specification deprecates such

   line folding except within the message/http media type

   (Section 8.3.1).  A sender MUST NOT generate a message that includes

   line folding (i.e., that has any field-value that contains a match to

   the obs-fold rule) unless the message is intended for packaging

   within the message/http media type."
(Reporter)

Comment 1

4 years ago
nick, I assigned this to you as it might be best done near the hpack code.. let me know if that's cray cray.
(Reporter)

Comment 2

4 years ago
edge already rejects this, and chrome is expected to make a corresponding change
(Assignee)

Comment 3

4 years ago
Yeah, I agree this makes the most sense in the hpack code. Will have a patch up soon-ish.
(Assignee)

Comment 4

4 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8651919 - Flags: review?(mcmanus)
(Assignee)

Comment 5

4 years ago
Note - this patch also includes a bunch of "goto fail"-style fixes that I noticed while I was in the HPACK code :)
(Assignee)

Comment 6

4 years ago
So... based on the latest reading of the wg list (and 7230) it looks like we've already got the appropriate behavior here. Gotta love spec lawyering :)

Patrick, I'll leave it to you to make the decision here, since you're module owner (and more versed in spec-land than I am), but my $0.02 is to leave our code as is (and I can make another bug to fix the goto-fail stuff, since I've already got a patch for it.)
(Reporter)

Comment 7

4 years ago
based on https://lists.w3.org/Archives/Public/ietf-http-wg/2015JulSep/0254.html

I think we still want this.. deferring a bit until more clear :)
(Reporter)

Comment 8

4 years ago
let's do this with rst_stream/protocol_error
(Assignee)

Comment 9

4 years ago
(In reply to Patrick McManus [:mcmanus] from comment #8)
> let's do this with rst_stream/protocol_error

OK, yeah, based on the new latest wg reading, that seems right.

That makes the current patch wrong, as we currently treat any error return from the compressor as a GOAWAY, so we'll need to fix that, as well.
(Reporter)

Comment 10

4 years ago
Comment on attachment 8651919 [details] [diff] [review]
patch

Review of attachment 8651919 [details] [diff] [review]:
-----------------------------------------------------------------

can you confirm with a trace that we are just resetting the stream and not the session? (I've read the comments too :) - just making sure).

thanks!

::: netwerk/protocol/http/Http2Compression.cpp
@@ +824,5 @@
>      return rv;
> +  }
> +
> +  int32_t newline = 0;
> +  while ((newline = value.FindChar('\n', newline)) != -1) {

add a comment about what we are enforcing
Attachment #8651919 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 11

4 years ago
This is just a patch I came up with (that I'd actually been meaning to come up with for a while) that makes debugging server-side issues on the h2 (and spdy) test server easier by allowing the user to run it independent of the xpcshell process
Attachment #8664626 - Flags: review?(mcmanus)
(Assignee)

Comment 12

4 years ago
Here's the patch to disallow folded headers, along with some 'goto-fail' type fixes (from the original version of the patch). This also fixes the situations where we could get into compression state mismatches with the server, as well as some whitespace funkiness (tabs!) that crept into the tests.
Attachment #8664629 - Flags: review?(mcmanus)
(Assignee)

Updated

4 years ago
Attachment #8651919 - Attachment is obsolete: true
(Reporter)

Comment 13

4 years ago
Comment on attachment 8664626 [details] [diff] [review]
helper patch used during debugging

Review of attachment 8664626 [details] [diff] [review]:
-----------------------------------------------------------------

should probably put this bug number on the checkin.. for tracking the review if nothing else :)
Attachment #8664626 - Flags: review?(mcmanus) → review+
(Reporter)

Comment 14

4 years ago
Comment on attachment 8664629 [details] [diff] [review]
Disallow folded headers in h2

Review of attachment 8664629 [details] [diff] [review]:
-----------------------------------------------------------------

thanks man - this was surprsingly hairy
Attachment #8664629 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/03aee3398396
https://hg.mozilla.org/mozilla-central/rev/d6059530b031
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

3 years ago
Depends on: 1248769
You need to log in before you can comment on or make changes to this bug.