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

CVE-2017-17669: heap-buffer-overflow in Exiv2::Internal::PngChunk::keyTXTChunk #187

Closed
Young-X opened this issue Dec 10, 2017 · 9 comments
Closed
Assignees
Milestone

Comments

@Young-X
Copy link

Young-X commented Dec 10, 2017

Description

There is a heap-buffer-overflow vulnerability in Exiv2.

The command is: ./exiv2 POC

Stack trace with asan:

==2826==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300000edb9 at pc 0x7f7c43e7a77a bp 0x7ffe0d1d6df0 sp 0x7ffe0d1d6de8
READ of size 1 at 0x60300000edb9 thread T0
    #0 0x7f7c43e7a779 in Exiv2::Internal::PngChunk::keyTXTChunk(Exiv2::DataBuf const&, bool) /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/pngchunk_int.cpp:111:17
    #1 0x7f7c43e7a000 in Exiv2::Internal::PngChunk::decodeTXTChunk(Exiv2::Image*, Exiv2::DataBuf const&, Exiv2::Internal::PngChunk::TxtChunkType) /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/pngchunk_int.cpp:78:23
    #2 0x7f7c43d3ff3b in Exiv2::PngImage::readMetadata() /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/pngimage.cpp:445:21
    #3 0x52f84c in Action::Print::printSummary() /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/actions.cpp:288:9
    #4 0x52e389 in Action::Print::run(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/actions.cpp:240:44
    #5 0x509598 in main /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/exiv2.cpp:166:19
    #6 0x7f7c422e082f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #7 0x4347b8 in _start (/home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/build_clang_with_sym/bin/exiv2+0x4347b8)

0x60300000edb9 is located 0 bytes to the right of 25-byte region [0x60300000eda0,0x60300000edb9)
allocated by thread T0 here:
    #0 0x506030 in operator new[](unsigned long) (/home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/build_clang_with_sym/bin/exiv2+0x506030)
    #1 0x55b31f in Exiv2::DataBuf::DataBuf(long) /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/include/exiv2/types.hpp:206:46
    #2 0x7f7c43d3fb50 in Exiv2::PngImage::readMetadata() /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/pngimage.cpp:420:25
    #3 0x52f84c in Action::Print::printSummary() /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/actions.cpp:288:9
    #4 0x52e389 in Action::Print::run(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/actions.cpp:240:44
    #5 0x509598 in main /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/exiv2.cpp:166:19
    #6 0x7f7c422e082f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/rg/fuzz_project/exiv2/exiv2_test/exiv2_github_1210/src/pngchunk_int.cpp:111:17 in Exiv2::Internal::PngChunk::keyTXTChunk(Exiv2::DataBuf const&, bool)
Shadow bytes around the buggy address:
  0x0c067fff9d60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff9d70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff9d80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff9d90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fff9da0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c067fff9db0: fa fa fa fa 00 00 00[01]fa fa 00 00 00 fa fa fa
  0x0c067fff9dc0: 00 00 00 00 fa fa fd fd fd fd fa fa fd fd fd fd
  0x0c067fff9dd0: fa fa fd fd fd fa fa fa 00 00 02 fa fa fa 00 00
  0x0c067fff9de0: 02 fa fa fa 00 00 02 fa fa fa 00 00 02 fa fa fa
  0x0c067fff9df0: 00 00 00 fa fa fa 00 00 00 01 fa fa 00 00 00 fa
  0x0c067fff9e00: 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
  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
==2826==ABORTING

PoC

PoC https://github.com/Young-X/pocs/blob/master/Exiv2/issue_187

Author

Credit to Young_X@VARAS, IIE

@clanmills clanmills self-assigned this Dec 10, 2017
@clanmills
Copy link
Collaborator

Thanks, I'll take a look at this.

Young-X added a commit to Young-X/pocs that referenced this issue Dec 10, 2017
@carnil
Copy link

carnil commented Jan 1, 2018

This issue was assigned CVE-2017-17669

@D4N
Copy link
Member

D4N commented Jan 6, 2018

@Young-X Which commit of exiv2 did you build? And can you please retry with the current HEAD?

@fgeek
Copy link

fgeek commented Jan 8, 2018

4be0655 crashes with similar heap-buffer-overflow output from ASan in amd64. Is there a good way to add this reproducer to automated tests?

@brianmay
Copy link

I believe the relevant code for the first error is this:

        const byte *key = data.pData_ + (stripHeader ? 8 : 0);

        // Find null string at end of key.
        int keysize=0;
        for ( ; key[keysize] != 0 ; keysize++)
        {
            // look if keysize is valid.
            if (keysize >= data.size_)
                throw Error(14);
        }

I think there are several errors here, that are applicable if the key is too big or isn't correctly null terminated.

  1. If stripHeader is True, we add 8 bytes to the pointer, but we don't decrease data.size_ by the corresponding amount. So we may read 8 bytes past the end of the buffer,

  2. As per the for loop, after each iteration, we:

    1. Increment keysize.
    2. Check key[keysize].
    3. Check keysize isn't too big.

    However, by the time we have checked keysize in step 3, we have already potentially read past the end of the buffer in step 2. i.e. off by one error.

@brianmay
Copy link

brianmay commented Jan 12, 2018

Here is a potential patch that I believe will solve the issues:

diff --git a/src/pngchunk.cpp b/src/pngchunk.cpp
index da4ccd01..b54bcdac 100644
--- a/src/pngchunk.cpp
+++ b/src/pngchunk.cpp
@@ -107,15 +107,17 @@ namespace Exiv2 {
     {
         // From a tEXt, zTXt, or iTXt chunk,
         // we get the key, it's a null terminated string at the chunk start
-        if (data.size_ <= (stripHeader ? 8 : 0)) throw Error(14);
-        const byte *key = data.pData_ + (stripHeader ? 8 : 0);
+        const int offset = stripHeader ? 8 : 0;
+        if (data.size_ <= offset) throw Error(14);
+        const byte *key = data.pData_ + offset;
 
         // Find null string at end of key.
         int keysize=0;
-        for ( ; key[keysize] != 0 ; keysize++)
+        while (key[keysize] != 0)
         {
+            keysize++;
             // look if keysize is valid.
-            if (keysize >= data.size_)
+            if (keysize+offset >= data.size_)
                 throw Error(14);
         }

@D4N
Copy link
Member

D4N commented Jan 12, 2018

@brianmay Thanks for your investigation. I believe you are right and the patch looks good too. Thanks a lot!

@clanmills
Copy link
Collaborator

Thank You, Brian for this contribution. Looks about right to me! If you'd like to investigate other issues, the team would value your contribution. We're all over-worked and under-paid volunteers. All assistance is appreciated! https://www.youtube.com/watch?v=3Fv57Lbhmqg

@D4N D4N mentioned this issue Jan 22, 2018
D4N added a commit that referenced this issue Jan 27, 2018
@D4N D4N changed the title heap-buffer-overflow in Exiv2::Internal::PngChunk::keyTXTChunk CVE-2017-17669: donheap-buffer-overflow in Exiv2::Internal::PngChunk::keyTXTChunk Mar 19, 2018
@D4N D4N changed the title CVE-2017-17669: donheap-buffer-overflow in Exiv2::Internal::PngChunk::keyTXTChunk CVE-2017-17669: heap-buffer-overflow in Exiv2::Internal::PngChunk::keyTXTChunk Mar 19, 2018
@D4N
Copy link
Member

D4N commented Mar 19, 2018

The fix for this and the reproducer are on master.

@D4N D4N closed this as completed Mar 19, 2018
@clanmills clanmills added this to the v0.27 milestone Nov 8, 2018
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

No branches or pull requests

6 participants