Specify max header and URL in request line lengths

[avalchev]

I think that good security enhancement would be some max lengths configurations:

• maximum length of URL in request line
• maximum length of header's name and header's value

These are possible attack surfaces.

[vinipsmaker]

With the new parser, the limits are bound to the buffer size:

boost.http/include/boost/http/socket-inl.hpp

Line 802 in dc1dfee

if (used_size == asio::buffer_size(buffer)) {

But I need to add code to reply with the appropriate status code (URL too long, header field too long...) and I need to update documentation.

[avalchev]

Oh, buffer size limitation works fine in this case.

It's just better from user perspective to be able to set these configuration options.

[vinipsmaker]

Oh, buffer size limitation works fine in this case.

It adds more security, but I still need to write documentation. And a maximum number of headers would still be desirable.

I'll leave the issue open until I update the documentation.

[avalchev]

What do you at first creating class basic_socket::settings with default values (zero config) and add additional constructor to basic_socket which takes this class.

struct settings 
{
  std::size_t max_url_length = 8192; /* Default value for NodeJS  */
  std::size_t max_header_line_length = 8192; /* Default value for NodeJS  */
  std::size_t max_header_name_length = 1024;
  std::size_t max_header_name_length = 7168;  
};

Then checks probably should be added to on_async_read_message in do-while loop:

case token::code::field_name:
  if (field_name_size > settings_.max_header_name_length)
    {
         handler(system::error_code{http_errc::buffer_exhausted});
         return;
    }
case token::code::field_value:
  if (field_name_size > settings_.max_header_value_length)
    {
         handler(system::error_code{http_errc::buffer_exhausted});
         return;
    }

Please notice that reader is returning immediately, because there is no point to continue message parsing: no room for more data.

[vinipsmaker]

std::size_t max_header_line_length = 8192; /* Default value for NodeJS */

RFC7230 suggests a buffer that can process a request-line of 8000 characters, so I think this is a realistic size. However the user only needs to accept requests it is able to process (e.g. if it doesn't have any URL this big, there is no reason to accept such long...).

What do you [...]

Boost.Http tries to make a clear separation between two layers. The first is a "message-passing framework" which can be used to request and reply HTTP messages. The second is an embedded server implementation that exposes this layer (basic_socket<T>). I'll add more implementations (embedded HTTP/2.0 server and ZeroMQ-based to allow distributed load-balanced servers).

The idea is that you will be able to create a single handler that will respond to HTTP requests coming from multiple backends (embedded HTTP/1.1 server, embedded HTTPS/1.1 server, HTTP/2.0 server, ZeroMQ load-balancing, Apache module...) or change your code to use a different backend without needing to change the handler.

Long fields could be rejected (actually discarded) in the Message container. This would only work reliably for field names, not field values. If I introduce this customization point here, the server won't reply "field too long..." messages. Therefore I don't think it's wise to add them here (but they're already allowed if you implement a custom Message).

Long fields could also be rejected in the backend (ServerSocket implementation). If I introduce the customization point here, every ServerSocket implementation will require this (a restriction that I don't really want to add) or I could only implement for basic_socket (I prefer this option).

This settings object is interesting because it can be used to reject based on line length. However, we may be interested in discarding some fields, not only rejecting full requests. If we know we only check a limited set of fields, we may want to discard every other field. In this use case, the settings object doesn't fit nicely.

I'd prefer a Predicate-like object we inject in the basic_socket object. Technically a predicate only returns true or false, but I want one of three answers ("save field", "discard field", "reject request"). This would also work for line lengths, so it's more general.

[avalchev]

I started to understand you design :).

Just to add a few more points, from administrative point of view.

If the library is used on low-memory devices, then long header name / fields and body (probably user also should be able to configure this) should be discarded or message should be rejected as soon as possible and memory to be released.

Even this is not limited only to low-memory devices. I can be a bad guy and send 2GB of header name to cloud machine with 1GB memory.

These type of attack is pretty common.