Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Require CRLF line endings in request line and headers #138

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Require CRLF line endings in request line and headers #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/webrick/httprequest.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@request_method = $1
@unparsed_uri = $2
@http_version = HTTPVersion.new($3 ? $3 : "0.9")
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (@request_bytes += line.bytesize) > MAX_HEADER_LENGTH
raise HTTPStatus::RequestEntityTooLarge, 'headers too large'
end
Expand Down
10 changes: 6 additions & 4 deletions lib/webrick/httputils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}'."
Expand Down
2 changes: 1 addition & 1 deletion test/webrick/test_filehandler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading