1136727 - Firefox accepts undefined or invalid pseudo-header fields in HTTP/2

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Tatsuhiro Tsujikawa]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36

Steps to reproduce:

1. Make a HTTP/2 server return pseudo-header fields not defined for response (e.g., ":version"), which makes response malformed.
2. Access that server with Firefox Nightly.


Actual results:

Firefox Nightly accepts malformed response from that server without error.


Expected results:

Firefox Nightly must reject the malformed response.
https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-8.1.2.1 states that endpoints MUST treat a request or response that contains undefined or invalid pseudo-headers as malformed, which implies RST_STREAM of type PROTOCOL_ERROR or something.

"""
   Pseudo-header fields are only valid in the context in which they are
   defined.  Pseudo-header fields defined for requests MUST NOT appear
   in responses; pseudo-header fields defined for responses MUST NOT
   appear in requests.  Pseudo-header fields MUST NOT appear in
   trailers.  Endpoints MUST treat a request or response that contains
   undefined or invalid pseudo-header fields as malformed
   (Section 8.1.2.6).
"""

Currently, twitter returns ":version" header field even if h2 is negotiated.
I reported this to twitter, they said they'll fix it.
FYI, chrome 40 also happily accepts this kind of malformed response in h2.

Comment 1

[Patrick McManus [:mcmanus]]

thanks - we'll tighten this up after twitter makes a change

Comment 2

[u408661]

Twitter appears to have fixed this on their side in the last 4 months, so we can tighten this up. Nate, why don't you take a look at this in between iterations of the hpack memory reporter patch?

Comment 3

[nhughes]

Tightening up on allowable pseudo-header fields as part of patch: only allowing :status in the response header

Comment 4

[nhughes]

Attachment: Proposed Bug 1136727 Patch

Hey Nick - I was hoping you could give me some feedback on this patch when you have a chance. I've tightened things up to only allow for the :status header field...and I believe the other conditions for a properly-formed header are being checked: lower-case and :status preceding other fields. Also, this change opened up a door to refactor some of the code in Http2DecompressorOutputHeader(). Thanks!!

Comment 5

[u408661]

Comment on attachment 8623258 [details] [diff] [review]
Proposed Bug 1136727 Patch

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

So a couple things:

1. This looks like a diff from one in-progress version of your patch to another, which made it a little confusing to read. Please try to only show the full m-c to your current state diff unless someone specifically requests an interdiff (diff between 2 patch versions)

2. I think I would rather move the handling of :status/non-:status inside the existing isColonHeader block, instead of having multiple places where we check for a status/colon header. Your current method is a bit confusing - clarity is key when writing code!

Comment 6

[nhughes]

(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #5)
> Comment on attachment 8623258 [details] [diff] [review]
> Proposed Bug 1136727 Patch
> 
> Review of attachment 8623258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So a couple things:
> 
> 1. This looks like a diff from one in-progress version of your patch to
> another, which made it a little confusing to read. Please try to only show
> the full m-c to your current state diff unless someone specifically requests
> an interdiff (diff between 2 patch versions)
> 
> 2. I think I would rather move the handling of :status/non-:status inside
> the existing isColonHeader block, instead of having multiple places where we
> check for a status/colon header. Your current method is a bit confusing -
> clarity is key when writing code!

Thanks for the feedback! Sorry about the terrible diff; I've worked out how to get a proper diff after multiple commits now

Comment 7

[nhughes]

Attachment: Updated patch

Comment 8

[u408661]

Comment on attachment 8623431 [details] [diff] [review]
Updated patch

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

Looking better! A few more things to move around, then I think we'll be there.

::: netwerk/protocol/http/Http2Compression.cpp
@@ +379,5 @@
>      }
>    }
>  
> +  // http/2 transport level headers shouldn't be gatewayed into http/1
> +  bool isStatusHeader = false; // :status is the only allowed pseudo-header field

keep this named isColonHeader

@@ +385,5 @@
> +  for (const char *cPtr = name.BeginReading();
> +       cPtr && cPtr < name.EndReading();
> +       ++cPtr) {
> +    if (*cPtr == ':') {
> +      if(name.EqualsLiteral(":status"))

move this check into the if (isColonHeader) that's below here (probably make it if (!name.Equals...) and then do the error handling, that way you can keep the "good" code path as unindented as possible)

@@ +401,5 @@
> +      break;
> +    }
> +  }
> +
> +  if(isStatusHeader) {

nit: space between if and (

@@ +402,5 @@
> +    }
> +  }
> +
> +  if(isStatusHeader) {
> +    sAutoCString status(NS_LITERAL_CSTRING("HTTP/2.0 "));

move this bit after if (mSeenNonColonHeader) so we don't overwrite w/a smuggled header

Comment 9

[u408661]

Also, when you upload a new patch, always mark the one(s) that replaces as obsolete, so the attachment listing doesn't grow overly long

Comment 10

[nhughes]

Attachment: Updated patch

Comment 11

[u408661]

Comment on attachment 8623976 [details] [diff] [review]
Updated patch

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

::: netwerk/protocol/http/Http2Compression.cpp
@@ +400,4 @@
>        return NS_ERROR_ILLEGAL_VALUE;
>      }
> +
> +    status.Append(value);

This whole block modifying status (down through and including mHeaderStatus = value) should go beneath the mSeenNonColonHeader check (and, indeed, beneath the declaration of status you already put down there, else the code won't complile).

@@ +415,1 @@
>      LOG(("HTTP Decompressor not gatewaying %s into http/1",

Since we'll only get here if name == ":status", just make this into a plain string with no printf-style formatting.

Comment 12

[nhughes]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02b9719a5e3a

Comment 13

[nhughes]

Attachment: try: -b d -p all -u none -t none

Comment 14

[nhughes]

Please ignore the previous comment ^
Still figuring out Bugzilla-mq integration

Comment 15

[nhughes]

Attachment: Updated patch

Patch builds in debug-mode on all platforms (see link above^^)

Comment 16

[u408661]

Comment on attachment 8624830 [details] [diff] [review]
Updated patch

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

Cool! The functionality is all where we want it, just a few nits around non-code that need fixed up and then this will be ready to land.

Also, before setting checkin-needed, you should do a try run that exercises the h2 unit tests, just to make sure nothing there got broken. The best way to do this is with the try syntax

try: -b do -p linux64 -u xpcshell -t none

Lastly, let's double-check with Patrick about tightening this up this much before landing.

Pat - the spec says only :status is defined for responses (we were, in addition to things totally unmentioned in the spec, allowing :method, :authority, etc in responses). Nate did some checking around with google, twitter, and akamai, and no one with live sites that we knew of sent anything other than :status, so this would seem to be a safe change. Are there any other sites we should check with? Even so, are you comfortable with this change?

::: netwerk/protocol/http/Http2Compression.cpp
@@ +379,5 @@
>      }
>    }
>  
> +  // http/2 transport level headers shouldn't be gatewayed into http/1
> +  bool isColonHeader = false; // :status is the only allowed pseudo-header field

move the comment on this line down next to the "if (!name.EqualsLiteral..." line, and specify that :status is the only allowed field *in responses* (so future readers don't get confused thinking that comment also applies to requests)

@@ +394,5 @@
>      }
>    }
> +
> +  if (isColonHeader) {
> +    if(!name.EqualsLiteral(":status")) {

space between "if" and "("

@@ +395,5 @@
>    }
> +
> +  if (isColonHeader) {
> +    if(!name.EqualsLiteral(":status")) {
> +      LOG(("HTTP Decompressor found undefined : header"));

replace the "undefined : header" with "illegal response pseudo-header", and put the unexpected header name in the log as well:

LOG(("... header %s", name.BeginReading()));

@@ +405,5 @@
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    }
> +
> +    nsAutoCString status(NS_LITERAL_CSTRING("HTTP/2.0 "));
> +

get rid of this blank line

@@ +415,1 @@
>      LOG(("HTTP Decompressor not gatewaying %s into http/1",

No need to be doing string interpolation here, just make this "not gatewaying :status into http/1" and drop the call to name.BeginReading()

Comment 17

[u408661]

Patrick - see my question near the top of the previous comment for the info we need :)

Comment 18

[Patrick McManus [:mcmanus]]

yep.. we haven't broken the h2 web too much yet - so let's not be part of the inevitable slide. carry on!

Comment 19

[nhughes]

Patch build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=184c0bd9c2d3

Comment 20

[nhughes]

Attachment: Response Header Field Validation Patch

Build passes the h2 unit tests (xpcshell unit test suite) ^

Comment 21

[u408661]

Comment on attachment 8624920 [details] [diff] [review]
Response Header Field Validation Patch

Don't set checkin? until you've received an r+ - setting checkin? is a sign that the code has been reviewed and is ready to land whenever the sheriffs get to doing landings.

And usually, checkin? is only used if there's a single patch out of many attached that needs checked in. For a case like this (one patch, one attachment), just set the checkin-needed keyword (but again, not until whoever's reviewing sets r+)

Comment 22

[u408661]

Comment on attachment 8624920 [details] [diff] [review]
Response Header Field Validation Patch

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

Woohoo! Once your xpcshell runs in your latest try push turn green (meaning the tests have passed), go ahead and set the checkin-needed keyword on the bug. The sheriffs will take care of it from there on out.

Comment 23

[u408661]

Actually, it looks like your patch is missing the appropriate metadata at the beginning of it for a proper landing, so you'll need to upload a new version with that metadata. Instructions on that are at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Once you've created a single patch that has metadata at the top (in #-comments, you'll see) with no other changes outside the patch that's already here, attach it to this bug, obsolete the old one, and carry forward the r+ on the patch (that means just set r+ on your patch when you attach it - yes, you can do that). THEN you can set checkin-needed.

Comment 24

[nhughes]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55cf7d5d7368

Comment 25

[nhughes]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ed0f978c27

Comment 26

[nhughes]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a67a8098183

Comment 27

[nhughes]

Attachment: header_field_validation.diff

Nick - I was hoping to get another review on this patch whenever you have a chance. I had to make some changes since I received the 'r+' a couple of weeks ago. The patch now considers push responses and is passing the h2 tests.

See try build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c93634f4a99e

Thanks!

Comment 28

[u408661]

Comment on attachment 8631103 [details] [diff] [review]
header_field_validation.diff

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

Looks pretty good to me. r=hurley with the changes below.

So, next steps are - make the changes mentioned in the comments, change your commit message to end with "r=hurley", upload a new patch (make sure to obsolete this one!), carry forward r+, and set checkin-needed

Woo!

::: netwerk/protocol/http/Http2Compression.cpp
@@ +385,5 @@
>      status.Append(value);
>      status.AppendLiteral("\r\n");
>      mOutput->Insert(status, 0);
>      mHeaderStatus = value;
> +  } else if (name.EqualsLiteral(":authority") && mIsPush) {

Let's not have these mIsPush checks in this chain of "else if"s - it's a bit much for churn, and doesn't buy us anything (since we'll error out on the later check, anyway). The assignments are harmless in the !mIsPush situation, anyway.

@@ +409,5 @@
>        break;
>      }
>    }
>    if(isColonHeader) {
> +    // :status is the only pseudo-header field allowed in (non-push) responses

This comment is slightly misleading, since :status is the only field allowed in any response (including push); better to say something like ":status is the only pseudo-header field allowed in received HEADERS frames, PUSH_PROMISE allows the other pseudo-headers"

Comment 29

[nhughes]

Attachment: header_field_validation.diff

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c28d55bf04

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0c28d55bf04

Comment 32

[Kohei Yoshino]

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/undefined-pseudo-header-fields-are-no-longer-accepted-in-http-2/

Comment 33

[Jean-Yves Perrier [:teoli]]

Added a note in https://developer.mozilla.org/en-US/Firefox/Releases/42#HTTP