Added overflow check for CVE-2021-29338

[Eharve14]

Added a multiplication check before the call to calloc, see my comment on 79c7d7a

[rouault]

Added a multiplication check before the call to calloc, see my comment on 79c7d7a

I saw your comment, but calloc() should really check for the overflow. I'd expect only old and unmaintained platform to have a buggy implementation and we probably don't care about them. Or said otherwise: can you quote one recent enough platform of interest whose calloc() wouldn't check the overflow of the multiplication of the argument ?

[rouault]

ok, answering myself I see the list is in the link : https://www.avertium.com/blog/overview-badalloc-vulnerabilities.
Well none of them qualify as of major players as far as I'm concerned to justify cluttering openjpeg codebase. This is a bug of those C libraries, not openjpeg.

[Eharve14]

Rodger that, just wanted to bring some attention to the topic since I had seen the update and I had done some research on the topic.

[Eharve14]

I forgot to point out there still is an integer overflow in the following for loops, it was mentioned in my comment on 79c7d7a:
Line 535 of ops_dump.c:
for (it_image = 0; it_image < num_images; it_image++) {
dirptr->filename[it_image] = dirptr->filename_buf + it_image * OPJ_PATH_LEN;
}
Line 1974 of opj_compress.c
for (i = 0; i < num_images; i++) {
dirptr->filename[i] = dirptr->filename_buf + i * OPJ_PATH_LEN;
}
Line1393 of opj_decompress.c:
for (it_image = 0; it_image < num_images; it_image++) {
dirptr->filename[it_image] = dirptr->filename_buf + it_image * OPJ_PATH_LEN;
}

This for loop can produce similar behavior to the original CVE.

[Eharve14]

Specifically when the strcpy function is called in the load_images function.

[rouault]

casting the iterator i / it_image to size_t before doing the multiplication should fix it, right ? I could take such a fix if you want to issue a PR with it (I don't think this PR protects against that)

[Eharve14]

Forgive me for the repeated comment I haven't looked at this in a while and now that I am digging into it I am finding all of the issues that lead to the fix I tried to push originally:
Before the calloc there is an integer overflow in the get num images function:
`int get_num_images(char *imgdirpath)
{
DIR dir;
struct dirent content;
int num_images = 0;

/*Reading the input images from given input directory*/

dir = opendir(imgdirpath);
if (!dir) {
    fprintf(stderr, "Could not open Folder %s\n", imgdirpath);
    return 0;
}

while ((content = readdir(dir)) != NULL) {
    if (strcmp(".", content->d_name) == 0 || strcmp("..", content->d_name) == 0) {
        continue;
    }
    num_images++;
}
closedir(dir);
return num_images;

}'

the maximum number of file in a dir in windows 10 is equal to SIZE_MAX, meaning that the signed integer operation will overflow if the number of files in the dir is > 2147483647. The integer will wrap and the calloc will allocate an amount of memory.

The cast to size_t to fix the multiplication will still have an integer overflow error.

The original solution prevents all of this by ensuring that num_images is < SIZE_MAX/(OPJ_PATH_LEN* 8) and I added a check to ensure num_images is not negative, resulting in unintended execution.

[rouault]

the maximum number of file in a dir in windows 10 is equal to SIZE_MAX, meaning that the signed integer operation will overflow if the number of files in the dir is > 2147483647

that's really a theoretical concern... In any case if we wanted to address this, this would need to be checked in get_num_images() before the overflow occurs

[Eharve14]

I understand that the the concern is theoretical and unrealistic for normal use. It is however a very exploitable path and a real security vulnerability.

It's the same exploitation path as the original CVE just with a larger number of files. Point to a dir with enough named files to create the overflow, use the file names to inject data onto the stack as the are read in past the end of the allocated memory.

[Eharve14]

Should I move the check for num_images to the get_num_images function? Also are you suggesting the function should throw an error and exit? or are we adding a flag for overflow and checking the result?

[rouault]

Should I move the check for num_images to the get_num_images function? Also are you suggesting the function should throw an error and exit? or are we adding a flag for overflow and checking the result?

I'd prefer first that the scope of this PR is only to add the (size_t) casts so that we keep focus on one thing at a time

For another PR to address the overflow in get_num_images, I would just break from the for loop in get_num_images before the overflow of the counter occurs.

[Eharve14]

Rodger that, I will fix this PR to just add the casts. I will open a separate pull request down the line for the get_num_images.

[Eharve14]

Thanks for the assist, Sorry for spamming the commits.