New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 787581 link

Starred by 0 users
Status: Fixed
Owner:
b...@chromium.org
Closed: Nov 2017
Cc:
█ lassey@chromium.org
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug



Sign in to add a comment

Reject HTTP/2 header field values containing invalid characters.

Project Member Reported by b...@chromium.org, Nov 21 2017 
Firefox conforms to the HTTP/2 specification in terms of allowed characters in header field values, see https://bugzilla.mozilla.org/show_bug.cgi?id=1411659#c7.  Chrome, however, is too permissive.  In particular, it allows CR or LF characters in a header field (but not a CR-LF pair).

RFC7540 Section 10.3 says "Any request or response that contains a character not permitted in a header field value MUST be treated as malformed (Section 8.1.2.6). Valid characters are defined by the field-content ABNF rule in Section 3.2 of [RFC7230]."  RFC7230 says
field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
field-vchar    = VCHAR / obs-text
obs-text       = %x80-FF

For VCHAR, RFC7230 defers to RFC5234 Appendix B.1, which states:
VCHAR          =  %x21-7E

HeaderCoalescer::AddHeader() should enforce this.
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5438515ef5fc2b8043b77ec1270d5f765502f31

commit e5438515ef5fc2b8043b77ec1270d5f765502f31
Author: Bence Béky <bnc@chromium.org>
Date: Wed Nov 22 23:07:42 2017

Join cookies with ; in tests.

Currently, AppendToHeaderBlock() joins values for duplicate header names
with a \0 character, which is incorrect for cookies.  What makes this
especially bad is that HpackEncoder only splits a header value at \0
characters if the header name is not "cookie", so such incorrect header
blocks are encoded with a \0 character on the wire, which is against the
specification.  In particular, if I fix HeaderCoalescer to signal an
error on a \0 character in a header value, then
SpdyNetworkTransactionTest.ResponseHeaders fails because of this.

Instead, this CL makes AppendToHeaderBlock() defer to SpdyHeaderBlock's
built-in header name joining mechanism, which does the right thing.
Also update SpdyNetworkTransactionTest.ResponseHeaders to reflect this
behavior, and clean it up a bit.

Bug:  787581 
Change-Id: I58f8b8b72992c06bb5b365347f1eae75da6dd97b
Reviewed-on: https://chromium-review.googlesource.com/786335
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518787}
[modify] https://crrev.com/e5438515ef5fc2b8043b77ec1270d5f765502f31/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/e5438515ef5fc2b8043b77ec1270d5f765502f31/net/spdy/chromium/spdy_test_util_common.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc5041fa78d1d5ccfc6d40f9c88511e07c48c204

commit dc5041fa78d1d5ccfc6d40f9c88511e07c48c204
Author: Bence Béky <bnc@chromium.org>
Date: Tue Nov 28 18:40:48 2017

Reject invalid characters in HTTP/2 header names.

Bug:  787581 
Change-Id: Iba4ae13dbeacbf8f731c4a9d712eb1e0f95f61d1
Reviewed-on: https://chromium-review.googlesource.com/786612
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519764}
[modify] https://crrev.com/dc5041fa78d1d5ccfc6d40f9c88511e07c48c204/net/spdy/chromium/header_coalescer.cc
[modify] https://crrev.com/dc5041fa78d1d5ccfc6d40f9c88511e07c48c204/net/spdy/chromium/header_coalescer_test.cc

Comment 3 by b...@chromium.org, Nov 28 2017

Status: Fixed (was: Started)
►

Sign in to add a comment