From 9fe57c0fc5561088a2df42e4438992591e9d917e Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Fri, 12 Feb 2021 12:59:54 -0600 Subject: [PATCH] Refactor CVE-2021-22881 fix Follow-up to 83a6ac3fee8fd538ce7e0088913ff54f0f9bcb6f. This allows `HTTP_HOST` to be omitted as before, and reduces the number of object allocations per request. Benchmark: ```ruby # frozen_string_literal: true require "benchmark/memory" HOST = "example.com:80" BEFORE_REGEXP = /\A(?[a-z0-9.-]+|\[[a-f0-9]*:[a-f0-9.:]+\])(:\d+)?\z/ AFTER_REGEXP = /(?:\A|,[ ]?)([a-z0-9.-]+|\[[a-f0-9]*:[a-f0-9.:]+\])(?::\d+)?\z/i Benchmark.memory do |x| x.report("BEFORE (non-nil X-Forwarded-Host)") do origin_host = BEFORE_REGEXP.match(HOST.to_s.downcase)[:host] forwarded_host = BEFORE_REGEXP.match(HOST.to_s.split(/,\s?/).last)[:host] end x.report("BEFORE (nil X-Forwarded-Host)") do origin_host = BEFORE_REGEXP.match(HOST.to_s.downcase)[:host] forwarded_host = BEFORE_REGEXP.match(nil.to_s.split(/,\s?/).last) end x.report("AFTER (non-nil X-Forwarded-Host)") do origin_host = HOST&.slice(AFTER_REGEXP, 1) || "" forwarded_host = HOST&.slice(AFTER_REGEXP, 1) || "" end x.report("AFTER (nil X-Forwarded-Host)") do origin_host = HOST&.slice(AFTER_REGEXP, 1) || "" forwarded_host = nil&.slice(AFTER_REGEXP, 1) || "" end end ``` Results: ``` BEFORE (non-nil X-Forwarded-Host) 616.000 memsize ( 208.000 retained) 9.000 objects ( 2.000 retained) 2.000 strings ( 1.000 retained) BEFORE (nil X-Forwarded-Host) 328.000 memsize ( 0.000 retained) 5.000 objects ( 0.000 retained) 2.000 strings ( 0.000 retained) AFTER (non-nil X-Forwarded-Host) 248.000 memsize ( 168.000 retained) 3.000 objects ( 1.000 retained) 1.000 strings ( 0.000 retained) AFTER (nil X-Forwarded-Host) 40.000 memsize ( 0.000 retained) 1.000 objects ( 0.000 retained) 1.000 strings ( 0.000 retained) ``` [CVE-2021-22942] --- .../middleware/host_authorization.rb | 20 +++++++------------ .../test/dispatch/host_authorization_test.rb | 4 ++-- .../application/middleware/remote_ip_test.rb | 1 - railties/test/isolation/abstract_unit.rb | 2 +- 4 files changed, 10 insertions(+), 17 deletions(-) diff --git a/actionpack/lib/action_dispatch/middleware/host_authorization.rb b/actionpack/lib/action_dispatch/middleware/host_authorization.rb index ad209c2669..8e39aee87d 100644 --- a/actionpack/lib/action_dispatch/middleware/host_authorization.rb +++ b/actionpack/lib/action_dispatch/middleware/host_authorization.rb @@ -86,21 +86,15 @@ def call(env) end private + HOSTNAME = /[a-z0-9.-]+|\[[a-f0-9]*:[a-f0-9.:]+\]/i + VALID_ORIGIN_HOST = /\A(#{HOSTNAME})(?::\d+)?\z/ + VALID_FORWARDED_HOST = /(?:\A|,[ ]?)(#{HOSTNAME})(?::\d+)?\z/ + def authorized?(request) - valid_host = / - \A - (?[a-z0-9.-]+|\[[a-f0-9]*:[a-f0-9.:]+\]) - (:\d+)? - \z - /x + origin_host = request.get_header("HTTP_HOST")&.slice(VALID_ORIGIN_HOST, 1) || "" + forwarded_host = request.x_forwarded_host&.slice(VALID_FORWARDED_HOST, 1) || "" - origin_host = valid_host.match( - request.get_header("HTTP_HOST").to_s.downcase) - forwarded_host = valid_host.match( - request.x_forwarded_host.to_s.split(/,\s?/).last) - - origin_host && @permissions.allows?(origin_host[:host]) && ( - forwarded_host.nil? || @permissions.allows?(forwarded_host[:host])) + @permissions.allows?(origin_host) && (forwarded_host.blank? || @permissions.allows?(forwarded_host)) end def mark_as_authorized(request) diff --git a/actionpack/test/dispatch/host_authorization_test.rb b/actionpack/test/dispatch/host_authorization_test.rb index 4a39747f8d..63562c9046 100644 --- a/actionpack/test/dispatch/host_authorization_test.rb +++ b/actionpack/test/dispatch/host_authorization_test.rb @@ -207,11 +207,11 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest @app = ActionDispatch::HostAuthorization.new(App, ".example.com") get "/", env: { - "HOST" => "example.com#sub.example.com", + "HOST" => "attacker.com#x.example.com", } assert_response :forbidden - assert_match "Blocked host: example.com#sub.example.com", response.body + assert_match "Blocked host: attacker.com#x.example.com", response.body end test "blocks requests to similar host" do diff --git a/railties/test/application/middleware/remote_ip_test.rb b/railties/test/application/middleware/remote_ip_test.rb index 0dc3dc3979..4c6fa1b371 100644 --- a/railties/test/application/middleware/remote_ip_test.rb +++ b/railties/test/application/middleware/remote_ip_test.rb @@ -11,7 +11,6 @@ class RemoteIpTest < ActiveSupport::TestCase def remote_ip(env = {}) remote_ip = nil env = Rack::MockRequest.env_for("/").merge(env).merge!( - "HTTP_HOST" => "example.com", "action_dispatch.show_exceptions" => false, "action_dispatch.key_generator" => ActiveSupport::CachingKeyGenerator.new( ActiveSupport::KeyGenerator.new("b3c631c314c0bbca50c1b2843150fe33", iterations: 1000) diff --git a/railties/test/isolation/abstract_unit.rb b/railties/test/isolation/abstract_unit.rb index 1d10238683..584d307690 100644 --- a/railties/test/isolation/abstract_unit.rb +++ b/railties/test/isolation/abstract_unit.rb @@ -82,7 +82,7 @@ def extract_body(response) end def get(path) - @app.call(::Rack::MockRequest.env_for(path, "HTTP_HOST" => "example.com")) + @app.call(::Rack::MockRequest.env_for(path)) end def assert_welcome(resp) -- 2.30.2