Skip to content

Commit

Permalink
Merge pull request from GHSA-h99w-9q5r-gjq9
Browse files Browse the repository at this point in the history
* Fix tests when run on GH Actions and repo isn't named 'puma'

* Test updates for CVE

* Lib Updates for CVE

* cleint.rb - make validation values constants

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
  • Loading branch information
nateberkopec and MSP-Greg committed Mar 30, 2022
1 parent 706534a commit b8439ff
Show file tree
Hide file tree
Showing 6 changed files with 293 additions and 15 deletions.
65 changes: 54 additions & 11 deletions lib/puma/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ module Puma

class ConnectionError < RuntimeError; end

class HttpParserError501 < IOError; end

# An instance of this class represents a unique request from a client.
# For example, this could be a web request from a browser or from CURL.
#
Expand All @@ -35,7 +37,21 @@ class ConnectionError < RuntimeError; end
# Instances of this class are responsible for knowing if
# the header and body are fully buffered via the `try_to_finish` method.
# They can be used to "time out" a response via the `timeout_at` reader.
#
class Client

# this tests all values but the last, which must be chunked
ALLOWED_TRANSFER_ENCODING = %w[compress deflate gzip].freeze

# chunked body validation
CHUNK_SIZE_INVALID = /[^\h]/.freeze
CHUNK_VALID_ENDING = "\r\n".freeze

# Content-Length header value validation
CONTENT_LENGTH_VALUE_INVALID = /[^\d]/.freeze

TE_ERR_MSG = 'Invalid Transfer-Encoding'

# The object used for a request with no body. All requests with
# no body share this one object since it has no state.
EmptyBody = NullIO.new
Expand Down Expand Up @@ -284,24 +300,40 @@ def setup_body
body = @parser.body

te = @env[TRANSFER_ENCODING2]

if te
if te.include?(",")
te.split(",").each do |part|
if CHUNKED.casecmp(part.strip) == 0
return setup_chunked_body(body)
end
te_lwr = te.downcase
if te.include? ','
te_ary = te_lwr.split ','
te_count = te_ary.count CHUNKED
te_valid = te_ary[0..-2].all? { |e| ALLOWED_TRANSFER_ENCODING.include? e }
if te_ary.last == CHUNKED && te_count == 1 && te_valid
@env.delete TRANSFER_ENCODING2
return setup_chunked_body body
elsif te_count >= 1
raise HttpParserError , "#{TE_ERR_MSG}, multiple chunked: '#{te}'"
elsif !te_valid
raise HttpParserError501, "#{TE_ERR_MSG}, unknown value: '#{te}'"
end
elsif CHUNKED.casecmp(te) == 0
return setup_chunked_body(body)
elsif te_lwr == CHUNKED
@env.delete TRANSFER_ENCODING2
return setup_chunked_body body
elsif ALLOWED_TRANSFER_ENCODING.include? te_lwr
raise HttpParserError , "#{TE_ERR_MSG}, single value must be chunked: '#{te}'"
else
raise HttpParserError501 , "#{TE_ERR_MSG}, unknown value: '#{te}'"
end
end

@chunked_body = false

cl = @env[CONTENT_LENGTH]

unless cl
if cl
# cannot contain characters that are not \d
if cl =~ CONTENT_LENGTH_VALUE_INVALID
raise HttpParserError, "Invalid Content-Length: #{cl.inspect}"
end
else
@buffer = body.empty? ? nil : body
@body = EmptyBody
set_ready
Expand Down Expand Up @@ -450,7 +482,13 @@ def decode_chunk(chunk)
while !io.eof?
line = io.gets
if line.end_with?("\r\n")
len = line.strip.to_i(16)
# Puma doesn't process chunk extensions, but should parse if they're
# present, which is the reason for the semicolon regex
chunk_hex = line.strip[/\A[^;]+/]
if chunk_hex =~ CHUNK_SIZE_INVALID
raise HttpParserError, "Invalid chunk size: '#{chunk_hex}'"
end
len = chunk_hex.to_i(16)
if len == 0
@in_last_chunk = true
@body.rewind
Expand Down Expand Up @@ -481,7 +519,12 @@ def decode_chunk(chunk)

case
when got == len
write_chunk(part[0..-3]) # to skip the ending \r\n
# proper chunked segment must end with "\r\n"
if part.end_with? CHUNK_VALID_ENDING
write_chunk(part[0..-3]) # to skip the ending \r\n
else
raise HttpParserError, "Chunk size mismatch"
end
when got <= len - 2
write_chunk(part)
@partial_part_left = len - part.size
Expand Down
8 changes: 5 additions & 3 deletions lib/puma/const.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class UnsupportedOption < RuntimeError
508 => 'Loop Detected',
510 => 'Not Extended',
511 => 'Network Authentication Required'
}
}.freeze

# For some HTTP status codes the client only expects headers.
#
Expand All @@ -85,7 +85,7 @@ class UnsupportedOption < RuntimeError
204 => true,
205 => true,
304 => true
}
}.freeze

# Frequently used constants when constructing requests or responses. Many times
# the constant just refers to a string with the same contents. Using these constants
Expand Down Expand Up @@ -144,9 +144,11 @@ module Const
408 => "HTTP/1.1 408 Request Timeout\r\nConnection: close\r\nServer: Puma #{PUMA_VERSION}\r\n\r\n".freeze,
# Indicate that there was an internal error, obviously.
500 => "HTTP/1.1 500 Internal Server Error\r\n\r\n".freeze,
# Incorrect or invalid header value
501 => "HTTP/1.1 501 Not Implemented\r\n\r\n".freeze,
# A common header for indicating the server is too busy. Not used yet.
503 => "HTTP/1.1 503 Service Unavailable\r\n\r\nBUSY".freeze
}
}.freeze

# The basic max request size we'll try to read.
CHUNK_SIZE = 16 * 1024
Expand Down
9 changes: 9 additions & 0 deletions lib/puma/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ def run(background=true)
client.write_error(400)
client.close

@events.parse_error self, client.env, e
rescue HttpParserError501 => e
client.write_error(501)
client.close
@events.parse_error self, client.env, e
rescue ConnectionError, EOFError
client.close
Expand Down Expand Up @@ -530,7 +534,12 @@ def process_client(client, buffer)
client.write_error(400)

@events.parse_error self, client.env, e
rescue HttpParserError501 => e
lowlevel_error(e, client.env)

client.write_error(501)

@events.parse_error self, client.env, e
# Server error
rescue StandardError => e
lowlevel_error(e, client.env)
Expand Down
3 changes: 3 additions & 0 deletions test/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ def skip_unless(eng, bt: caller)
Minitest::Test.include TestSkips

class Minitest::Test

REPO_NAME = ENV['GITHUB_REPOSITORY'] ? ENV['GITHUB_REPOSITORY'][/[^\/]+\z/] : 'puma'

def self.run(reporter, options = {}) # :nodoc:
prove_it!
super
Expand Down
5 changes: 4 additions & 1 deletion test/test_puma_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -446,17 +446,20 @@ def test_Expect_100
def test_chunked_request
body = nil
content_length = nil
transfer_encoding = nil
server_run app: ->(env) {
body = env['rack.input'].read
content_length = env['CONTENT_LENGTH']
transfer_encoding = env['HTTP_TRANSFER_ENCODING']
[200, {}, [""]]
}

data = send_http_and_read "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n\r\n"
data = send_http_and_read "GET / HTTP/1.1\r\nConnection: close\r\nTransfer-Encoding: gzip,chunked\r\n\r\n1\r\nh\r\n4\r\nello\r\n0\r\n\r\n"

assert_equal "HTTP/1.1 200 OK\r\nConnection: close\r\nContent-Length: 0\r\n\r\n", data
assert_equal "hello", body
assert_equal 5, content_length
assert_nil transfer_encoding
end

def test_chunked_request_pause_before_value
Expand Down
Loading

0 comments on commit b8439ff

Please sign in to comment.