insufficient sanitization of user-supplied data

GoogleCodeExporter commented 9 years ago

It seems that in many cases, properties read from a JPEG 2000 image are used 
for determining memory read/write addresses without first verifying that the 
values are valid at all (such as image and tile dimensions, component and 
colorspace indices, etc.).

I have been made available a collection of files which all result in read/write 
access violations when used with the latest SVN trunk code. A dozen fixes for 
the encountered issues can be found at 
https://github.com/zeniko/openjpeg/commits/fixes including explanations for why 
they are necessary. A systematic code review by someone more accustomed with 
openjpeg will likely find further such potentially exploitable issues.

In case you have a regression test suite and aren't able to produce minimal 
test cases for all of these issues yourselves, please tell me where I can send 
some of the images that lead to these fixes. They would have to be treated 
confidentially, though, per the files' original author's request (no public 
sharing).

Original issue reported on code.google.com by zeniko on 8 Jun 2013 at 6:31

GoogleCodeExporter commented 9 years ago

FYI: The fixes have moved to 
http://git.ghostscript.com/?p=user/zeniko/openjpeg.git;a=shortlog;h=refs/heads/z
eniko_fixes . For many of them, I've shared testcases with Mathieu within a 
week of this report. Is there anything missing for you to be able to review the 
changes and merge them into your own repository?

Original comment by zeniko on 4 Oct 2013 at 3:08

GoogleCodeExporter commented 9 years ago

FYI: The current state of the patches have moved to 
http://git.ghostscript.com/?p=user/zeniko/ghostpdl.git;a=history;f=gs/openjpeg;h
b=HEAD . Also, there are further fixes for newly discovered issues.

Original comment by zeniko on 13 Jan 2014 at 6:01

GoogleCodeExporter commented 9 years ago

Actual patch can also be found at:

https://code.google.com/p/sumatrapdf/source/browse/trunk/ext/_patches/openjpeg.p
atch

Original comment by mathieu.malaterre on 25 Feb 2014 at 10:01

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2406.

Original comment by mathieu.malaterre on 25 Feb 2014 at 10:02

GoogleCodeExporter commented 9 years ago

Original comment by mathieu.malaterre on 25 Feb 2014 at 12:43

Added labels: Priority-High
Removed labels: Priority-Medium

GoogleCodeExporter commented 9 years ago

$ wget 
"http://git.ghostscript.com/?p=user/zeniko/ghostpdl.git;a=patch;h=f4139d70255964
9e577a5df9cfd64b0ca6107a7a"
$ patch -p4 < ghostpdl.git-f4139d702559649e577a5df9cfd64b0ca6107a7a.patch

Applied as commit r2413

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:30

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2414.

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:33

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2415.

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:38

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2416.

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:39

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2417.

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:50

GoogleCodeExporter commented 9 years ago

I cannot apply the following patch:

http://git.ghostscript.com/?p=user/zeniko/ghostpdl.git;a=patch;h=f56309341f78282
81f05ea7c43d63691f68d1c81

Need to inspect case. But clearly 451.pdf.SIGSEGV.ce9.3723.jp2 produce invalid 
write access:

$ valgrind ./bin/opj_decompress -i 
data/input/nonregression/451.pdf.SIGSEGV.ce9.3723.jp2 -o toto.pgx
==13435== Memcheck, a memory error detector
==13435== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==13435== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==13435== Command: ./bin/opj_decompress -i 
data/input/nonregression/451.pdf.SIGSEGV.ce9.3723.jp2 -o toto.pgx
==13435== 

[INFO] Start to read j2k main header (256).
[INFO] Main header has been correctly decoded.
[INFO] No decoded area parameters, set the decoded area to the whole image
==13435== Invalid write of size 8
==13435==    at 0x4E47B95: opj_j2k_add_tlmarker (j2k.c:6413)
==13435==    by 0x4E49FC6: opj_j2k_read_tile_header (j2k.c:7388)
==13435==    by 0x4E4E487: opj_j2k_decode_tiles (j2k.c:9011)
==13435==    by 0x4E48F1E: opj_j2k_exec (j2k.c:6934)
==13435==    by 0x4E4EC08: opj_j2k_decode (j2k.c:9227)
==13435==    by 0x4E52A20: opj_jp2_decode (jp2.c:1178)
==13435==    by 0x4E5797B: opj_decode (openjpeg.c:525)
==13435==    by 0x404B55: main (opj_decompress.c:818)
==13435==  Address 0x64c4850 is not stack'd, malloc'd or (recently) free'd

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:57

GoogleCodeExporter commented 9 years ago

I would need test case to import:

http://git.ghostscript.com/?p=user/zeniko/ghostpdl.git;a=commitdiff;h=b13ef73290
7219b48757e2f501486f71b9f21030

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:58

GoogleCodeExporter commented 9 years ago

Same issue with this one, I would need input dataset:

http://git.ghostscript.com/?p=user/zeniko/ghostpdl.git;a=commitdiff;h=5b0c9985e3
359aca9b3fcfd94424166aa61a141a

Original comment by mathieu.malaterre on 25 Feb 2014 at 1:59

GoogleCodeExporter commented 9 years ago

What about patch:

http://git.ghostscript.com/?p=user/zeniko/ghostpdl.git;a=commitdiff;h=d6121c78a0
fc6c8cd87c6495e11d327068518c35

Should I add MSVC idefs blockers ?

Original comment by mathieu.malaterre on 25 Feb 2014 at 2:02

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2418.

Original comment by mathieu.malaterre on 25 Feb 2014 at 2:03

GoogleCodeExporter commented 9 years ago

This patch is supicious:

http://git.ghostscript.com/?p=user/zeniko/ghostpdl.git;a=commitdiff;h=0f07c3247c
c9211e57bb5429634f79c8c762fbfa

I cannot find how jp2.c could allow:

if (p_colr_header_size > 7) {}

Original comment by mathieu.malaterre on 25 Feb 2014 at 2:09

GoogleCodeExporter commented 9 years ago

r2415 has been reverted in r2419 since it makes test such as 
ETS-C1P1-p1_02.j2k-decode fails.

Original comment by mathieu.malaterre on 25 Feb 2014 at 2:15

GoogleCodeExporter commented 9 years ago

for ref 0f07c3247cc9211e57bb5429634f79c8c762fbfa see issue 247 (Part-2 file)

Original comment by mathieu.malaterre on 25 Feb 2014 at 3:50

Added labels: Milestone-Release2.1

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2428.

Original comment by mathieu.malaterre on 25 Feb 2014 at 4:17

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2430.

Original comment by mathieu.malaterre on 25 Feb 2014 at 4:36

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2450.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:03

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2451.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:04

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2452.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:05

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2453.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:06

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2454.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:08

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2455.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:09

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2456.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:10

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2457.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:10

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2458.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:11

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2459.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:13

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2460.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:14

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2462.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:17

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2463.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:21

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2466.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:25

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2467.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:26

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2465.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:27

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2468.

Original comment by mathieu.malaterre on 26 Feb 2014 at 11:29

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2470.

Original comment by mathieu.malaterre on 26 Feb 2014 at 12:33

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2484.

Original comment by mathieu.malaterre on 26 Feb 2014 at 3:42

GoogleCodeExporter commented 9 years ago

Thanks for merging most of our patches.

WRT comment #12 and comment #13: I've sent you the two requested files.

WRT comment #14: That's up to you. If you don't, we'll have to keep patching 
that bit ourselves in order to get our own test suite to run through.

WRT comment #16: jp2.c passes in size and data of the COLR box without prior 
sanity checks (only opj_jp2_read_colr knows what values are expected). 
p_colr_header_size can have whatever value a (broken) file wants it to be.

WRT comment #17: Does this mean that you'll look into alternative fixes so that 
the crashes don't remain?

Original comment by zeniko on 26 Feb 2014 at 9:46

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2502.

Original comment by mathieu.malaterre on 27 Feb 2014 at 2:42

GoogleCodeExporter commented 9 years ago

I am now working on patch from:
http://bugs.ghostscript.com/show_bug.cgi?format=multiple&id=694893

If I try to reproduce it from a linux+valgrind (exp-sgcheck) on amd64 I cannot 
trigger the stack smashing issue reported. I am tempted to create a separate 
issue, since it really smell like issue 231

Original comment by mathieu.malaterre on 27 Feb 2014 at 2:46

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2504.

Original comment by mathieu.malaterre on 27 Feb 2014 at 3:00

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2505.

Original comment by mathieu.malaterre on 27 Feb 2014 at 3:01

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2521.

Original comment by mathieu.malaterre on 28 Feb 2014 at 2:53

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2524.

Original comment by mathieu.malaterre on 28 Feb 2014 at 3:24

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2525.

Original comment by mathieu.malaterre on 28 Feb 2014 at 3:27

GoogleCodeExporter commented 9 years ago

This issue was updated by revision r2549.

Original comment by mathieu.malaterre on 3 Mar 2014 at 11:48

GoogleCodeExporter commented 9 years ago

Comments #12, #13, #14, #16 and #17 have now all been addressed. Only a single 
one is left: comment #42, which is already reference in issue 231. Follow 
updates from there.
Closing now.

Original comment by mathieu.malaterre on 3 Mar 2014 at 11:52

Changed state: Fixed

xinxinlx / openjpeg

insufficient sanitization of user-supplied data #225