unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
From: Eric Wong <bofh@yhbt.net>
To: unicorn-public@yhbt.net
Cc: Sam Sanoop <sams@snyk.io>
Subject: [RFC] http_response: ignore invalid header response characters
Date: Thu, 26 Nov 2020 11:59:02 +0000	[thread overview]
Message-ID: <20201126115902.GA8883@dcvr> (raw)

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.

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,

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
-          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" }
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'
+  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', {}, [])

             reply	other threads:[~2020-11-26 11:59 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-26 11:59 Eric Wong [this message]
2021-01-06 17:53 ` [RFC] http_response: ignore invalid header response characters Eric Wong
2021-01-13 23:20   ` Sam Sanoop
2021-01-14  4:37     ` Eric Wong
2021-01-28 17:29       ` Sam Sanoop
2021-01-28 22:39         ` Eric Wong
2021-02-17 21:46           ` Sam Sanoop
2021-02-26 11:15 ` Eric Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information: https://yhbt.net/unicorn/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201126115902.GA8883@dcvr \
    --to=bofh@yhbt.net \
    --cc=sams@snyk.io \
    --cc=unicorn-public@yhbt.net \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).