summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPaul Buetow <paul@buetow.org>2026-07-07 15:07:09 +0300
committerPaul Buetow <paul@buetow.org>2026-07-07 15:07:09 +0300
commiteb46afd5d10b0d2c95a90eade231d519415bdd80 (patch)
treebb933dfbf69be582098190115f7405ce8b61e47b
parent1f28ff5ac4a8a466113296d9510fe91f8cd13bc3 (diff)
yhttpd: fix malformed Content-Length crash and unchecked accept
Two residual stability bugs in yhttpd's own sock.cpp (ychat never had them -- yhttpd's read_http request parser is structurally different from ychat's), found while auditing for ychat engine-fix backports: 1. sock::read_http malformed Content-Length crash (a3908e1-class). read_http matched the header on the 15-char prefix "Content-Length:" (no space required) but then assumed the canonical "Content-Length: <value>" form and substr'd from index 16. Two reachable crash cases for any unauthenticated client: - bare "Content-Length:" (15 chars): substr(16, len-16) had pos > size -> std::out_of_range throw -> uncaught -> process crash. - "Content-Length:\n" (16 chars, no value): the substring was empty so the do/while digit scan read past the buffer (OOB read) until a stray '\n' in adjacent memory. Now guarded: require the space separator + a value before substr, and bound the scan to the substring length. Verified in Docker: both malformed cases close gracefully, a valid Content-Length: 0 POST still returns 200, server stays up. 2. sock::start unchecked accept(). The accept() return was used unchecked; on failure (fd == -1, e.g. EMFILE/ENFILE under fd exhaustion, EINTR) FD_SET(-1, &active_fd_set) is UB (bit-op on a negative index) and the later _create_container(-1) would read/write fd -1 (EBADF). Now bails with ACCPERR and continues on any accept error (the accept-bail half of ychat's 1c36abe, which the original yhttpd port only carried the size_t->socklen_t init of). yhttpd is not deployed to the cluster (no Helm chart/ArgoCD app); this is a build-and-verify-in-Docker project, so no deploy step. ycurses shares no socket/template engine with ychat (it is a standalone curses library demo) so nothing applies there.
-rw-r--r--yhttpd/DOCKER.md19
-rw-r--r--yhttpd/src/sock/sock.cpp30
2 files changed, 48 insertions, 1 deletions
diff --git a/yhttpd/DOCKER.md b/yhttpd/DOCKER.md
index bba0284..7375189 100644
--- a/yhttpd/DOCKER.md
+++ b/yhttpd/DOCKER.md
@@ -39,6 +39,25 @@ yhttpd shares ychat's buggy files, so most ychat fixes apply. Ported / fixed:
- **`tool::shell_command` (CGI):** replaced `popen("/bin/sh -c …")` with
`fork/execve` of the file directly (no shell), so URL-derived metacharacters
can't inject when `httpd.enablecgi=true` (dormant/off by default).
+- **`sock::read_http` malformed `Content-Length:` crash (a3908e1-class):**
+ `read_http` matched the header on the 15-char prefix `Content-Length:`
+ (no space required) but then assumed the canonical `Content-Length:
+ <value>` form and `substr`'d from index 16. Two reachable crash cases
+ leaked through for any unauthenticated client: a bare `Content-Length:`
+ (15 chars) made `substr(16, len-16)` throw `std::out_of_range` (pos > size)
+ > uncaught > process crash; `Content-Length:\n` (16 chars, no value) left
+ the substring empty so the `do/while` digit scan read past the buffer (OOB
+ read) until a stray `\n` in adjacent memory. Now guarded: require the
+ space separator + a value before `substr`, and bound the scan to the
+ substring length. (ychat's a3908e1 fixed the same malformed-input-crash
+ class in its own structurally different request parser.)
+- **`sock::start` unchecked `accept`:** the `accept()` return was used
+ unchecked; on failure (`fd == -1`, e.g. EMFILE/ENFILE under fd exhaustion,
+ EINTR) `FD_SET(-1, &active_fd_set)` is UB (bit-op on a negative index) and
+ the later `_create_container(-1)` would read/write fd -1 (EBADF). Now bails
+ with `ACCPERR` and `continue`s on any `accept` error (the accept-bail half
+ of ychat's 1c36abe, which the original yhttpd port only carried the
+ `size_t`→`socklen_t` init of).
- **`tool::replace` and the pervasive `unsigned` string positions:** yhttpd
declared many find/substr/replace positions as `unsigned` (32-bit) but
`std::string::find` returns `size_t` (64-bit) `npos`. Truncating `npos` to
diff --git a/yhttpd/src/sock/sock.cpp b/yhttpd/src/sock/sock.cpp
index 279123d..b8ce6ce 100644
--- a/yhttpd/src/sock/sock.cpp
+++ b/yhttpd/src/sock/sock.cpp
@@ -228,6 +228,24 @@ sock::read_http(socketcontainer *p_sock, char *c_zbuf, int i_buflen, int &i_post
if (s_line.compare(0, 15, "Content-Length:"))
continue;
+ // Malformed Content-Length guard. The match above is on the 15-char
+ // prefix "Content-Length:" with no space required, but the code below
+ // assumes the canonical "Content-Length: <value>" form (it substrs from
+ // index 16, skipping the space at index 15). Two crash cases leaked
+ // through:
+ // - "Content-Length:" (15 chars, no space/value): substr(16, len-16)
+ // had pos > size -> std::out_of_range throw -> uncaught -> process
+ // crash.
+ // - "Content-Length:\n" (16 chars, no value): s_content_length ended
+ // up empty, and the do/while digit scan read s_content_length[z]
+ // past the buffer (OOB read) until a stray '\n' appeared in
+ // adjacent memory.
+ // Require the space separator and at least one char of value
+ // (length > 16) before substr, and bound the scan to the substring
+ // length so it can never read past the buffer even with no newline.
+ if (s_line.length() <= 16 || s_line[15] != ' ')
+ return -1;
+
// Match found on Content-Length:... process, and then break out and get us to the promised land
s_content_length = s_line.substr( 16 /*strlen("Content-Length: ")*/,
s_line.length() - 16 /*strlen("Content-Length: ")*/);
@@ -248,7 +266,7 @@ sock::read_http(socketcontainer *p_sock, char *c_zbuf, int i_buflen, int &i_post
z++;
}
- while(ch != '\n');
+ while(z < (int)s_content_length.length() && ch != '\n');
break;
}
@@ -451,6 +469,16 @@ sock::start()
size = sizeof(clientname);
i_new_sock = accept (i_sock, (struct sockaddr *) &clientname, &size);
+ // Bail on any accept error (EMFILE/ENFILE under fd exhaustion,
+ // EINTR, etc.) instead of proceeding with i_new_sock == -1:
+ // FD_SET(-1, &active_fd_set) is UB (bit-op on a negative index) and
+ // the later _create_container(-1) would read/write fd -1 (EBADF).
+ if (i_new_sock < 0)
+ {
+ wrap::system_message(ACCPERR);
+ continue;
+ }
+
#ifdef OPENSSL
if (_main_loop_do_ssl_stuff(i_new_sock))