Files
CoreUtils/HTTPRequest.cpp
T
Brad ArantandClaude Sonnet 5 2cea81c996 Fix heap corruption from unvalidated Content-Length causing repeated production crashes
HTTPRequest::parse() passed the parsed Content-Length straight into a
bounds check and a buffer read with no validation. ZString::asInteger()
accumulates digits into a plain 32-bit int with no overflow check, so a
request claiming an absurd Content-Length (confirmed live: a classic
SSTP vulnerability-scanner probe sending "Content-Length:
18446744073709551615") silently overflowed to -1. That negative value
passed the "buffer.getLength() < contentLength" guard for any real
buffer size, then got used as a length in ZString(cursor,
contentLength) - reinterpreted as an enormous size_t, this read far
past the real buffer and corrupted the heap, crashing the process
(sometimes immediately, sometimes much later in an unrelated objects
destructor) every time this scanner traffic reached it.

This single bug was silently crash-looping barant.com (3x in ~30
hours) and the ClockPlanet server (3x over the same window) before
systemds auto-restart each time masked the outage - confirmed via
live core dump analysis (gdb bt full showing double free or
corruption (out) inside MString::~MString, called from
HTTPRequest::parse, with contentLength = -1 in the frame). Fixed by
rejecting a negative Content-Length immediately, before it is ever
used - the same throw-on-malformed-input pattern this function already
uses for a bad request line and unsupported Transfer-Encoding.

Verified live against the exact attack string on both barant.com and
the ClockPlanet server: the connection is now cleanly rejected and the
service stays up with no restart.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-10-10 07:39:46 +00:00

180 lines
7.8 KiB
C++

#include "HTTPRequest.h"
#include "Exception.h"
#include <strings.h>
namespace coreutils {
HTTPRequest::HTTPRequest(StreamReader &reader) : Parser(reader) {}
HTTPRequest::~HTTPRequest() {
delete lineParser;
delete headerParser;
delete formParser;
}
Parser::Status HTTPRequest::parse() {
MString &buffer = reader.getBuffer();
switch (state) {
case REQUEST_LINE: {
if (!lineParser)
lineParser = new LineParser(reader);
if (!checkParse(*lineParser))
return Status::INCOMPLETE;
{
// line is a view into reader's own buffer (see
// LineParser::parse()), not into lineParser itself - safe to
// delete lineParser immediately, before the parts.size()
// check below can throw. Previously this delete sat AFTER
// that check, so a malformed request line (any count other
// than exactly 3 space-separated parts - confirmed live via
// real scanner/bot traffic hitting this daily) leaked
// lineParser and left the member non-NULL. Since state is
// never advanced past REQUEST_LINE on this throw, ANY later
// reentry into this same case (state itself surviving the
// exception, e.g. if this HTTPRequest is ever driven again
// before its owning session tears down) would see a
// non-NULL lineParser and skip allocating a fresh one,
// reusing an already-completed, now-stale parser instance
// instead - a real invariant violation that a heap-
// corruption crash (munmap_chunk(): invalid pointer) was
// traced back to.
ZString line = lineParser->getLine();
delete lineParser;
lineParser = NULL;
ZString sp(" ");
std::vector<ZString> &parts = line.split(sp);
if (parts.size() != 3)
throw coreutils::Exception("HTTPRequest: malformed request line "
"(expected \"METHOD /path HTTP/1.1\").");
method = MString(parts[0]);
path = MString(parts[1]);
version = MString(parts[2]);
}
{
// Split the query string off before anything else sees `path` -
// a route table (see the APIServer plan) should never have to
// deal with a trailing "?a=1&b=2" itself.
int qpos = path.find(ZString("?"));
if (qpos != -1) {
queryString = MString(path.substring(qpos + 1));
path = MString(path.substring(0, qpos));
}
}
state = HEADERS;
return Status::DATA_AVAIL;
}
case HEADERS: {
if (!headerParser)
headerParser = new IMFHeader(reader);
// The actual throw for a malformed header (e.g. a header line
// with no ':') happens deep inside checkParse() -> IMFHeader::
// parse(), well before headerParser->getHeaders() below ever
// runs - same leaked-pointer/stale-reentry hazard as
// REQUEST_LINE above, confirmed as the other half of the same
// real crash. Cleaning up here before rethrowing keeps
// headerParser NULL on every exit path, matching the
// REQUEST_LINE fix.
bool complete;
try {
complete = checkParse(*headerParser);
} catch (...) {
delete headerParser;
headerParser = NULL;
throw;
}
if (!complete)
return Status::INCOMPLETE;
headers = headerParser->getHeaders();
delete headerParser;
headerParser = NULL;
state = BODY;
return Status::DATA_AVAIL;
}
case BODY: {
MString contentLengthHeader = getHeader("Content-Length");
if (contentLengthHeader.getLength() == 0) {
if (getHeader("Transfer-Encoding").getLength() > 0)
throw coreutils::Exception("HTTPRequest: chunked Transfer-Encoding "
"is not supported (Content-Length only).");
state = DONE;
return Status::COMPLETE;
}
int contentLength = contentLengthHeader.asInteger();
// asInteger() accumulates digits into a plain 32-bit int with no
// overflow check - a Content-Length long enough to overflow it
// (confirmed live via real scanner/bot traffic: a request for
// "/sra_{...}/' claiming "Content-Length: 18446744073709551615",
// a classic SSTP vulnerability-probe pattern) silently wraps to a
// garbage value, observed as -1. That negative value then passed
// the "(int)buffer.getLength() < contentLength" check below for
// every real buffer length (nothing is ever less than -1), so
// execution fell through to "ZString(buffer.getCursor(),
// contentLength)" with a negative length - reinterpreted as a
// huge size_t, this read far past the real buffer and corrupted
// the heap, crashing (sometimes immediately, sometimes much
// later, in a wholly unrelated MString's destructor) on every
// production apiserver this scanner traffic reached. Rejecting
// a negative Content-Length here, before it is ever used, closes
// this off at the only place that actually needs to care.
if (contentLength < 0)
throw coreutils::Exception("HTTPRequest: invalid Content-Length.");
MString contentType = getHeader("Content-Type");
if (contentType.startsWith("application/x-www-form-urlencoded") ||
contentType.startsWith("multipart/form-data")) {
if (!formParser) {
// Mirrors __jet.cpp's identical CGI-path fix: find() leaves
// contentType's own cursor at the match, and CGIFormData's
// constructor copy-constructs its own contentType member
// from whatever we pass, inheriting that same moved cursor
// - its startsWith() dispatch checks are cursor-relative
// and would silently miss (falling through to UNKNOWN)
// without the reset().
int boundaryPos = contentType.find(ZString("boundary="));
if (boundaryPos != -1)
boundary = MString(contentType.substring(boundaryPos + 9));
contentType.reset();
formParser = new CGIFormData(reader, contentType, contentLength, boundary);
}
if (!checkParse(*formParser))
return Status::INCOMPLETE;
state = DONE;
return Status::COMPLETE;
}
if ((int)buffer.getLength() < contentLength)
return Status::INCOMPLETE;
body = MString(ZString(buffer.getCursor(), contentLength));
buffer.setCursor(buffer.getCursor() + contentLength);
state = DONE;
return Status::COMPLETE;
}
case DONE:
return Status::COMPLETE;
}
throw coreutils::Exception("HTTPRequest: unreachable state.");
}
MString HTTPRequest::getMethod() { return method; }
MString HTTPRequest::getPath() { return path; }
MString HTTPRequest::getQueryString() { return queryString; }
MString HTTPRequest::getVersion() { return version; }
MString HTTPRequest::getBody() { return body; }
std::map<MString, MString> &HTTPRequest::getHeaders() { return headers; }
CGIFormData *HTTPRequest::getFormData() { return formParser; }
MString HTTPRequest::getHeader(const char *name) {
for (auto &pair : headers) {
MString key = pair.first;
if (strcasecmp(key.c_str(), name) == 0)
return pair.second;
}
return MString();
}
}