Openjpeg-2.1.2 DoS vulnerability due to some logic error

[twelveand0]

Overview

I have found a vulnerability in openjpeg-2.1.2 (an open-source JPEG 2000 codec written in C language) using AFL (http://lcamtuf.coredump.cx/afl/). The vulnerability exists in code responsible for decoding the input image. The vulnerability is caused by an improper assumption: if the decoding process successfully finishes, buffer of each component corresponding to each channel (Red, Green, Blue, Alpha) has already been allocated and filled with data; however, the assumption is not always valid, i.e. these buffers has not been allocated when the decoding process successfully returns. The vulnerability can be viewed as a logic error. The vulnerability can trigger many different crash points by crafting the PoC image file and cause Denial-of-Service due to Null Pointer Reference. It’s probably that the vulnerability can cause crashes of some other type and cause more critical impact by crafting the PoC.

Analysis and PoC

The detail analysis and poc file can be found in the attachment. report2_+poc.zip

Author

name: twelveand0 @ VARAS of IIE email: l.bingchang.bc@gmail.com org: IIE (http://iie.ac.cn)

Notes

I have reported this to RedHat Security Team and they suggested me to report it here before assigning cve id.

[attritionorg]

Dupe to #857?

[twelveand0]

@attritionorg The bug can cause many different crash points where image->comps[i].data is accessed directly or indirectly after function "opj_decode" returns TRUE. #857 is one of them due to the same root cause analyzed in the report. The crash of #857 can be easily changed to by make the following check (line 1771-1780 in convert.c) to be FALSE.

1771       if ((force_split == 0) &&
1772                (ncomp == 2 /* GRAYA */
1773            || (ncomp > 2 /* RGB, RGBA */
1774               && image->comps[0].dx == image->comps[1].dx
1775                && image->comps[1].dx == image->comps[2].dx
1776                && image->comps[0].dy == image->comps[1].dy
1777                && image->comps[1].dy == image->comps[2].dy
1778                && image->comps[0].prec == image->comps[1].prec
1779                && image->comps[1].prec == image->comps[2].prec
1780                )))
1781        {

Among the variables in above check, image->comps[i].dx, image->comps[i].dy, image->comps[i].prec and ncomp can be directly controlled from the input image file (as analyzed in the report of #862 and this report).

Some of similar crash points have been described in the attachment report.

[szukw000]

@twelveand0, @attritionorg

I have tested both files in question: PoC1.jp2 and PoC2.j2k. And changed 'opj_decompress.c'. See attached dif file.

Could you apply this patch to your library and make some more tests?

winfried issue-682_and_683.dif.zip

[twelveand0]

@szukw000 For #862 , I tested it and the crash didn't happen again. However, I am not sure whether it is correct to limit comps[0].dx = comps[1].dx = comps[2].dx = comps[3].dx, comps[0].dy = comps[1].dy = comps[2].dy = comps[3].dy ... as the patch does:

+       for (ui = 1; ui < numcomps; ++ui) {
+           if (image->comps[0].dx != image->comps[ui].dx) {
+               break;
+           }
+           if (image->comps[0].dy != image->comps[ui].dy) {
+               break;
+           }
+           if (image->comps[0].prec != image->comps[ui].prec) {
+               break;
+           }
+           if (image->comps[0].sgnd != image->comps[ui].sgnd) {
+               break;
+           }
+       }
+       if (ui != numcomps) {
+           fprintf(stderr,"opj_decompress: All components\n    shall have "
+            "the same subsampling, same bit depth, same sign.\n"
+            "    Aborting\n");
+           failed = 1; goto broken_file;
+       }

Because I saw the following code in function "color_sycc_to_rgb" (called by function "main"):

src/bin/common/color.c
319 if((img->comps[0].dx == 1)
320 && (img->comps[1].dx == 2)
321 && (img->comps[2].dx == 2)
322 && (img->comps[0].dy == 1)
323 && (img->comps[1].dy == 2)
324 && (img->comps[2].dy == 2))/* horizontal and vertical sub-sample */
325  {
326     sycc420_to_rgb(img);
327  }
328 else
329 if((img->comps[0].dx == 1)
330 && (img->comps[1].dx == 2)
331 && (img->comps[2].dx == 2)
332 && (img->comps[0].dy == 1)
333 && (img->comps[1].dy == 1)
334 && (img->comps[2].dy == 1))/* horizontal sub-sample only */
335  {
336     sycc422_to_rgb(img);
337  }

The vulnerability is triggered only when output to ".pnm \ .ppm", i.e. call into function "imagetopnm", why not directly patch it in this function like:

*** 
--- src/bin/jp2/convert.c   2016-11-22 11:38:02.677050614 +0800
***************
--- 1770,1790 ----

      if ((force_split == 0) &&
                (ncomp == 2 /* GRAYA */
!             || (ncomp == 3 /* RGB */
                  && image->comps[0].dx == image->comps[1].dx
                  && image->comps[1].dx == image->comps[2].dx
                  && image->comps[0].dy == image->comps[1].dy
                  && image->comps[1].dy == image->comps[2].dy
                  && image->comps[0].prec == image->comps[1].prec
                 && image->comps[1].prec == image->comps[2].prec)
+             || (ncomp > 3 /* RGBA */
+                 && image->comps[0].dx == image->comps[1].dx
+                 && image->comps[1].dx == image->comps[2].dx
+                 && image->comps[2].dx == image->comps[3].dx
+                 && image->comps[0].dy == image->comps[1].dy
+                 && image->comps[1].dy == image->comps[2].dy
+                 && image->comps[2].dy == image->comps[3].dy
+                 && image->comps[0].prec == image->comps[1].prec
+                 && image->comps[1].prec == image->comps[2].prec
                  )))
        {

I am not sure whether it is needed to make sure comps[0].prec = comps[1].prec = comps[2].prec = comps[3].prec, also comps[i].sgnd.

Besides, I find other converter functions either are not necessary to add this limitation or already have it:

• unnecessary: imagetopgx (.pgx), imagetoraw (.raw), imagetorawl
• already have: imagetotif (.tif), imagetotga (.tga), imagetopng (.png)
• both: imagetobmp (.bmp)

I wonder if the added limitation in your patch will cause "imagetopgx", "imagetoraw", "imagetorawl" work incorrectly. I am not familiar with the specification.

[twelveand0]

@szukw000 @attritionorg For #863, I think it has not been solved. "comps[compno].data" can be referred in color-related functions before the patched position (i.e. code from src/bin/jp2/opj_decompress.c: 1367 to src/bin/jp2/opj_decompress.c:1461), including "color_sycc_to_rgb", "color_cmyk_to_rgb", "color_esycc_to_rgb", "color_apply_icc_profile", "color_cielab_to_rgb", "clip_component", "scale_component", "convert_gray_to_rgb".

For example, sycc422_to_rgb in src/bin/common.c : 130 (called by “color_sycc_to_rgb” by “main”)：

144    y = img->comps[0].data;
145    cb = img->comps[1].data;
147    cr = img->comps[2].data;
...
167    for(j=0U; j < (loopmaxw & ~(size_t)1U); j += 2U)
168    {
169        sycc_to_rgb(offset, upb, *y, *cb, *cr, r, g, b);

Why not patch it in function "opj_j2k_decode" or "opj_j2k_deocde_tiles" as analyzed in the report. However, I am not familiar with the specification and not sure whether it is feasible.

The attachment is a poc file which can again trigger the above-noted bug, and the related analysis can be found in "Further analysis" section of this report. color_related_sample.zip

[szukw000]

@twelveand0, @attritionorg

PoC1

bin/opj_decompress -i /tmp/PoC1.jp2 -o PoC1.jp2.ppm

opj_decompress.c:1497: RGB(0x7fe398bc4010,0x7fe3989c2010,0x7fe3987c0010) Segmentation fault

Here is the reason (unpatched opj_decompress.c):

bin/opj_decompress -i /tmp/PoC1.jp2 -o PoC1.jp2.png [INFO] Stream reached its end ! opj_decompress.c:1497: RGB(0x7f5415944010,0x7f5415742010,0x7f5415540010) imagetopng: All components shall have the same subsampling, same bit depth, same sign. Aborting [ERROR] Error generating png file. Outfile PoC1.jp2.png not generated decode time: 9 ms

I supposed you yourself did create the file PoC1:

FILE(/tmp/PoC1.jp2) LENG(414)

read_ihdr w(32) h(32) nc(4) bpc(255) <==================== signed(1) depth(128) compress(7) unknown_c(0) ipr(0)

[2]marker(0xff51) read_siz len(50) capabilities(0)[extended: 0] x(0 : 16416) y(0 : 32) <==================== xt(0 : 32) yt(0 : 32) <==================== IMAGE w(16416) h(32) TILE w(32) h(32) <========= nr_components(4) component[0] signed(0) prec(5) hsep(1) vsep(1) component[1] signed(0) prec(5) hsep(1) vsep(1) component[2] signed(0) prec(5) hsep(1) vsep(1) component[3] signed(0) prec(1) hsep(1) vsep(129)

And here the result with the patch:

bin/opj_decompress -i /tmp/PoC1.jp2 -o PoC1.jp2.ppm

[INFO] Stream reached its end ! opj_decompress: All components shall have the same subsampling, same bit depth, same sign. Aborting

PoC2

bin/opj_decompress -i /tmp/PoC2.j2k -o PoC2.j2k.ppm

[INFO] Start to read j2k main header (0). [INFO] Main header has been correctly decoded. [INFO] No decoded area parameters, set the decoded area to the whole image opj_decompress.c:1497: RGB((nil),(nil),(nil)) Segmentation fault

NAME(/tmp/PoC2.j2k) LENG(74)

[2]marker(0xff51) siz len(47) capabilities(0)[extended: 0] x(0 : 49198) y(0 : 1) <================== xt(0 : 6714467) yt(0 : 1879048256) <================== IMAGE w(49198) h(1) TILE w(6714467) h(1879048256) <===== nr_components(3) component[0] signed(0) prec(8) hsep(1) vsep(1) component[1] signed(0) prec(8) hsep(1) vsep(1) component[2] signed(0) prec(8) hsep(1) vsep(1)

And here is the result with the patch:

bin/opj_decompress -i /tmp/PoC2.j2k -o PoC2.j2k.ppm

[INFO] Start to read j2k main header (0). [INFO] Main header has been correctly decoded. [INFO] No decoded area parameters, set the decoded area to the whole image ERROR -> image->comps[0].data == NULL opj_decompress: Outfile PoC2.j2k.ppm not generated Aborting

The pointer to 'color_sycc_to_rgb()' is OK. And can be consided: the patch should not be inserted at line 1497 but at line 1401 of 'opj_decompress.c'.

winfried

[twelveand0]

@szukw000 @attritionorg

PoC1

The patch can absolutely solve the crash of #862. I know the reason is comps[3].dy != comps[0].dy and comps[3].dx != comps[0].dx. What my question is whether it is correct to require all images should satisfy comps[0].dx (dy / prec / sgnd) = comps[1].dx (dy / prec / sgnd) = comps[2].dx (dy / prec / sgnd) = comps[3].dx (dy /prec /sgnd) (if numcomps is 4) before converting to another format.

First, from the code in function "color_sycc_to_rgb", I guess the following situation is allowed

comps[0].dx = 1; comps[1].dx = 2; comps[2].dx = 2
comps[0].dy = 1; comps[1].dy = 2; comps[2].dy = 2

and I did create such sample.

Second, "imagetopgx" and "imagetoraw" do not require the image must satisfy last-noted precondition. So I wonder whether the limitation in the patch is too strong.

Also all converter functions except "imagetopnm" either have already had the similar check or don't need this.

I think it may be better to patch "imagetopnm".

PoC2

I think the patch should be added before line 1374 (maybe a little different):

1374        if(image->color_space == OPJ_CLRSPC_SYCC){
1375            color_sycc_to_rgb(image);
1376        }

Otherwise, the crash will happen again when the execution steps into function "sycc422_to_rgb" through "color_sycc_to_rgb", which can be verified by "color_related_sample". The following is the ASAN exception:

===========================================
The extension of this file is incorrect.
FOUND s_49. SHOULD BE .j2k or .jpc or .j2c
===========================================

[INFO] Start to read j2k main header (0).
[INFO] Main header has been correctly decoded.
[INFO] No decoded area parameters, set the decoded area to the whole image
ASAN:DEADLYSIGNAL
=================================================================
==26463==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x08179bd6 bp 0xbfee27a8 sp 0xbfee24f0 T0)
    #0 0x8179bd5  (/home/fire/bing/afl/libraries/openjpeg-2.1.2/build-patch-asan/bin/opj_decompress+0x8179bd5)
    #1 0x8176aad  (/home/fire/bing/afl/libraries/openjpeg-2.1.2/build-patch-asan/bin/opj_decompress+0x8176aad)
    #2 0x813ab31  (/home/fire/bing/afl/libraries/openjpeg-2.1.2/build-patch-asan/bin/opj_decompress+0x813ab31)
    #3 0xb739f636  (/lib/i386-linux-gnu/libc.so.6+0x18636)
    #4 0x805f977  (/home/fire/bing/afl/libraries/openjpeg-2.1.2/build-patch-asan/bin/opj_decompress+0x805f977)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/home/fire/bing/afl/libraries/openjpeg-2.1.2/build-patch-asan/bin/opj_decompress+0x8179bd5) 
==26463==ABORTING

[twelveand0]

Hello @szukw000 , may i know the status?

[szukw000]

@twelveand0, @attritionorg,

I have now changed the patch.

The test in 'opj_decompress.c' can be shorter in case 'j2k.c' is changed: image->comps[ui].data is tested in 'j2k.c'.

I am unsure about the test in RAW.

winfried flop.dif.zip

[twelveand0]

@szukw000 @attritionorg I have tested it with all of my similar samples and the 2 crashes did not happen again. I think the patch has solved the 2 issues.

For test in RAW, I think the check code in the patch is not needed and can be deleted. The reason is: In converter function "imagetoraw_common", for each comps[compno].data, the total access length is "w h" where w is comps[compno].w and h is comps[compno].h, i.e. for each comps[compno].data the total access length is "comps[compno].w comps[compno].h" instead of "*comps[0].w comps[0].h". This can be got from the following code. (note: all the following line number is the one of unpatched version)**

@src/bin/jp2/convert.c
2156        w = (int)image->comps[compno].w;
2157        h = (int)image->comps[compno].h;
2158
2159        if(image->comps[compno].prec <= 8)
2160        {
2161            if(image->comps[compno].sgnd == 1)
2162            {
2163                mask = (1 << image->comps[compno].prec) - 1;
2164                ptr = image->comps[compno].data;
2165                for (line = 0; line < h; line++) {
2166                    for(row = 0; row < w; row++)    {
2167                        curr = *ptr;
2168                        if(curr > 127) curr = 127; else if(curr < -128) curr = -128;
2169                        uc = (unsigned char) (curr & mask);
2170                        res = fwrite(&uc, 1, 1, rawFile);
2171                        if( res < 1 ) {
2172                            fprintf(stderr, "failed to write 1 byte for %s\n", outfile);
2173                            goto fin;
2174                        }
2175                        ptr++;
2176                    }
2177                }
2178            }

On the other hand, I find the allocation length of each comps[compno].data is also "*comps[compno].w comps[compno].h", which can be found with reverse debugging (e.g. rr). The allocation code for each comps[compno].data** is shown in the following:

@src/lib/openjp2/j2k.c
8218        l_img_comp_dest = p_output_image->comps;
8219
8220        for (i=0; i<l_image_src->numcomps; i++) {
8221
8222                /* Allocate output component buffer if necessary */
8223                if (!l_img_comp_dest->data) {
8224                        OPJ_SIZE_T l_width = l_img_comp_dest->w;
8225                        OPJ_SIZE_T l_height = l_img_comp_dest->h;
8226
8227                        if ((l_height == 0U) || (l_width > (SIZE_MAX / l_height))) {
8228                                /* would overflow */
8229                                return OPJ_FALSE;
8230                        }
8231                        l_img_comp_dest->data = (OPJ_INT32*) opj_calloc(l_width * l_height, sizeof(OPJ_INT32));
8232                        if (! l_img_comp_dest->data) {
8233                                return OPJ_FALSE;
8234                        }
8235                }

From above, I find that for each comps[compno].data the total access length and the allocation length are the same (i.e. *comps[compno].w comps[compno].h**). So there is no need to add check in RAW.

[szukw000]

@twelveand0, @attritionorg,

the conclusion is not true.

RAW must follow the same limitations as e.g. PNG.

In the following example I have limited the number of componentes to be WRITTEN to 3.

opj_compress -h

 -F <width>,<height>,<ncomp>,<bitdepth>,{s,u}@<dx1>x<dy1>:...:<dxn>x<dyn>
    Characteristics of the raw input image
    If subsampling is omitted, 1x1 is assumed for all components
      Example: -F 512,512,3,8,u@1x1:2x2:2x2
               for raw 512x512 image with 4:2:0 subsampling

STEP 1

bin/opj_decompress -i /tmp/PoC1.jp2 -o PoC1.jp2.raw

[INFO] Stream reached its end ! Raw image characteristics: 3 components Component 0 characteristics: 16416x32x5 unsigned Component 1 characteristics: 16416x32x5 unsigned Component 2 characteristics: 16416x32x5 unsigned [INFO] Generated Outfile PoC1.jp2.raw decode time: 10 ms

STEP 2

mv PoC1.jp2.raw PoC1.jp2-16416x32x5x3.raw

STEP 3

bin/opj_compress -i PoC1.jp2-16416x32x5x3.raw -o PoC1.jp2-16416x32x5x3.raw.jp2 -F 16416,32,3,5,u@1x1:1x1:1x1

[INFO] tile number 1 / 1 [INFO] Generated outfile PoC1.jp2-16416x32x5x3.raw.jp2 encode time: 79 ms

This means that for an RGB image all dx/dy/prec/sgnd pairs must be the same for 3 channels.

And if the source image has 4 channels the fourth channel must have the same pairs as the 3 channels.

Otherwise the creation to RAW fails.

PoC1.jp2 succeeds with 3 channels but must fail with 4 channels.

winfried PoC1.jp2.raw.jp2.zip

[twelveand0]

@szukw000 @attritionorg I got it. I think the patch has correctly solved the 2 issues. Thanks. Now, can i continue to request CVE ids?

[szukw000]

@twelveand0 ,

Now, can i continue to request CVE ids?

Eh, I do not understand.

But: somebody should send a final patch to the developers.

winfried

[twelveand0]

@szukw000,

Can you send the final patch to the developers? I find you are one of the contributors.

[twelveand0]

@szukw000 Thanks for your work.

[lfam]

What's the status of this commit "Patch for crashes #811,#862,#863,#871 and #872" (7ee3b42ad967da5c9d0582a14994e20a6b6d1ad6)?

Will it be added to the official OpenJPEG repository?

[rouault]

I believe this has now been addressed by #895