From af29fd087527e5dc5a87691fe1025556e7358022 Mon Sep 17 00:00:00 2001 From: Scott Claiborne Date: Thu, 11 Jun 2026 15:41:24 -0700 Subject: [PATCH] Guard server receive window against rollover and bound debug client copy The server-side HttpShiftReceivePointer decremented MaxReceiveLength without a rollover check, so an accumulated request larger than the receive buffer wrapped the UDINT and let the TCP layer overrun the buffer. Mirror the client-side guard: refuse the shift, raise LLHTTP_ERR_PACKET_SIZE_TOO_BIG, and drop the connection. The end-of-cycle debug copy read sizeof(internal.clients) from pClients, which is allocated with only numClients elements, reading past the allocation whenever numClients < LLHTTP_MAX_NUM_CLIENTS. Bound the copy by both sizes. Co-Authored-By: Claude Fable 5 --- src/Ar/LLHttp/HttpServer.c | 23 +++++++--- test/tests.cpp | 92 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 5 deletions(-) diff --git a/src/Ar/LLHttp/HttpServer.c b/src/Ar/LLHttp/HttpServer.c index 3dfb8fb..f08b3a4 100644 --- a/src/Ar/LLHttp/HttpServer.c +++ b/src/Ar/LLHttp/HttpServer.c @@ -32,9 +32,15 @@ #define min(a,b) (((a)<(b))?(a):(b)) -void HttpShiftReceivePointer(LLHttpServerInternalClient_typ* client, unsigned long bytes) { - client->tcpStream.IN.PAR.pReceiveData += bytes; - client->tcpStream.IN.PAR.MaxReceiveLength -= bytes; // TODO: Dont allow rollover +BOOL HttpShiftReceivePointer(LLHttpServerInternalClient_typ* client, unsigned long bytes) { + if( ( (int)client->tcpStream.IN.PAR.MaxReceiveLength - bytes ) > 0 ) { + client->tcpStream.IN.PAR.pReceiveData += bytes; + client->tcpStream.IN.PAR.MaxReceiveLength -= bytes; + return 1; + } + else { + return 0; + } } void HttpResetReceivePointer(LLHttpServerInternalClient_typ* client) { client->tcpStream.IN.PAR.pReceiveData = client->pReceiveData; @@ -185,7 +191,13 @@ void LLHttpServer(LLHttpServer_typ* t) { // TODO: Handle partial packets... // Shift pointer past the newly received data so the next segment is appended - HttpShiftReceivePointer(client, client->tcpStream.OUT.ReceivedDataLength); + if(HttpShiftReceivePointer(client, client->tcpStream.OUT.ReceivedDataLength) == 0) { + // Request does not fit in the receive buffer. Drop the connection like the + // client does, rather than letting MaxReceiveLength roll over and the TCP + // layer overrun the buffer + HttpServerSetError(t, client, LLHTTP_ERR_PACKET_SIZE_TOO_BIG); + HttpDisconnect(client); + } } else if(client->parser.error) { HttpServerSetError(t, client, client->parser.errorId); @@ -323,7 +335,8 @@ void LLHttpServer(LLHttpServer_typ* t) { } - memcpy(&t->internal.clients, t->internal.pClients, sizeof(t->internal.clients)); // For debug + // For debug. pClients only holds numClients elements, so copy no more than that (and no more than the debug array holds) + memcpy(&t->internal.clients, t->internal.pClients, min(t->internal.numClients, LLHTTP_MAX_NUM_CLIENTS) * sizeof(LLHttpServerInternalClient_typ)); // Set output t->ident = &t->internal.api; diff --git a/test/tests.cpp b/test/tests.cpp index 7a40954..4379734 100644 --- a/test/tests.cpp +++ b/test/tests.cpp @@ -435,4 +435,96 @@ TEST_CASE( "Test HTTP Server POST body split across TCP segments", "[LLHttp]") { CHECK_THAT(serverCallbackBody, Catch::Matchers::Equals(bodySegment)); CHECK_THAT((char*)&client->pCurrentRequest->contentStart, Catch::Matchers::Equals(bodySegment)); +} + +TEST_CASE( "Test HTTP Server oversized request does not roll over receive window", "[LLHttp]") { + + LLHttpServer_typ server = {}; + + // A request whose body can never fit in the receive buffer. The server should error + // out with LLHTTP_ERR_PACKET_SIZE_TOO_BIG instead of letting MaxReceiveLength roll + // over (UDINT) and the TCP layer overrun the buffer + const char* headerSegment = + "POST /api/data HTTP/1.1\r\n"\ + "Host: plc.local\r\n"\ + "content-type: application/octet-stream\r\n"\ + "content-length: 10000000\r\n"\ + "\r\n"; + + server.enable = true; + server.numClients = LLHTTP_MAX_NUM_CLIENTS; + server.bufferSize = 2000; + + // First cycle initializes internals and allocates the client receive buffers + LLHttpServer(&server); + + LLHttpServerInternalClient_typ* client = &server.internal.pClients[0]; + + // Preload the header segment so it is in place when the stubbed connection manager + // accepts the incoming connection + strcpy((char*)client->pReceiveData, headerSegment); + client->tcpStream.Internal.FUB.Receive.recvlen = strlen(headerSegment); + + int cycles = 0; + while(!client->connected && ++cycles < 10) { + LLHttpServer(&server); + } + REQUIRE(client->connected == 1); + CHECK(server.error == false); + CHECK(client->parser.partialContent == true); + + // Keep feeding body segments until the buffer is exhausted. Like the real TCP layer, + // never deliver more than the advertised MaxReceiveLength in one segment + cycles = 0; + while(!server.error && ++cycles < 1000) { + UDINT chunk = client->tcpStream.IN.PAR.MaxReceiveLength < 1000 ? client->tcpStream.IN.PAR.MaxReceiveLength : 1000; + memset((char*)client->tcpStream.IN.PAR.pReceiveData, 'a', chunk); + client->tcpStream.Internal.FUB.Receive.recvlen = chunk; + LLHttpServer(&server); + } + + CHECK(server.error == true); + CHECK(server.errorId == LLHTTP_ERR_PACKET_SIZE_TOO_BIG); + CHECK(client->errorId == LLHTTP_ERR_PACKET_SIZE_TOO_BIG); + CHECK(client->connected == 0); // Connection is dropped, like the client side does + + // The receive window must never extend past the allocated buffer + CHECK(client->tcpStream.IN.PAR.MaxReceiveLength < client->receiveDataSize); + CHECK(client->tcpStream.IN.PAR.pReceiveData + client->tcpStream.IN.PAR.MaxReceiveLength + <= client->pReceiveData + client->receiveDataSize); + +} + +TEST_CASE( "Test HTTP Server debug client copy respects allocation size", "[LLHttp]") { + + LLHttpServer_typ server = {}; + + server.enable = true; + server.numClients = 2; // Fewer than the debug array holds (LLHTTP_MAX_NUM_CLIENTS) + server.bufferSize = 2000; + + // First cycle initializes internals; pClients is allocated with only numClients elements + LLHttpServer(&server); + + // Plant a sentinel in the debug array beyond numClients. If the debug copy reads a full + // sizeof(clients) from the 2-element allocation, it reads past the heap block and + // overwrites the sentinel with whatever lives there + memset(&server.internal.clients[2], 0xAA, sizeof(LLHttpServerInternalClient_typ) * (LLHTTP_MAX_NUM_CLIENTS - 2)); + + LLHttpServer(&server); + + // The first numClients elements mirror the allocation... + CHECK(memcmp(&server.internal.clients[0], server.internal.pClients, sizeof(LLHttpServerInternalClient_typ) * 2) == 0); + + // ...and elements beyond numClients are untouched + const unsigned char* sentinel = (const unsigned char*)&server.internal.clients[2]; + bool sentinelIntact = true; + for(size_t k = 0; k < sizeof(LLHttpServerInternalClient_typ) * (LLHTTP_MAX_NUM_CLIENTS - 2); k++) { + if(sentinel[k] != 0xAA) { + sentinelIntact = false; + break; + } + } + CHECK(sentinelIntact); + } \ No newline at end of file