Merge pull request #8860 from jdgleaver/rpng-fix

(RPNG) Fix undefined behaviour when loading bad/corrupt PNG images

libretro-common/formats/image_texture.c

@@ -175,7 +175,7 @@ static bool image_texture_load_internal(
    if (!img)
       goto end;
 
-   image_transfer_set_buffer_ptr(img, type, (uint8_t*)ptr);
+   image_transfer_set_buffer_ptr(img, type, (uint8_t*)ptr, len);
 
    if (!image_transfer_start(img, type))
       goto end;

libretro-common/formats/image_transfer.c

@@ -177,13 +177,14 @@ bool image_transfer_is_valid(
 void image_transfer_set_buffer_ptr(
       void *data,
       enum image_type_enum type,
-      void *ptr)
+      void *ptr,
+      size_t len)
 {
    switch (type)
    {
       case IMAGE_TYPE_PNG:
 #ifdef HAVE_RPNG
-         rpng_set_buf_ptr((rpng_t*)data, (uint8_t*)ptr);
+         rpng_set_buf_ptr((rpng_t*)data, (uint8_t*)ptr, len);
 #endif
          break;
       case IMAGE_TYPE_JPEG:

libretro-common/formats/png/rpng.c

@@ -127,6 +127,7 @@ struct rpng
    struct idat_buffer idat_buf;
    struct png_ihdr ihdr;
    uint8_t *buff_data;
+   uint8_t *buff_end;
    uint32_t palette[256];
 };
 
@@ -921,20 +922,38 @@ error:
    return NULL;
 }
 
-static bool read_chunk_header(uint8_t *buf, struct png_chunk *chunk)
+static bool read_chunk_header(uint8_t *buf, uint8_t *buf_end, struct png_chunk *chunk)
 {
    unsigned i;
    uint8_t dword[4];
 
    dword[0] = '\0';
 
+   /* Check whether reading the header will overflow
+    * the data buffer */
+   if (buf_end - buf < 8)
+      return false;
+
    for (i = 0; i < 4; i++)
       dword[i] = buf[i];
 
    chunk->size = dword_be(dword);
 
+   /* Check whether chunk will overflow the data buffer */
+   if (buf + 8 + chunk->size > buf_end)
+      return false;
+
    for (i = 0; i < 4; i++)
-      chunk->type[i] = buf[i + 4];
+   {
+      uint8_t byte = buf[i + 4];
+
+      /* All four bytes of the chunk type must be
+       * ASCII letters (codes 65-90 and 97-122) */
+      if ((byte < 65) || ((byte > 90) && (byte < 97)) || (byte > 122))
+         return false;
+
+      chunk->type[i] = byte;
+   }
 
    return true;
 }
@@ -968,8 +987,12 @@ bool rpng_iterate_image(rpng_t *rpng)
    chunk.type[0]          = 0;
    chunk.data             = NULL;
 
-   if (!read_chunk_header(buf, &chunk))
+   /* Check whether data buffer pointer is valid */
-      return false;
+   if (buf > rpng->buff_end)
+      goto error;
+
+   if (!read_chunk_header(buf, rpng->buff_end, &chunk))
+      goto error;
 
 #if 0
    for (i = 0; i < 4; i++)
@@ -1072,6 +1095,10 @@ bool rpng_iterate_image(rpng_t *rpng)
 
    rpng->buff_data += chunk.size + 12;
 
+   /* Check whether data buffer pointer is valid */
+   if (rpng->buff_data > rpng->buff_end)
+      goto error;
+
    return true;
 
 error:
@@ -1151,6 +1178,11 @@ bool rpng_start(rpng_t *rpng)
    if (!rpng)
       return false;
 
+   /* Check whether reading the header will overflow
+    * the data buffer */
+   if (rpng->buff_end - rpng->buff_data < 8)
+      return false;
+
    header[0] = '\0';
 
    for (i = 0; i < 8; i++)
@@ -1169,21 +1201,21 @@ bool rpng_is_valid(rpng_t *rpng)
    if (!rpng)
       return false;
 
-   if (rpng->has_ihdr)
+   /* A valid PNG image must contain an IHDR chunk,
-      return true;
+    * one or more IDAT chunks, and an IEND chunk */
-   if (rpng->has_idat)
+   if (rpng->has_ihdr && rpng->has_idat && rpng->has_iend)
-      return true;
-   if (rpng->has_iend)
       return true;
+
    return false;
 }
 
-bool rpng_set_buf_ptr(rpng_t *rpng, void *data)
+bool rpng_set_buf_ptr(rpng_t *rpng, void *data, size_t len)
 {
-   if (!rpng)
+   if (!rpng || (len < 1))
       return false;
 
    rpng->buff_data = (uint8_t*)data;
+   rpng->buff_end  = rpng->buff_data + (len - 1);
 
    return true;
 }

libretro-common/include/formats/image.h

@@ -84,7 +84,8 @@ bool image_transfer_start(void *data, enum image_type_enum type);
 void image_transfer_set_buffer_ptr(
       void *data,
       enum image_type_enum type,
-      void *ptr);
+      void *ptr,
+      size_t len);
 
 int image_transfer_process(
       void *data,

libretro-common/include/formats/rpng.h

@@ -38,7 +38,7 @@ rpng_t *rpng_init(const char *path);
 
 bool rpng_is_valid(rpng_t *rpng);
 
-bool rpng_set_buf_ptr(rpng_t *rpng, void *data);
+bool rpng_set_buf_ptr(rpng_t *rpng, void *data, size_t len);
 
 rpng_t *rpng_alloc(void);
 

tasks/task_image.c

@@ -88,7 +88,12 @@ static int task_image_process(
       unsigned *width,
       unsigned *height)
 {
-   int retval = image_transfer_process(
+   int retval;
+
+   if (!image_transfer_is_valid(image->handle, image->type))
+      return IMAGE_PROCESS_ERROR;
+
+   retval = image_transfer_process(
          image->handle,
          image->type,
          &image->ti.pixels, image->size, width, height);
@@ -131,8 +136,9 @@ static int task_image_iterate_process_transfer(struct nbio_image_handle *image)
 
    for (i = 0; i < image->processing_pos_increment; i++)
    {
-      if ((retval = task_image_process(image,
+      retval = task_image_process(image, &width, &height);
-               &width, &height) != IMAGE_PROCESS_NEXT))
+
+      if (retval != IMAGE_PROCESS_NEXT)
          break;
    }
 
@@ -192,7 +198,7 @@ static int cb_nbio_image_thumbnail(void *data, size_t len)
 
    ptr                             = nbio_get_ptr(nbio->handle, &len);
 
-   image_transfer_set_buffer_ptr(image->handle, image->type, ptr);
+   image_transfer_set_buffer_ptr(image->handle, image->type, ptr, len);
 
    image->size                     = len;
    image->pos_increment            = (len / 2) ?
@@ -231,7 +237,8 @@ bool task_image_load_handler(retro_task_t *task)
             if (image->handle && image->cb)
             {
                size_t len = 0;
-               image->cb(nbio, len);
+               if (image->cb(nbio, len) == -1)
+                  return false;
             }
             if (image->is_blocking_on_processing)
                image->status = IMAGE_STATUS_PROCESS_TRANSFER;
@@ -253,7 +260,8 @@ bool task_image_load_handler(retro_task_t *task)
             if (image->handle && image->cb)
             {
                size_t len = 0;
-               image->cb(nbio, len);
+               if (image->cb(nbio, len) == -1)
+                  return false;
             }
             if (!image->is_finished)
                break;
@@ -261,7 +269,7 @@ bool task_image_load_handler(retro_task_t *task)
    }
 
    if (     nbio->is_finished
-         && (image && image->is_finished )
+         && (image && image->is_finished)
          && (!task_get_cancelled(task)))
    {
       struct texture_image *img = (struct texture_image*)malloc(sizeof(struct texture_image));