Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Openjpeg-2.1.2 DoS vulnerability due to some logic error #863

Closed
twelveand0 opened this Issue Nov 2, 2016 · 17 comments

Comments

Projects
None yet
5 participants
@twelveand0
Copy link

twelveand0 commented Nov 2, 2016

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

This comment has been minimized.

Copy link

attritionorg commented Nov 2, 2016

Dupe to #857?

@twelveand0

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 3, 2016

@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

This comment has been minimized.

Copy link
Contributor

szukw000 commented Nov 21, 2016

@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

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 22, 2016

@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

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 22, 2016

@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

This comment has been minimized.

Copy link
Contributor

szukw000 commented Nov 22, 2016

@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

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 22, 2016

@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

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 23, 2016

Hello @szukw000 , may i know the status?

@szukw000

This comment has been minimized.

Copy link
Contributor

szukw000 commented Nov 24, 2016

@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

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 25, 2016

@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

This comment has been minimized.

Copy link
Contributor

szukw000 commented Nov 26, 2016

@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

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 26, 2016

@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

This comment has been minimized.

Copy link
Contributor

szukw000 commented Nov 26, 2016

@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

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 26, 2016

@szukw000,

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

@twelveand0

This comment has been minimized.

Copy link
Author

twelveand0 commented Nov 30, 2016

@szukw000
Thanks for your work.

@lfam

This comment has been minimized.

Copy link

lfam commented Jan 21, 2017

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

Will it be added to the official OpenJPEG repository?

@rouault

This comment has been minimized.

Copy link
Collaborator

rouault commented Aug 9, 2017

I believe this has now been addressed by #895

@rouault rouault closed this Aug 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.