Invalid image causes write past end of heap buffer #476

Closed
gcode-importer opened this Issue Feb 17, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@gcode-importer

Originally reported on Google Code with ID 476

The invalid, fuzzer-generated image files in the attachment cause the following code
in opj_pi_next_cprl or opj_pi_next_lrcp to write past the end of a heap buffer, corrupting
the malloc arena and causing a crash:

    if (!pi->include[index]) {
        pi->include[index] = 1;
        return OPJ_TRUE;
    }

What steps will reproduce the problem?
1. unzip openjpeg-svn-id000179-opj_pi_next_cprl-sampes.zip
2. bin/opj_decompress -i openjpeg-svn-id000179-opj_pi_next_cprl-buffer-overflow.jp2
-o output.pnm

What is the expected output? What do you see instead?
It crashes due to corruption of malloc's data structures:
*** Error in `bin/opj_decompress': free(): invalid next size (fast): 0x00007f998ceab550
***

What version of the product are you using? On what operating system?
Tested with both 2.10 and SVN revision r2997 on 64bit Linux.

Please provide any additional information below.
This allows an attacker to change an 0x0000 to an 0x0001 at some location beyond the
end of the heap buffer, not necessarily immediately after it - I've seen overwrites
16 bytes or more beyond the end. While the attached images merely crash the decoder,
a clever attacker might be able to use this for arbitrary code execution either by
corrupting malloc's data structures or other openjpeg data structures.

Reported by makosoft on 2015-02-17 12:29:39


- _Attachment: [openjpeg-svn-id000179-opj_pi_next_cprl-samples.zip](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-476/comment-0/openjpeg-svn-id000179-opj_pi_next_cprl-samples.zip)_
@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer Feb 21, 2015

Reported by detonin on 2015-02-21 14:32:36

  • Labels added: Priority-Critical, Restrict-View-CoreTeam
  • Labels removed: Priority-Medium

Reported by detonin on 2015-02-21 14:32:36

  • Labels added: Priority-Critical, Restrict-View-CoreTeam
  • Labels removed: Priority-Medium
@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer Feb 21, 2015

Thanks for marking this private, and sorry again about that.

So, I got around to having another look earlier. Turns out that the problem is that
there are two COD markers in the MH which differ in the number of layers they claim
the image has. The first one sets the number of layers to 9, then there are a bunch
of POC entries which have their layer numbers clamped based on that number, then another
COD marker comes along and changes the number of layers to 2, leaving a bunch of POC
data structures referring to layer numbers that are now out of bounds.

The attached patch disallows COD markers after the first POC marker is read for that
tile which should fix the problem.

Reported by makosoft on 2015-02-21 21:16:19


- _Attachment: [0001-Don-t-allow-COD-marker-after-first-POC-for-that-tile.patch](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-476/comment-2/0001-Don-t-allow-COD-marker-after-first-POC-for-that-tile.patch)_
Thanks for marking this private, and sorry again about that.

So, I got around to having another look earlier. Turns out that the problem is that
there are two COD markers in the MH which differ in the number of layers they claim
the image has. The first one sets the number of layers to 9, then there are a bunch
of POC entries which have their layer numbers clamped based on that number, then another
COD marker comes along and changes the number of layers to 2, leaving a bunch of POC
data structures referring to layer numbers that are now out of bounds.

The attached patch disallows COD markers after the first POC marker is read for that
tile which should fix the problem.

Reported by makosoft on 2015-02-21 21:16:19


- _Attachment: [0001-Don-t-allow-COD-marker-after-first-POC-for-that-tile.patch](https://storage.googleapis.com/google-code-attachments/openjpeg/issue-476/comment-2/0001-Don-t-allow-COD-marker-after-first-POC-for-that-tile.patch)_
@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer Apr 20, 2015

+cc jun fang so that it can follow the bug on openjpeg side.

Sorry I did not have to opportunity to dig into this earlier.
Thanks to Makosoft for having investigated the origin of the problem. The J2K spec
does not allow more than one COD marker in MH so I guess a better patch would be to
add a check for this without connecting this to the presence or absence of any POC
marker. Could you update the patch accordingly ? I'll apply it as soon as it's available.
Many thanks. 

Reported by detonin on 2015-04-20 21:14:28

  • Status changed: Accepted
+cc jun fang so that it can follow the bug on openjpeg side.

Sorry I did not have to opportunity to dig into this earlier.
Thanks to Makosoft for having investigated the origin of the problem. The J2K spec
does not allow more than one COD marker in MH so I guess a better patch would be to
add a check for this without connecting this to the presence or absence of any POC
marker. Could you update the patch accordingly ? I'll apply it as soon as it's available.
Many thanks. 

Reported by detonin on 2015-04-20 21:14:28

  • Status changed: Accepted
@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer Apr 20, 2015

Hi antonin,

Can you cc thestig@chromium.org and tsepez@chromium.org? Thanks!

Reported by jun_fang@foxitsoftware.com on 2015-04-20 21:25:04

Hi antonin,

Can you cc thestig@chromium.org and tsepez@chromium.org? Thanks!

Reported by jun_fang@foxitsoftware.com on 2015-04-20 21:25:04

@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer May 1, 2015

I don't think I understand the openjpeg code well enough to write a patch that does
that, sorry. There doesn't appear to be any way of telling whether a COD marker has
been parsed in the current tile that I can see, and I'm not sure how to safely add
a flag for it either.

Reported by makosoft on 2015-05-01 12:45:15

I don't think I understand the openjpeg code well enough to write a patch that does
that, sorry. There doesn't appear to be any way of telling whether a COD marker has
been parsed in the current tile that I can see, and I'm not sure how to safely add
a flag for it either.

Reported by makosoft on 2015-05-01 12:45:15

@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer May 19, 2015

Reported by mayeut on 2015-05-19 19:17:12

  • Status changed: Started

Reported by mayeut on 2015-05-19 19:17:12

  • Status changed: Started
@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer May 19, 2015

Reported by mayeut on 2015-05-19 19:17:33

Reported by mayeut on 2015-05-19 19:17:33

@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer May 19, 2015

This issue was updated by revision r2998.

Reported by mayeut on 2015-05-19 19:50:47

This issue was updated by revision r2998.

Reported by mayeut on 2015-05-19 19:50:47

@gcode-importer

This comment has been minimized.

Show comment
Hide comment
@gcode-importer

gcode-importer May 19, 2015

This issue was closed by revision r2999.

Reported by mayeut on 2015-05-19 20:13:47

  • Status changed: Fixed
This issue was closed by revision r2999.

Reported by mayeut on 2015-05-19 20:13:47

  • Status changed: Fixed

rouault added a commit that referenced this issue Nov 30, 2017

opj_j2k_read_cod: remove check for 'No more than one COD marker per t…
…ile' (fixes #1043)

This check was added per daed8cc
to fix #476 , but it does not seem
to be necessary with latest master (issue476.jp2 doesn't cause memory issues),
and breaks reading legit files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment