From c22712fc93284a45a93f9ad7023888f3a65524f3 Mon Sep 17 00:00:00 2001 From: Nate Berkopec Date: Fri, 28 Feb 2020 12:53:29 -0600 Subject: [PATCH] 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 | 7 +++++ lib/puma/const.rb | 2 +- lib/puma/server.rb | 10 +++++-- test/test_puma_server.rb | 65 +++++++++++++++++++++++++++++++++------- 4 files changed, 71 insertions(+), 13 deletions(-) diff --git a/History.md b/History.md index e2f17dab7b..25eab1b640 100644 --- a/History.md +++ b/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 diff --git a/lib/puma/const.rb b/lib/puma/const.rb index 2e320b7aaf..1cd1a9d397 100644 --- a/lib/puma/const.rb +++ b/lib/puma/const.rb @@ -228,7 +228,7 @@ module Const 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 diff --git a/lib/puma/server.rb b/lib/puma/server.rb index a6bbb5f286..1c72a61c9c 100644 --- a/lib/puma/server.rb +++ b/lib/puma/server.rb @@ -663,6 +663,7 @@ def handle_request(req, lines) 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 @@ def handle_request(req, lines) 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 @@ def handle_request(req, lines) 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 @@ def handle_request(req, lines) 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 @@ def self.current 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 6e198b7886..2fe4ecedc3 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -772,22 +772,67 @@ def test_open_connection_wait_no_queue 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