Potential crash in png_do_write_transformations

[davidtr1037]

There is a crash in png_do_write_transformations which is reachable from the png_write_png API. This happens because of an invalid pointer increment (sp) in png_do_strip_channel without checking the width, for exmaple:

if (row_info->bit_depth == 8)
{
   if (at_start != 0) /* Skip initial filler */
      ++sp;
   else          /* Skip initial channel and, for sp, the filler */
   {
      sp += 2; ++dp;
   }

   /* For a 1 pixel wide image there is nothing to do */
   while (sp < ep)
   {
      *dp++ = *sp; sp += 2;
   }

   row_info->pixel_depth = 8;
}

To reproduce the bug:

configure CFLAGS="-O0 -g -fsanitize=address,leak,undefined"
gcc -O0 -g -fsanitize=address,leak,undefined test.c <build>/.libs/libpng16.a -I libpng/ -I <build>/ -lm -lz -o test
./test

The code for test.c:

#include <png.h>
#include <pngpriv.h>

void write_handler(png_structp png, png_bytep data, png_size_t size) {

}

int main(int argc, char *argv[]) {
    png_struct *png = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL);
    png_set_write_fn(png, NULL, write_handler, NULL);

    png_info *info = png_create_info_struct(png);
    png_uint_32 width = 0;
    info->width = width;
    info->color_type = PNG_COLOR_TYPE_GRAY;
    info->bit_depth = 16;
    info->channels = 2;
    info->height = 1;
    info->pixel_depth = info->channels * info->bit_depth;
    info->rowbytes = PNG_ROWBYTES(info->pixel_depth, info->width);

    png_bytep *image = malloc(info->height * sizeof(png_bytep));
    for (unsigned i = 0; i < info->height; i++) {
        image[i] = calloc(info->rowbytes, 1);
    }
    png_set_rows(png, info, image);

    int transforms = PNG_TRANSFORM_STRIP_FILLER_AFTER | \
                     PNG_TRANSFORM_INVERT_MONO;
    png_write_png(png, info, transforms, NULL);

    return 0;
}

[benkasminbullock]

The row_info->bit_depth in the code you claim is faulty is 8 bits but you've given an example where you set the bit_depth to 16. That is copied into row_info->bit_depth. So how can your code trip a bug in that? Also your code needs

#include <stdlib.h>

to compile. You are already asking calloc for zero bytes of memory then passing that into libpng, so essentially you are getting libpng to dereference an invalid thing. There is no way for a C program to know that a pointer is invalid or points to invalid memory. I don't think this is a bug in libpng at all.

[davidtr1037]

@benkasminbullock thank you for the response:)

You are right about the bit_depth, I meant this snippet (from the same png_do_strip_channel):

else if (row_info->bit_depth == 16)
{
   if (at_start != 0) /* Skip initial filler */
      sp += 2;
   else          /* Skip initial channel and, for sp, the filler */
   {
      sp += 4; dp += 2;
   }

   while (sp < ep)
   {
      *dp++ = *sp++; *dp++ = *sp; sp += 3;
   }

   row_info->pixel_depth = 16;
}

Note that even if you change the allocation to:

image[i] = calloc(info->rowbytes + 100, 1);

which gives you a valid pointer (with much more space than you actually need), you still have the crash:

==6440==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000efd1 at pc 0x000000486a2f bp 0x7ffcd8994040 sp 0x7ffcd8994030
READ of size 1 at 0x60200000efd1 thread T0
    #0 0x486a2e in png_do_invert ../libpng/pngtrans.c:277
    #1 0x49f710 in png_do_write_transformations ../libpng/pngwtran.c:571
    #2 0x490df5 in png_write_row ../libpng/pngwrite.c:862
    #3 0x48f4d6 in png_write_image ../libpng/pngwrite.c:626
    #4 0x4930f0 in png_write_png ../libpng/pngwrite.c:1455
    #5 0x4023ad in main test.c:30
    #6 0x7ffb8377b83f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
    #7 0x401c28 in _start (test+0x401c28)

The reason for that is that sp is incremented before checking the width (see the attached snippet).

[benkasminbullock]

You've bypassed the protection which libpng provides against a zero-width, which is to set the header using png_set_IHDR. If you set a zero-width there, it will error. You've also included libpng's own header file pngpriv.h. Then you've sent in pointers to invalid memory. I'm not sure whether libpng should guard against this by checking the width, although it sounds like a reasonable thing to do, but your way of doing things isn't the recommended workflow for using libpng, so could you justify why the protection against zero width images needs to be in that place and not in png_set_IHDR?

[davidtr1037]

Please note that the passed pointers are all valid (see my previous comment). Regarding the row_width: As far as I understand the documentation, it says that the png_info structure must be initialized, but it's not clear if it can be done only using png_set_IHDR. Therefore, if not called, the user is exposed to this memory corruption.

Note that the analogous png_read_png API does check the zero-width scenario (in png_handle_IHDR), so it would make sense to be consistent and add a call to png_check_IHDR in the relevant png_write_* API's as well.

[benkasminbullock]

As far as I understand the documentation, it says that the png_info structure must be initialized, but it's not clear if it can be done only using png_set_IHDR.

You've brought up the topic of the API, which I find surprising. You are including the pngpriv.h header in your program to access the internals of structures which are not part of the API. The "priv" part of this file is short for "private". It isn't even installed in a public place. You've accessed it by including it from the libpng source code directory. Your example program bypasses the API and accesses non-API parts of libpng which you aren't supposed to access. You've caused this problem by using non-API access to set the width to zero, which would have been caught if you'd used the API. So as I said before this looks like a non-bug to me. If you can produce the bug using the libpng API that would be different, but you definitely haven't used the API in your example.