HTTP Injection - fix bug + 1 more vector (#2136)

+ Fixes a problem in 4.3.2/3.12.3 where we were not splitting newlines in headers according to Rack spec
+ Fixes another vector for HTTP injection - early hints

History.md

@@ -23,6 +23,13 @@
   * Simplify `Runner#start_control` URL parsing (#2111)
   * Removed the IOBuffer extension and replaced with Ruby (#1980)
 
+
+## 4.3.3 and 3.12.4 / 2020-02-28
+  * Bugfixes
+    * Fix: Fixes a problem where we weren't splitting headers correctly on newlines (#2132)
+  * Security
+    * Fix: Prevent HTTP Response splitting via CR in early hints.
+
 ## 4.3.2 and 3.12.3 / 2020-02-27
 
 * Security

lib/puma/const.rb

@@ -228,7 +228,7 @@ module Puma
     COLON = ": ".freeze
 
     NEWLINE = "\n".freeze
-    CRLF_REGEX = /[\r\n]/.freeze
+    HTTP_INJECTION_REGEX = /[\r\n]/.freeze
 
     HIJACK_P = "rack.hijack?".freeze
     HIJACK = "rack.hijack".freeze

lib/puma/server.rb

@@ -663,6 +663,7 @@ module Puma
             headers.each_pair do |k, vs|
               if vs.respond_to?(:to_s) && !vs.to_s.empty?
                 vs.to_s.split(NEWLINE).each do |v|
+                  next if possible_header_injection?(v)
                   fast_write client, "#{k}: #{v}\r\n"
                 end
               else
@@ -687,8 +688,6 @@ module Puma
           status, headers, res_body = @app.call(env)
 
           return :async if req.hijacked
-          # Checking to see if an attacker is trying to inject headers into the response
-          headers.reject! { |_k, v| CRLF_REGEX =~ v.to_s }
 
           status = status.to_i
 
@@ -766,6 +765,7 @@ module Puma
         headers.each do |k, vs|
           case k.downcase
           when CONTENT_LENGTH2
+            next if possible_header_injection?(vs)
             content_length = vs
             next
           when TRANSFER_ENCODING
@@ -778,6 +778,7 @@ module Puma
 
           if vs.respond_to?(:to_s) && !vs.to_s.empty?
             vs.to_s.split(NEWLINE).each do |v|
+              next if possible_header_injection?(v)
               lines.append k, colon, v, line_ending
             end
           else
@@ -1048,5 +1049,10 @@ module Puma
     def shutting_down?
       @status == :stop || @status == :restart
     end
+
+    def possible_header_injection?(header_value)
+      HTTP_INJECTION_REGEX =~ header_value.to_s
+    end
+    private :possible_header_injection?
   end
 end

test/test_puma_server.rb

@@ -772,22 +772,67 @@ EOF
     test_open_connection_wait
   end
 
-  # https://github.com/ruby/ruby/commit/d9d4a28f1cdd05a0e8dabb36d747d40bbcc30f16
-  def test_prevent_response_splitting_headers
-    server_run app: ->(_) { [200, {'X-header' => "malicious\r\nCookie: hack"}, ["Hello"]] }
+  # Rack may pass a newline in a header expecting us to split it.
+  def test_newline_splits
+    server_run app: ->(_) { [200, {'X-header' => "first line\nsecond line"}, ["Hello"]] }
+
     data = send_http_and_read "HEAD / HTTP/1.0\r\n\r\n"
-    refute_match 'hack', data
+
+    assert_match "X-header: first line\r\nX-header: second line\r\n", data
   end
 
-  def test_prevent_response_splitting_headers_cr
-    server_run app: ->(_) { [200, {'X-header' => "malicious\rCookie: hack"}, ["Hello"]] }
+  def test_newline_splits_in_early_hint
+    server_run early_hints: true, app: ->(env) do
+      env['rack.early_hints'].call({'X-header' => "first line\nsecond line"})
+      [200, {}, ["Hello world!"]]
+    end
+
     data = send_http_and_read "HEAD / HTTP/1.0\r\n\r\n"
-    refute_match 'hack', data
+
+    assert_match "X-header: first line\r\nX-header: second line\r\n", data
   end
 
-  def test_prevent_response_splitting_headers_lf
-    server_run app: ->(_) { [200, {'X-header' => "malicious\nCookie: hack"}, ["Hello"]] }
+  # To comply with the Rack spec, we have to split header field values
+  # containing newlines into multiple headers.
+  def assert_does_not_allow_http_injection(app, opts = {})
+    server_run(early_hints: opts[:early_hints], app: app)
+
     data = send_http_and_read "HEAD / HTTP/1.0\r\n\r\n"
-    refute_match 'hack', data
+
+    refute_match(/[\r\n]Cookie: hack[\r\n]/, data)
+  end
+
+  # HTTP Injection Tests
+  #
+  # Puma should prevent injection of CR and LF characters into headers, either as
+  # CRLF or CR or LF, because browsers may interpret it at as a line end and
+  # allow untrusted input in the header to split the header or start the
+  # response body. While it's not documented anywhere and they shouldn't be doing
+  # it, Chrome and curl recognize a lone CR as a line end. According to RFC,
+  # clients SHOULD interpret LF as a line end for robustness, and CRLF is the
+  # specced line end.
+  #
+  # There are three different tests because there are three ways to set header
+  # content in Puma. Regular (rack env), early hints, and a special case for
+  # overriding content-length.
+  {"cr" => "\r", "lf" => "\n", "crlf" => "\r\n"}.each do |suffix, line_ending|
+    # The cr-only case for the following test was CVE-2020-5247
+    define_method("test_prevent_response_splitting_headers_#{suffix}") do
+      app = ->(_) { [200, {'X-header' => "untrusted input#{line_ending}Cookie: hack"}, ["Hello"]] }
+      assert_does_not_allow_http_injection(app)
+    end
+
+    define_method("test_prevent_response_splitting_headers_early_hint_#{suffix}") do
+      app = ->(env) do
+        env['rack.early_hints'].call("X-header" => "untrusted input#{line_ending}Cookie: hack")
+        [200, {}, ["Hello"]]
+      end
+      assert_does_not_allow_http_injection(app, early_hints: true)
+    end
+
+    define_method("test_prevent_content_length_injection_#{suffix}") do
+      app = ->(_) { [200, {'content-length' => "untrusted input#{line_ending}Cookie: hack"}, ["Hello"]] }
+      assert_does_not_allow_http_injection(app)
+    end
   end
 end