From 6eb0d0c38d95fe06ecd5a56875e8d691aa5e980b Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 22 Mar 2021 13:56:32 +1000 Subject: [PATCH] SECURITY: Fix is_private_ip for RateLimiter to cover all cases (#12464) 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. --- lib/middleware/request_tracker.rb | 4 +-- .../middleware/request_tracker_spec.rb | 28 ++++++++++++------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/lib/middleware/request_tracker.rb b/lib/middleware/request_tracker.rb index 6cd2984e825..45231f63b91 100644 --- a/lib/middleware/request_tracker.rb +++ b/lib/middleware/request_tracker.rb @@ -215,11 +215,9 @@ class Middleware::RequestTracker log_request_info(env, result, info) unless !log_request || env["discourse.request_tracker.skip"] 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) ip = IPAddr.new(ip) rescue nil - !!(ip && ip.to_s.match?(PRIVATE_IP)) + !!(ip && (ip.private? || ip.loopback?)) end def rate_limit(request) diff --git a/spec/components/middleware/request_tracker_spec.rb b/spec/components/middleware/request_tracker_spec.rb index b1d502b3682..f321f75ec54 100644 --- a/spec/components/middleware/request_tracker_spec.rb +++ b/spec/components/middleware/request_tracker_spec.rb @@ -185,13 +185,18 @@ describe Middleware::RequestTracker do global_setting :max_reqs_per_ip_mode, 'warn+block' 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(status).to eq(429) + expect(Rails.logger.warnings).to eq(warn_count) + expect(status).to eq(429) + warn_count += 1 + end end describe "register_ip_skipper" do @@ -227,13 +232,16 @@ describe Middleware::RequestTracker do global_setting :max_reqs_per_ip_mode, 'warn+block' 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) - expect(Rails.logger.warnings).to eq(0) - expect(status).to eq(200) + expect(Rails.logger.warnings).to eq(0) + expect(status).to eq(200) + end end it "does warn if rate limiter is enabled via warn+block" do