refactor: Clean up the checking of chunk lengths and allocation limits

Internal changes only.

Move chunk length checks to fewer places:

Change `png_struct::user_chunk_malloc_max` to always have a non-zero
value, in order to avoid the need to check for zero in multiple places.

Add `png_chunk_max(png_ptr)`, a function-like macro defined in pngpriv.h
which expresses all the previous checks on the various USER_LIMITS and
system limitations.  Replace the code which implemented such checks with
`png_chunk_max`.

Move the malloc limit length check in `png_read_chunk_header` to
`png_handle_chunk` and make it conditional on the chunk type.

Progressive reader: call `png_read_chunk_header`.

Correct the handling of pHYs.

Reviewed-by: Cosmin Truta <[email protected]>
Signed-off-by: John Bowler <[email protected]>
Signed-off-by: Cosmin Truta <[email protected]>

png.c

@@ -278,11 +278,18 @@ png_create_png_struct,(png_const_charp user_png_ver, png_voidp error_ptr,
       create_struct.user_chunk_cache_max = PNG_USER_CHUNK_CACHE_MAX;
 #     endif
 
-#     ifdef PNG_USER_CHUNK_MALLOC_MAX
       /* Added at libpng-1.2.43 and 1.4.1, required only for read but exists
        * in png_struct regardless.
        */
+#     if PNG_USER_CHUNK_MALLOC_MAX > 0 /* default to compile-time limit */
       create_struct.user_chunk_malloc_max = PNG_USER_CHUNK_MALLOC_MAX;
+
+      /* No compile time limit so initialize to the system limit: */
+#     elif (defined PNG_MAX_MALLOC_64K/* legacy system limit */
+      create_struct.user_chunk_malloc_max = 65536U;
+
+#     else                            /* modern system limit SIZE_MAX (C99) */
+      create_struct.user_chunk_malloc_max = PNG_SIZE_MAX;
 #     endif
 #  endif
 

pngmem.c

@@ -72,30 +72,29 @@ png_malloc_base,(png_const_structrp png_ptr, png_alloc_size_t size),
     * to implement a user memory handler.  This checks to be sure it isn't
     * called with big numbers.
     */
-#ifndef PNG_USER_MEM_SUPPORTED
-   PNG_UNUSED(png_ptr)
-#endif
+#  ifdef PNG_MAX_MALLOC_64K
+      /* This is support for legacy systems which had segmented addressing
+       * limiting the maximum allocation size to 65536.  It takes precedence
+       * over PNG_SIZE_MAX which is set to 65535 on true 16-bit systems.
+       *
+       * TODO: libpng-1.8: finally remove both cases.
+       */
+      if (size > 65536U) return NULL;
+#  endif
 
-   /* Some compilers complain that this is always true.  However, it
-    * can be false when integer overflow happens.
+   /* This is checked too because the system malloc call below takes a (size_t).
     */
-   if (size > 0 && size <= PNG_SIZE_MAX
-#     ifdef PNG_MAX_MALLOC_64K
-         && size <= 65536U
-#     endif
-      )
-   {
-#ifdef PNG_USER_MEM_SUPPORTED
+   if (size > PNG_SIZE_MAX) return NULL;
+
+#  ifdef PNG_USER_MEM_SUPPORTED
       if (png_ptr != NULL && png_ptr->malloc_fn != NULL)
          return png_ptr->malloc_fn(png_constcast(png_structrp,png_ptr), size);
+#  else
+      PNG_UNUSED(png_ptr)
+#  endif
 
-      else
-#endif
-         return malloc((size_t)size); /* checked for truncation above */
-   }
-
-   else
-      return NULL;
+   /* Use the system malloc */
+   return malloc((size_t)/*SAFE*/size); /* checked for truncation above */
 }
 
 #if defined(PNG_TEXT_SUPPORTED) || defined(PNG_sPLT_SUPPORTED) ||\

pngpread.c

@@ -193,17 +193,8 @@ png_push_read_chunk(png_structrp png_ptr, png_inforp info_ptr)
     */
    if ((png_ptr->mode & PNG_HAVE_CHUNK_HEADER) == 0)
    {
-      png_byte chunk_length[4];
-      png_byte chunk_tag[4];
-
       PNG_PUSH_SAVE_BUFFER_IF_LT(8)
-      png_push_fill_buffer(png_ptr, chunk_length, 4);
-      png_ptr->push_length = png_get_uint_31(png_ptr, chunk_length);
-      png_reset_crc(png_ptr);
-      png_crc_read(png_ptr, chunk_tag, 4);
-      png_ptr->chunk_name = PNG_CHUNK_FROM_STRING(chunk_tag);
-      png_check_chunk_name(png_ptr, png_ptr->chunk_name);
-      png_check_chunk_length(png_ptr, png_ptr->push_length);
+      png_ptr->push_length = png_read_chunk_header(png_ptr);
       png_ptr->mode |= PNG_HAVE_CHUNK_HEADER;
    }
 

pngpriv.h

@@ -1081,6 +1081,25 @@ PNG_INTERNAL_FUNCTION(png_uint_32,png_fixed_ITU,(png_const_structrp png_ptr,
 PNG_INTERNAL_FUNCTION(int,png_user_version_check,(png_structrp png_ptr,
    png_const_charp user_png_ver),PNG_EMPTY);
 
+#ifdef PNG_READ_SUPPORTED /* should only be used on read */
+/* Security: read limits on the largest allocations while reading a PNG.  This
+ * avoids very large allocations caused by PNG files with damaged or altered
+ * chunk 'length' fields.
+ */
+#ifdef PNG_SET_USER_LIMITS_SUPPORTED /* run-time limit */
+#  define png_chunk_max(png_ptr) ((png_ptr)->user_chunk_malloc_max)
+
+#elif PNG_USER_CHUNK_MALLOC_MAX > 0 /* compile-time limit */
+#  define png_chunk_max(png_ptr) ((void)png_ptr, PNG_USER_CHUNK_MALLOC_MAX)
+
+#elif (defined PNG_MAX_MALLOC_64K)  /* legacy system limit */
+#  define png_chunk_max(png_ptr) ((void)png_ptr, 65536U)
+
+#else                               /* modern system limit SIZE_MAX (C99) */
+#  define png_chunk_max(png_ptr) ((void)png_ptr, PNG_SIZE_MAX)
+#endif
+#endif /* READ */
+
 /* Internal base allocator - no messages, NULL on failure to allocate.  This
  * does, however, call the application provided allocator and that could call
  * png_error (although that would be a bug in the application implementation.)
@@ -1584,12 +1603,6 @@ typedef enum
    handled_ok          /* known, supported and handled without error */
 } png_handle_result_code;
 
-PNG_INTERNAL_FUNCTION(void,png_check_chunk_name,(png_const_structrp png_ptr,
-    png_uint_32 chunk_name),PNG_EMPTY);
-
-PNG_INTERNAL_FUNCTION(void,png_check_chunk_length,(png_const_structrp png_ptr,
-    png_uint_32 chunk_length),PNG_EMPTY);
-
 PNG_INTERNAL_FUNCTION(png_handle_result_code,png_handle_unknown,
     (png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length, int keep),
     PNG_EMPTY);