From ee60354bcb84ec33b9245e1d1aa6e1f7e8132101 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Tue, 25 Jun 2024 14:39:04 -0700 Subject: [PATCH] Require CRLF line endings in request line and headers Disallow bare CR, LF, NUL in header and request lines. Tighten parsing of request lines to only allow single spaces, as specified in the RFCs. Forcing this RFC-compliant behavior breaks a lot of tests, so fix the tests to correctly use CRLF instead of LF for requests (other than the specific checks for handling of bad requests). Fixes #137 --- lib/webrick/httprequest.rb | 4 +- lib/webrick/httputils.rb | 10 ++- test/webrick/test_filehandler.rb | 2 +- test/webrick/test_httprequest.rb | 149 +++++++++++++++++++++++++------ 4 files changed, 133 insertions(+), 32 deletions(-) diff --git a/lib/webrick/httprequest.rb b/lib/webrick/httprequest.rb index 80b01e9..62ea54c 100644 --- a/lib/webrick/httprequest.rb +++ b/lib/webrick/httprequest.rb @@ -458,7 +458,7 @@ def read_request_line(socket) end @request_time = Time.now - if /^(\S+)\s+(\S++)(?:\s+HTTP\/(\d+\.\d+))?\r?\n/mo =~ @request_line + if /^(\S+) (\S++)(?: HTTP\/(\d+\.\d+))?\r\n/mo =~ @request_line @request_method = $1 @unparsed_uri = $2 @http_version = HTTPVersion.new($3 ? $3 : "0.9") @@ -471,7 +471,7 @@ def read_request_line(socket) def read_header(socket) if socket while line = read_line(socket) - break if /\A(#{CRLF}|#{LF})\z/om =~ line + break if /\A#{CRLF}\z/om =~ line if (@request_bytes += line.bytesize) > MAX_HEADER_LENGTH raise HTTPStatus::RequestEntityTooLarge, 'headers too large' end diff --git a/lib/webrick/httputils.rb b/lib/webrick/httputils.rb index 1653a07..ea67fdb 100644 --- a/lib/webrick/httputils.rb +++ b/lib/webrick/httputils.rb @@ -173,16 +173,18 @@ def parse_header(raw) field = nil raw.each_line{|line| case line - when /^([A-Za-z0-9!\#$%&'*+\-.^_`|~]+):(.*?)\z/om - field, value = $1, $2.strip + when /^([A-Za-z0-9!\#$%&'*+\-.^_`|~]+):([^\r\n\0]*?)\r\n\z/om + field, value = $1, $2 field.downcase! header[field] = HEADER_CLASSES[field].new unless header.has_key?(field) header[field] << value - when /^\s+(.*?)/om - value = line.strip + when /^\s+([^\r\n\0]*?)\r\n/om unless field raise HTTPStatus::BadRequest, "bad header '#{line}'." end + value = line + value.lstrip! + value.slice!(-2..-1) header[field][-1] << " " << value else raise HTTPStatus::BadRequest, "bad header '#{line}'." diff --git a/test/webrick/test_filehandler.rb b/test/webrick/test_filehandler.rb index 452667d..5165439 100644 --- a/test/webrick/test_filehandler.rb +++ b/test/webrick/test_filehandler.rb @@ -33,7 +33,7 @@ def make_range_request(range_spec) Range: #{range_spec} END_OF_REQUEST - return StringIO.new(msg.gsub(/^ {6}/, "")) + return StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n")) end def make_range_response(file, range_spec) diff --git a/test/webrick/test_httprequest.rb b/test/webrick/test_httprequest.rb index 87a2752..fa18177 100644 --- a/test/webrick/test_httprequest.rb +++ b/test/webrick/test_httprequest.rb @@ -11,7 +11,7 @@ def teardown def test_simple_request msg = <<-_end_of_message_ -GET / +GET /\r _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) @@ -24,7 +24,7 @@ def test_parse_09 foobar # HTTP/0.9 request don't have header nor entity body. _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal("GET", req.request_method) assert_equal("/", req.unparsed_uri) assert_equal(WEBrick::HTTPVersion.new("0.9"), req.http_version) @@ -41,7 +41,7 @@ def test_parse_10 _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal("GET", req.request_method) assert_equal("/", req.unparsed_uri) assert_equal(WEBrick::HTTPVersion.new("1.0"), req.http_version) @@ -58,7 +58,7 @@ def test_parse_11 _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal("GET", req.request_method) assert_equal("/path", req.unparsed_uri) assert_equal("", req.script_name) @@ -77,7 +77,7 @@ def test_request_uri_too_large _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) assert_raise(WEBrick::HTTPStatus::RequestURITooLarge){ - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) } end @@ -89,11 +89,101 @@ def test_invalid_content_length_header _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) assert_raise(WEBrick::HTTPStatus::BadRequest){ - req.parse(StringIO.new(msg.gsub(/^ {8}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {8}/, "").gsub("\n", "\r\n"))) } end end + def test_bare_lf_request_line + msg = <<-_end_of_message_ + GET / HTTP/1.1 + Content-Length: 0\r + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + end + + def test_bare_lf_header + msg = <<-_end_of_message_ + GET / HTTP/1.1\r + Content-Length: 0 + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + end + + def test_bare_cr_request_line + msg = <<-_end_of_message_ + GET / HTTP/1.1\r\r + Content-Length: 0\r + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + end + + def test_bare_cr_header + msg = <<-_end_of_message_ + GET / HTTP/1.1\r + Content-Type: foo\rbar\r + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + end + + def test_invalid_request_lines + msg = <<-_end_of_message_ + GET / HTTP/1.1\r + Content-Length: 0\r + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + + msg = <<-_end_of_message_ + GET / HTTP/1.1\r + Content-Length: 0\r + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + + msg = <<-_end_of_message_ + GET /\r HTTP/1.1\r + Content-Length: 0\r + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + + msg = <<-_end_of_message_ + GET / HTTP/1.1 \r + Content-Length: 0\r + \r + _end_of_message_ + req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) + assert_raise(WEBrick::HTTPStatus::BadRequest){ + req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + } + end + def test_duplicate_content_length_header msg = <<-_end_of_message_ GET / HTTP/1.1 @@ -102,7 +192,7 @@ def test_duplicate_content_length_header _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) assert_raise(WEBrick::HTTPStatus::BadRequest){ - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) } end @@ -118,13 +208,13 @@ def test_parse_headers Accept-Language: en;q=0.5, *; q=0 Accept-Language: ja Content-Type: text/plain - Content-Length: 7 + Content-Length: 8 X-Empty-Header: foobar _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal( URI.parse("http://test.ruby-lang.org:8080/path"), req.request_uri) assert_equal("test.ruby-lang.org", req.host) @@ -135,9 +225,9 @@ def test_parse_headers req.accept) assert_equal(%w(gzip compress identity *), req.accept_encoding) assert_equal(%w(ja en *), req.accept_language) - assert_equal(7, req.content_length) + assert_equal(8, req.content_length) assert_equal("text/plain", req.content_type) - assert_equal("foobar\n", req.body) + assert_equal("foobar\r\n", req.body) assert_equal("", req["x-empty-header"]) assert_equal(nil, req["x-no-header"]) assert(req.query.empty?) @@ -146,7 +236,7 @@ def test_parse_headers def test_parse_header2() msg = <<-_end_of_message_ POST /foo/bar/../baz?q=a HTTP/1.0 - Content-Length: 9 + Content-Length: 10 User-Agent: FOO BAR BAZ @@ -154,14 +244,14 @@ def test_parse_header2() hogehoge _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal("POST", req.request_method) assert_equal("/foo/baz", req.path) assert_equal("", req.script_name) assert_equal("/foo/baz", req.path_info) - assert_equal("9", req['content-length']) + assert_equal("10", req['content-length']) assert_equal("FOO BAR BAZ", req['user-agent']) - assert_equal("hogehoge\n", req.body) + assert_equal("hogehoge\r\n", req.body) end def test_parse_headers3 @@ -171,7 +261,7 @@ def test_parse_headers3 _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal(URI.parse("http://test.ruby-lang.org/path"), req.request_uri) assert_equal("test.ruby-lang.org", req.host) assert_equal(80, req.port) @@ -182,7 +272,7 @@ def test_parse_headers3 _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal(URI.parse("http://192.168.1.1/path"), req.request_uri) assert_equal("192.168.1.1", req.host) assert_equal(80, req.port) @@ -193,7 +283,7 @@ def test_parse_headers3 _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal(URI.parse("http://[fe80::208:dff:feef:98c7]/path"), req.request_uri) assert_equal("[fe80::208:dff:feef:98c7]", req.host) @@ -205,7 +295,7 @@ def test_parse_headers3 _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal(URI.parse("http://192.168.1.1:8080/path"), req.request_uri) assert_equal("192.168.1.1", req.host) assert_equal(8080, req.port) @@ -216,7 +306,7 @@ def test_parse_headers3 _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) assert_equal(URI.parse("http://[fe80::208:dff:feef:98c7]:8080/path"), req.request_uri) assert_equal("[fe80::208:dff:feef:98c7]", req.host) @@ -231,7 +321,7 @@ def test_parse_get_params _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) query = req.query assert_equal("1", query["foo"]) assert_equal(["1", "2", "3"], query["foo"].to_ary) @@ -251,7 +341,7 @@ def test_parse_post_params #{param} _end_of_message_ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) query = req.query assert_equal("1", query["foo"]) assert_equal(["1", "2", "3"], query["foo"].to_ary) @@ -270,6 +360,7 @@ def test_chunked _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") File.open(__FILE__){|io| while chunk = io.read(100) msg << chunk.size.to_s(16) << crlf @@ -335,6 +426,7 @@ def test_forwarded _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert_equal("server.example.com", req.server_name) @@ -355,6 +447,7 @@ def test_forwarded _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert_equal("server.example.com", req.server_name) @@ -377,6 +470,7 @@ def test_forwarded _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert_equal("server.example.com", req.server_name) @@ -399,6 +493,7 @@ def test_forwarded _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert_equal("server1.example.com", req.server_name) @@ -421,6 +516,7 @@ def test_forwarded _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert_equal("server1.example.com", req.server_name) @@ -443,6 +539,7 @@ def test_forwarded _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert_equal("server1.example.com", req.server_name) @@ -460,6 +557,7 @@ def test_continue_sent _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert req['expect'] @@ -476,6 +574,7 @@ def test_continue_not_sent _end_of_message_ msg.gsub!(/^ {6}/, "") + msg.gsub!("\n", "\r\n") req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) req.parse(StringIO.new(msg)) assert !req['expect'] @@ -495,7 +594,7 @@ def test_bad_messages _end_of_message_ assert_raise(WEBrick::HTTPStatus::LengthRequired){ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) req.body } @@ -508,7 +607,7 @@ def test_bad_messages _end_of_message_ assert_raise(WEBrick::HTTPStatus::BadRequest){ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) req.body } @@ -521,7 +620,7 @@ def test_bad_messages _end_of_message_ assert_raise(WEBrick::HTTPStatus::NotImplemented){ req = WEBrick::HTTPRequest.new(WEBrick::Config::HTTP) - req.parse(StringIO.new(msg.gsub(/^ {6}/, ""))) + req.parse(StringIO.new(msg.gsub(/^ {6}/, "").gsub("\n", "\r\n"))) req.body } end