madler / zlib

A massively spiffy yet delicately unobtrusive compression library.
http://zlib.net/
Other
5.55k stars 2.42k forks source link

Infinite loop found in crc32_combine_gen64 #904

Closed PromptFuzz closed 7 months ago

PromptFuzz commented 7 months ago

Dear Team, I found that the zlib API crc32_combine_gen64 contains an infinite loop issue.

See the PoC program below, I pass an empty mode of gzopen() and let it return a NULL. Then gzoffset64(NULL) will return a value of 0xffffffffffffffff. If the argument passed to crc32_combine_gen64() is 0xffffffffffffffff, it will trap in an infinite loop.

PoC program:

#include <zlib.h>
#include <stdlib.h>
#include <stdio.h>
#include <stdint.h>

extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
    if(size<54) return 0;

    FILE *input_file_ptr = fopen("input_file", "wb");
    if (input_file_ptr == NULL) {return 0;}
    fwrite(data, sizeof(uint8_t), size, input_file_ptr);
    fclose(input_file_ptr);

    gzFile file;

    // Step 2: Open the gzip file
    file = gzopen("input_file", "");

    uLong crc32_checksum = crc32_combine_gen64(gzoffset64(file));

    // Step 10: Clean up and release resources

    gzclose(file);
    return 0;
}
madler commented 7 months ago

Yes, the argument of crc32_combine_gen64() is the length of a sequence of bytes, which must, of course, be non-negative. Though it is obvious, I will add a note to that effect in zlib.h.

PromptFuzz commented 7 months ago

Thank you for your response; I concur with your viewpoint.

Regarding the issue at hand, my confusion stems from the usage of the z_off64_t type. This type is employed in both the gzoffset64() and crc32_combine_gen64() functions. As users of the library, we may not possess the same familiarity with the implementation of crc32_combine_gen64() as you do. Consequently, users might inadvertently write problematic code patterns such as crc32_combine_gen64(gzoffset64(file)), unaware of the potential negative consequences.

Thus, I believe it is necessary to mitigate the potential negative impact by providing appropriate documentation or safeguards within the library.

madler commented 7 months ago

Anyone who writes crc32_combine_gen64(gzoffset64(file)) or for that matter crc32_combine_gen(gzoffset(file)) is implicitly ignoring a possible error indication in the return value of gzoffset(), which is a no-no. Errors must always be checked for.