Skip to content

client: perform strict chunk size parsing #4

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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jow-
Copy link
Contributor

@jow- jow- commented May 23, 2024

  • client: perform stricter HTTP request parsing

Introduce infrastructure and logic to perform less lenient parsing of HTTP request headers, chunk size headers and content-length values.

We can not rely on strtoul() to parse hexadecimal chunk sizes or content length values as it accepts a wider range of inputs than what is allowed by the HTTP spec.

Decode the chunk sizes and length values manually and fix skipping chunk extension headers while we're at it. Also ensure that there's no trailing garbage after the size and that we bail out on overflows.

Also rework the parsing of request header lines, to reject malformed header lines or illegal header names.

Fixes: #3
Fixes: #5

  • client: don't advertise keep-alive in uh_header_error()

The function uh_header_error() will tear down the connection after sending the HTTP reply message, so update the connection_close flag accordingly to avoid incorrectly emitting a Connection: keep-alive header.

  • client: don't advertise keep-alive when sending error replies

Commit 15346de ("client: Always close connection with request body in case of error") added logic to close keep alive connections on HTTP errors due to unconsumed request body data.

However, since the check happens after emitting the standard HTTP headers, uhttpd might incorrectly reply with a Connection: keep-alive even if it is going to close the connection.

Move the check before the emitting of the response headers in order to ensure that we're sending the correct Connection: close line.

Fixes: 15346de ("client: Always close connection with request body in case of error")

  • file: don't keep alive after file requests with payload

Since we're not consuming any body data when serving file requests, forcibly shut down keep-alive connections after requests indicating either a content-length or a chunked transfer encoding in order to avoid interpreting request body data as subsequent HTTP request.

@jow- jow- force-pushed the fix-chunk-size-parsing branch from cf071f4 to 3b723c4 Compare May 23, 2024 23:03
@kenballus
Copy link

Consider the case in which the chunk size is just \t; some-junk. Then, we break out of the first while loop, and the chunk-ext passes, and the chunk size counts as 0 even though it's invalid.

@kenballus
Copy link

Also, chunk-extensions do have a grammar that it may be worth considering enforcing.

jow- added 3 commits May 25, 2024 00:14
Since we're not consuming any body data when serving file requests,
forcibly shut down keep-alive connections after requests indicating
either a content-length or a chunked transfer encoding in order to
avoid interpreting request body data as subsequent HTTP request.

Signed-off-by: Jo-Philipp Wich <[email protected]>
Commit 15346de ("client: Always close connection with request body in case
of error") added logic to close keep alive connections on HTTP errors due
to unconsumed request body data.

However, since the check happens after emitting the standard HTTP headers,
uhttpd might incorrectly reply with a `Connection: keep-alive` even if it
is going to close the connection.

Move the check before the emitting of the response headers in order to
ensure that we're sending the correct `Connection: close` line.

Fixes: 15346de ("client: Always close connection with request body in case of error")
Signed-off-by: Jo-Philipp Wich <[email protected]>
The function `uh_header_error()` will tear down the connection after
sending the HTTP reply message, so update the `connection_close` flag
accordingly to avoid incorrectly emitting a `Connection: keep-alive`
header.

Signed-off-by: Jo-Philipp Wich <[email protected]>
@jow- jow- force-pushed the fix-chunk-size-parsing branch from 3b723c4 to 3c0fcf1 Compare May 24, 2024 22:18
Introduce infrastructure and logic to perform less lenient parsing of
HTTP request headers, chunk size headers and content-length values.

We can not rely on `strtoul()` to parse hexadecimal chunk sizes or
content length values as it accepts a wider range of inputs than what
is allowed by the HTTP spec.

Decode the chunk sizes and length values manually and fix skipping
chunk extension headers while we're at it. Also ensure that there's
no trailing garbage after the size and that we bail out on overflows.

Also rework the parsing of request header lines, to reject malformed
header lines or illegal header names.

Fixes: openwrt#3
Fixes: openwrt#5
Signed-off-by: Jo-Philipp Wich <[email protected]>
@jow- jow- force-pushed the fix-chunk-size-parsing branch from 3c0fcf1 to 3c8b991 Compare May 24, 2024 22:21
@JeppW
Copy link

JeppW commented Apr 4, 2025

This line in client_poll_post_data assumes that the chunk body is followed by a CRLF sequence without actually checking it.

ustream_consume(cl->us, sep + 2 - buf);

This can also be problematic if the proxy allows trailing data after the chunk contents (for example, see this bug in pound).

Might be worth incorporating into this change.

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

Successfully merging this pull request may close these issues.

uhttpd incorrectly accepts empty header names. Incorrect chunk size parsing behavior
3 participants