From 4d5e859a1247200f04c5b0da123458c909a0ea40 Mon Sep 17 00:00:00 2001 From: Howard Wilson Date: Mon, 9 Nov 2015 13:43:50 +0100 Subject: [PATCH 1/6] Add note to README.md about scoping of Fail2Ban filters --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 0472c09..61d25e2 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,8 @@ Rack::Attack.blacklist('fail2ban pentesters') do |req| end ``` +Note that `Fail2Ban` filters are not automatically scoped to the blacklist, so when using multiple filters in an application the scoping must be added to the discriminator e.g. `"pentest:#{req.ip}"`. + #### Allow2Ban `Allow2Ban.filter` works the same way as the `Fail2Ban.filter` except that it *allows* requests from misbehaving clients until such time as they reach maxretry at which they are cut off as per normal. From bd27009f4382b9625af07caac539dba1001bd697 Mon Sep 17 00:00:00 2001 From: Aaron Suggs Date: Fri, 18 Dec 2015 08:54:19 -0500 Subject: [PATCH 2/6] Add Guard testing support While developing rack-attack, run tests continuously with `bundle exec guard` --- Gemfile | 9 +++++++-- Guardfile | 10 ++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 Guardfile diff --git a/Gemfile b/Gemfile index 41e7aff..5d00c12 100644 --- a/Gemfile +++ b/Gemfile @@ -1,4 +1,9 @@ -source "https://rubygems.org" +source 'https://rubygems.org' + gemspec -#gem 'debugger', platform: 'ruby' +group :development do + gem 'pry' + gem 'guard' # NB: this is necessary in newer versions + gem 'guard-minitest' +end diff --git a/Guardfile b/Guardfile new file mode 100644 index 0000000..ebf900b --- /dev/null +++ b/Guardfile @@ -0,0 +1,10 @@ +# A sample Guardfile +# More info at https://github.com/guard/guard#readme + +guard :minitest do + # with Minitest::Spec + watch(%r{^spec/(.*)_spec\.rb$}) + watch(%r{^lib/(.+)\.rb$}) { |m| "spec/#{m[1]}_spec.rb" } + watch(%r{^spec/spec_helper\.rb$}) { 'spec' } +end + From 11faea4526b262619533944880666177870dd671 Mon Sep 17 00:00:00 2001 From: Aaron Suggs Date: Fri, 18 Dec 2015 08:55:09 -0500 Subject: [PATCH 3/6] specs: use pry instead of debugger --- spec/spec_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index dec8563..5382087 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -8,9 +8,9 @@ require 'active_support' require "rack/attack" begin - require 'debugger' + require 'pry' rescue LoadError - #nothing to do here + #nothing to do here end class MiniTest::Spec From 76c2e3143099d938883ae5654527b47e9e6a8977 Mon Sep 17 00:00:00 2001 From: Aaron Suggs Date: Fri, 18 Dec 2015 09:13:30 -0500 Subject: [PATCH 4/6] Normalize request paths when using Rails' ActionDispatch The issue --- When using rack-attack with a rails app, developers expect the request path to be normalized. In particular, trailing slashes are stripped so a request path "/login/" becomes "/login" by the time you're in ActionController. Since Rack::Attack runs before ActionDispatch, the request path is not yet normalized. This can cause throttles and blacklists to not work as expected. E.g., a throttle: throttle('logins', ...) {|req| req.path == "/login" } would not match a request to '/login/', though Rails would route '/login/' to the same '/login' action. The solution --- This patch looks if ActionDispatch's request normalization is loaded, and if so, uses it to normalize the path before processing throttles, blacklists, etc. If it's not loaded, the request path is not modified. Credit --- Thanks to Andres Riancho at Include Security for reporting this issue. --- gemfiles/activesupport3.2.gemfile | 1 + gemfiles/activesupport4.0.gemfile | 1 + gemfiles/activesupport4.1.gemfile | 1 + gemfiles/activesupport4.2.gemfile | 2 +- lib/rack/attack.rb | 2 ++ lib/rack/attack/path_normalizer.rb | 27 ++++++++++++++++++++++++ rack-attack.gemspec | 1 + spec/rack_attack_path_normalizer_spec.rb | 17 +++++++++++++++ spec/rack_attack_spec.rb | 11 ++++++++++ spec/spec_helper.rb | 5 +++++ 10 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 lib/rack/attack/path_normalizer.rb create mode 100644 spec/rack_attack_path_normalizer_spec.rb diff --git a/gemfiles/activesupport3.2.gemfile b/gemfiles/activesupport3.2.gemfile index 61efc24..a79d34e 100644 --- a/gemfiles/activesupport3.2.gemfile +++ b/gemfiles/activesupport3.2.gemfile @@ -3,5 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 3.2.0" +gem "actionpack", "~> 3.2.0" gemspec :path => "../" diff --git a/gemfiles/activesupport4.0.gemfile b/gemfiles/activesupport4.0.gemfile index 697ad67..3e6650c 100644 --- a/gemfiles/activesupport4.0.gemfile +++ b/gemfiles/activesupport4.0.gemfile @@ -3,5 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 4.0.0" +gem "actionpack", "~> 4.0.0" gemspec :path => "../" diff --git a/gemfiles/activesupport4.1.gemfile b/gemfiles/activesupport4.1.gemfile index 670c4f8..a64a9dd 100644 --- a/gemfiles/activesupport4.1.gemfile +++ b/gemfiles/activesupport4.1.gemfile @@ -3,5 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 4.1.0" +gem "actionpack", "~> 4.1.0" gemspec :path => "../" diff --git a/gemfiles/activesupport4.2.gemfile b/gemfiles/activesupport4.2.gemfile index 67154bc..7e517e7 100644 --- a/gemfiles/activesupport4.2.gemfile +++ b/gemfiles/activesupport4.2.gemfile @@ -2,6 +2,6 @@ source "https://rubygems.org" -gem "activesupport", "~> 4.2.1" +gem "actionpack", "~> 4.2.1" gemspec :path => "../" diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index d8308a5..1e0ae9a 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -3,6 +3,7 @@ require 'forwardable' class Rack::Attack autoload :Cache, 'rack/attack/cache' + autoload :PathNormalizer, 'rack/attack/path_normalizer' autoload :Check, 'rack/attack/check' autoload :Throttle, 'rack/attack/throttle' autoload :Whitelist, 'rack/attack/whitelist' @@ -91,6 +92,7 @@ class Rack::Attack end def call(env) + env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO']) req = Rack::Attack::Request.new(env) if whitelisted?(req) diff --git a/lib/rack/attack/path_normalizer.rb b/lib/rack/attack/path_normalizer.rb new file mode 100644 index 0000000..afeb2f9 --- /dev/null +++ b/lib/rack/attack/path_normalizer.rb @@ -0,0 +1,27 @@ +class Rack::Attack + + # When using Rack::Attack with a Rails app, developers expect the request path + # to be normalized. In particular, trailing slashes are stripped. + # (See http://git.io/v0rrR for implementation.) + # + # Look for an ActionDispatch utility class that Rails folks would expect + # to normalize request paths. If unavailable, use a fallback class that + # doesn't normalize the path (as a non-Rails rack app developer expects). + + module FallbackPathNormalizer + def self.normalize_path(path) + path + end + end + + PathNormalizer = if defined?(::ActionDispatch::Journey::Router::Utils) + # For Rails 4+ apps + ::ActionDispatch::Journey::Router::Utils + elsif defined?(::Journey::Router::Utils) + # for Rails 3.2 + ::Journey::Router::Utils + else + FallbackPathNormalizer + end + +end diff --git a/rack-attack.gemspec b/rack-attack.gemspec index bef0f45..057aa6d 100644 --- a/rack-attack.gemspec +++ b/rack-attack.gemspec @@ -28,6 +28,7 @@ Gem::Specification.new do |s| s.add_development_dependency 'rake' s.add_development_dependency 'appraisal' s.add_development_dependency 'activesupport', '>= 3.0.0' + s.add_development_dependency 'actionpack', '>= 3.0.0' s.add_development_dependency 'redis-activesupport' s.add_development_dependency 'dalli' s.add_development_dependency 'connection_pool' diff --git a/spec/rack_attack_path_normalizer_spec.rb b/spec/rack_attack_path_normalizer_spec.rb new file mode 100644 index 0000000..1c5b66e --- /dev/null +++ b/spec/rack_attack_path_normalizer_spec.rb @@ -0,0 +1,17 @@ +require_relative 'spec_helper' + +describe Rack::Attack::PathNormalizer do + subject { Rack::Attack::PathNormalizer } + + it 'should have a normalize_path method' do + subject.normalize_path('/foo').must_equal '/foo' + end + + describe 'FallbackNormalizer' do + subject { Rack::Attack::FallbackPathNormalizer } + + it '#normalize_path does not change the path' do + subject.normalize_path('').must_equal '' + end + end +end diff --git a/spec/rack_attack_spec.rb b/spec/rack_attack_spec.rb index cd6b599..3084ffb 100644 --- a/spec/rack_attack_spec.rb +++ b/spec/rack_attack_spec.rb @@ -3,6 +3,17 @@ require_relative 'spec_helper' describe 'Rack::Attack' do allow_ok_requests + describe 'normalizing paths' do + before do + Rack::Attack.blacklist("banned_path") {|req| req.path == '/foo' } + end + + it 'blocks requests with trailing slash' do + get '/foo/' + last_response.status.must_equal 403 + end + end + describe 'blacklist' do before do @bad_ip = '1.2.3.4' diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5382087..118d1fe 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -5,6 +5,11 @@ require "minitest/autorun" require "minitest/pride" require "rack/test" require 'active_support' +require 'action_dispatch' + +# Load Journey for Rails 3.2 +require 'journey' if ActionPack::VERSION::MAJOR == 3 + require "rack/attack" begin From 4ec58e36de08d4cf61d60eae1f9b7ea986ad2aae Mon Sep 17 00:00:00 2001 From: Aaron Suggs Date: Fri, 18 Dec 2015 10:09:38 -0500 Subject: [PATCH 5/6] Version v4.3.1 --- CHANGELOG.md | 6 ++++++ lib/rack/attack/version.rb | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fb3adc..d4a5d65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,13 @@ # Changlog ## master (unreleased) + +## v4.3.1 - 18 Dec 2015 + - SECURITY FIX: Normalize request paths when using ActionDispatch. Thanks + Andres Riancho at @includesecurity for reporting it. - Remove support for ruby 1.9.x + - Add Code of Conduct + - Several documentation and testing improvements ## v4.3.0 - 22 May 2015 diff --git a/lib/rack/attack/version.rb b/lib/rack/attack/version.rb index e7a3735..97612cc 100644 --- a/lib/rack/attack/version.rb +++ b/lib/rack/attack/version.rb @@ -1,5 +1,5 @@ module Rack class Attack - VERSION = '4.3.0' + VERSION = '4.3.1' end end From 57f513e1e9445695ccf0ecf66fc17bfd1c4c61ac Mon Sep 17 00:00:00 2001 From: Aaron Suggs Date: Mon, 21 Dec 2015 09:34:21 -0500 Subject: [PATCH 6/6] Fix Appraisals & gemfile tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gemfiles had drifted from the Appraisals file from which they’re generated. --- Appraisals | 8 ++++++++ gemfiles/activesupport3.2.gemfile | 6 ++++++ gemfiles/activesupport4.0.gemfile | 6 ++++++ gemfiles/activesupport4.1.gemfile | 6 ++++++ gemfiles/activesupport4.2.gemfile | 9 ++++++++- gemfiles/dalli1.1.gemfile | 6 ++++++ gemfiles/dalli2.gemfile | 6 ++++++ 7 files changed, 46 insertions(+), 1 deletion(-) diff --git a/Appraisals b/Appraisals index 85c670b..bd04b28 100644 --- a/Appraisals +++ b/Appraisals @@ -1,13 +1,21 @@ appraise 'activesupport3.2' do gem 'activesupport', '~> 3.2.0' + gem 'actionpack', '~> 3.2.0' end appraise 'activesupport4.0' do gem 'activesupport', '~> 4.0.0' + gem 'actionpack', '~> 4.0.0' end appraise 'activesupport4.1' do gem 'activesupport', '~> 4.1.0' + gem 'actionpack', '~> 4.1.0' +end + +appraise 'activesupport4.2' do + gem 'activesupport', '~> 4.2.0' + gem 'actionpack', '~> 4.2.0' end appraise 'dalli1.1' do diff --git a/gemfiles/activesupport3.2.gemfile b/gemfiles/activesupport3.2.gemfile index a79d34e..67813b6 100644 --- a/gemfiles/activesupport3.2.gemfile +++ b/gemfiles/activesupport3.2.gemfile @@ -5,4 +5,10 @@ source "https://rubygems.org" gem "activesupport", "~> 3.2.0" gem "actionpack", "~> 3.2.0" +group :development do + gem "pry" + gem "guard" + gem "guard-minitest" +end + gemspec :path => "../" diff --git a/gemfiles/activesupport4.0.gemfile b/gemfiles/activesupport4.0.gemfile index 3e6650c..de2923f 100644 --- a/gemfiles/activesupport4.0.gemfile +++ b/gemfiles/activesupport4.0.gemfile @@ -5,4 +5,10 @@ source "https://rubygems.org" gem "activesupport", "~> 4.0.0" gem "actionpack", "~> 4.0.0" +group :development do + gem "pry" + gem "guard" + gem "guard-minitest" +end + gemspec :path => "../" diff --git a/gemfiles/activesupport4.1.gemfile b/gemfiles/activesupport4.1.gemfile index a64a9dd..891419a 100644 --- a/gemfiles/activesupport4.1.gemfile +++ b/gemfiles/activesupport4.1.gemfile @@ -5,4 +5,10 @@ source "https://rubygems.org" gem "activesupport", "~> 4.1.0" gem "actionpack", "~> 4.1.0" +group :development do + gem "pry" + gem "guard" + gem "guard-minitest" +end + gemspec :path => "../" diff --git a/gemfiles/activesupport4.2.gemfile b/gemfiles/activesupport4.2.gemfile index 7e517e7..1b122d1 100644 --- a/gemfiles/activesupport4.2.gemfile +++ b/gemfiles/activesupport4.2.gemfile @@ -2,6 +2,13 @@ source "https://rubygems.org" -gem "actionpack", "~> 4.2.1" +gem "activesupport", "~> 4.2.0" +gem "actionpack", "~> 4.2.0" + +group :development do + gem "pry" + gem "guard" + gem "guard-minitest" +end gemspec :path => "../" diff --git a/gemfiles/dalli1.1.gemfile b/gemfiles/dalli1.1.gemfile index 06ca591..eddf5c3 100644 --- a/gemfiles/dalli1.1.gemfile +++ b/gemfiles/dalli1.1.gemfile @@ -4,4 +4,10 @@ source "https://rubygems.org" gem "dalli", "1.1.5" +group :development do + gem "pry" + gem "guard" + gem "guard-minitest" +end + gemspec :path => "../" diff --git a/gemfiles/dalli2.gemfile b/gemfiles/dalli2.gemfile index e9fe487..93d097e 100644 --- a/gemfiles/dalli2.gemfile +++ b/gemfiles/dalli2.gemfile @@ -4,4 +4,10 @@ source "https://rubygems.org" gem "dalli", "~> 2.0" +group :development do + gem "pry" + gem "guard" + gem "guard-minitest" +end + gemspec :path => "../"