Comment 41 for bug 711061

I reviewed openjpeg version 1:1.5.2-3.1 as checked into Xenial. This
should not be considered a full security audit.

Security team NAK for moving openjpeg to main for 16.04.

The source is nearly unreadable due to terrible formatting. I could not
find any tab-sizes that allowed the code to be legible by any reasonable
standard. I strongly recommend upstream should (a) run the entire codebase
through indent(1) or a similar tool (b) enforce a standard, any standard.
K&R recommended but really anything beats what this currently has.

In one day of fuzzing with my laptop I found eight crashers that caused
segmentation violation faults. They may be read-only or perhaps they may
allow remote code execution but the source code is so difficult to read
that I cannot volunteer my time to discover the causes.

cppcheck as packaged with 14.04 LTS issued 21 warnings; almost all
represent real bugs in the package.

Upstream's 2.1.0 release is a mixed bag -- cppcheck reported 61 warnings;
many looked similar to issues from 1.5.2. However, the eight crashing
inputs I found no longer crashed the library. Less than four hours of
fuzzing on my laptop found an input that crashes.

Memory management is sloppy and may in fact provide routes for exploits.

Integer types seem very casual; this library was not written with a
strong awareness of C's extremely dangerous behaviours around signed
integers, the usual integer promotion rules, and sign extension rules.

In addition, this looks like a poor library interface -- errors are sent
directly to stderr without any attempts to offer client programs any
mediation, the error messages never include strerror() reasons meaning
users have no feedback on what failed.

We cannot support this library as it stands. Before we can support it,
the source code layout needs to be addressed, cppcheck should be clean,
and gcc warnings should be addressed. I strongly recommend sending this
through Coverity as well.

Here's a more concrete list of issues I found; I hope these are useful:

- applications/codec/j2k_dump.c allocates a hugely oversized block of
  memory for getting directory listings. If the dirptr or dirptr->filename
  memory allocations fail, the program dies via NULL pointer write errors.
  This should instead work on one file at a time and avoid the entire
  disaster.
- applications/codec/j2k_dump.c parameters.outfile misleading error
  message, the fopen() was for writing, not reading
- applications/JavaOpenJPEG/JavaOpenJPEGDecoder.c get_num_images(),
  load_images() leak dir
- Java_org_openJpeg_OpenJPEGJavaDecoder_internalDecodeJ2KtoImage() fails
  to close 'fsrc' in this error path: if(src == NULL) goto fin
- OPJMarkerTree::OnSelChanged() allocates buffer with new[] but
  deallocates with delete
- bmptoimage() never checks getc() error return code
- bmptoimage() leaks IN
- imagetopgx() leaks name via if (!fdest) error -- memory handling here is
  far too complicated
- applications/codec/image_to_j2k.c main() far too complicated
- applications/codec/image_to_j2k.c main() leaks f (not really relevant
  since the 'return 1' exits, but once this code is broken apart into
  something legible this will matter more)
- yuv_num_frames() leaks f via if (end_of_f < frame_size) error
- applications/mj2/mj2_to_frames.c main() far too complicated; leaks
  frame_codestream in the event of realloc() failure
- xml_write_mdia() writes garbage via fprintf() with too few arguments
- test_image() calls opj_image_destroy(image); on an image without a valid
  value via if(cio == NULL) error -- note doubled if(image == NULL) goto
  fin; checks as well, looks like copy-and-paste errors in this function
  too
- read_siz_marker() does no fread()-related error checking
- read_siz_marker() left-shifts plain 'char' values, on platforms with
  default signed chars this will probably cause a hugely negative value,
  which when passed to malloc(size_t) will be turned into a hugely
  positive value
- applications/mj2/wrap_j2k_in_mj2.c main() far too complicated
- applications/mj2/wrap_j2k_in_mj2.c main() calls free(buf) with undefined
  'buf' value via if(cinfo == NULL) error path

Notes to the future:

Find a jpeg2000 image somewhere (I'll include one in my tarball of
crashers) and stick it in fuzz/in/
ccmake to change the CC to /usr/bin/afl-gcc
Build with AFL_HARDEN=1 make
mkdir -p fuzz/in fuzz/out
cd fuzz
afl -i in -o out -f foo.j2k ../bin/<whatever the executable is> -i @@ -o foo.bmp

Thanks