Unhandled NoMethodError for empty chunk size

[kenballus]

The bug

When the chunked body parser attempts encounters an invalid chunk size, it attempts to raise an error using the dump method:

unless length =~ VALID_CHUNK_LENGTH
  raise BadRequest, "Invalid chunk length: #{length.dump}"
end

(from lib/protocol/http1/body/chunked.rb:47-49)

However, if the received chunk size is empty, then length is nil, which doesn't have a dump method. This causes an unhandled NoMethodError.

Steps to reproduce

1. Start a simple HTTP server that reads incoming request bodies, such as this:

require 'socket'
require 'protocol/http1/connection'
require 'protocol/http/body/buffered'

def handle_connection(connection)
  loop do
    request = connection.read_request
    break unless request

    _authority, _method, _path, version, _headers, body_reader = request
    body = body_reader ? body_reader.join : nil
    if not body
      body = ''
    end
    connection.write_response(version, 200, [])
    connection.write_body(version, Protocol::HTTP::Body::Buffered.wrap([body]))

    break unless connection.persistent
  rescue Protocol::HTTP1::InvalidRequest, Protocol::HTTP1::BadRequest, Protocol::HTTP1::BadHeader
    break
  end
end

Addrinfo.tcp('0.0.0.0', 80).listen do |server|
  loop do
    client, _address = server.accept
    connection = Protocol::HTTP1::Connection.new(client)
    handle_connection(connection)
  end
end

2. Send it a request that's missing a chunk size:

printf 'POST / HTTP/1.1\r\nHost: whatever\r\nTransfer-Encoding: chunked\r\n\r\n\r\n\r\n' \
  | nc localhost 80

3. Watch it crash:

/var/lib/gems/3.1.0/gems/protocol-http1-0.20.0/lib/protocol/http1/body/chunked.rb:48:in `read': undefined method `dump' for nil:NilClass (NoMethodError)

						raise BadRequest, "Invalid chunk length: #{length.dump}"
						                                                 ^^^^^
	from /var/lib/gems/3.1.0/gems/protocol-http-0.29.0/lib/protocol/http/body/readable.rb:80:in `each'
	from /var/lib/gems/3.1.0/gems/protocol-http-0.29.0/lib/protocol/http/body/readable.rb:94:in `join'
	from server.rb:11:in `block in handle_connection'
	from server.rb:6:in `loop'
	from server.rb:6:in `handle_connection'
	from server.rb:28:in `block (2 levels) in <main>'
	from server.rb:25:in `loop'
	from server.rb:25:in `block in <main>'
	from /usr/lib/ruby/3.1.0/socket.rb:209:in `listen'
	from server.rb:24:in `<main>'

Suggested fix

Remove ${length.dump} from the error response. It's bad to put attacker-controlled data in HTTP responses if it can be avoided.

[ioquatix]

That makes sense, thanks for the detailed report.

[ioquatix]

For the first pass, I'm going to rewrite .dump to .inspect which will work with nil values.

I appreciate your point about not including user-controlled data in logs. However, it makes debugging extremely difficult, so it's a balance.