diff options
| -rw-r--r-- | yhttpd/DOCKER.md | 19 | ||||
| -rw-r--r-- | yhttpd/src/sock/sock.cpp | 30 |
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)) |