CVE-2020-13111: Improper Input Validation (CWE-20)
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 receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
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 {