CVE-2020-13111: Out-of-bounds Write (CWE-787)
value: NaviServer 4.99.4 to 4.99.19 allows denial of service due to the nsd/driver.c ChunkedDecode function not properly validating the length of a chunk. A remote attacker can craft a chunked-transfer request that will result in a negative value being passed to memmove via the size parameter, causing the process to crash.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 7.5
Published: 2020-05-16T15:15Z
Repository: https://bitbucket.org/naviserver/naviserver
Language: Other
Files changed: nsd/driver.c, tests/http_chunked.test
Commit date: 2020-05-16 13:34:36 +0200
Commit message: fix for bug https://sourceforge.net/p/naviserver/bugs/89/

A negative value provided as chunk encoding length led to a potential crash.
A test case for this case was added to the regression test.

Diff: diff --git a/nsd/driver.c b/nsd/driver.c
index 30b001cf..418ffd6d 100644
--- a/nsd/driver.c
+++ b/nsd/driver.c
@@ -285,11 +285,11 @@ static void PollReset(PollData *pdata)
     NS_GNUC_NONNULL(1);
 static NS_POLL_NFDS_TYPE PollSet(PollData *pdata, NS_SOCKET sock, short type, const Ns_Time *timeoutPtr)
     NS_GNUC_NONNULL(1);
 static int PollWait(const PollData *pdata, int timeout)
     NS_GNUC_NONNULL(1);
-static bool ChunkedDecode(Request *reqPtr, bool update)
+static SockState ChunkedDecode(Request *reqPtr, bool update)
     NS_GNUC_NONNULL(1);
 static WriterSock *WriterSockRequire(const Conn *connPtr)
     NS_GNUC_NONNULL(1);
 static void WriterSockRelease(WriterSock *wrSockPtr)
     NS_GNUC_NONNULL(1);
@@ -3372,62 +3372,70 @@ SockClose(Sock *sockPtr, int keep)
  *      repeatedly and with incomplete input and overwrites the buffer
  *      with the decoded data optionally. The decoded data is always
  *      shorter than the encoded one.
  *
  * Results:
- *      NS_TRUE when chunk was complete, NS_FALSE otherwise
+ *      SOCK_READY when chunk was complete, SOCK_MORE when more data is
+ *      requried, or some error condition.
  *
  * Side effects:
- *      updates the buffer if update is true (and adjusts reqPtr->chunkWriteOff)
- *      updates always reqPtr->chunkStartOff to allow incremental operations
+ *      Updates the buffer if update is true (and adjusts
+ *      reqPtr->chunkWriteOff).  Updates always reqPtr->chunkStartOff to allow
+ *      incremental operations.
  *
  *----------------------------------------------------------------------
  */
-static bool
+static SockState
 ChunkedDecode(Request *reqPtr, bool update)
 {
     const Tcl_DString *bufPtr;
     const char        *end, *chunkStart;
-    bool              success = NS_TRUE;
+    SockState         result = SOCK_READY;
 
     NS_NONNULL_ASSERT(reqPtr != NULL);
 
     bufPtr = &reqPtr->buffer;
     end = bufPtr->string + bufPtr->length;
     chunkStart = bufPtr->string + reqPtr->chunkStartOff;
 
     while (reqPtr->chunkStartOff <  (size_t)bufPtr->length) {
         char   *p = strstr(chunkStart, "\r\n");
-        size_t  chunk_length;
+        long    chunkLength;
 
         if (p == NULL) {
             Ns_Log(DriverDebug, "ChunkedDecode: chunk did not find end-of-line");
-            success = NS_FALSE;
+            result = SOCK_MORE;
             break;
         }
 
         *p = '\0';
-        chunk_length = (size_t)strtol(chunkStart, NULL, 16);
+        chunkLength = strtol(chunkStart, NULL, 16);
+        *p = '\r';
+        if (chunkLength < 0) {
+            Ns_Log(Warning, "ChunkedDecode: negative chunk length");
+            result = SOCK_BADREQUEST;
+            break;
+        }
         *p = '\r';
 
-        if (p + 2 + chunk_length > end) {
+        if (p + 2 + chunkLength > end) {
             Ns_Log(DriverDebug, "ChunkedDecode: chunk length past end of buffer");
-            success = NS_FALSE;
+            result = SOCK_MORE;
             break;
         }
         if (update) {
             char *writeBuffer = bufPtr->string + reqPtr->chunkWriteOff;
 
-            memmove(writeBuffer, p + 2, chunk_length);
-            reqPtr->chunkWriteOff += chunk_length;
-            *(writeBuffer + chunk_length) = '\0';
+            memmove(writeBuffer, p + 2, (size_t)chunkLength);
+            reqPtr->chunkWriteOff += (size_t)chunkLength;
+            *(writeBuffer + chunkLength) = '\0';
         }
-        reqPtr->chunkStartOff += (size_t)(p - chunkStart) + 4u + chunk_length;
+        reqPtr->chunkStartOff += (size_t)(p - chunkStart) + 4u + (size_t)chunkLength;
         chunkStart = bufPtr->string + reqPtr->chunkStartOff;
     }
 
-    return success;
+    return result;
 }
 
 
 /*
  *----------------------------------------------------------------------
@@ -4112,38 +4120,41 @@ SockParse(Sock *sockPtr)
     assert(reqPtr->request.line != NULL);
 
     /*
      * Check if all content has arrived.
      */
-    Ns_Log(Dev, "=== length < avail (length %" PRIuz
+    Ns_Log(Debug, "=== length < avail (length %" PRIuz
            ", avail %" PRIuz ") tfd %d tfile %p chunkStartOff %" PRIuz,
            reqPtr->length, reqPtr->avail, sockPtr->tfd,
            (void *)sockPtr->tfile, reqPtr->chunkStartOff);
 
     if (reqPtr->chunkStartOff != 0u) {
         /*
          * Chunked encoding was provided.
          */
-        bool   complete;
-        size_t currentContentLength;
+        SockState chunkState;
+        size_t    currentContentLength;
 
-        complete = ChunkedDecode(reqPtr, NS_TRUE);
+        chunkState = ChunkedDecode(reqPtr, NS_TRUE);
         currentContentLength = reqPtr->chunkWriteOff - reqPtr->coff;
 
         /*
          * A chunk might be complete, but it might not be the last
          * chunk from the client. The best thing would be to be able
          * to read until EOF here. In cases, where the (optional)
          * "expectedLength" was provided by the client, we terminate
          * depending on that information
          */
-        if ((!complete)
+        if ((chunkState == SOCK_MORE)
             || (reqPtr->expectedLength != 0u && currentContentLength < reqPtr->expectedLength)) {
             /*
              * ChunkedDecode wants more data.
              */
             return SOCK_MORE;
+
+        } else if (chunkState != SOCK_READY) {
+            return chunkState;
         }
         /*
          * ChunkedDecode has enough data.
          */
         reqPtr->length = (size_t)currentContentLength;
diff --git a/tests/http_chunked.test b/tests/http_chunked.test
index 71deed41..e5e57fd9 100644
--- a/tests/http_chunked.test
+++ b/tests/http_chunked.test
@@ -128,12 +128,31 @@ test http_chunked-2.3 {
         GET /tclchunked
 } -cleanup {
     ns_unregister_op GET /tclchunked
 } -result {200 {} 5 01234}
 
+test http_chunked-2.4 {
 
-test http_chunked-3-1 {
+    POST request with invalid chunked content
+
+} -constraints serverListen -setup {
+    ns_register_proc PUT /tclchunked {
+        set content [ns_conn content]
+        #ns_log notice "PUT received <$content>"
+        ns_return 200 text/plain $content
+    }
+} -body {
+    nstest::http-0.9 -http 1.1 \
+        -omitcontentlength 1 \
+        -setheaders {Transfer-Encoding Chunked} \
+        -getheaders {Transfer-Encoding Content-Length} -getbody 1 \
+        PUT /tclchunked "-1\r\nchunk\r\n0\r\n\r\n"
+} -cleanup {
+    ns_unregister_op PUT /tclchunked
+} -match glob -result {200 {} 0 {HTTP/1.0 400 Bad Request*}}
+
+test http_chunked-3.1 {
 
     PUT with Transfer-Encoding Chunked and unknown Content-Length (but
     provided X-Expected-Entity-Length). This is the format that WebDAV
     client of macOS uses (via Finder).
 

Vulnerable code: static bool ChunkedDecode(Request *reqPtr, bool update)
 *      NS_TRUE when chunk was complete, NS_FALSE otherwise
 *      updates the buffer if update is true (and adjusts reqPtr->chunkWriteOff)
 *      updates always reqPtr->chunkStartOff to allow incremental operations
static bool
    bool              success = NS_TRUE;
        size_t  chunk_length;
            success = NS_FALSE;
        chunk_length = (size_t)strtol(chunkStart, NULL, 16);
        if (p + 2 + chunk_length > end) {
            success = NS_FALSE;
            memmove(writeBuffer, p + 2, chunk_length);
            reqPtr->chunkWriteOff += chunk_length;
            *(writeBuffer + chunk_length) = '\0';
        reqPtr->chunkStartOff += (size_t)(p - chunkStart) + 4u + chunk_length;
    return success;
    Ns_Log(Dev, "=== length < avail (length %" PRIuz
        bool   complete;
        size_t currentContentLength;
        complete = ChunkedDecode(reqPtr, NS_TRUE);
        if ((!complete)
test http_chunked-3-1 {
Fixed code: static SockState ChunkedDecode(Request *reqPtr, bool update)
 *      SOCK_READY when chunk was complete, SOCK_MORE when more data is
 *      requried, or some error condition.
 *      Updates the buffer if update is true (and adjusts
 *      reqPtr->chunkWriteOff).  Updates always reqPtr->chunkStartOff to allow
 *      incremental operations.
static SockState
    SockState         result = SOCK_READY;
        long    chunkLength;
            result = SOCK_MORE;
        chunkLength = strtol(chunkStart, NULL, 16);
        *p = '\r';
        if (chunkLength < 0) {
            Ns_Log(Warning, "ChunkedDecode: negative chunk length");
            result = SOCK_BADREQUEST;
            break;
        }
        if (p + 2 + chunkLength > end) {
            result = SOCK_MORE;
            memmove(writeBuffer, p + 2, (size_t)chunkLength);
            reqPtr->chunkWriteOff += (size_t)chunkLength;
            *(writeBuffer + chunkLength) = '\0';
        reqPtr->chunkStartOff += (size_t)(p - chunkStart) + 4u + (size_t)chunkLength;
    return result;
    Ns_Log(Debug, "=== length < avail (length %" PRIuz
        SockState chunkState;
        size_t    currentContentLength;
        chunkState = ChunkedDecode(reqPtr, NS_TRUE);
        if ((chunkState == SOCK_MORE)

        } else if (chunkState != SOCK_READY) {
            return chunkState;
test http_chunked-2.4 {
    POST request with invalid chunked content

} -constraints serverListen -setup {
    ns_register_proc PUT /tclchunked {
        set content [ns_conn content]
        #ns_log notice "PUT received <$content>"
        ns_return 200 text/plain $content
    }
} -body {
    nstest::http-0.9 -http 1.1 \
        -omitcontentlength 1 \
        -setheaders {Transfer-Encoding Chunked} \
        -getheaders {Transfer-Encoding Content-Length} -getbody 1 \
        PUT /tclchunked "-1\r\nchunk\r\n0\r\n\r\n"
} -cleanup {
    ns_unregister_op PUT /tclchunked
} -match glob -result {200 {} 0 {HTTP/1.0 400 Bad Request*}}

test http_chunked-3.1 {