Closed Bug 1197847 Opened 5 years ago Closed 5 years ago

dont allow line folding in h2 headers

Categories

(Core :: Networking: HTTP, defect)

32 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mcmanus, Assigned: u408661)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [spdy])

Attachments

(2 files, 1 obsolete file)

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."
nick, I assigned this to you as it might be best done near the hpack code.. let me know if that's cray cray.
edge already rejects this, and chrome is expected to make a corresponding change
Yeah, I agree this makes the most sense in the hpack code. Will have a patch up soon-ish.
Attached patch patch (obsolete) — Splinter Review
Attachment #8651919 - Flags: review?(mcmanus)
Note - this patch also includes a bunch of "goto fail"-style fixes that I noticed while I was in the HPACK code :)
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.)
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 :)
let's do this with rst_stream/protocol_error
(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.
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+
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)
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)
Attachment #8651919 - Attachment is obsolete: true
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+
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+
Depends on: 1248769
You need to log in before you can comment on or make changes to this bug.