From 5e6e94465a9356c602dc6c69a23931510290b900 Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Tue, 25 Jan 2022 16:45:21 -0500 Subject: [PATCH] Deprecate key_space_limit It was determined that as this limit did not affect nested parameter hashes, it didn't actually prevent an attacker from using more than limited number of bytes for parameter keys, so this limit isn't actually doing anything useful. It is confusing people when it gets in the way of desired large parameter requests. --- README.rdoc | 13 +++++------- lib/rack/query_parser.rb | 30 +++++++++++++-------------- lib/rack/utils.rb | 12 ++++++----- test/spec_multipart.rb | 13 +----------- test/spec_request.rb | 44 ++-------------------------------------- test/spec_utils.rb | 30 +++++++++++++++++++++++++-- 6 files changed, 58 insertions(+), 84 deletions(-) diff --git a/README.rdoc b/README.rdoc index 66cde310c..3bdaa9741 100644 --- a/README.rdoc +++ b/README.rdoc @@ -178,15 +178,8 @@ Several parameters can be modified on Rack::Utils to configure \Rack behaviour. e.g: - Rack::Utils.key_space_limit = 128 + Rack::Utils.param_depth_limit = 3 -=== key_space_limit - -The default number of bytes to allow all parameters keys in a given parameter hash to take up. -Does not affect nested parameter hashes, so doesn't actually prevent an attacker from using -more than this many bytes for parameter keys. - -Defaults to 65536 characters. === param_depth_limit @@ -214,6 +207,10 @@ Set to 0 for no limit. Can also be set via the +RACK_MULTIPART_PART_LIMIT+ environment variable. +=== key_space_limit + +No longer has an effect, deprecated. + == Changelog See {CHANGELOG.md}[https://github.com/rack/rack/blob/master/CHANGELOG.md]. diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index 28670fc18..9a85a4cae 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -14,15 +14,22 @@ class ParameterTypeError < TypeError; end # sequence. class InvalidParameterError < ArgumentError; end - def self.make_default(key_space_limit, param_depth_limit) - new Params, key_space_limit, param_depth_limit + def self.make_default(_key_space_limit=(not_deprecated = true; nil), param_depth_limit) + unless not_deprecated + warn("`first argument `key_space limit` is deprecated and no longer has an effect. Please call with only one argument, which will be required in a future version of Rack", uplevel: 1) + end + + new Params, param_depth_limit end - attr_reader :key_space_limit, :param_depth_limit + attr_reader :param_depth_limit + + def initialize(params_class, _key_space_limit=(not_deprecated = true; nil), param_depth_limit) + unless not_deprecated + warn("`second argument `key_space limit` is deprecated and no longer has an effect. Please call with only two arguments, which will be required in a future version of Rack", uplevel: 1) + end - def initialize(params_class, key_space_limit, param_depth_limit) @params_class = params_class - @key_space_limit = key_space_limit @param_depth_limit = param_depth_limit end @@ -120,15 +127,11 @@ def normalize_params(params, name, v, depth) end def make_params - @params_class.new @key_space_limit - end - - def new_space_limit(key_space_limit) - self.class.new @params_class, key_space_limit, param_depth_limit + @params_class.new end def new_depth_limit(param_depth_limit) - self.class.new @params_class, key_space_limit, param_depth_limit + self.class.new @params_class, param_depth_limit end private @@ -154,8 +157,7 @@ def unescape(s) end class Params - def initialize(limit) - @limit = limit + def initialize @size = 0 @params = {} end @@ -165,8 +167,6 @@ def [](key) end def []=(key, value) - @size += key.size if key && !@params.key?(key) - raise RangeError, 'exceeded available parameter key space' if @size > @limit @params[key] = value end diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb index 3a23a9d17..7d7719c0b 100644 --- a/lib/rack/utils.rb +++ b/lib/rack/utils.rb @@ -23,9 +23,10 @@ module Utils class << self attr_accessor :default_query_parser end - # The default number of bytes to allow parameter keys to take up. - # This helps prevent a rogue client from flooding a Request. - self.default_query_parser = QueryParser.make_default(65536, 32) + # The default amount of nesting to allowed by hash parameters. + # This helps prevent a rogue client from triggering a possible stack overflow + # when parsing parameters. + self.default_query_parser = QueryParser.make_default(32) module_function @@ -70,11 +71,12 @@ def self.param_depth_limit=(v) end def self.key_space_limit - default_query_parser.key_space_limit + warn("`Rack::Utils.key_space_limit` is deprecated as this value no longer has an effect. It will be removed in a future version of Rack", uplevel: 1) + 65536 end def self.key_space_limit=(v) - self.default_query_parser = self.default_query_parser.new_space_limit(v) + warn("`Rack::Utils.key_space_limit=` is deprecated and no longer has an effect. It will be removed in a future version of Rack", uplevel: 1) end if defined?(Process::CLOCK_MONOTONIC) diff --git a/test/spec_multipart.rb b/test/spec_multipart.rb index f4de71cf8..87902a1f5 100644 --- a/test/spec_multipart.rb +++ b/test/spec_multipart.rb @@ -98,17 +98,6 @@ def multipart_file(name) params['user_sid'].encoding.must_equal Encoding::UTF_8 end - it "raise RangeError if the key space is exhausted" do - env = Rack::MockRequest.env_for("/", multipart_fixture(:content_type_and_no_filename)) - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 1 - begin - lambda { Rack::Multipart.parse_multipart(env) }.must_raise(RangeError) - ensure - Rack::Utils.key_space_limit = old - end - end - it "parse multipart form webkit style" do env = Rack::MockRequest.env_for '/', multipart_fixture(:webkit) env['CONTENT_TYPE'] = "multipart/form-data; boundary=----WebKitFormBoundaryWLHCs9qmcJJoyjKR" @@ -219,7 +208,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - query_parser = Rack::QueryParser.new c, 65536, 100 + query_parser = Rack::QueryParser.new c, 100 env = Rack::MockRequest.env_for("/", multipart_fixture(:text)) params = Rack::Multipart.parse_multipart(env, query_parser) params[:files][:type].must_equal "text/plain" diff --git a/test/spec_request.rb b/test/spec_request.rb index 111f49121..c2c71dc4b 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -295,7 +295,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - parser = Rack::QueryParser.new(c, 65536, 100) + parser = Rack::QueryParser.new(c, 100) c = Class.new(Rack::Request) do define_method(:query_parser) do parser @@ -316,32 +316,6 @@ def initialize(*) req.params.must_equal "foo" => "bar", "quux" => "b;la;wun=duh" end - it "limit the keys from the GET query string" do - env = Rack::MockRequest.env_for("/?foo=bar") - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 1 - begin - req = make_request(env) - lambda { req.GET }.must_raise RangeError - ensure - Rack::Utils.key_space_limit = old - end - end - - it "limit the key size per nested params hash" do - nested_query = Rack::MockRequest.env_for("/?foo%5Bbar%5D%5Bbaz%5D%5Bqux%5D=1") - plain_query = Rack::MockRequest.env_for("/?foo_bar__baz__qux_=1") - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 3 - begin - exp = { "foo" => { "bar" => { "baz" => { "qux" => "1" } } } } - make_request(nested_query).GET.must_equal exp - lambda { make_request(plain_query).GET }.must_raise RangeError - ensure - Rack::Utils.key_space_limit = old - end - end - it "limit the allowed parameter depth when parsing parameters" do env = Rack::MockRequest.env_for("/?a#{'[a]' * 40}=b") req = make_request(env) @@ -388,7 +362,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - parser = Rack::QueryParser.new(c, 65536, 100) + parser = Rack::QueryParser.new(c, 100) c = Class.new(Rack::Request) do define_method(:query_parser) do parser @@ -438,20 +412,6 @@ def initialize(*) req.params.must_equal "foo" => "bar", "quux" => "bla" end - it "limit the keys from the POST form data" do - env = Rack::MockRequest.env_for("", - "REQUEST_METHOD" => 'POST', - :input => "foo=bar&quux=bla") - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 1 - begin - req = make_request(env) - lambda { req.POST }.must_raise RangeError - ensure - Rack::Utils.key_space_limit = old - end - end - it "parse POST data with explicit content type regardless of method" do req = make_request \ Rack::MockRequest.env_for("/", diff --git a/test/spec_utils.rb b/test/spec_utils.rb index f753f85b6..a26739276 100644 --- a/test/spec_utils.rb +++ b/test/spec_utils.rb @@ -114,7 +114,7 @@ def assert_nested_query(exp, act) ex = { "foo" => nil } ex["foo"] = ex - params = Rack::Utils::KeySpaceConstrainedParams.new(65536) + params = Rack::Utils::KeySpaceConstrainedParams.new params['foo'] = params params.to_params_hash.to_s.must_equal ex.to_s end @@ -123,6 +123,32 @@ def assert_nested_query(exp, act) Rack::Utils.parse_nested_query(nil).must_equal({}) end + it "should warn using deprecated Rack::Util.key_space_limit=" do + begin + warn_arg = nil + Rack::Utils.define_singleton_method(:warn) do |*args| + warn_arg = args.first + end + Rack::Utils.key_space_limit = 65536 + warn_arg.must_equal("`Rack::Utils.key_space_limit=` is deprecated and no longer has an effect. It will be removed in a future version of Rack") + ensure + Rack::Utils.singleton_class.send(:remove_method, :warn) + end + end + + it "should warn using deprecated Rack::Util.key_space_limit" do + begin + warn_arg = nil + Rack::Utils.define_singleton_method(:warn) do |*args| + warn_arg = args.first + end + Rack::Utils.key_space_limit + warn_arg.must_equal("`Rack::Utils.key_space_limit` is deprecated as this value no longer has an effect. It will be removed in a future version of Rack") + ensure + Rack::Utils.singleton_class.send(:remove_method, :warn) + end + end + it "raise an exception if the params are too deep" do len = Rack::Utils.param_depth_limit @@ -259,7 +285,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - Rack::Utils.default_query_parser = Rack::QueryParser.new(param_parser_class, 65536, 100) + Rack::Utils.default_query_parser = Rack::QueryParser.new(param_parser_class, 100) h1 = Rack::Utils.parse_query(",foo=bar;,", ";,") h1[:foo].must_equal "bar" h2 = Rack::Utils.parse_nested_query("x[y][][z]=1&x[y][][w]=2")