Web: restricted HTTP header lookup to the header section, and fixed an out-of-bounds read

[COOOkies4U]

Bugfix for Issue #397

[fdesbiens]

Hi @COOOkies4U.

Thank you for this contribution!

Before we can accept it, you need to sign the Eclipse Contributor Agreement (ECA). The purpose of the ECA is to provide a written record that you have agreed to provide your code and documentation contributions under the licenses used by the Eclipse ThreadX project. It also makes it clear that you are promising that what you are contributing to Eclipse is code you wrote, and you have the necessary rights to contribute it to our projects. And finally, it documents a commitment from you that your open source contributions will be permanently on the public record.

Signing the ECA requires an Eclipse Foundation account if you do not already have one. You can create one for free at https://accounts.eclipse.org.

Be sure to use the same email address when you register for the account that you intend to use on Git commit records. Also, please add your GitHub ID to your Eclipse account. This enables synchronisation between Eclipse-owned infrastructure and GitHub.

Here is the link to sign the ECA:
https://accounts.eclipse.org/user/login?destination=user/eca

[fdesbiens]

Thanks @COOOkies4U — the report in #397 is correct and was the hard part of this. Header parsing must stop at the first CRLFCRLF, _nx_web_http_server_field_value_get() did scan the whole packet, and _nx_web_http_server_chunked_check() seeing chunked encoding on a Content-Length request is exactly the consequence you describe. Bounding the scan at the content offset is also the right approach.

I have reworked the implementation and pushed it to this branch, and I want to explain why rather than just replace your diff — the reason the first version did not hold is genuinely non-obvious, and it is worth having on the record.

Why the first patch did not take effect

This function has no "field not found" flag. It infers absence from the scan pointer having run off the end of the packet: the loop exits on ch + name_length < append_ptr, then ch += name_length + 1 pushes ch past append_ptr, and if (ch >= append_ptr) return NX_WEB_HTTP_NOT_FOUND fires.

A break at the header boundary exits that loop with ch still in the middle of the packet, so the check no longer triggers. Control falls through into the value-copying code and reads from a fixed offset of name_length + 2 past the header — 19 bytes into the body when the query is Transfer-Encoding.

I built a harness from the real code — the parser, _nx_web_http_server_memicmp() and _nx_web_http_server_calculate_content_offset() extracted verbatim — and ran the same requests through dev, your patch, and the rework:

                                        dev        first patch    reworked
1. body contains field, offset > 0     SUCCESS     SUCCESS        NOT_FOUND
2. body STARTS with field name         SUCCESS     SUCCESS        NOT_FOUND
3. field ABSENT, body has CRLF         NOT_FOUND   SUCCESS        NOT_FOUND
4. real field, no CRLFCRLF             "chunked"   "ost: h"       "chunked"
5. field present in header (control)   "chunked"   "chunked"      "chunked"

Case 1 is the request from your issue, and it still returned SUCCESS. Because the read offset is fixed, the string Transfer-Encoding is not even needed in the body any more — a body of 19 filler bytes followed by chunked was enough to make the parser return chunked for a request whose header contained no Transfer-Encoding at all.

Three further consequences came from the same root cause:

• Case 3 is a new false positive. A field absent from the request entirely returned SUCCESS with bytes lifted out of the body. That affects all four call sites — If-Modified-Since, Content-Type, Transfer-Encoding and Connection — so an ordinary POST could be read as carrying a header it never sent.
• Case 4: _nx_web_http_server_calculate_content_offset() returns 0 when it does not find the terminator, which made the test ch > prepend_ptr + 0 fire on the first iteration. Only offset 0 was ever compared, so a packet whose header is not yet complete could not match any field.
• Case 2 is an off-by-one: content_offset is the offset of the first body byte, and the test was > rather than an inclusive bound, so a name starting exactly at the boundary still matched.

None of this is obvious from reading the function, which is why I would rather write it down than leave you to infer it from a rewritten diff.

What is on the branch now

Two commits, and the PR title has been widened to match.

cb671d6 — Restricted HTTP header field lookup to the header section. Your commit and the rework are squashed into one, authored to you, with the body noting that the implementation was reworked during review. Squashed because the intermediate state was worse than dev in the case-3 sense, and leaving that in the history would be a trap for anyone bisecting or cherry-picking.

The fix tracks the outcome in an explicit found flag, so the boundary can be enforced without disturbing it. The scan bound is (ch + name_length) <= scan_end, requiring the whole field name to sit inside the header, which closes case 2. And when content_offset is 0 the packet carries no body, so scan_end is the end of the packet and all of it is scanned — which is what the code did before and keeps case 4 working.

It also carries a regression test, test/regression/web_test/netx_web_header_boundary_test.c, with seven unit-level cases: the request from your issue, a body beginning with the field name, an absent field whose body holds a CRLF, a packet with no terminator, a body crafted for the fixed read offset, a header field competing with a body decoy, and the plain control case. It calls the parser directly, which is the pattern netx_web_host_field_test.c already uses in that suite.

I checked that the test actually discriminates rather than assuming it does: two cases fail on unpatched dev, five fail on the first patch, all seven pass on the rework.

9b150d0 — Fixed an out-of-bounds read in the header value whitespace skip. Separate, and unrelated to your change. The whitespace skip read the character before testing the bound, so a header value made up entirely of spaces running to the last byte of packet data caused one read at nx_packet_append_ptr. The status returned was already correct, so the over-read had no observable effect, but it was still a read past the end. Confirmed with AddressSanitizer against an exact-size buffer: heap-buffer-overflow READ of size 1 before, clean after, same NX_WEB_HTTP_NOT_FOUND in both cases. It is kept as its own commit because it predates your patch and stays independently cherry-pickable for release branches.

Built and run across all three web configurations — default_build_coverage, no_tls_build_coverage and digest_authenticate_build — 54/54 pass in each, clean under -Werror -Wall -Wextra.

One practical note

The branch history was rewritten, so if you have it checked out locally a plain git pull will conflict. git fetch && git reset --hard origin/master on your side will line it up.

Thanks again for the report. The diagnosis was accurate and clearly written, and it found something that had been in the Web add-on since its first public commit. If you spot the same pattern elsewhere in the add-ons, please do open issues the same way.

[fdesbiens]

If you are fine with the current state of the PR, I will merge it into dev. This would ship in our Q3 2026 release in September.

[COOOkies4U]

Thanks, @fdesbiens, for the detailed review of my pull request. I agree with your changes and the current state of the PR.