Skip to content

Commit 5bb7d20

Browse files
Merge pull request from GHSA-h99w-9q5r-gjq9
* 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>
1 parent c6340d1 commit 5bb7d20

File tree

6 files changed

+289
-15
lines changed

6 files changed

+289
-15
lines changed

Diff for: lib/puma/client.rb

+54-11
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ module Puma
2323

2424
class ConnectionError < RuntimeError; end
2525

26+
class HttpParserError501 < IOError; end
27+
2628
# An instance of this class represents a unique request from a client.
2729
# For example, this could be a web request from a browser or from CURL.
2830
#
@@ -35,7 +37,21 @@ class ConnectionError < RuntimeError; end
3537
# Instances of this class are responsible for knowing if
3638
# the header and body are fully buffered via the `try_to_finish` method.
3739
# They can be used to "time out" a response via the `timeout_at` reader.
40+
#
3841
class Client
42+
43+
# this tests all values but the last, which must be chunked
44+
ALLOWED_TRANSFER_ENCODING = %w[compress deflate gzip].freeze
45+
46+
# chunked body validation
47+
CHUNK_SIZE_INVALID = /[^\h]/.freeze
48+
CHUNK_VALID_ENDING = "\r\n".freeze
49+
50+
# Content-Length header value validation
51+
CONTENT_LENGTH_VALUE_INVALID = /[^\d]/.freeze
52+
53+
TE_ERR_MSG = 'Invalid Transfer-Encoding'
54+
3955
# The object used for a request with no body. All requests with
4056
# no body share this one object since it has no state.
4157
EmptyBody = NullIO.new
@@ -302,24 +318,40 @@ def setup_body
302318
body = @parser.body
303319

304320
te = @env[TRANSFER_ENCODING2]
305-
306321
if te
307-
if te.include?(",")
308-
te.split(",").each do |part|
309-
if CHUNKED.casecmp(part.strip) == 0
310-
return setup_chunked_body(body)
311-
end
322+
te_lwr = te.downcase
323+
if te.include? ','
324+
te_ary = te_lwr.split ','
325+
te_count = te_ary.count CHUNKED
326+
te_valid = te_ary[0..-2].all? { |e| ALLOWED_TRANSFER_ENCODING.include? e }
327+
if te_ary.last == CHUNKED && te_count == 1 && te_valid
328+
@env.delete TRANSFER_ENCODING2
329+
return setup_chunked_body body
330+
elsif te_count >= 1
331+
raise HttpParserError , "#{TE_ERR_MSG}, multiple chunked: '#{te}'"
332+
elsif !te_valid
333+
raise HttpParserError501, "#{TE_ERR_MSG}, unknown value: '#{te}'"
312334
end
313-
elsif CHUNKED.casecmp(te) == 0
314-
return setup_chunked_body(body)
335+
elsif te_lwr == CHUNKED
336+
@env.delete TRANSFER_ENCODING2
337+
return setup_chunked_body body
338+
elsif ALLOWED_TRANSFER_ENCODING.include? te_lwr
339+
raise HttpParserError , "#{TE_ERR_MSG}, single value must be chunked: '#{te}'"
340+
else
341+
raise HttpParserError501 , "#{TE_ERR_MSG}, unknown value: '#{te}'"
315342
end
316343
end
317344

318345
@chunked_body = false
319346

320347
cl = @env[CONTENT_LENGTH]
321348

322-
unless cl
349+
if cl
350+
# cannot contain characters that are not \d
351+
if cl =~ CONTENT_LENGTH_VALUE_INVALID
352+
raise HttpParserError, "Invalid Content-Length: #{cl.inspect}"
353+
end
354+
else
323355
@buffer = body.empty? ? nil : body
324356
@body = EmptyBody
325357
set_ready
@@ -478,7 +510,13 @@ def decode_chunk(chunk)
478510
while !io.eof?
479511
line = io.gets
480512
if line.end_with?("\r\n")
481-
len = line.strip.to_i(16)
513+
# Puma doesn't process chunk extensions, but should parse if they're
514+
# present, which is the reason for the semicolon regex
515+
chunk_hex = line.strip[/\A[^;]+/]
516+
if chunk_hex =~ CHUNK_SIZE_INVALID
517+
raise HttpParserError, "Invalid chunk size: '#{chunk_hex}'"
518+
end
519+
len = chunk_hex.to_i(16)
482520
if len == 0
483521
@in_last_chunk = true
484522
@body.rewind
@@ -509,7 +547,12 @@ def decode_chunk(chunk)
509547

510548
case
511549
when got == len
512-
write_chunk(part[0..-3]) # to skip the ending \r\n
550+
# proper chunked segment must end with "\r\n"
551+
if part.end_with? CHUNK_VALID_ENDING
552+
write_chunk(part[0..-3]) # to skip the ending \r\n
553+
else
554+
raise HttpParserError, "Chunk size mismatch"
555+
end
513556
when got <= len - 2
514557
write_chunk(part)
515558
@partial_part_left = len - part.size

Diff for: lib/puma/const.rb

+5-3
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ class UnsupportedOption < RuntimeError
7676
508 => 'Loop Detected',
7777
510 => 'Not Extended',
7878
511 => 'Network Authentication Required'
79-
}
79+
}.freeze
8080

8181
# For some HTTP status codes the client only expects headers.
8282
#
@@ -85,7 +85,7 @@ class UnsupportedOption < RuntimeError
8585
204 => true,
8686
205 => true,
8787
304 => true
88-
}
88+
}.freeze
8989

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

152154
# The basic max request size we'll try to read.
153155
CHUNK_SIZE = 16 * 1024

Diff for: lib/puma/server.rb

+3
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,9 @@ def client_error(e, client)
515515
when HttpParserError
516516
client.write_error(400)
517517
@events.parse_error e, client
518+
when HttpParserError501
519+
client.write_error(501)
520+
@events.parse_error e, client
518521
else
519522
client.write_error(500)
520523
@events.unknown_error e, nil, "Read"

Diff for: test/helper.rb

+3
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,9 @@ def skip_unless(eng, bt: caller)
174174
Minitest::Test.include TestSkips
175175

176176
class Minitest::Test
177+
178+
REPO_NAME = ENV['GITHUB_REPOSITORY'] ? ENV['GITHUB_REPOSITORY'][/[^\/]+\z/] : 'puma'
179+
177180
def self.run(reporter, options = {}) # :nodoc:
178181
prove_it!
179182
super

Diff for: test/test_puma_server.rb

+4-1
Original file line numberDiff line numberDiff line change
@@ -602,17 +602,20 @@ def test_Expect_100
602602
def test_chunked_request
603603
body = nil
604604
content_length = nil
605+
transfer_encoding = nil
605606
server_run { |env|
606607
body = env['rack.input'].read
607608
content_length = env['CONTENT_LENGTH']
609+
transfer_encoding = env['HTTP_TRANSFER_ENCODING']
608610
[200, {}, [""]]
609611
}
610612

611-
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"
613+
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"
612614

613615
assert_equal "HTTP/1.1 200 OK\r\nConnection: close\r\nContent-Length: 0\r\n\r\n", data
614616
assert_equal "hello", body
615617
assert_equal "5", content_length
618+
assert_nil transfer_encoding
616619
end
617620

618621
def test_large_chunked_request

0 commit comments

Comments
 (0)