CVE-2020-5249: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: In Puma (RubyGem) before 4.3.3 and 3.12.4, if an application using Puma allows untrusted input in an early-hints header, an attacker can use a carriage return character to end the header and inject malicious content, such as additional headers or an entirely new response body. This vulnerability is known as HTTP Response Splitting. While not an attack in itself, response splitting is a vector for several other attacks, such as cross-site scripting (XSS). This is related to CVE-2020-5247, which fixed this vulnerability but only for regular responses. This has been fixed in 4.3.3 and 3.12.4.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 6.5
Published: 2020-03-02T16:15Z
Repository: https://github.com/puma/puma
Language: Ruby
Files changed: History.md, lib/puma/const.rb, lib/puma/server.rb, test/test_puma_server.rb
Commit date: 2020-02-28 12:53:29 -0600
Commit message: 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
Diff: diff --git a/History.md b/History.md
index e2f17dab..25eab1b6 100644
--- a/History.md
+++ b/History.md
@@ -21,10 +21,17 @@
   * Remove unused loader argument from Plugin initializer (#2095)
   * Simplify `Configuration.random_token` and remove insecure fallback (#2102)
   * 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
   * Fix: Prevent HTTP Response splitting via CR/LF in header values. CVE-2020-5247.
 
diff --git a/lib/puma/const.rb b/lib/puma/const.rb
index 2e320b7a..1cd1a9d3 100644
--- a/lib/puma/const.rb
+++ b/lib/puma/const.rb
@@ -226,11 +226,11 @@ module Puma
     CHUNKED = "chunked".freeze
 
     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
     HIJACK_IO = "rack.hijack_io".freeze
 
diff --git a/lib/puma/server.rb b/lib/puma/server.rb
index a6bbb5f2..1c72a61c 100644
--- a/lib/puma/server.rb
+++ b/lib/puma/server.rb
@@ -661,10 +661,11 @@ module Puma
             fast_write client, "HTTP/1.1 103 Early Hints\r\n".freeze
 
             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
                 fast_write client, "#{k}: #{vs}\r\n"
               end
@@ -685,12 +686,10 @@ module Puma
       begin
         begin
           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
 
           if status == -1
             unless headers.empty? and res_body == []
@@ -764,10 +763,11 @@ module Puma
         response_hijack = nil
 
         headers.each do |k, vs|
           case k.downcase
           when CONTENT_LENGTH2
+            next if possible_header_injection?(vs)
             content_length = vs
             next
           when TRANSFER_ENCODING
             allow_chunked = false
             content_length = nil
@@ -776,10 +776,11 @@ module Puma
             next
           end
 
           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
             lines.append k, colon, line_ending
           end
@@ -1046,7 +1047,12 @@ module Puma
     end
 
     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
diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb
index 6e198b78..2fe4eced 100644
--- a/test/test_puma_server.rb
+++ b/test/test_puma_server.rb
@@ -770,24 +770,69 @@ EOF
   def test_open_connection_wait_no_queue
     @server = Puma::Server.new @app, @events, queue_requests: false
     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

Vulnerable code:     CRLF_REGEX = /[\r\n]/.freeze
          # Checking to see if an attacker is trying to inject headers into the response
          headers.reject! { |_k, v| CRLF_REGEX =~ v.to_s }
  # https://github.com/ruby/ruby/commit/d9d4a28f1cdd05a0e8dabb36d747d40bbcc30f16
  def test_prevent_response_splitting_headers
    server_run app: ->(_) { [200, {'X-header' => "malicious\r\nCookie: hack"}, ["Hello"]] }
    refute_match 'hack', data
  def test_prevent_response_splitting_headers_cr
    server_run app: ->(_) { [200, {'X-header' => "malicious\rCookie: hack"}, ["Hello"]] }
    refute_match 'hack', data
  def test_prevent_response_splitting_headers_lf
    server_run app: ->(_) { [200, {'X-header' => "malicious\nCookie: hack"}, ["Hello"]] }
    refute_match 'hack', data
Fixed code: 
## 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.

    HTTP_INJECTION_REGEX = /[\r\n]/.freeze
                  next if possible_header_injection?(v)
            next if possible_header_injection?(vs)
              next if possible_header_injection?(v)

    def possible_header_injection?(header_value)
      HTTP_INJECTION_REGEX =~ header_value.to_s
    end
    private :possible_header_injection?
  # 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"]] }


    assert_match "X-header: first line\r\nX-header: second line\r\n", data
  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


    assert_match "X-header: first line\r\nX-header: second line\r\n", data
  # 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)


    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