Permalink
Browse files

multipart/parser: avoid unbounded #gets method

Malicious clients may send excessively long lines
to trigger out-of-memory errors in a Rack web server.
  • Loading branch information...
1 parent 0869122 commit f95113402b7239f225282806673e1b6424522b18 Eric Wong committed with raggi Aug 22, 2012
Showing with 63 additions and 3 deletions.
  1. +10 −3 lib/rack/multipart/parser.rb
  2. +53 −0 test/spec_multipart.rb
View
13 lib/rack/multipart/parser.rb
@@ -70,9 +70,16 @@ def rx
def fast_forward_to_first_boundary
loop do
- read_buffer = @io.gets
- break if read_buffer == full_boundary
- raise EOFError, "bad content body" if read_buffer.nil?
+ content = @io.read(BUFSIZE)
+ raise EOFError, "bad content body" unless content
+ @buf << content
+
+ while @buf.gsub!(/\A([^\n]*\n)/, '')
+ read_buffer = $1
+ return if read_buffer == full_boundary
+ end
+
+ raise EOFError, "bad content body" if Utils.bytesize(@buf) >= BUFSIZE
end
end
View
53 test/spec_multipart.rb
@@ -48,6 +48,59 @@ def multipart_file(name)
params['profile']['bio'].should.include 'hello'
end
+ should "reject insanely long boundaries" do
+ # using a pipe since a tempfile can use up too much space
+ rd, wr = IO.pipe
+
+ # we only call rewind once at start, so make sure it succeeds
+ # and doesn't hit ESPIPE
+ def rd.rewind; end
+ wr.sync = true
+
+ # mock out length to make this pipe look like a Tempfile
+ def rd.length
+ 1024 * 1024 * 8
+ end
+
+ # write to a pipe in a background thread, this will write a lot
+ # unless Rack (properly) shuts down the read end
+ thr = Thread.new do
+ begin
+ wr.write("--AaB03x")
+
+ # make the initial boundary a few gigs long
+ longer = "0123456789" * 1024 * 1024
+ (1024 * 1024).times { wr.write(longer) }
+
+ wr.write("\r\n")
+ wr.write('Content-Disposition: form-data; name="a"; filename="a.txt"')
+ wr.write("\r\n")
+ wr.write("Content-Type: text/plain\r\n")
+ wr.write("\r\na")
+ wr.write("--AaB03x--\r\n")
+ wr.close
+ rescue => err # this is EPIPE if Rack shuts us down
+ err
+ end
+ end
+
+ fixture = {
+ "CONTENT_TYPE" => "multipart/form-data; boundary=AaB03x",
+ "CONTENT_LENGTH" => rd.length.to_s,
+ :input => rd,
+ }
+
+ env = Rack::MockRequest.env_for '/', fixture
+ lambda {
+ Rack::Multipart.parse_multipart(env)
+ }.should.raise(EOFError)
+ rd.close
+
+ err = thr.value
+ err.should.be.instance_of Errno::EPIPE
+ wr.close
+ end
+
should "parse multipart upload with text file" do
env = Rack::MockRequest.env_for("/", multipart_fixture(:text))
params = Rack::Multipart.parse_multipart(env)

0 comments on commit f951134

Please sign in to comment.