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

sprintf buffer overflow #1088

Closed
setharnold opened this issue Feb 22, 2018 · 15 comments
Closed

sprintf buffer overflow #1088

setharnold opened this issue Feb 22, 2018 · 15 comments

Comments

@setharnold
Copy link
Contributor

openjpeg/src/bin/mj2/opj_mj2_extract.c

Line 143 in 06f7d41

sprintf(outfilename, "%s_%05d.j2k", argv[2], snum);

Hello, it looks like this sprintf(3) could overflow the buffer that is supplied for it.

Thanks
@kbabioch
Copy link
Contributor

kbabioch commented Mar 2, 2018 •
edited
Loading

This is indeed a problem, when appropriate binary is built (-DBUILD_MJ2=On).

./build/bin/opj_mj2_extract Speedway.mj2 01234567890123456789012345678901234567890123456789                                                                                                               1 ↵
Extracting 200 frames from file...
=================================================================
==22782==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffc920b6792 at pc 0x000000437008 bp 0x7ffc920b6610 sp 0x7ffc920b5da8
WRITE of size 61 at 0x7ffc920b6792 thread T0
    #0 0x437007 in __interceptor_vsprintf /home/abuild/rpmbuild/BUILD/llvm-3.8.0.src/stage2/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:1128:1
    #1 0x43832b in __interceptor_sprintf /home/abuild/rpmbuild/BUILD/llvm-3.8.0.src/stage2/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:1159:1
    #2 0x4dd97a in main /home/kbabioch/openjpeg/src/bin/mj2/opj_mj2_extract.c:143:9
    #3 0x7fc128c5b724 in __libc_start_main /usr/src/debug/glibc-2.22/csu/libc-start.c:289
    #4 0x41ba68 in _start /home/abuild/rpmbuild/BUILD/glibc-2.22/csu/../sysdeps/x86_64/start.S:118

Address 0x7ffc920b6792 is located in stack of thread T0 at offset 146 in frame
    #0 0x4dcfaf in main /home/kbabioch/openjpeg/src/bin/mj2/opj_mj2_extract.c:75

  This frame has 3 object(s):
    [32, 56) 'event_mgr'
    [96, 146) 'outfilename'
    [192, 16612) 'parameters' <== Memory access at offset 146 partially underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /home/abuild/rpmbuild/BUILD/llvm-3.8.0.src/stage2/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:1128:1 in __interceptor_vsprintf
Shadow bytes around the buggy address:
  0x10001240eca0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ecb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ecc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ecd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ece0: f1 f1 f1 f1 00 00 00 f2 f2 f2 f2 f2 00 00 00 00
=>0x10001240ecf0: 00 00[02]f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00
  0x10001240ed00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ed10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ed20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ed30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001240ed40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==22782==ABORTING

kbabioch added a commit to kbabioch/openjpeg that referenced this issue Mar 2, 2018
@kbabioch
opj_mj2_extract: Check provided output prefix for length
048b447 
This uses snprintf() with correct buffer length instead of sprintf(). This
prevents a buffer overflow when providing a long output prefix. Furthermore
the program exits with an error when the provided output prefix is too long.

Fixes uclouvain#1088.
kbabioch added a commit to kbabioch/openjpeg that referenced this issue Mar 2, 2018
@kbabioch
opj_mj2_extract: Check provided output prefix for length
6d8c0c0 
This uses snprintf() with correct buffer length instead of sprintf(). This
prevents a buffer overflow when providing a long output prefix. Furthermore
the program exits with an error when the provided output prefix is too long.

Fixes uclouvain#1088.
kbabioch added a commit to kbabioch/openjpeg that referenced this issue Mar 2, 2018
@kbabioch
opj_mj2_extract: Check provided output prefix for length
b1f02d4 
This uses snprintf() with correct buffer length instead of sprintf(), which
prevents a buffer overflow when providing a long output prefix. Furthermore
the program exits with an error when the provided output prefix is too long.

Fixes uclouvain#1088.
@kbabioch kbabioch mentioned this issue Mar 2, 2018
Closed
@kbabioch
Copy link
Contributor

kbabioch commented Mar 2, 2018

This has been assigned CVE-2018-7648

@OrenGitHub
Copy link

Hi @kbabioch , is it possible to publish the file Speedway.mj2 for completeness?
I'm currently working on building a one stop public shop for string CVE's and I want to have complete examples. Thanks!

@kbabioch
Copy link
Contributor

kbabioch commented Mar 8, 2018

Hi @OrenGitHub,

here you go: Speedway.mj2.tar.gz. Unfortunately I had to put it into an tarball, as GitHub does not support this filetype.

@OrenGitHub
Copy link

Thanks, it works. I apologize for being greedy, but would it be possible to upload a file with just 1 frame instead of 200? from the CVE point of view, a minimal example is best, and the overflow occurs in the very first iteration in:

for (snum = 0; snum < track->num_samples; snum++) {

(I've tried editing the file with bless, but can't seem to find the 200 frames number)
Thanks!!

@kbabioch
Copy link
Contributor

The file is really not relevant here. The buffer overflow affects the CLI parameter processing, not the image handling itself.

@OrenGitHub
Copy link

I agree, but for example, if the mj2 file has incorrect format, then the buffer overflow statement might not be executed. I'm trying to synthesize the smallest legal mj2 file that will enable the execution of the buffer overflow statement:

sprintf(outfilename, "%s_%05d.j2k", argv[2], snum);

@OrenGitHub
Copy link

I've tried manually editing the *.mj2 file (to make it smaller but still valid) but I keep getting:

[ERROR] MOOV box not found in file

Which means I'm somehow breaking its validity ... any advice? thanks!
I've also posted on SO, in case other users can help: https://stackoverflow.com/questions/49234803/synthesize-a-small-legal-mj2-file

@szukw000
Copy link
Contributor

@setharnold, @kbabioch, @OrenGitHub,

the simplest patch follows:

--- openjpeg2-2018-02-16/src/bin/mj2/opj_mj2_extract.c.orig 2018-03-12 17:22:15.956095125 +0100
+++ openjpeg2-2018-02-16/src/bin/mj2/opj_mj2_extract.c  2018-03-12 17:24:26.382097168 +0100
@@ -91,6 +91,11 @@
         return 1;
     }

+    if(strlen(argv[2]) + 11 > sizeof(outfilename)) {
+        fprintf(stderr,"filename %d too long\n",strlen(argv[2]) + 10);
+        return 1;
+    }
+
     file = fopen(argv[1], "rb");

     if (!file) {

@OrenGitHub ,
manually editing the *.mj2 file is a very bad idea. What do you want to do with this
MJ2 file?
winfried

@OrenGitHub
Copy link

I'm analyzing the openjpeg package with symbolic execution (SE).
The SE engine tests the program ( opj_mj2_extract ) by considering symbolic inputs for it.
The size of the minimal (legal) *.mj2 file is a measure for how difficult it will be for the SE engine to find
an *.mj2 file. I agree with previous posts that say that the file itself is irrelevant -- however, it does have to be a legal *.mj2 file, otherwise the sprintf statement won't be reached ...

@setharnold
Copy link
Contributor Author

setharnold commented Mar 12, 2018 via email

@szukw000
Copy link
Contributor

@OrenGitHub ,

do you want to test the validity of the file BEFORE calling 'opj_mj2_extract'?

Normally the validity is checked in

    if (mj2_read_struct(file, movie)) { /* Creating the movie structure*/
        return 1;
    }

which you can find in 'src/lib/openmj2/mj2.c'

Here are two tests:

mkdir /tmp/VERSUCH1

bin/opj_mj2_extract C_18_4K_24_FULL_CBR_CIRCLE_000.j2k /tmp/VERSUCH1/
[ERROR] Error: Expected JP Marker

bin/opj_mj2_extract openjump_jpeg2000_erdas.jp2  /tmp/VERSUCH1/
[ERROR] Unknown box in MJ2 stream
[ERROR] Unknown box in MJ2 stream
[ERROR] Unknown box in MJ2 stream
[ERROR] MOOV box not found in file

winfried

@OrenGitHub
Copy link

@szukw000 no, I'm interested in the smallest *.mj2 file that will pass all the sanity checks from your post ... like MOOV box etc. ...

@szukw000
Copy link
Contributor

@OrenGitHub ,
the attached file is the first frame of Speedway.mj2. There is no other frame in
this file.
winfried
first.mj2.zip

@OrenGitHub
Copy link

Thank you very much @szukw000 !!! this is what I was looking for !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

opj_mj2_extract: Check provided output prefix for length kbabioch/openjpeg
4 participants
@kbabioch @szukw000 @setharnold @OrenGitHub