1197847 - dont allow line folding in h2 headers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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."

Comment 1

[Patrick McManus [:mcmanus]]

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

Comment 2

[Patrick McManus [:mcmanus]]

edge already rejects this, and chrome is expected to make a corresponding change

Comment 3

[u408661]

Yeah, I agree this makes the most sense in the hpack code. Will have a patch up soon-ish.

Comment 4

[u408661]

Attachment: patch

Comment 5

[u408661]

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

Comment 6

[u408661]

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.)

Comment 7

[Patrick McManus [:mcmanus]]

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 :)

Comment 8

[Patrick McManus [:mcmanus]]

let's do this with rst_stream/protocol_error

Comment 9

[u408661]

(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 10

[Patrick McManus [:mcmanus]]

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

Comment 11

[u408661]

Attachment: helper patch used during debugging

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

Comment 12

[u408661]

Attachment: Disallow folded headers in h2

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.

Comment 13

[Patrick McManus [:mcmanus]]

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 :)

Comment 14

[Patrick McManus [:mcmanus]]

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

Comment 15

[u408661]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cb61b645368

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03aee3398396
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6059530b031

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/03aee3398396
https://hg.mozilla.org/mozilla-central/rev/d6059530b031

Comment 18

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/line-breaks-in-http2-headers-are-no-longer-allowed/

Comment 19

[Jean-Yves Perrier [:teoli]]

Updated: https://developer.mozilla.org/en-US/Firefox/Releases/44#HTTP