unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
* [RFC] http_response: ignore invalid header response characters
@ 2020-11-26 11:59 Eric Wong
  2021-01-06 17:53 ` Eric Wong
  2021-02-26 11:15 ` Eric Wong
  0 siblings, 2 replies; 8+ messages in thread
From: Eric Wong @ 2020-11-26 11:59 UTC (permalink / raw)
  To: unicorn-public; +Cc: Sam Sanoop

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-02-26 11:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-26 11:59 [RFC] http_response: ignore invalid header response characters Eric Wong
2021-01-06 17:53 ` 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

Code repositories for project(s) associated with this public inbox

	https://yhbt.net/unicorn.git/

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).