From 6018ab9de4e94452d671429f2231dad28b5ee28a Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Sat, 11 Nov 2023 16:46:08 -0800 Subject: [PATCH 1/5] Fix bug chunk extension detection This fixes a request smuggling vulnerability (Fixes #124). Co-authored-by: Ben Kallus --- lib/webrick/httprequest.rb | 2 +- test/webrick/test_httprequest.rb | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/lib/webrick/httprequest.rb b/lib/webrick/httprequest.rb index 7a1686b..0cfc1c9 100644 --- a/lib/webrick/httprequest.rb +++ b/lib/webrick/httprequest.rb @@ -542,7 +542,7 @@ def read_body(socket, block) def read_chunk_size(socket) line = read_line(socket) - if /^([0-9a-fA-F]+)(?:;(\S+))?/ =~ line + if /\A([0-9a-fA-F]+)(?:;(\S+(?:=\S+)?))?\r\n\z/ =~ line chunk_size = $1.hex chunk_ext = $2 [ chunk_size, chunk_ext ] diff --git a/test/webrick/test_httprequest.rb b/test/webrick/test_httprequest.rb index 9033217..73fa8a3 100644 --- a/test/webrick/test_httprequest.rb +++ b/test/webrick/test_httprequest.rb @@ -289,6 +289,31 @@ def test_chunked assert_equal(expect, dst.string) end + def test_bad_chunked + crlf = "\x0d\x0a" + expect = File.binread(__FILE__).freeze + msg = <<-_end_of_message_ + POST /path HTTP/1.1\r + Transfer-Encoding: chunked\r + \r + 01x1\r + \r + 1 + _end_of_message_ + msg.gsub!(/^ {6}/, "") + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + req.parse(StringIO.new(msg)) + assert_raise(WEBrick::HTTPStatus::BadRequest){ req.body } + + # chunked req.body_reader + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + req.parse(StringIO.new(msg)) + dst = StringIO.new + assert_raise(WEBrick::HTTPStatus::BadRequest) do + IO.copy_stream(req.body_reader, dst) + end + end + def test_forwarded msg = <<-_end_of_message_ GET /foo HTTP/1.1 From 44cb9cb31cb9494fd820671738219ca4fc5e4f2a Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Mon, 13 Nov 2023 12:28:57 -0800 Subject: [PATCH 2/5] Make \r optional in chunk size detection As pointed out by Ken Ballus, WEBrick allows \n without preceding \r generally. It probably shouldn't, but since it does, do not require \r for chunk size detection. --- lib/webrick/httprequest.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/webrick/httprequest.rb b/lib/webrick/httprequest.rb index 0cfc1c9..02c5dcb 100644 --- a/lib/webrick/httprequest.rb +++ b/lib/webrick/httprequest.rb @@ -542,7 +542,7 @@ def read_body(socket, block) def read_chunk_size(socket) line = read_line(socket) - if /\A([0-9a-fA-F]+)(?:;(\S+(?:=\S+)?))?\r\n\z/ =~ line + if /\A([0-9a-fA-F]+)(?:;(\S+(?:=\S+)?))?\r?\n\z/ =~ line chunk_size = $1.hex chunk_ext = $2 [ chunk_size, chunk_ext ] From 4ec1194ce6fa794b015b9eefac2d23d83e7c304f Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Mon, 13 Nov 2023 12:31:30 -0800 Subject: [PATCH 3/5] Remove unneeded lines in new test --- test/webrick/test_httprequest.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/webrick/test_httprequest.rb b/test/webrick/test_httprequest.rb index 73fa8a3..471005c 100644 --- a/test/webrick/test_httprequest.rb +++ b/test/webrick/test_httprequest.rb @@ -290,8 +290,6 @@ def test_chunked end def test_bad_chunked - crlf = "\x0d\x0a" - expect = File.binread(__FILE__).freeze msg = <<-_end_of_message_ POST /path HTTP/1.1\r Transfer-Encoding: chunked\r From cb6d63677792f3a8e8df612084c54e33df04746c Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 1 Dec 2023 11:29:47 -0800 Subject: [PATCH 4/5] Revert "Make \r optional in chunk size detection" This reverts commit 44cb9cb31cb9494fd820671738219ca4fc5e4f2a. --- lib/webrick/httprequest.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/webrick/httprequest.rb b/lib/webrick/httprequest.rb index 02c5dcb..0cfc1c9 100644 --- a/lib/webrick/httprequest.rb +++ b/lib/webrick/httprequest.rb @@ -542,7 +542,7 @@ def read_body(socket, block) def read_chunk_size(socket) line = read_line(socket) - if /\A([0-9a-fA-F]+)(?:;(\S+(?:=\S+)?))?\r?\n\z/ =~ line + if /\A([0-9a-fA-F]+)(?:;(\S+(?:=\S+)?))?\r\n\z/ =~ line chunk_size = $1.hex chunk_ext = $2 [ chunk_size, chunk_ext ] From dcaeea6813d9e25516710d1176d883f08289b3ee Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 1 Dec 2023 12:47:24 -0800 Subject: [PATCH 5/5] Reject null bytes in header lines Fixes #126 --- lib/webrick/httprequest.rb | 3 +++ test/webrick/test_httprequest.rb | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/lib/webrick/httprequest.rb b/lib/webrick/httprequest.rb index 0cfc1c9..43e4e8a 100644 --- a/lib/webrick/httprequest.rb +++ b/lib/webrick/httprequest.rb @@ -475,6 +475,9 @@ def read_header(socket) if (@request_bytes += line.bytesize) > MAX_HEADER_LENGTH raise HTTPStatus::RequestEntityTooLarge, 'headers too large' end + if line.include?("\x00") + raise HTTPStatus::BadRequest, 'null byte in header' + end @raw_header << line end end diff --git a/test/webrick/test_httprequest.rb b/test/webrick/test_httprequest.rb index 471005c..c0fb2e9 100644 --- a/test/webrick/test_httprequest.rb +++ b/test/webrick/test_httprequest.rb @@ -312,6 +312,17 @@ def test_bad_chunked end end + def test_null_byte_in_header + msg = <<-_end_of_message_ + POST /path HTTP/1.1\r + Evil: evil\x00\r + \r + _end_of_message_ + msg.gsub!(/^ {6}/, "") + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ req.parse(StringIO.new(msg)) } + end + def test_forwarded msg = <<-_end_of_message_ GET /foo HTTP/1.1