diff options
author | John Hawthorn <john@hawthorn.email> | 2022-12-08 15:54:28 -0800 |
---|---|---|
committer | Aaron Patterson <tenderlove@ruby-lang.org> | 2023-03-02 15:00:33 -0800 |
commit | 8e8869d625e73e16b576b6d31b50208e9ec8002f (patch) | |
tree | aeb318c8a22250da36b40a512efab9d17ee8738b | |
parent | 0b26518acc4c946ca96dfe3d9e68a05ca84439f7 (diff) | |
download | rack-8e8869d625e73e16b576b6d31b50208e9ec8002f.tar.gz |
Limit all multipart parts, not just files
Previously we would limit the number of multipart parts which were
files, but not other parts. In some cases this could cause parsing of
maliciously crafted inputs to take longer than expected.
[CVE-2023-27530]
-rw-r--r-- | README.md | 26 | ||||
-rw-r--r-- | lib/rack/multipart/parser.rb | 22 | ||||
-rw-r--r-- | lib/rack/utils.rb | 19 | ||||
-rw-r--r-- | test/spec_multipart.rb | 14 | ||||
-rw-r--r-- | test/spec_request.rb | 18 |
5 files changed, 84 insertions, 15 deletions
@@ -186,19 +186,35 @@ but this query string would not be allowed: Limiting the depth prevents a possible stack overflow when parsing parameters. -### `multipart_part_limit` +### `multipart_file_limit` ```ruby -Rack::Utils.multipart_part_limit = 128 # default +Rack::Utils.multipart_file_limit = 128 # default ``` -The maximum number of parts a request can contain. Accepting too many parts can -lead to the server running out of file handles. +The maximum number of parts with a filename a request can contain. Accepting +too many parts can lead to the server running out of file handles. The default is 128, which means that a single request can't upload more than 128 files at once. Set to 0 for no limit. -Can also be set via the `RACK_MULTIPART_PART_LIMIT` environment variable. +Can also be set via the `RACK_MULTIPART_FILE_LIMIT` environment variable. + +(This is also aliased as `multipart_part_limit` and `RACK_MULTIPART_PART_LIMIT` for compatibility) + + +### `multipart_total_part_limit` + +The maximum total number of parts a request can contain of any type, including +both file and non-file form fields. + +The default is 4096, which means that a single request can't contain more than +4096 parts. + +Set to 0 for no limit. + +Can also be set via the `RACK_MULTIPART_TOTAL_PART_LIMIT` environment variable. + ## Changelog diff --git a/lib/rack/multipart/parser.rb b/lib/rack/multipart/parser.rb index c49c80e6..36a68f29 100644 --- a/lib/rack/multipart/parser.rb +++ b/lib/rack/multipart/parser.rb @@ -11,6 +11,10 @@ module Rack include BadRequest end + class MultipartTotalPartLimitError < StandardError + include BadRequest + end + # Use specific error class when parsing multipart request # that ends early. class EmptyContentError < ::EOFError @@ -176,7 +180,7 @@ module Rack @mime_parts[mime_index] = klass.new(body, head, filename, content_type, name) - check_open_files + check_part_limits end def on_mime_body(mime_index, content) @@ -188,13 +192,23 @@ module Rack private - def check_open_files - if Utils.multipart_part_limit > 0 - if @open_files >= Utils.multipart_part_limit + def check_part_limits + file_limit = Utils.multipart_file_limit + part_limit = Utils.multipart_total_part_limit + + if file_limit && file_limit > 0 + if @open_files >= file_limit @mime_parts.each(&:close) raise MultipartPartLimitError, 'Maximum file multiparts in content reached' end end + + if part_limit && part_limit > 0 + if @mime_parts.size >= part_limit + @mime_parts.each(&:close) + raise MultipartTotalPartLimitError, 'Maximum total multiparts in content reached' + end + end end end diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb index 1796c992..d77fb9bc 100644 --- a/lib/rack/utils.rb +++ b/lib/rack/utils.rb @@ -58,13 +58,24 @@ module Rack end class << self - attr_accessor :multipart_part_limit + attr_accessor :multipart_total_part_limit + + attr_accessor :multipart_file_limit + + # multipart_part_limit is the original name of multipart_file_limit, but + # the limit only counts parts with filenames. + alias multipart_part_limit multipart_file_limit + alias multipart_part_limit= multipart_file_limit= end - # The maximum number of parts a request can contain. Accepting too many part - # can lead to the server running out of file handles. + # The maximum number of file parts a request can contain. Accepting too + # many parts can lead to the server running out of file handles. # Set to `0` for no limit. - self.multipart_part_limit = (ENV['RACK_MULTIPART_PART_LIMIT'] || 128).to_i + self.multipart_file_limit = (ENV['RACK_MULTIPART_PART_LIMIT'] || ENV['RACK_MULTIPART_FILE_LIMIT'] || 128).to_i + + # The maximum total number of parts a request can contain. Accepting too + # many can lead to excessive memory use and parsing time. + self.multipart_total_part_limit = (ENV['RACK_MULTIPART_TOTAL_PART_LIMIT'] || 4096).to_i def self.param_depth_limit default_query_parser.param_depth_limit diff --git a/test/spec_multipart.rb b/test/spec_multipart.rb index 14d0f6a5..48d03e43 100644 --- a/test/spec_multipart.rb +++ b/test/spec_multipart.rb @@ -696,7 +696,7 @@ content-type: image/jpeg\r end end - it "reach a multipart limit" do + it "reach a multipart file limit" do begin previous_limit = Rack::Utils.multipart_part_limit Rack::Utils.multipart_part_limit = 3 @@ -708,6 +708,18 @@ content-type: image/jpeg\r end end + it "reach a multipart total limit" do + begin + previous_limit = Rack::Utils.multipart_total_part_limit + Rack::Utils.multipart_total_part_limit = 5 + + env = Rack::MockRequest.env_for '/', multipart_fixture(:three_files_three_fields) + lambda { Rack::Multipart.parse_multipart(env) }.must_raise Rack::Multipart::MultipartTotalPartLimitError + ensure + Rack::Utils.multipart_total_part_limit = previous_limit + end + end + it "return nil if no UploadedFiles were used" do data = Rack::Multipart.build_multipart("people" => [{ "submit-name" => "Larry", "files" => "contents" }]) data.must_be_nil diff --git a/test/spec_request.rb b/test/spec_request.rb index f4e940ce..9a94b35f 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -1352,7 +1352,7 @@ EOF f[:tempfile].size.must_equal 76 end - it "MultipartPartLimitError when request has too many multipart parts if limit set" do + it "MultipartPartLimitError when request has too many multipart file parts if limit set" do begin data = 10000.times.map { "--AaB03x\r\ncontent-type: text/plain\r\ncontent-disposition: attachment; name=#{SecureRandom.hex(10)}; filename=#{SecureRandom.hex(10)}\r\n\r\ncontents\r\n" }.join("\r\n") data += "--AaB03x--\r" @@ -1368,6 +1368,22 @@ EOF end end + it "MultipartPartLimitError when request has too many multipart total parts if limit set" do + begin + data = 10000.times.map { "--AaB03x\r\ncontent-type: text/plain\r\ncontent-disposition: attachment; name=#{SecureRandom.hex(10)}\r\n\r\ncontents\r\n" }.join("\r\n") + data += "--AaB03x--\r" + + options = { + "CONTENT_TYPE" => "multipart/form-data; boundary=AaB03x", + "CONTENT_LENGTH" => data.length.to_s, + :input => StringIO.new(data) + } + + request = make_request Rack::MockRequest.env_for("/", options) + lambda { request.POST }.must_raise Rack::Multipart::MultipartTotalPartLimitError + end + end + it 'closes tempfiles it created in the case of too many created' do begin data = 10000.times.map { "--AaB03x\r\ncontent-type: text/plain\r\ncontent-disposition: attachment; name=#{SecureRandom.hex(10)}; filename=#{SecureRandom.hex(10)}\r\n\r\ncontents\r\n" }.join("\r\n") |