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.

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

Heap-buffer-overflow and OOB memory access in src/jp2image.cpp:359:77 #1522

Closed
yuawn opened this issue Apr 5, 2021 · 11 comments · Fixed by #1523
Closed

Heap-buffer-overflow and OOB memory access in src/jp2image.cpp:359:77 #1522

yuawn opened this issue Apr 5, 2021 · 11 comments · Fixed by #1523
Assignees
Milestone

Comments

@yuawn
Copy link

yuawn commented Apr 5, 2021

Hi, I found a vulnerability in current master src/jp2image.cpp:359:77
There was an improper check of the rawData.size_, it can lead to heap overflow and memory access out-of-bounds.

Here is the poc and the ASAN details:

$ ./exiv2/asan-build/bin/exiv2 ./poc.jpg
=================================================================
==3600084==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000071 at pc 0x0000007b4185 bp 0x7fff12f1f970 sp 0x7fff12f1f968
READ of size 1 at 0x602000000071 thread T0
    #0 0x7b4184 in Exiv2::Jp2Image::readMetadata() /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/jp2image.cpp:359:77
    #1 0x61d1ce in Action::Print::printSummary() /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/actions.cpp:310:16
    #2 0x61c350 in Action::Print::run(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/actions.cpp:270:84
    #3 0x5c7dd6 in main /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/exiv2.cpp:167:29
    #4 0x7f13625780b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #5 0x4a7f7d in _start (/home/yuawn/fuzz-targets/exiv2/test/exiv2/asan-build/bin/exiv2+0x4a7f7d)

0x602000000071 is located 0 bytes to the right of 1-byte region [0x602000000070,0x602000000071)
allocated by thread T0 here:
    #0 0x586347 in operator new[](unsigned long) /home/yuawn/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:102:3
    #1 0x89ba53 in Exiv2::DataBuf::alloc(long) /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/types.cpp:161:22
    #2 0x61d1ce in Action::Print::printSummary() /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/actions.cpp:310:16
    #3 0x61c350 in Action::Print::run(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/actions.cpp:270:84
    #4 0x7f13625780b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/yuawn/fuzz-targets/exiv2/test/exiv2/src/jp2image.cpp:359:77 in Exiv2::Jp2Image::readMetadata()
Shadow bytes around the buggy address:
  0x0c047fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c047fff8000: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa[01]fa
  0x0c047fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8050: 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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
  Shadow gap:              cc
==3600084==ABORTING
@clanmills clanmills added this to the v0.27.4 milestone Apr 5, 2021
@clanmills clanmills self-assigned this Apr 5, 2021
@clanmills
Copy link
Collaborator

clanmills commented Apr 5, 2021

Please provide a proper description of how you produced this error and your input file. Please provide:

  1. The options used to build exiv2.
  2. A zip or tar archive with your test image.
  3. The command-line options you used.
  4. Accuratly pin-point the version code in used. Did you obtain this version recently?
  5. The platform being used.

We have discontinued the 'master' branch and renamed it 'old-master'. We are planning to ship v0.27.4 from branch 0.27-maintenance on 2021-04-30. If we can accurately identify and reproduce your report, a fix is likely to be included in the release.

@yuawn
Copy link
Author

yuawn commented Apr 5, 2021

Hi @clanmills

  1. CC=clang CXX=clang++ cmake .. -DCMAKE_BUILD_TYPE=Release -DEXIV2_TEAM_USE_SANITIZERS=ON (disable UBSAN)
    clang version 13.0.0 (https://github.com/llvm/llvm-project 3a016e31ecef7eeb876b540c928a25a7c5d2e07a)
  2. poc.jpg.gz
  3. exiv2 ./poc.jpg
  4. current master a750ea2
  5. Ubuntu 20.04.2 (Linux 5.10.13-051013-generic #202102032337 SMP Thu Feb 4 00:17:21 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux)

In poc It only allocate 1 byte by rawData.alloc(box.length - (sizeof(box) + sizeof(uuid)));, so there will have security problem at rawData.pData_[1] and integer underflow at rawData.size_-(long)sizeof(exifHeader).

@clanmills
Copy link
Collaborator

Thank you for getting back to me. And thank you for providing accurate details to reproduce your report.

I can confirm I have reproduced this on branch 0.27-maintenance and hope to provide you with a patch later today. I will delay releasing Exiv2 v0.27.4 RC2 to ensure we reach closure on this issue.

@clanmills
Copy link
Collaborator

diff --git a/src/jp2image.cpp b/src/jp2image.cpp
index eb31cea4..f10bbc44 100644
--- a/src/jp2image.cpp
+++ b/src/jp2image.cpp
@@ -353,7 +353,7 @@ static void boxes_check(size_t b,size_t m)
                             if (io_->error()) throw Error(kerFailedToReadImageData);
                             if (bufRead != rawData.size_) throw Error(kerInputDataReadFailed);
 
-                            if (rawData.size_ > 0)
+                            if (rawData.size_ > 8) // "II*\0long"
                             {
                                 // Find the position of Exif header in bytes array.
                                 long pos = (     (rawData.pData_[0]      == rawData.pData_[1])

And when I run your file:

1581 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance/build $ exiv2 ~/Downloads/poc.jpg 
Warning: Failed to decode Exif metadata.
File name       : /Users/rmills/Downloads/poc.jpg
File size       : 268 Bytes
MIME type       : image/jp2
Image size      : 0 x 0
/Users/rmills/Downloads/poc.jpg: No Exif data found in the file
1582 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance/build $ 

@kmilos
Copy link
Collaborator

kmilos commented Apr 5, 2021

Btw, would the same/similar bmff fix help here for the 0x0 image size as well?

@clanmills
Copy link
Collaborator

Fix submitted: #1523.

@clanmills
Copy link
Collaborator

@kmilos. Ah, no this is a parsing error. We should consider replacing jp2image.cpp with bmffimage.cpp for v1.0 because it's shorter and simpler. I didn't do that for v0.27.4 for the following reasons:

  1. jp2image.cpp has been in the code base for about 10 years and given good service. This has been the first issue for several years concerning this code.
  2. bmffimage.cpp is read-only and jp2image.cpp is a read-write format.

Reasons to consider using bmffimage.cpp to parse jp2 in v1.00 are:

  1. bmffimage.cpp has a recursive design that's designed to reflect the bmff spec. jp2image.cpp uses loops and state variables.
  2. There are tables concerning blanks in jp2image.cpp that should be investigated. I don't recall anything similar in bmffimage.cpp
  3. It's probably straightforward to port jp2image::writeMetadata() to bmffimage.cpp and remove 500+ lines in jp2image::readMedata() and jp2image::printStructure() as bmffimage.cpp performs the same task.

@kmilos
Copy link
Collaborator

kmilos commented Apr 5, 2021

Ah, sorry, I just assumed the code base was very similar since the jp2 and bmff specs are pretty much the same, should've checked first... Merging them in the future makes total sense.

@clanmills
Copy link
Collaborator

@kmilos. I've opened a new issue for v1.00 #1525. Perhaps @hassec will volunteer. He did excellent work on CR3 support in bmffimage.cpp for v0.27.4.

@carnil
Copy link

carnil commented Apr 6, 2021

This issue appears to have been assigned CVE-2021-3482.

@clanmills
Copy link
Collaborator

@yuawn As you can see, PR #1523 has been code reviewed and approved. If you have further comments to make, please let me know as soon as possible. I hope to merge this later today and release Exiv2 v0.27.4 RC2 with this fix tomorrow.

@clanmills clanmills linked a pull request Apr 6, 2021 that will close this issue
This was referenced Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants