Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Limit the size of parameter keys
Signed-off-by: James Tucker <jftucker@gmail.com>
  • Loading branch information
evanphx authored and raggi committed Dec 27, 2011
1 parent 7b5af22 commit 5b9d09a
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 0 deletions.
10 changes: 10 additions & 0 deletions lib/rack/multipart/parser.rb
Expand Up @@ -14,6 +14,9 @@ def parse

fast_forward_to_first_boundary

max_key_space = Utils.key_space_limit
bytes = 0

loop do
head, filename, content_type, name, body =
get_current_head_and_filename_and_content_type_and_name_and_body
Expand All @@ -28,6 +31,13 @@ def parse

filename, data = get_data(filename, body, content_type, name, head)

if name
bytes += name.size
if bytes > max_key_space
raise RangeError, "exceeded available parameter key space"
end
end

Utils.normalize_params(@params, name, data) unless data.nil?

# break if we're at the end of a buffer, but not if it is the end of a field
Expand Down
30 changes: 30 additions & 0 deletions lib/rack/utils.rb
Expand Up @@ -47,6 +47,14 @@ def unescape(s, encoding = nil)

DEFAULT_SEP = /[&;] */n

class << self
attr_accessor :key_space_limit
end

# The default number of bytes to allow parameter keys to take up.
# This helps prevent a rogue client from flooding a Request.
self.key_space_limit = 65536

# Stolen from Mongrel, with some small modifications:
# Parses a query string by breaking it up at the '&'
# and ';' characters. You can also use this to parse
Expand All @@ -55,8 +63,19 @@ def unescape(s, encoding = nil)
def parse_query(qs, d = nil)
params = {}

max_key_space = Utils.key_space_limit
bytes = 0

(qs || '').split(d ? /[#{d}] */n : DEFAULT_SEP).each do |p|
k, v = p.split('=', 2).map { |x| unescape(x) }

if k
bytes += k.size
if bytes > max_key_space
raise RangeError, "exceeded available parameter key space"
end
end

if cur = params[k]
if cur.class == Array
params[k] << v
Expand All @@ -75,8 +94,19 @@ def parse_query(qs, d = nil)
def parse_nested_query(qs, d = nil)
params = {}

max_key_space = Utils.key_space_limit
bytes = 0

(qs || '').split(d ? /[#{d}] */n : DEFAULT_SEP).each do |p|
k, v = p.split('=', 2).map { |s| unescape(s) }

if k
bytes += k.size
if bytes > max_key_space
raise RangeError, "exceeded available parameter key space"
end
end

normalize_params(params, k, v)
end

Expand Down
11 changes: 11 additions & 0 deletions test/spec_multipart.rb
Expand Up @@ -30,6 +30,17 @@ def multipart_file(name)
params["text"].should.equal "contents"
end

should "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) }.should.raise(RangeError)
ensure
Rack::Utils.key_space_limit = old
end
end

should "parse multipart form webkit style" do
env = Rack::MockRequest.env_for '/', multipart_fixture(:webkit)
env['CONTENT_TYPE'] = "multipart/form-data; boundary=----WebKitFormBoundaryWLHCs9qmcJJoyjKR"
Expand Down
26 changes: 26 additions & 0 deletions test/spec_request.rb
Expand Up @@ -125,6 +125,18 @@
req.params.should.equal "foo" => "bar", "quux" => "bla"
end

should "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 = Rack::Request.new(env)
lambda { req.GET }.should.raise(RangeError)
ensure
Rack::Utils.key_space_limit = old
end
end

should "not unify GET and POST when calling params" do
mr = Rack::MockRequest.env_for("/?foo=quux",
"REQUEST_METHOD" => 'POST',
Expand Down Expand Up @@ -157,6 +169,20 @@
req.params.should.equal "foo" => "bar", "quux" => "bla"
end

should "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 = Rack::Request.new(env)
lambda { req.POST }.should.raise(RangeError)
ensure
Rack::Utils.key_space_limit = old
end
end

should "parse POST data with explicit content type regardless of method" do
req = Rack::Request.new \
Rack::MockRequest.env_for("/",
Expand Down

5 comments on commit 5b9d09a

@TomK32
Copy link

@TomK32 TomK32 commented on 5b9d09a Jan 18, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue related to this commit, what's your suggestion to raise this limit? Yes, I have gigantic forms (not multipart forms) that send 100kb of data.

@leahneukirchen
Copy link
Member

@leahneukirchen leahneukirchen commented on 5b9d09a Jan 19, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evanphx
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have 100k of key data though? This doesn't limit the size of the POST values, just the keys. I highly doubt you have 100kb of key data.

@TomK32
Copy link

@TomK32 TomK32 commented on 5b9d09a Jan 19, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mongodb's keys are 32 characters long each. I have answers embedded results (= 2 long IDs per data key), 50 results, 10 answers each with 5 fields (remember a checkbox in rails adds a hiddenfield just for the fun of it) brings me (with "only" 2500 fields) easy above your limit. The rails action only needs barely a second to process all that.

Still, I don't understand the motivation of your change to rack, is there an issue # related?

@evanphx
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is related to the DOS attacks that were being discussed a couple weeks ago. 2500 fields seems well out side the normal usage of most users. As @chneukirchen said, feel free to set the max to whatever giant value works for you.

Please sign in to comment.