A possible overflow in png_progressive_combine_row followed by png_read_png

[hopper-vul]

Hi,
when fuzzing libpng, we found png_progressive_combine_row() could be crashed if it is called after png_read_png().

In png_read_png,
it calls png_read_info(png_ptr,..)->png_handle_IHDR(png_ptr,..) to read bytes (exactly 13 bytes) form png io to get a IDHR chunk.
If the png io is initialized with a file beforehand, the bytes will read from this file and set the read IDHR information into png_ptr, such as png_ptr->width.

Then,
If png_progressive_combine_row(png_ptr, old_row, ..) is called currently and the length of old_row smaller than png_ptr->width, and if we can change the contents of the input file, an overflow of old_row will happen in the inner of png_progressive_combine_row at this statement:

memcpy(dp, sp, PNG_ROWBYTES(pixel_depth, row_width))
where dp is old_row and the copy length is png_ptr->width.

e.g.

struct _IO_FILE *v14 = v12; // handler of input file
png_init_io(png_ptr, v14); 
png_read_png(png_ptr, ...); // read chunk data and set png_ptr fields (where png_ptr->width = 0x20)
char old_row[0x1a];
png_progressive_combine_row(png_ptr, old_row, ..) // Overflow of old_row! size: 0x1a, write:0x20

[hopper-vul]

This is the full trigger case, the overflow will be triggered if this file was compiled and linked with AddressSanitizer (add '-fsanitize=address' flag).
In this case, the 17th-20th bytes of will set png_ptr->width = 0x20 and cause latter overflow.

#include "png.h"
#include <stdlib.h>
#include <string.h>
typedef uint8_t   u8;   
typedef int8_t  i8;
typedef int32_t i32;
int main() {
    i8 v0_tmp[] = {49, 46, 54, 46, 51, 55, 0, }; // user_png_ver
    i8 *v0 = malloc(sizeof v0_tmp);
    memcpy(v0, v0_tmp, sizeof v0_tmp);
    i8 *v1 = v0; // user_png_ver
    void *v2 = NULL; // error_ptr
    void (*v3)(struct png_struct_def *,i8 *) = NULL; // error_fn
    void (*v4)(struct png_struct_def *,i8 *) = NULL; // warn_fn
    struct png_struct_def *v5 = png_create_read_struct(v1, v2, v3, v4); // png_ptr
    if (v5 == NULL) return 0;
    struct png_struct_def *v7 = v5; // png_ptr
    u8 v8_tmp[] = {137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82, 0, 0, 0, 32, 0, 0, 0, 32, 8, 3, 0, 0, 0, 68, 164, 138, 198, 0, 0, 0, 4, 103, 65, 77, 65, 0, 0, 156, 64, 32, 13, 228, 203, 0, 0, 0, 15, 80, 76, 84, 69, 102, 204, 204, 255, 255, 255, 0, 0, 0, 51, 153, 102, 153, 255, 204, 62, 76, 175, 21, 0, 0, 0, 25, 116, 69, 88, 116, 83, 111, 102, 116, 119, 97, 114, 101, 0, 65, 100, 111, 98, 101, 32, 73, 109, 97, 103, 101, 82, 101, 97, 100, 121, 113, 201, 101, 60, 0, 0, 0, 91, 73, 68, 65, 84, 56, 203, 221, 147, 65, 10, 128, 64, 12, 3, 51, 205, 254, 255, 205, 30, 84, 216, 69, 219, 122, 241, 160, 115, 205, 64, 32, 180, 162, 65, 16, 5, 32, 98, 228, 40, 16, 49, 148, 226, 159, 10, 0, 149, 112, 172, 155, 10, 128, 61, 27, 87, 193, 150, 192, 105, 197, 158, 23, 194, 89, 83, 8, 75, 126, 47, 76, 121, 34, 52, 75, 186, 17, 150, 33, 223, 169, 248, 236, 201, 201, 57, 241, 228, 121, 27, 54, 210, 102, 3, 53, 127, 203, 12, 109, 0, 0, 0, 0, 73, 69, 78, 68, 174, 66, 96, 130, 0, }; // file_buf
    u8 *v8 = malloc(sizeof v8_tmp);
    memcpy(v8, v8_tmp, sizeof v8_tmp);
    char* v9 = "file___filename_8"; // __filename
    FILE *f_v9 = fopen(v9, "wb");
    fwrite(v8, sizeof v8_tmp, 1, f_v9);
    fclose(f_v9);
    i8 v10_tmp[] = {114, 98, 43, 0, }; // __modes
    i8 *v10 = malloc(sizeof v10_tmp);
    memcpy(v10, v10_tmp, sizeof v10_tmp);
    i8 *v11 = v10; // __modes
    struct _IO_FILE *v12 = fopen(v9, v11); // fp
    if (v12 == NULL) return 0;
    struct _IO_FILE *v14 = v12; // fp
    png_init_io(v7, v14); // $relative
    struct png_info_def *v16 = png_create_info_struct(v7); // info_ptr
    if (v16 == NULL) return 0;
    struct png_info_def *v18 = v16; // info_ptr
    i32 v19 = -913172479; // transforms
    void *v20 = NULL; // params
    png_read_png(v7, v18, v19, v20); // $relative
    u8 v24_tmp[] = {64, 249, 188, 95, 38, 140, 228, 46, 93, 5, 159, 6, 49, 121, 12, 122, 142, 232, 111, 123, 37, 92, 17, 38, 12, 0, }; // old_row
    u8 *v24 = malloc(sizeof v24_tmp);
    memcpy(v24, v24_tmp, sizeof v24_tmp);
    u8 *v25 = v24; // old_row, size : 0x1a
    u8 v26_tmp[] = {98, 158, 36, 70, 84, 205, 110, 126, 199, 88, 199, 90, 19, 54, 63, 64, 215, 132, 212, 88, 102, 161, 162, 90, 196, 243, 122, 167, 0, }; // new_row
    u8 *v26 = malloc(sizeof v26_tmp);
    memcpy(v26, v26_tmp, sizeof v26_tmp);
    u8 *v27 = v26; // new_row
    png_progressive_combine_row(v7, v25, v27); // $relative
}

[thealberto]

Hi @hopper-vul ,
Interesting finding. I'd like to understand it better but I cannot work on it atm.

I saw that you found another possible bug described in the issue #454 , did you improve the fuzzer or just created a custom one? Maybe the current could be improved that to your suggestions

Thanks

[hopper-vul]

I saw that you found another possible bug described in the issue #454 , did you improve the fuzzer or just created a custom one?

@thealberto Yes, we have created a custom one.

Briefly speaking, the created fuzzer could automatically synthesizes programs from library headers (e.g. API declarations) and then links and interprets them to execute. We have spent a lot efforts to infer API relations, constraints and usages, and designed an IR to represent, interpret and execute the generated programs. Now, the fuzzer is still under development.

[thealberto]

@hopper-vul ,
Amazing! Are you going to release it or integrate it inside libpng? An integration with oss-fuzz would be even better.

Any idea regarding that?

Thanks

[hopper-vul]

@thealberto
Sorry, i'm afraid not, whether/how it should be released or open sourced is not up to me, and i haven't heard anything about releasing it yet.

Nevertheless, we are going to write and public some technical papers about it, so that more people will be learned how to build an nicer library fuzzer.

What i can do now is picking the reliable/credible crashes from the output of the fuzzer, and help you or communities to fix them.

best regards

[hopper-vul]

@thealberto

Good news, our team woould like to open source the fuzzer tool after this tool was completed. At that time, it was possible to integrate it to libpng and more projects.

[thealberto]

@hopper-vul
Amazing thanks. Soon I'll be working on your reports trying to help to create a fix.

Thanks for all

[jbowler]

As the original report says quite clearly this is an application bug; the application deliberately calls png_progressive_combine_row with a buffer that is too small! This is not a libpng bug; in fact the overwrite happens in memcpy, which is a standard library routine!

The test case needs to include <stdint.h>

In addition the sequential and progressive readers are incompatible and, anyway, png_read_png does the interlace handling. I find it almost impossible to believe that a real app developer would actually make mistakes like this.

Apart from that it is up to the caller of png_progressive_combine_row to make sure that the 'old_row' and 'new_row' pointers actually match the current state of the (progressive) read engine. To rewrite the code to make the bug more clear:

    png_byte old_row[26] = {64, 249, 188, 95, 38, 140, 228, 46, 93, 5, 159, 6, 49, 121, 12, 122, 142, 232, 111, 123, 37, 92, 17, 38, 12, 0, };
    png_byte new_row[29] = {98, 158, 36, 70, 84, 205, 110, 126, 199, 88, 199, 90, 19, 54, 63, 64, 215, 132, 212, 88, 102, 161, 162, 90, 196, 243, 122, 167, 0, };
    png_progressive_combine_row(png_ptr, old_row, new_row);

'new_row' is ignored; technically it should be png_ptr->row_buf+1, the 'new' row of pixels set up by the progressive reader, but it's just checked for being non-null. The code is clearly bogus:

Breakpoint 1, png_progressive_combine_row (png_ptr=0x5555555892c0, old_row=0x55555558dd90 "@\371\274_&\214\344.]\005\237\0061y\fz\216\350o{%\\\021&\f",
    new_row=0x55555558ddc0 "b\236$FT\315n~\307X\307Z\0236?@ׄ\324Xf\241\242Z\304\363z\247") at ../code/pngpread.c:1068
1068       if (new_row != NULL)
(gdb) print png_ptr->interlaced
$1 = 0 '\000'
(gdb) print png_ptr->rowbytes
$2 = 32
(gdb) up
#1  0x0000555555556a42 in main () at 455.c:48
48          png_progressive_combine_row(v7, v25, v27); // $relative
(gdb) print sizeof v24_tmp
$3 = 26

As the documentation says it just does a memcpy of 32 bytes in a buffer the size of v24_tmp; 26 bytes....

[jbowler]

No response since June; @ctruta, please close.