Disallow \r\n in Header text

[ehamberg]

As the following program illustrates, it is possible to set more than one HTTP header by passing a string containing \r\n to setHeader, possibly leading to duplicate headers:

{-# LANGUAGE OverloadedStrings #-}

import Web.Scotty

main = scotty 7000 $ get "/" $ do
         addHeader "X-Foo" "Hey\r\nContent-Type: bla"
         text "..."

$ curl -i 'http://localhost:7000'
HTTP/1.1 200 OK
Transfer-Encoding: chunked
Date: Thu, 22 May 2014 20:05:28 GMT
Server: Warp/2.1.5.1
Content-Type: text/plain
X-Foo: Hey
Content-Type: bla

...

Of course, data should ideally be sanitized before it reaches all they way to setHeader, but it would nonetheless be good to disallow this at the setHeader level, or possibly even lower-level than that.

[xich]

Hmmm, I can see how this would lead to unexpected headers...

Unfortunately, we can't simply disallow \n, because headers can be multi-line:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

Each additional line of the header must begin with at least one space or tab to be considered part of the previous line. So:

addHeader "X-Foo" "Hey\r\n Content-Type: bla"

should parse as a single X-Foo header with a multi-line value.

I'm trying to think of the most graceful way to handle this. Maybe emit a warning to the console?

"addHeader: header value contains newline character - X-Foo Hey\r\nContent-Type: blah"

[dpwiz]

Header Injection is a security issue and should be treated accordingly.

I suggest disallowing \n\r in headers by default (better safe than sorry) and adding addHeaderMultiline with \r\n validation.

[alexanderkjeldaas]

+1 to assuming this is a security violation. An alternative to addHeaderMultiline is to introduce a data ValidatedHeader = ValidatedHeader Text which can pre-cook a string to skip the check for \n\r.

[mhitza]

Explicitly disallowing multiline headers by default it's an additional prevention against HTTP Response Splitting, by which Cache Poisoning can be done

[ocramz]

If I understand correctly, multiline headers have been deprecated since RFC9110 superseded RFC2616 : https://www.rfc-editor.org/rfc/rfc9110.html#section-5.5-5 :

Field values containing CR, LF, or NUL characters are invalid and dangerous, due to the varying ways that implementations might parse and interpret those characters; a recipient of CR, LF, or NUL within a field value MUST either reject the message or replace each of those characters with SP before further processing or forwarding of that message. Field values containing other CTL characters are also invalid; however, recipients MAY retain such characters for the sake of robustness when they appear within a safe context (e.g., an application-specific quoted string that will not be processed by any downstream HTTP parser).

So I would vote for sanitizing headers in one way or another.

[ocramz]

Design notes from a former PR review: #359 (comment)