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

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: tatsuhiro.t, Assigned: nhughes, Mentored)

Tracking

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

Trunk
mozilla42
x86_64
Linux
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: spdy)

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

4 years ago
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.
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
thanks - we'll tighten this up after twitter makes a change
Whiteboard: spdy

Comment 2

4 years ago
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?
Assignee: nobody → nhughes
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

4 years ago
Mentor: hurley
(Assignee)

Comment 3

4 years ago
Tightening up on allowable pseudo-header fields as part of patch: only allowing :status in the response header
(Assignee)

Comment 4

4 years ago
Posted patch Proposed Bug 1136727 Patch (obsolete) — Splinter Review
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!!
Attachment #8623258 - Flags: feedback?(hurley)

Comment 5

4 years ago
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!
Attachment #8623258 - Flags: feedback?(hurley)
(Assignee)

Comment 6

4 years ago
(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
(Assignee)

Comment 7

4 years ago
Posted patch Updated patch (obsolete) — Splinter Review
Attachment #8623431 - Flags: review?(hurley)

Comment 8

4 years ago
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
Attachment #8623431 - Flags: review?(hurley) → review-

Comment 9

4 years ago
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
(Assignee)

Comment 10

4 years ago
Posted patch Updated patch (obsolete) — Splinter Review
Attachment #8623258 - Attachment is obsolete: true
Attachment #8623431 - Attachment is obsolete: true
Attachment #8623976 - Flags: review?(hurley)

Comment 11

4 years ago
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.
Attachment #8623976 - Flags: review?(hurley)
(Assignee)

Comment 13

4 years ago
(Assignee)

Comment 14

4 years ago
Please ignore the previous comment ^
Still figuring out Bugzilla-mq integration
(Assignee)

Comment 15

4 years ago
Posted patch Updated patch (obsolete) — Splinter Review
Patch builds in debug-mode on all platforms (see link above^^)
Attachment #8623976 - Attachment is obsolete: true
Attachment #8624819 - Attachment is obsolete: true
Attachment #8624830 - Flags: review?(hurley)

Comment 16

4 years ago
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()
Attachment #8624830 - Flags: review?(hurley)

Comment 17

4 years ago
Patrick - see my question near the top of the previous comment for the info we need :)
Flags: needinfo?(mcmanus)
yep.. we haven't broken the h2 web too much yet - so let's not be part of the inevitable slide. carry on!
Flags: needinfo?(mcmanus)
(Assignee)

Comment 20

4 years ago
Build passes the h2 unit tests (xpcshell unit test suite) ^
Attachment #8624830 - Attachment is obsolete: true
Attachment #8624920 - Flags: review?(hurley)
Attachment #8624920 - Flags: checkin?(hurley)

Comment 21

4 years ago
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+)
Attachment #8624920 - Flags: checkin?(hurley)

Comment 22

4 years ago
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.
Attachment #8624920 - Flags: review?(hurley) → review+

Comment 23

4 years ago
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.
(Assignee)

Comment 27

4 years ago
Posted patch header_field_validation.diff (obsolete) — Splinter Review
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!
Attachment #8624920 - Attachment is obsolete: true
Attachment #8631103 - Flags: review?(hurley)

Comment 28

4 years ago
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"
Attachment #8631103 - Flags: review?(hurley) → review+
(Assignee)

Comment 29

4 years ago
Attachment #8631103 - Attachment is obsolete: true
Attachment #8631262 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f0c28d55bf04
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Keywords: dev-doc-needed, site-compat
Added a note in https://developer.mozilla.org/en-US/Firefox/Releases/42#HTTP
Keywords: dev-doc-needed → dev-doc-complete

Updated

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