remove_chunked_transfer_coding(): Reject invalid input sooner

Prevents invalid reads in case of corrupt input.
Bug discovered with alf-fuzz and ASAN.

diff --git a/filters.c b/filters.c
index 4b4f33e..d425e3c 100644 (file)
--- a/filters.c
+++ b/filters.c
@@ -1,4 +1,4 @@
-const char filters_rcs[] = "$Id: filters.c,v 1.195 2015/12/27 12:46:34 fabiankeil Exp $";
+const char filters_rcs[] = "$Id: filters.c,v 1.196 2015/12/27 12:53:39 fabiankeil Exp $";

 /*********************************************************************
  *
  * File        :  $Source: /cvsroot/ijbswa/current/filters.c,v $
 /*********************************************************************
  *
  * File        :  $Source: /cvsroot/ijbswa/current/filters.c,v $


@@ -2053,6 +2053,7 @@ static jb_err remove_chunked_transfer_coding(char *buffer, size_t *size)
    size_t newsize = 0;
    unsigned int chunksize = 0;
    char *from_p, *to_p;
    size_t newsize = 0;
    unsigned int chunksize = 0;
    char *from_p, *to_p;

+   const char *end_of_buffer = buffer + *size;

 
    assert(buffer);
    from_p = to_p = buffer;
 
    assert(buffer);
    from_p = to_p = buffer;


@@ -2065,27 +2066,62 @@ static jb_err remove_chunked_transfer_coding(char *buffer, size_t *size)
 
    while (chunksize > 0U)
    {
 
    while (chunksize > 0U)
    {

+      /*
+       * If the chunk-size is valid, we should have at least
+       * chunk-size bytes of chunk-data and five bytes of
+       * meta data (chunk-size, CRLF, CRLF) left in the buffer.
+       */
+      if (chunksize + 5 >= *size - newsize)
+      {
+         log_error(LOG_LEVEL_ERROR,
+            "Chunk size %u exceeds buffered data left. "
+            "Already digested %u of %u buffered bytes.",
+            chunksize, (unsigned int)newsize, (unsigned int)*size);
+         return JB_ERR_PARSE;
+      }
+
+      /*
+       * Skip the chunk-size, the optional chunk-ext and the CRLF
+       * that is supposed to be located directly before the start
+       * of chunk-data.
+       */

       if (NULL == (from_p = strstr(from_p, "\r\n")))
       {
          log_error(LOG_LEVEL_ERROR, "Parse error while stripping \"chunked\" transfer coding");
          return JB_ERR_PARSE;
       }
       if (NULL == (from_p = strstr(from_p, "\r\n")))
       {
          log_error(LOG_LEVEL_ERROR, "Parse error while stripping \"chunked\" transfer coding");
          return JB_ERR_PARSE;
       }

+      from_p += 2;

 
 

-      if (chunksize >= *size - newsize)
+      /*
+       * The previous strstr() does not enforce chunk-validity
+       * and is sattisfied as long a CRLF is left in the buffer.
+       *
+       * Make sure the bytes we consider chunk-data are within
+       * the valid range.
+       */
+      if (from_p + chunksize >= end_of_buffer)

       {
          log_error(LOG_LEVEL_ERROR,
       {
          log_error(LOG_LEVEL_ERROR,

-            "Chunk size %u exceeds buffered data left. "
-            "Already digested %u of %u buffered bytes.",
-            chunksize, (unsigned int)newsize, (unsigned int)*size);
+            "End of chunk is beyond the end of the buffer.");

          return JB_ERR_PARSE;
       }
          return JB_ERR_PARSE;
       }

-      newsize += chunksize;
-      from_p += 2;

 
       memmove(to_p, from_p, (size_t) chunksize);
 
       memmove(to_p, from_p, (size_t) chunksize);

+      newsize += chunksize;

       to_p = buffer + newsize;
       to_p = buffer + newsize;

-      from_p += chunksize + 2;
+      from_p += chunksize;

 
 

+      /*
+       * Not merging this check with the previous one allows us
+       * to keep chunks without trailing CRLF. It's not clear
+       * if we actually have to care about those, though.
+       */
+      if (from_p + 2 >= end_of_buffer)
+      {
+         log_error(LOG_LEVEL_ERROR, "Not enough room for trailing CRLF.");
+         return JB_ERR_PARSE;
+      }
+      from_p += 2;

       if (sscanf(from_p, "%x", &chunksize) != 1)
       {
          log_error(LOG_LEVEL_INFO, "Invalid \"chunked\" transfer encoding detected and ignored.");
       if (sscanf(from_p, "%x", &chunksize) != 1)
       {
          log_error(LOG_LEVEL_INFO, "Invalid \"chunked\" transfer encoding detected and ignored.");