Require CRLF line endings in request line and headers #138

jeremyevans · 2024-06-25T21:46:03Z

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

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 ruby#137

kenballus · 2024-06-25T21:55:04Z

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


I think the old behavior is technically RFC-compliant.

RFC 9112 section 3:

Although the request-line grammar rule requires that each of the component elements be separated by a single SP octet, recipients MAY instead parse on whitespace-delimited word boundaries and, aside from the CRLF terminator, treat any form of whitespace as the SP separator while ignoring preceding or trailing whitespace; such whitespace includes one or more of the following octets: SP, HTAB, VT (%x0B), FF (%x0C), or bare CR. However, lenient parsing can result in request smuggling security vulnerabilities if there are multiple recipients of the message and each has its own unique interpretation of robustness (see Section 11.2).

That said, this isn't a bad change. Apache rejects request lines that use whitespace other than a single SP octet, so I don't think you'll break compatibility with any clients by tightening this up.

kenballus · 2024-06-25T21:58:48Z

lib/webrick/httprequest.rb

@@ -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


Similarly, breaking lines on bare LF is allowed by the RFCs, so the old behavior was not a violation. From RFC 9112, section 2.2:

Although the line terminator for the start-line and fields is the sequence CRLF, a recipient MAY recognize a single LF as a line terminator and ignore any preceding CR.

Again, many popular servers (Apache, Lighttpd, Node.js, Passenger, Puma) require CRLF line endings, so you're unlikely to break compatibility by joining that club.

jeremyevans · 2024-06-25T23:04:53Z

@kenballus Thank you very much for your review. Since the RFCs do not require (MAY not MUST) the loose behavior, I would feel better about making things strict in order to reduce the chance of security issues, especially since other web servers are similarly strict.

kenballus · 2024-06-25T23:07:16Z

Agreed. LGTM.

ioquatix · 2024-07-02T13:58:02Z

test/webrick/test_httprequest.rb

@@ -11,7 +11,7 @@ def teardown

  def test_simple_request
    msg = <<-_end_of_message_
-GET /
+GET /\r


Shouldn't this be \r\n?

The \n comes from the line ending (it's a heredoc).

ioquatix · 2024-07-02T13:58:27Z

test/webrick/test_httprequest.rb

@@ -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")))


There is a lot of duplicated setup. Would it make sense to extract it into a helper? Or use <<~ instead?

Sure. If someone wants to work on cleaning up the specs in a later PR, that would be great.

ioquatix

Basically looks okay to me, but these tests look kind of crufty in general (not the fault of the PR).

jeremyevans mentioned this pull request Jun 25, 2024

CR incorrectly permitted within header values #137

Closed

kenballus reviewed Jun 25, 2024

View reviewed changes

ioquatix reviewed Jul 2, 2024

View reviewed changes

ioquatix approved these changes Jul 2, 2024

View reviewed changes

jeremyevans merged commit ee60354 into ruby:master Jul 2, 2024
19 checks passed

nate2014jatc mentioned this pull request Sep 25, 2024

Bump webrick from 1.8.1 to 1.8.2 nate2014jatc/AGSTATUS#28

Merged

richardhussey mentioned this pull request Oct 28, 2024

Bump webrick from 1.8.1 to 1.8.2 richardhussey/richardhussey.github.io#23

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require CRLF line endings in request line and headers #138

Require CRLF line endings in request line and headers #138

jeremyevans commented Jun 25, 2024

kenballus Jun 25, 2024

kenballus Jun 25, 2024

jeremyevans commented Jun 25, 2024

kenballus commented Jun 25, 2024

ioquatix Jul 2, 2024

jeremyevans Jul 2, 2024

ioquatix Jul 2, 2024

ioquatix Jul 2, 2024 •

edited

Loading

jeremyevans Jul 2, 2024

ioquatix left a comment

Require CRLF line endings in request line and headers #138

Require CRLF line endings in request line and headers #138

Conversation

jeremyevans commented Jun 25, 2024

kenballus Jun 25, 2024

Choose a reason for hiding this comment

kenballus Jun 25, 2024

Choose a reason for hiding this comment

jeremyevans commented Jun 25, 2024

kenballus commented Jun 25, 2024

ioquatix Jul 2, 2024

Choose a reason for hiding this comment

jeremyevans Jul 2, 2024

Choose a reason for hiding this comment

ioquatix Jul 2, 2024

Choose a reason for hiding this comment

ioquatix Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

jeremyevans Jul 2, 2024

Choose a reason for hiding this comment

ioquatix left a comment

Choose a reason for hiding this comment

ioquatix Jul 2, 2024 •

edited

Loading