Catch images broken by AFL #975

szukw000 · 2017-07-31T12:02:28Z

@rouault ,
the minimal archive is attached.
winfried
openjpeg-crashes-2017-07-30.zip

szukw000 · 2017-07-31T12:07:56Z

@rouault ,
I have called 'cmake -DWITH_ASTYLE=on ..', then called 'make', then - from the BUILD directory -
called '../ scripts/prepare-commit.sh'.
winfried

rouault · 2017-07-31T14:27:57Z

I can see that there are new test failures. Do you know if they are regressions or issues that are actually fixed ?

NR-DEC-issue414.jp2-110-decode

NR-DEC-issue414.jp2-110-decode-md5

NR-DEC-issue458.jp2-120-decode

NR-DEC-issue458.jp2-120-decode-md5

szukw000 · 2017-07-31T17:51:56Z

@rouault ,
issue458.jp2: Csiz(4)

comp sign prec dx dy

0     0    5   1   1
1     0    5   1   1
2     0    5   1   1
3     0    1   1   1

issue414.jp2: Csiz(5)

comp sign prec dx  dy

0     0    8   1   1
1     0    8   1   1
2     0    8   1   1
3     0    8   1   1
4     0    8   1   1

Changed code in j2k.c, line 2341:

if (l_image->numcomps > 3 //== 4 /* RGBA */

winfried

rouault · 2017-07-31T20:02:29Z

Changed code in j2k.c, line 2341:

I don't think all the checks you added in j2k.c related to color space and number of channels should be put there. The standard doesn't mention them (as far as I can see in "I.5.3.3 Colour Specification box"), so I guess we should accept images with apparently inconsistant channel number vs color space. We can reject them at the conversion stage, but in the library itself, we should probably allow such weird images, ie the color space is just a hint

rouault · 2017-07-31T16:41:48Z

src/bin/jp2/converttif.c

@@ -711,8 +709,8 @@ int imagetotif(opj_image_t * image, const char *outfile)
        TIFFClose(tif);
        return 1;
    }
-    buffer32s = (OPJ_INT32 *)malloc((OPJ_SIZE_T)width * numcomps * sizeof(
-                                        OPJ_INT32));
+    buffer32s = (OPJ_INT32 *)malloc((OPJ_SIZE_T)(width * numcomps * sizeof(


With your new version, uint32 overflow might occur on both 32 and 64 bit builds. In the previous version, it would only occur on 32 bit builds. So checking for width > UINT_MAX / numcomps || width * numcomps > UINT_MAX / sizeof(OPJ_INT32) might be needed

rouault · 2017-07-31T17:14:33Z

src/bin/jp2/converttif.c

@@ -1421,8 +1445,9 @@ opj_image_t* tiftoimage(const char *filename, opj_cparameters_t *parameters)
        opj_image_destroy(image);
        return NULL;
    }
-    rowStride = ((OPJ_SIZE_T)w * tiSpp * tiBps + 7U) / 8U;
-    buffer32s = (OPJ_INT32 *)malloc((OPJ_SIZE_T)w * tiSpp * sizeof(OPJ_INT32));
+    rowStride = (tmsize_t)((tiWidth * tiSpp * tiBps + 7U) / 8U);


As above, potential uint32 overflow

rouault · 2017-07-31T20:03:00Z

src/bin/jp2/opj_dump.c

@@ -577,6 +577,8 @@ int main(int argc, char *argv[])
        opj_set_warning_handler(l_codec, warning_callback, 00);
        opj_set_error_handler(l_codec, error_callback, 00);

+        parameters.dump_state = 1; /* AFL test */


AFL test comment to remove and in other locations

rouault · 2017-07-31T20:06:58Z

src/lib/openjp2/j2k.c

@@ -2276,6 +2305,122 @@ static OPJ_BOOL opj_j2k_read_siz(opj_j2k_t *p_j2k,
            l_cp->m_specific_param.m_dec.m_reduce; /* reducing factor per component */
        ++l_img_comp;
    }
+    if (!p_j2k->dump_state) {


As discussed in another comment, I don't think those checks should be put in the library code

rouault · 2017-07-31T20:10:17Z

src/lib/openjp2/openjpeg.h

@@ -564,6 +564,7 @@ typedef struct opj_dparameters {
    /* <<UniPG */

    unsigned int flags;
+    unsigned int dump_state; /* AFL test */


This will change the ABI. An alternative way would be to add a OPJ_DPARAMETERS_DUMP_FLAG 0x0002 flag (see existing OPJ_DPARAMETERS_IGNORE_PCLR_CMAP_CDEF_FLAG)

rouault · 2017-07-31T20:12:59Z

(note in case you're not familiar with github & pull requests : you can add new commits to your changes-for-afl-tests branch and push it, and they will be added to this pull request)

szukw000 · 2017-08-01T17:25:26Z

Yes, I am not familiar with github. As I am not familiar I do not know 1. howto add a new commit 2. howto push it winfried

…

Even Rouault ***@***.***> hat am 31. Juli 2017 um 22:13 geschrieben: (note in case you're not familiar with github & pull requests : you can add new commits to your changes-for-afl-tests branch and push it, and they will be added to this pull request)

rouault · 2017-08-01T17:30:01Z

@szukw000 Do changes in your local tree and git commit them as usual, and then git push your branch to your github account as you did for the initial pull request. github will realize that your branch was the source of this pull request, so it will queue the new commits to it

rouault · 2017-08-04T16:32:41Z

@szukw000 2 issues:

There's a style issue (see https://travis-ci.org/uclouvain/openjpeg/jobs/260252276) that you can fix by running "scripts/astyle.sh src/bin/jp2/converttif.c"
There's a compilation issue related to " error: unknown type name ‘tmsize_t’" on one target that haveOPJ_CI_PROFILE=1 and do a run with "cmake "-DCMAKE_C_FLAGS=-pg -O3" -DCMAKE_EXE_LINKER_FLAGS=-pg -DCMAKE_SHARED_LINKER_FLAGS=-pg -DBUILD_SHARED_LIBS=OFF .." ( see end of tools/travis-ci/run.sh). From the log, the system libtiff is used (instead of the one in third_party) and it must be a libtiff 3.X version that doesn't have tmsize_t. It would be good if openjpeg could still build against libtiff 3.X. It doesn't look like your change of tsize_t to tmsize_t is needed since in libtiff 4.X both types are equivalent because of "typedef tmsize_t tsize_t" in tiffio.h. See https://travis-ci.org/uclouvain/openjpeg/jobs/260252281 for full log

szukw000 · 2017-08-06T05:39:22Z

@rouault, libopenjpeg uses libtiff-4.0.6 in 'thirdparty/libtiff'. I suppose there is a problem with libtiff < 4.x.y on 64-bit machines: tiff-3.8.0/test: ./ascii_tag sizeof TSIZE_T(4) UINT_MAX(4294967295l) tiff-4.0.7/test: ./ascii_tag sizeof TSIZE_T(8) UINT_MAX(4294967295l)

…

Even Rouault ***@***.***> hat am 4. August 2017 um 18:32 geschrieben: @szukw000 https://github.com/szukw000 2 issues: * There's a style issue (see https://travis-ci.org/uclouvain/openjpeg/jobs/260252276) that you can fix by running "scripts/astyle.sh src/bin/jp2/converttif.c" * There's a compilation issue related to " error: unknown type name ‘tmsize_t’" on one target that haveOPJ_CI_PROFILE=1 and do a run with "cmake "-DCMAKE_C_FLAGS=-pg -O3" -DCMAKE_EXE_LINKER_FLAGS=-pg -DCMAKE_SHARED_LINKER_FLAGS=-pg -DBUILD_SHARED_LIBS=OFF .." ( see end of tools/travis-ci/run.sh). From the log, the system libtiff is used (instead of the one in third_party) and it must be a libtiff 3.X version that doesn't have tmsize_t. It would be good if openjpeg could still build against libtiff 3.X. It doesn't look like your change of tsize_t to tmsize_t is needed since in libtiff 4.X both types are equivalent because of "typedef tmsize_t tsize_t" in tiffio.h. See https://travis-ci.org/uclouvain/openjpeg/jobs/260252281 for full log — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #975 (comment) , or mute the thread https://github.com/notifications/unsubscribe-auth/AEzsjY17Fi0fGRzjzo9uAr4bq7KKOjQ5ks5sU0eqgaJpZM4OoSHD .

rouault · 2017-08-06T09:18:26Z

I suppose there is a problem with libtiff < 4.x.y on 64-bit machines:

Not really, on libtiff 3.X, that's per design: "typedef int32 tsize_t; /* i/o size in bytes */" .

Why did your change to tmsize_t was supposed to fix ?

szukw000 · 2017-08-06T13:49:50Z

@rouault, I know: 3.8.0: ============ #define TIFFLIB_VERSION 20051230 * NB: tsize_t is int32 and not uint32 because some functions * return -1. <TD>typedef int32 tsize_t;</TD> <TD>i/o size in bytes</TD> But an int32 type can not contain an UINT_MAX value but an INT_MAX value (2147483647) only. winfried

…

Even Rouault ***@***.***> hat am 6. August 2017 um 11:18 geschrieben: > > > I suppose there is a problem with libtiff < 4.x.y on 64-bit machines: > > > Not really, on libtiff 3.X, that's per design: "typedef int32 tsize_t; /* i/o size in bytes */" . Why did your change to tmsize_t was supposed to fix ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #975 (comment) , or mute the thread https://github.com/notifications/unsubscribe-auth/AEzsjZ7p6pTLQ8cj8FYEQAzAro5vrh4Lks5sVYTjgaJpZM4OoSHD .

rouault · 2017-08-06T13:59:55Z

But an int32 type can not contain an UINT_MAX value but an INT_MAX value (2147483647) only.

I agree, but I'm lost at what you are suggesting to do. What if we revert back to using tsize_t ? I'd say that even if there are some potential vulnerability with libtiff 3.X due to using its tsize_t, it is somehow OK. What matters is that we work fine with libtiff 4.X (IMHO)

szukw000 · 2017-08-06T20:15:18Z

@rouault, some tests. ``` 1. 32bit, linux-4.7.5, gcc-5.3.0, libtiff-4.0.6 : converttif.c:imagetotif: TMSIZE_T(4) TSIZE_T(4) INT64(8) UINT_MAX(4294967295) 583: strip_size = UINT_MAX; openjpeg2-2017-08-06/src/bin/jp2/converttif.c:583:15: warning: conversion of unsigned constant value to negative integer [-Wsign-conversion] strip_size = UINT_MAX; 2. 64bit, WIN7, VS Community 2017, libtiff-4.0.6: converttif.c:582: TSIZE_T(8) TMSIZE_T(8) UINT_MAX(4294967295) 3. 64bit, linux-4.12.3, gcc-7.1.0, libtiff-4.0.8: converttif.c:581: INT(4) TMSIZE_T(8) INT64(8) ``` Ignoring the fact that travis does use old buggy libraries one could replace 'tsize_t' or 'tmsize_t' with 'int64_t'. 'int64_t' can hold an UINT_MAX value. winfried

…

Even Rouault ***@***.***> hat am 6. August 2017 um 15:59 geschrieben: > > > But an int32 type can not contain an UINT_MAX value but an INT_MAX value (2147483647) only. > > > I agree, but I'm lost at what you are suggesting to do. What if we revert back to using tsize_t ? I'd say that even if there are some potential vulnerability with libtiff 3.X due to using its tsize_t, it is somehow OK. What matters is that we work fine with libtiff 4.X (IMHO) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #975 (comment) , or mute the thread https://github.com/notifications/unsubscribe-auth/AEzsjW3oxbtTx7hC0bfV2pLloP31cFDXks5sVcbbgaJpZM4OoSHD .

rouault · 2017-08-06T20:19:53Z

one could replace 'tsize_t' or 'tmsize_t' with 'int64_t'

Sounds good

szukw000 · 2017-08-07T14:53:54Z

@rouault ,
I have now changed 'src/bin/jp2/converttif.c'. The changes look clumsy. I have tested
the file with linux32bit and linux64bit.

winfried

rouault · 2017-08-07T16:16:53Z

@detonin The changes looks good to me. I let you look at them

detonin · 2017-08-08T23:52:07Z

Looks good indeed. Merging.
@szukw000 Thanks for the contribution !

format specifier mismatch in #975

PR #975 introduced a check that rejects images that have different bit depth/sign per compoment in SIZ marker if the JP2 IHDR box has BPC != 255 This didn't work properly if decoding a .j2k file since the new bit added in opj_cp_t wasn't initialized to the right value. For clarity, tThis new bit has also been renamed to allow_different_bit_depth_sign But looking closer at the code, it seems we were already tolerant to inconsistencies. For example we parsed a JP2 BPCC box even if BPC != 255 (just a warning is emitted) So failing hard in opj_j2k_read_siz() wouldn't be very inconsistent, and that alone cannot protect against other issues, so just emit a warning if BPC != 255 and the SIZ marker contains different bit depth/sign per component. Note: we could also check that the content of JP2 BPCC box is consistant with the one of the SIZ marker.

Catch images broken by AFL

00f4568

rouault mentioned this pull request Jul 31, 2017

Tests for AFL files #973

Closed

rouault reviewed Jul 31, 2017

View reviewed changes

First change on changes-for-afl-tests

57e36db

Changes for converttif.c to fix tsize_t

bc3cb74

detonin merged commit 0394f8d into uclouvain:master Aug 8, 2017

detonin added a commit that referenced this pull request Aug 9, 2017

Fix remaining warning

0c07950

format specifier mismatch in #975

rouault added a commit that referenced this pull request Aug 9, 2017

Remove useless opj_tcd_t::enumcs field added per #975

4b16e8d

rouault mentioned this pull request Aug 11, 2017

Changes in converttif.c for PPC64 #980

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch images broken by AFL #975

Catch images broken by AFL #975

szukw000 commented Jul 31, 2017

szukw000 commented Jul 31, 2017

rouault commented Jul 31, 2017

szukw000 commented Jul 31, 2017

rouault commented Jul 31, 2017

rouault Jul 31, 2017

rouault Jul 31, 2017

rouault Jul 31, 2017

rouault Jul 31, 2017

rouault Jul 31, 2017

rouault commented Jul 31, 2017

szukw000 commented Aug 1, 2017 via email

rouault commented Aug 1, 2017 •

edited

Loading

rouault commented Aug 4, 2017

szukw000 commented Aug 6, 2017 via email

rouault commented Aug 6, 2017

szukw000 commented Aug 6, 2017 via email

rouault commented Aug 6, 2017

szukw000 commented Aug 6, 2017 via email

rouault commented Aug 6, 2017

szukw000 commented Aug 7, 2017

rouault commented Aug 7, 2017

detonin commented Aug 8, 2017

Catch images broken by AFL #975

Catch images broken by AFL #975

Conversation

szukw000 commented Jul 31, 2017

szukw000 commented Jul 31, 2017

rouault commented Jul 31, 2017

szukw000 commented Jul 31, 2017

rouault commented Jul 31, 2017

rouault Jul 31, 2017

Choose a reason for hiding this comment

rouault Jul 31, 2017

Choose a reason for hiding this comment

rouault Jul 31, 2017

Choose a reason for hiding this comment

rouault Jul 31, 2017

Choose a reason for hiding this comment

rouault Jul 31, 2017

Choose a reason for hiding this comment

rouault commented Jul 31, 2017

szukw000 commented Aug 1, 2017 via email

rouault commented Aug 1, 2017 • edited Loading

rouault commented Aug 4, 2017

szukw000 commented Aug 6, 2017 via email

rouault commented Aug 6, 2017

szukw000 commented Aug 6, 2017 via email

rouault commented Aug 6, 2017

szukw000 commented Aug 6, 2017 via email

rouault commented Aug 6, 2017

szukw000 commented Aug 7, 2017

rouault commented Aug 7, 2017

detonin commented Aug 8, 2017

rouault commented Aug 1, 2017 •

edited

Loading