Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

count the URL and headers separately when checking for HPE_HEADER_OVERFLOW errors #463

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

caiolrm
Copy link

@caiolrm caiolrm commented Mar 16, 2019

Counting headers, URL and start lines separately because
it was counting everything as headers before the headers_done state,
throwing HPE_HEADER_OVERFLOW errors even when what really caused
the overflow was the request URL being too long

Creating an HPE_URL_OVERFLOW err to make sure embedders
still have a request line limit, but now differentiating what
went over such limit.

Counting headers, URL and start lines separately because
it was counting everything as headers before the headers_done state,
throwing HPE_HEADER_OVERFLOW errors even when what really caused
the overflow was the request URL being too long

Creating an HPE_URL_OVERFLOW err to make sure embedders
still have a request line limit, but now differentiating what
went over such limit.

Refs: nodejs/node#26553
@caiolrm caiolrm changed the title Url counting count the URL and headers separately when checking for HPE_HEADER_OVERFLOW errors Mar 16, 2019
@sam-github
Copy link
Contributor

I think it would be useful to refer to what is being limited with the names from RFC's BNF.

What's the "start line"? Nothing in RFC 2616 is called that.

Request-Line has more than a Request-URI, it also has Method, HTTP-Version, and whitespace, any of which could be unreasonably long when written maliciously.

Does this measure URI completely separately from the other elements of the Request-Line? If so, is there some other limit on the Request-Line size?

/* icecast */ \
XX(33, SOURCE, SOURCE) \
#define HTTP_METHOD_MAP(XX) \
XX(0, DELETE, DELETE, 6) \
Copy link
Member

Choose a reason for hiding this comment

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

HTTP_METHOD_MAP is a public API. Changing it makes it a semver-major change.

@caiolrm
Copy link
Author

caiolrm commented Mar 17, 2019

@sam-github

What's the "start line"? Nothing in RFC 2616 is called that.

Take a look at the section 4.1 of RFC 2616, it briefly mentions it, but there's more detailed info regarding that in RFC 7230 section 3 and 3.1.

Request-Line has more than a Request-URI, it also has Method, HTTP-Version, and whitespace, any of which could be unreasonably long when written maliciously.

Does this measure URI completely separately from the other elements of the Request-Line? If so, is there some other limit on the Request-Line size?

This does measure URI completely separately and you're absolutely correct, there should be a limit for the request line size seeing I changed the way it was validated before. Guess I was too focused on the URI thing and forgot about the rest. Maybe I should add it here:

http-parser/http_parser.c

Lines 740 to 742 in 3a5a436

} else if (PARSING_START_LINE(CURRENT_STATE())) {
nread++;
}

And throw a new error like HPE_START_LINE_OVERFLOW, also have something similar to HTTP_MAX_HEADER_SIZE and HTTP_MAX_URL_SIZE macros like a HTTP_MAX_START_LINE_SIZE. How does that sound?

@bnoordhuis Should I do something like this:

#define HTTP_METHOD_LENGTH_MAP(XX) \
  XX(0,  6)                        \
  XX(1,  3)                        \
  XX(2,  4)                        \
  XX(3,  4)                        \
  XX(4,  3)                        \
  XX(5,  7)                        \
  XX(6,  7)                        \
...

instead and then have the method_lengths declared using it? Like

static const uint8_t method_lengths[] =
  {
#define XX(num, length) length,
  HTTP_METHOD_LENGTH_MAP(XX)
#undef XX
  };

that would probably decrease the impact of this change.

@ploxiln
Copy link
Contributor

ploxiln commented Mar 18, 2019

you may be able to do something like:

static const uint8_t method_lengths[] = 
  {
#define XX(num, name, string) uint8_t(sizeof(#string) - 1),
  HTTP_METHOD_MAP(XX)
#undef XX
  }

@ploxiln
Copy link
Contributor

ploxiln commented Mar 18, 2019

(but there may be a slightly less expensive way to just error-out if the request line overall is too big ...)

@bnoordhuis
Copy link
Member

@ploxiln's suggestion works but it's still possible to exceed the overall limit. This PR stores state in stack locals url_offset and spaces_before_url but that information is lost when the input is spread over multiple http_parser_execute() calls.

@caiolrm
Copy link
Author

caiolrm commented Mar 21, 2019

@bnoordhuis true. I can probably do away with url_offset, it's just being used for comparison. Gonna try to figure something out for the spaces, though, since currently, we can have multiple before the actual url.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants