Skip to content

Commit

Permalink
opj_compress/opj_uncompress: fix integer overflow in num_images (#1395)
Browse files Browse the repository at this point in the history
Includes the fix for CVE-2021-29338
Credit to @kaniini based on #1346
Fixes #1338
  • Loading branch information
baparham authored Jan 12, 2022
1 parent fe2fa70 commit 79c7d7a
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 8 deletions.
4 changes: 2 additions & 2 deletions src/bin/jp2/opj_compress.c
Original file line number Diff line number Diff line change
Expand Up @@ -1959,9 +1959,9 @@ int main(int argc, char **argv)
num_images = get_num_images(img_fol.imgdirpath);
dirptr = (dircnt_t*)malloc(sizeof(dircnt_t));
if (dirptr) {
dirptr->filename_buf = (char*)malloc(num_images * OPJ_PATH_LEN * sizeof(
dirptr->filename_buf = (char*)calloc(num_images, OPJ_PATH_LEN * sizeof(
char)); /* Stores at max 10 image file names*/
dirptr->filename = (char**) malloc(num_images * sizeof(char*));
dirptr->filename = (char**) calloc(num_images, sizeof(char*));
if (!dirptr->filename_buf) {
ret = 0;
goto fin;
Expand Down
5 changes: 2 additions & 3 deletions src/bin/jp2/opj_decompress.c
Original file line number Diff line number Diff line change
Expand Up @@ -1374,14 +1374,13 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
/* Stores at max 10 image file names */
dirptr->filename_buf = (char*)malloc(sizeof(char) *
(size_t)num_images * OPJ_PATH_LEN);
dirptr->filename_buf = calloc((size_t) num_images, sizeof(char) * OPJ_PATH_LEN);
if (!dirptr->filename_buf) {
failed = 1;
goto fin;
}

dirptr->filename = (char**) malloc((size_t)num_images * sizeof(char*));
dirptr->filename = (char**) calloc((size_t) num_images, sizeof(char*));

if (!dirptr->filename) {
failed = 1;
Expand Down
7 changes: 4 additions & 3 deletions src/bin/jp2/opj_dump.c
Original file line number Diff line number Diff line change
Expand Up @@ -515,13 +515,14 @@ int main(int argc, char *argv[])
if (!dirptr) {
return EXIT_FAILURE;
}
dirptr->filename_buf = (char*)malloc((size_t)num_images * OPJ_PATH_LEN * sizeof(
char)); /* Stores at max 10 image file names*/
/* Stores at max 10 image file names*/
dirptr->filename_buf = (char*) calloc((size_t) num_images,
OPJ_PATH_LEN * sizeof(char));
if (!dirptr->filename_buf) {
free(dirptr);
return EXIT_FAILURE;
}
dirptr->filename = (char**) malloc((size_t)num_images * sizeof(char*));
dirptr->filename = (char**) calloc((size_t) num_images, sizeof(char*));

if (!dirptr->filename) {
goto fails;
Expand Down

1 comment on commit 79c7d7a

@Eharve14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I wrote a paper on this CVE for my masters program and never got around to fully implementing a fix. The call to calloc still has the same overflow issue as the malloc call, I proposed this exact fix in the paper I wrote. The two sources below outline why calloc still needs a multiplication check before calling, source A, and example CVE's that suffer from memory allocation issues related to integer overflows in calloc functions, source B .
Source-A: https://wiki.sei.cmu.edu/confluence/display/c/MEM07-C.+Ensure+that+the+arguments+to+calloc%28%29%2C+when+multiplied%2C+do+not+wrap

Source B:
https://www.avertium.com/blog/overview-badalloc-vulnerabilities

There is an additional integer overflow in the for loop on line 1969 in ops_compress, OPJ_PATH_LENGTH is 4096, it is multiplied by number of images. This loop should be covered by a multiplication check as well.

I committed the multiplication check version for review.
-Eric

Please sign in to comment.