restconf: wait for complete HTTP header before parsing#676
Conversation
hoxnox
force-pushed
the
fix-restconf-partial-header-667
branch
from
190d708 to
eb57d10
Compare
olofhagsand
left a comment
olofhagsand
left a comment
There was a problem hiding this comment.
Logic looks correct, had a minor style comment, a proposal for a timer.
Also, can one make a test for this case, ie that exercises the code?
It would mean sending a partial message, waiting, and then sending the rest, which may be non-trivial?
| * to reject with 400. Need >= 8 bytes (longest is "OPTIONS ") to | ||
| * classify; until then, assume incomplete and wait. */ | ||
| { | ||
| const char *inbuf = cbuf_get(sd->sd_inbuf); |
There was a problem hiding this comment.
Clixon convention requires all local variables declared at the top of the function, not in an inner block mid-function. The
inner {} block was added specifically to work around this. These should be hoisted to the function's variable declarations.
See CONTRIBUTING.md
| strncmp(inbuf, "PATCH ", 6) == 0 || | ||
| strncmp(inbuf, "DELETE ", 7) == 0; | ||
| if (looks_http && strstr(inbuf, "\r\n\r\n") == NULL){ | ||
| goto ok; |
There was a problem hiding this comment.
There should probably be a timer to avoid unbound waits on this session?
hoxnox
force-pushed
the
fix-restconf-partial-header-667
branch
from
eb57d10 to
c133150
Compare
|
I've added test and resolved all remarks. |
|
There is a problem with nc vs netcat on linux vs musl/alpine vs freebsd, I had those problems before, see test/lib.sh |
|
corrected
|
|
corrected tests |
|
test timing adjusted |
|
Hey, I understand the test might be difficult to do, we could pass it if you do some ad-hoc testing for this specific thing. |
All tests is green. Everything is ready to merge. |
|
My bad, I was looking at an earlier run |
|
Is there anything else I need to do for that PR? |
|
I will look at it soon |
olofhagsand
left a comment
olofhagsand
left a comment
There was a problem hiding this comment.
Consider squashing the history into a single commit
(Apologies for the delay)
| strncmp(inbuf, "PATCH ", 6) == 0 || | ||
| strncmp(inbuf, "DELETE ", 7) == 0; | ||
| if (looks_http && strstr(inbuf, "\r\n\r\n") == NULL) | ||
| if (looks_http && strstr(inbuf, "\r\n\r\n") == NULL){ |
There was a problem hiding this comment.
strstr works on null-terminated strings, but inbuf may not be, use memmem instead
| } | ||
| rsock = rc->rc_socket; | ||
| clixon_debug(CLIXON_DBG_RESTCONF, "%s", rsock->rs_description?rsock->rs_description:""); | ||
| if (restconf_header_timer_unreg(rc) < 0) |
There was a problem hiding this comment.
May leak an open fd, place after close (or do close even if unreg fails)
hoxnox
force-pushed
the
fix-restconf-partial-header-667
branch
from
28eccef to
f521ad4
Compare
Large HTTP headers (e.g. JWT Bearer tokens >2KB) may be fragmented across multiple TCP segments. The current code calls clixon_http1_parse_string() on every read and, if parsing fails, uses SSL_pending() / clixon_event_poll() to decide whether more data is coming. This check is racy: when the next TCP segment has not yet arrived in the kernel buffer (common over WAN links with >1ms inter-packet delay), poll returns 0 and a valid request is rejected with 400 Bad Request / malformed-message. Fix this by checking for the HTTP header terminator (\r\n\r\n, located with memmem() since the buffer is counted rather than null-terminated) before attempting to parse. If the terminator is not present, return to the event loop; the next readable event will append the remaining bytes to sd_inbuf and parsing will succeed on the complete header. To preserve immediate rejection of non-HTTP input (e.g. TLS handshake bytes sent to an HTTP port - test_restconf.sh test 15), only delay parsing when the request line begins with one of the RESTCONF-accepted methods: OPTIONS, HEAD, GET, POST, PUT, PATCH, DELETE. Anything else falls through to the parser for a proper 400. The wait is bounded by a one-shot timer (RESTCONF_HEADER_TIMEOUT_S = 10s) registered when parsing is first deferred and cancelled when the header parses successfully or the connection is closed; on expiry the connection is closed via restconf_close_ssl_socket(). Add test/test_restconf_partial_header.sh covering: a >2KB request split across two writes with a delay in between (must return 200, not 400), TLS handshake bytes on an HTTP port (must still get an immediate 400), and a stalled partial header (must be closed by the header timeout). Fixes clicon#667
|
done |
|
Thanks, I noticed memmem is non-posix, sorry, I will merge it and fix that later, |
2 participants
Large HTTP headers (e.g. JWT Bearer tokens >2KB) may be fragmented across multiple TCP segments. The current code calls clixon_http1_parse_string() on every read and, if parsing fails, uses SSL_pending() / clixon_event_poll() to decide whether more data is coming. This check is racy: when the next TCP segment hasn't yet arrived in the kernel buffer (common over WAN links with
Fix this by checking for the HTTP header terminator (\r\n\r\n) before attempting to parse. If the terminator is not present, return to the event loop; the next readable event will append the remaining bytes to sd_inbuf and parsing will succeed on the complete header.
Fixes #667