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.

Sign up for GitHub

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

Jump to bottom

Heap-buffer-overflow in opj_tcd_init_decode_tile #431

Closed
gcode-importer opened this issue Nov 14, 2014 · 11 comments
Closed

Heap-buffer-overflow in opj_tcd_init_decode_tile #431

gcode-importer opened this issue Nov 14, 2014 · 11 comments
Assignees
@mayeut
Labels
bug Priority-Critical Restrict-View-CoreTeam

Comments

@gcode-importer
Copy link

Originally reported on Google Code with ID 431

https://code.google.com/p/chromium/issues/detail?id=431288

Reproduced on trunk r2924 using MacOS x86 ASan build :
ASAN_OPTIONS="allocator_may_return_null=1" ./bin/opj_decompress -i ../../ex/0.jp2 -o
0.bmp

[INFO] Start to read j2k main header (119).
[INFO] Main header has been correctly decoded.
[INFO] No decoded area parameters, set the decoded area to the whole image
ERROR in tgt_create while allocating node of the tree
WARNING: No incltree created.
ERROR in tgt_create while allocating node of the tree
WARNING: No imsbtree created.
=================================================================
==3245==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x02f03d00 at pc 0x007f193c
bp 0xbffa61f8 sp 0xbffa61f4
READ of size 4 at 0x02f03d00 thread T0
    #0 0x7f193b in opj_tcd_code_block_dec_allocate /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/tcd.c:1051:13
    #1 0x7f0bed in opj_tcd_init_decode_tile /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/tcd.c:1012:1
    #2 0x79bfce in opj_j2k_read_tile_header /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:8008:15
    #3 0x7b3007 in opj_j2k_decode_tiles /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:9543:23
    #4 0x799b07 in opj_j2k_exec /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:7435:41
    #5 0x7a3723 in opj_j2k_decode /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:9762:15
    #6 0x7ba8df in opj_jp2_decode /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/jp2.c:1398:8
    #7 0x7c6c83 in opj_decode /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/openjpeg.c:412:10
    #8 0x5d12c in main /Users/Matt/Dev/OpenJpeg/issue398/src/bin/jp2/opj_decompress.c:821:10
    #9 0x9a68d6d8 in start (/usr/lib/system/libdyld.dylib+0x36d8)
    #10 0x4 (<unknown module>)

0x02f03d00 is located 0 bytes to the right of 7168-byte region [0x02f02100,0x02f03d00)
allocated by thread T0 here:
    #0 0x2c0dba in wrap_malloc (/Users/Matt/Dev/llvm-clang-3.5.0-macosx-apple-darwin/lib/clang/3.5.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x2fdba)
    #1 0x7f08b9 in opj_tcd_init_decode_tile /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/tcd.c:1012:1
    #2 0x79bfce in opj_j2k_read_tile_header /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:8008:15
    #3 0x7b3007 in opj_j2k_decode_tiles /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:9543:23
    #4 0x799b07 in opj_j2k_exec /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:7435:41
    #5 0x7a3723 in opj_j2k_decode /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/j2k.c:9762:15
    #6 0x7ba8df in opj_jp2_decode /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/jp2.c:1398:8
    #7 0x7c6c83 in opj_decode /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/openjpeg.c:412:10
    #8 0x5d12c in main /Users/Matt/Dev/OpenJpeg/issue398/src/bin/jp2/opj_decompress.c:821:10
    #9 0x9a68d6d8 in start (/usr/lib/system/libdyld.dylib+0x36d8)
    #10 0x4 (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow /Users/Matt/Dev/OpenJpeg/issue398/src/lib/openjp2/tcd.c:1051
opj_tcd_code_block_dec_allocate
Shadow bytes around the buggy address:
  0x205e0750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x205e0760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x205e0770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x205e0780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x205e0790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x205e07a0:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x205e07b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x205e07c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x205e07d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x205e07e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x205e07f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  ASan internal:           fe
==3245==ABORTING

Reported by mayeut on 2014-11-14 19:45:14
@gcode-importer
Copy link
Author

Reported by mayeut on 2014-11-14 19:45:41

- _Attachment: [issue431.jp2](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-431/comment-1/issue431.jp2)_

@gcode-importer
Copy link
Author 

@antonin,

The patch is OK against the test suite however we might want to do more than just this.

I do not this why the standard would reject such SIZ marker values but if there's a
reason, an early check should be added (in this case, Ysiz, Y0siz, YTsiz, YT0siz combination
seems valid to me even if YTsiz seems corrupted).

l_tile->x0 = (OPJ_INT32)opj_uint_max(l_cp->tx0 + p * l_cp->tdx, l_image->x0);
l_tile->y0 = (OPJ_INT32)opj_uint_max(l_cp->ty0 + q * l_cp->tdy, l_image->y0);
l_tile->x1 = (OPJ_INT32)opj_uint_min(l_cp->tx0 + (p + 1) * l_cp->tdx, l_image->x1);
l_tile->y1 = (OPJ_INT32)opj_uint_min(l_cp->ty0 + (q + 1) * l_cp->tdy, l_image->y1);

We can see that for x0/y0, the only thing that could go bad is them becoming negative
& we should probably add a check for this (this is if check on SIZ marker are OK since
tile->x0/y0 shall fall in the reference grid which is bound to 32 bit value right ?).
For x1/y1, the same check should exist but we can also see that ty0 + (q+1)* tdy can
overflow thus leading in an invalid min value being computed.

Any thoughts ?

Reported by mayeut on 2014-11-20 23:00:45

  • Status changed: Started
- _Attachment: [issue431.patch](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-431/comment-2/issue431.patch)_

@gcode-importer
Copy link
Author 

Update patch for r2955

Reported by mayeut on 2014-12-12 21:58:09

- _Attachment: [issue431.patch](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-431/comment-3/issue431.patch)_

@gcode-importer
Copy link
Author 

This issue was updated by revision r2991.

Reported by detonin on 2015-01-15 18:16:39

@gcode-importer
Copy link
Author 

@matthieu: thanks for the patch (which has been applied). Regarding the additional checks,
could you propose a patch ? A possible solution would also be to change the type of
x0/y0/x1/y1 to OPJ_UINT32, what do you think ?

Reported by detonin on 2015-01-15 18:20:38

@gcode-importer
Copy link
Author 

@antonin,

We should most probably change the type to OPJ_UINT32. There will be a need for overflow
check only for x1/y1 this way.
I'll try to see what I can do in the next couple days.

Reported by mayeut on 2015-01-20 21:52:17

@gcode-importer
Copy link
Author 

@antonin,

Unfortunately, I won't have time to get into this in the near future. Changing those
to OPJ_UINT32 has impacts in almost all files if we wan't to get this right and I just
don't have the time right now.

Reported by mayeut on 2015-01-25 15:39:22

@gcode-importer
Copy link
Author 

@antonin,

I added the "overflow check" (in fact it's a saturated add in the patch) which is needed
in any case.

There's no checks for negative values in case you wan't to go with the whole OPJ_UINT32
approach.

Reported by mayeut on 2015-01-25 16:44:04

- _Attachment: [issue431.patch](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-431/comment-8/issue431.patch)_

@gcode-importer
Copy link
Author 

This issue was closed by revision r2997.

Reported by detonin on 2015-02-02 16:11:37

  • Status changed: Fixed

@gcode-importer
Copy link
Author 

Thanks mdarbois

Reported by detonin on 2015-02-02 16:12:56

@eslerm
Copy link

eslerm commented Sep 16, 2022 •
edited
Loading

This was assigned CVE-2018-20847.

Commit 5d00b71 and 2d24b60 mention resolving this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mayeut mayeut

Labels
bug Priority-Critical Restrict-View-CoreTeam
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mayeut @gcode-importer @eslerm