Subject: Bare CR in header fields

From: Cory Benfield <[email-masked]>
Date: Wed, 20 Jan 2021 21:06:16 +0000

Hey folks,

I just got around to noticing this text in section 2.2 (Message
Parsing) of the proposed messaging draft as part of last-call review
(https://www.ietf.org/archive/id/draft-ietf-httpbis-messaging-14.html#section-2.2):

> A sender MUST NOT generate a bare CR (a CR character not immediately followed by LF) within any protocol elements other than the content. A recipient of such a bare CR MUST consider that element to be invalid or replace each bare CR with SP before processing the element or forwarding the message.

Out of curiosity I did some investigation, because I'd been shared an
example in the past of a client creating a bare CR as an LF in header
field parsing. My investigation revealed that Safari, Chrome, and
Opera all treat a bare CR as though it were a field line terminator,
and that Firefox appeared to stop parsing at the CR (but use all bytes
prior to that CR in the value).

Why are we specifying a normative requirement to do something no major
implementation appears to do? The web platform tests have an open
issue to add this test
(https://github.com/web-platform-tests/wpt/pull/13524), but so far as
I can see that test as written plans to encode the existing
non-spec-compliant behaviour.

On the original discussion thread on this issue
(https://github.com/httpwg/http-core/issues/31) Roy notes that this
has always been forbidden in HTTP/1.1. While I appreciate that, it is
not terribly helpful for us to specify behaviours that do not reflect
common implementer behaviour.

In particular, I'll note that Roy said, a year and a half ago, "If
some browsers interpret CR as a line delimiter in HTTP protocol fields
(other than payload content), they have security holes that should be
fixed." Is that the position of this working group? Who is
coordinating with these implementations to address that security hole?

Cory

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 21 Jan 2021 05:05:01 +0100

Hi Cory,

On Wed, Jan 20, 2021 at 09:06:16PM +0000, Cory Benfield wrote:
> fixed." Is that the position of this working group?

To be honest, that was my immediate thinking when I've read your scary
story about browsers taking them as field terminators! It indeed means
that it's likely possible to send them such responses and expect some
content smuggling over persistent connections:

   HTTP/1.1 200 OK
   Content-length: 10000
   X-foo: blah\rTransfer-encoding: chunked

   0\r\n
   HTTP/1.1 200 OK
   Content-length: 9900
   
   This is the content non-compliant clients will take as the response
   for the second request.

This could for example be used to inject malicious code bypassing client-side
anti-malware solutions, so it should not be taken lightly.

> Who is

That's unclear, as I think that all major implementations were represented
when this spec was written. However it's certain that it's not always trivial
for an implementation to spot issues that affect them in so detailed a spec.
It's also possible that some of these decided to still support that behavior
for various reasons and to apply special safety belts (e.g. no cache, no
persistent connections, no cookies etc).

Maybe the simplest would be to open a bug report in each of them's issue
tracker. Even if they decide to close it, the justfication will remain
there and accessible to anyone, should new threats pop up.

Just my two cents,
Willy

---

From: Cory Benfield <[email-masked]>
Date: Thu, 21 Jan 2021 07:49:52 +0000

> anti-malware solutions, so it should not be taken lightly.

Fortunately, as noted in the original GitHub thread, I don't think any
browsers are subject to a complete response-smuggling attack. This is
mostly because they appear to enforce different rules around the
termination of the header compared to termination of header fields. I
updated my test to attempt a response smuggling attack and no browser
appeared to fall for the smuggled response. CRCR does not appear to
cause any browser I tested to conclude the header is complete. This is
good news!

However, the parse is still bad. My test example sends:

HTTP/1.1 302 Found<CRLF>
Location: /redirect<CRLF>
Content-Length: 0<CRLF>
x-custom-header: ok<CRLF>
x-custom-header-2: y<CRCR>HTTP/1.1 404 Not Found<CR>Content-Length:
0<CRCR><CRLFCRLF>

Chrome does not get tricked by the 404: it does correctly spot that
the CRLFCRLF is the terminator for the header. However, it mis-parses
the header fields. In the web inspector Chrome clearly notes _two_
"Content-Length: 0" header field lines, and if I add additional header
field lines to the second block those will be parsed as though they
were syntactically correct header field lines.

For my part, I don't think there's a huge security risk here. What I
do think is that we need to be cautious when our specs differ widely
from actual deployment. We've done this in the past with notes in the
RFC that suggest that while the normative requirement is (and per Roy,
always has been) to do X, many implementations do Y. That may be an
option for us here. Otherwise, I'd like to hear from the
currently-mentioned user-agents as to whether they plan to remain
non-conformant in this way.

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 21 Jan 2021 09:36:29 +0100

> were syntactically correct header field lines.

Hmmm that's still ugly, it means for example that you could send
a Location header in a dummy header, or some cache-control to enforce
long-time storage, or even set cookies that are not inspected by
whatever privacy inspection tool is installed on the machine.

And by the way I just tested the injection of transfer-encoding, and
it does indeed affect Chromium. There's what I've done:

  $ printf "HTTP/1.1 200 OK\r\nContent-type: text/plain\r\nContent-Length: 6\r\nx-fun: yes\rTransfer-encoding: chunked\r\n\r\n22\r\nHello\nThis should not be there :-(\r\n0\r\n\r\n" | nc -lp7777

On Firefox it correctly says:

  22
  He

On Chromium it says:

  Hello
  This should not be there :-(

Thus it IS definitely possible to bypass upfront filtering to deliver
some uninspected contents to the browser this way.

> For my part, I don't think there's a huge security risk here.

Making a device consume more data than expected is always a problem.
The severity depends on what can be done through this, but above I'm
definitely seeing something that opens large doors to those who want
to play with it.

> What I
> from actual deployment.

It has always been known that browsers need to resort to a lot of ugly
tricks due to various bogus applications that remain in field that get
broken as soon as they try to be more compliant, but that must not
warrant making all implementations vulnerable, especially intermediaries
which could become amplifiers of such issues by normalizing what they
received on one side.

However it would be nice that such horrible tricks, if still required
(which for some part I doubt given that different browsers already act
differently), be clearly documented wiht strict limitations on their
side effects (e.g. never execute a script downloaded this way, never
follow a redirect, nor cache or whatever). I can understand the need
to support horribly bogus legacy applications, and the rise of IoT
really does not help, but these must not become the standard. If at
least the browsers could emit a warning on first access to such pages
like they do with bad certs, that would be a great way to discourage
new developers from continuing to deploy such bugs.

Willy