8000 Refactor CVE-2021-22881 fix · rails/rails@5e9973d · GitHub
[go: up one dir, main page]
Skip to content

Commit 5e9973d

Browse files
jonathanhefnertenderlove
jonathanhefner
authored and
tenderlove
committed
Refactor CVE-2021-22881 fix
Follow-up to 83a6ac3. 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(?<host>[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]
1 parent 8321702 com
8000
mit 5e9973d

File tree

4 files changed

+12
-19
lines changed

4 files changed

+12
-19
lines changed

‎actionpack/lib/action_dispatch/middleware/host_authorization.rb

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -102,21 +102,15 @@ def call(env)
102102
end
103103

104104
private
105+
HOSTNAME = /[a-z0-9.-]+|\[[a-f0-9]*:[a-f0-9.:]+\]/i
106+
VALID_ORIGIN_HOST = /\A(#{HOSTNAME})(?::\d+)?\z/
107+
VALID_FORWARDED_HOST = /(?:\A|,[ ]?)(#{HOSTNAME})(?::\d+)?\z/
108+
105109
def authorized?(request)
106-
valid_host = /
107-
\A
108-
(?<host>[a-z0-9.-]+|\[[a-f0-9]*:[a-f0-9.:]+\])
109-
(:\d+)?
110-
\z
111-
/x
112-
113-
origin_host = valid_host.match(
114-
request.get_header("HTTP_HOST").to_s.downcase)
115-
forwarded_host = valid_host.match(
116-
request.x_forwarded_host.to_s.split(/,\s?/).last)
117-
118-
origin_host && @permissions.allows?(origin_host[:host]) && (
119-
forwarded_host.nil? || @permissions.allows?(forwarded_host[:host]))
110+
origin_host = request.get_header("HTTP_HOST")&.slice(VALID_ORIGIN_HOST, 1) || ""
111+
forwarded_host = request.x_forwarded_host&.slice(VALID_FORWARDED_HOST, 1) || ""
112+
113+
@permissions.allows?(origin_host) && (forwarded_host.blank? || @permissions.allows?(forwarded_host))
120114
end
121115

122116
def excluded?(request)

‎actionpack/test/dispatch/host_authorization_test.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,15 +221,15 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
221221
assert_match "Blocked host: www.example.com", response.body
222222
end
223223

224-
test "only compare to valid hostnames" do
224+
test "blocks requests with invalid hostnames" do
225225
@app = ActionDispatch::HostAuthorization.new(App, ".example.com")
226226

227227
get "/", env: {
228-
"HOST" => "example.com#sub.example.com",
228+
"HOST" => "attacker.com#x.example.com",
229229
}
230230

231231
assert_response :forbidden
232-
assert_match "Blocked host: example.com#sub.example.com", response.body
232+
assert_match "Blocked host: attacker.com#x.example.com", response.body
233233
end
234234

235235
test "blocks requests to similar host" do

‎railties/test/application/middleware/remote_ip_test.rb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ class RemoteIpTest < ActiveSupport::TestCase
1111
def remote_ip(env = {})
1212
remote_ip = nil
1313
env = Rack::MockRequest.env_for("/").merge(env).merge!(
14-
"HTTP_HOST" => "example.com",
1514
"action_dispatch.show_exceptions" => false,
1615
"action_dispatch.key_generator" => ActiveSupport::CachingKeyGenerator.new(
1716
ActiveSupport::KeyGenerator.new("b3c631c314c0bbca50c1b2843150fe33", iterations: 1000)

‎railties/test/isolation/abstract_unit.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ def extract_body(response)
8282
end
8383

8484
def get(path)
85-
@app.call(::Rack::MockRequest.env_for(path, "HTTP_HOST" => "example.com"))
85+
@app.call(::Rack::MockRequest.env_for(path))
8686
end
8787

8888
def assert_welcome(resp)

0 commit comments

Comments
 (0)
0