http: do not search outside the header value #7606
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Measurements ⏳
|
mmcgee-jump
reviewed
Dec 18, 2025
src/waltz/http/fd_http_server.c
Outdated
| for( ulong i=0UL; i<num_headers; i++ ) { | ||
| if( FD_LIKELY( headers[ i ].name_len==22UL && !strncasecmp( headers[ i ].name, "Sec-WebSocket-Protocol", 22UL ) && strstr( headers[ i ].value, "compress-zstd" ) ) ) { | ||
| // Use `memmem` as the header is not guaranteed to be null terminated | ||
| // `memmem` also matches "randomprefix_compress-zstd_randomsuffix" but that's fine, we don't want to fully parse the header value |
Contributor
There was a problem hiding this comment.
Wouldn't it be better to just check value_len == 13 and use strncasecmp here too?
Contributor
Author
There was a problem hiding this comment.
I was thinking that there might be cases where there are multiple protocols specified in the header, but the existing code and documentation quite explicitly say that this should just be a single value.
Changed to use value_len == 13 and strncmp, because the header field value is usually case-sensitive.
Header values in picohttp are not null-terminated so `strstr` would happily search outside the header value and exceed the length of the header value. This could lead to DoS in artifical circumstances which don't apply to fd in practice. There is always a null byte in memory _somewhere_ after the header value even if it is not actually part of the header value. This is because `fd_http_server_ws_frame`s are allocated after the request buffer that contains the headers and the ws_frames contain four bytes of padding that is zero, because the whole memory we're operating on, has been allocated with `mmap` anonymously which zeroes the memory QED.
intrigus-lgtm
force-pushed
the
intrigus/fix/http-server-ws-strstr-oob
branch
from
05a7711 to
b06639f
Compare
Performance Measurements ⏳
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Header values in picohttp are not null-terminated so
strstrwould happily search outside the header value and exceed the length of the header value.This could lead to DoS in artifical circumstances which don't apply to fd in practice. There is always a null byte in memory somewhere after the header value even if it is not actually part of the header value. This is because
fd_http_server_ws_frames are allocated after the request buffer that contains the headers and the ws_frames contain four bytes of padding that is zero, because the whole memory we're operating on, has been allocated withmmapanonymously which zeroes the memory QED.Drive-by fix: the
Upgradeheader field's token has to contain the value "websocket", but should be treated as an ASCII case-insensitive value: https://datatracker.ietf.org/doc/html/rfc6455#section-4.2.1