IntenseWebs/discourse
3
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-02-25 18:55:32 -06:00
Code Issues Packages Projects Releases Wiki Activity

SECURITY: Fix is_private_ip for RateLimiter to cover all cases (#12464)

Browse Source 
The regular expression to detect private IP addresses did not always detect them successfully.
Changed to use ruby's in-built IPAddr.new(ip_address).private? method instead
which does the same thing but covers all cases.
This commit is contained in:
Martin Brennan
2021-03-22 13:56:32 +10:00
committed by GitHub
parent 9526c1a27b
commit 6eb0d0c38d
2 changed files with 19 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
lib/middleware/request_tracker.rb
View File

@@ -215,11 +215,9 @@ class Middleware::RequestTracker
log_request_info(env, result, info) unless !log_request || env["discourse.request_tracker.skip"] log_request_info(env, result, info) unless !log_request || env["discourse.request_tracker.skip"]
end end
PRIVATE_IP ||= /^(127\.)|(192\.168\.)|(10\.)|(172\.1[6-9]\.)|(172\.2[0-9]\.)|(172\.3[0-1]\.)|(::1$)|([fF][cCdD])/
def is_private_ip?(ip) def is_private_ip?(ip)
ip = IPAddr.new(ip) rescue nil ip = IPAddr.new(ip) rescue nil
!!(ip && ip.to_s.match?(PRIVATE_IP)) !!(ip && (ip.private? || ip.loopback?))
end end
def rate_limit(request) def rate_limit(request)

14
spec/components/middleware/request_tracker_spec.rb
View File

@@ -185,13 +185,18 @@ describe Middleware::RequestTracker do
global_setting :max_reqs_per_ip_mode, 'warn+block' global_setting :max_reqs_per_ip_mode, 'warn+block'
global_setting :max_reqs_rate_limit_on_private, true global_setting :max_reqs_rate_limit_on_private, true
env1 = env("REMOTE_ADDR" => "127.0.0.2") addresses = %w[127.1.2.3 127.0.0.2 192.168.1.2 10.0.1.2 172.16.9.8 172.19.1.2 172.20.9.8 172.29.1.2 172.30.9.8 172.31.1.2]
warn_count = 1
addresses.each do |addr|
env1 = env("REMOTE_ADDR" => addr)
status, _ = middleware.call(env1) status, _ = middleware.call(env1)
status, _ = middleware.call(env1) status, _ = middleware.call(env1)
expect(Rails.logger.warnings).to eq(1) expect(Rails.logger.warnings).to eq(warn_count)
expect(status).to eq(429) expect(status).to eq(429)
warn_count += 1
end
end end
describe "register_ip_skipper" do describe "register_ip_skipper" do
@@ -227,7 +232,9 @@ describe Middleware::RequestTracker do
global_setting :max_reqs_per_ip_mode, 'warn+block' global_setting :max_reqs_per_ip_mode, 'warn+block'
global_setting :max_reqs_rate_limit_on_private, false global_setting :max_reqs_rate_limit_on_private, false
env1 = env("REMOTE_ADDR" => "127.0.3.1") addresses = %w[127.1.2.3 127.0.3.1 192.168.1.2 10.0.1.2 172.16.9.8 172.19.1.2 172.20.9.8 172.29.1.2 172.30.9.8 172.31.1.2]
addresses.each do |addr|
env1 = env("REMOTE_ADDR" => addr)
status, _ = middleware.call(env1) status, _ = middleware.call(env1)
status, _ = middleware.call(env1) status, _ = middleware.call(env1)
@@ -235,6 +242,7 @@ describe Middleware::RequestTracker do
expect(Rails.logger.warnings).to eq(0) expect(Rails.logger.warnings).to eq(0)
expect(status).to eq(200) expect(status).to eq(200)
end end
end
it "does warn if rate limiter is enabled via warn+block" do it "does warn if rate limiter is enabled via warn+block" do
global_setting :max_reqs_per_ip_per_10_seconds, 1 global_setting :max_reqs_per_ip_per_10_seconds, 1