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

Fix Issue #428 #430

Merged
merged 5 commits into from Sep 11, 2018

Conversation

Projects
2 participants
@piponazo
Collaborator

piponazo commented Sep 2, 2018

This should fix the problem described in #428. It also adds a new regression test to reproduce the issue.

@piponazo piponazo self-assigned this Sep 2, 2018

@piponazo piponazo requested a review from D4N Sep 2, 2018

@codecov

This comment has been minimized.

codecov bot commented Sep 2, 2018

Codecov Report

Merging #430 into master will increase coverage by 0.07%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   62.02%   62.09%   +0.07%     
==========================================
  Files         149      149              
  Lines       20456    20465       +9     
==========================================
+ Hits        12687    12708      +21     
+ Misses       7769     7757      -12
Impacted Files Coverage Δ
src/pngchunk_int.cpp 89.88% <94.44%> (+4.8%) ⬆️
src/pngimage.cpp 90.63% <0%> (+0.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dce05ac...cfc7a1c. Read the comment docs.

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 3, 2018

Thanks for doing this, @piponazo

I'll review your changes this evening. My head is full of the MinGW/setMsg() puzzle at the moment (#421) And I'm working in the garden as well. Beautiful sunny day.

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 4, 2018

Apologies for not getting to this on Monday, Luis. I was exhausted (and delighted) by solving the MinGW/setMsg/GCC8.2.0 issue (#421). I've reproduced #428 and will work on it later today (Tuesday). First priority is to review this PR.

Thanks for working on this.

My old friend Gaylord lives in California and yesterday was his 80th birthday. I've known him for 40 years and went to visit him 2 weeks ago. We didn't go to California for his birthday party yesterday, however we had a little cake and candles on the patio. Life is good when retired! https://clanmills.com/2018/September/

dsc_8694

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 4, 2018

There's something not quite right about this. I've download the POCs:

1148 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ ls -salt ~/Downloads/*.png
4 -rw-r--r--+ 1 rmills staff  188 Sep  4 13:09 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png
4 -rw-r--r--+ 1 rmills staff 2598 Sep  4 13:09 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png
4 -rw-r--r--+ 1 rmills staff  456 Sep  4 13:08 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png
4 -rw-r--r--+ 1 rmills staff 1651 Sep  4 13:07 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png
4 -rw-r--r--+ 1 rmills staff  188 Sep  4 13:05 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png
4 -rw-r--r--+ 1 rmills staff  512 Sep  4 13:05 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png
1149 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $

When I apply your patch, I see:

for i in $(ls ~/Downloads/*.png|sort); do echo ---- $i ---- ; bin/exiv2 --verbose $i 2>&1 | grep SUMMARY ; echo '' ; done
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png ----
SUMMARY: AddressSanitizer: heap-buffer-overflow (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x52534) in wrap_atol

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png ----

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png ----

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png ----

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png ----
SUMMARY: AddressSanitizer: heap-buffer-overflow (libexiv2.26.dylib:x86_64+0x1256594) in Exiv2::Internal::PngChunk::readRawProfile(Exiv2::DataBuf const&, bool)

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png ----
SUMMARY: AddressSanitizer: heap-buffer-overflow (libexiv2.26.dylib:x86_64+0x1256594) in Exiv2::Internal::PngChunk::readRawProfile(Exiv2::DataBuf const&, bool)

1148 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ 

Your patch is a step in the right direction. The code is cavalier in galloping forward without checking that it is reading within the buffer. atol() can also gallop, so I've added const char* startOfLength; and delayed atol() until I feel safe.

Having decoded the length, the code gallops decoding hex/nibbles without bothering to test if the buffer has 2*length bytes. I've added const char* endOfLength to know the first byte following the length string. When we calculate nibbles = 2*length, I test it for sanity.

Like you, I think the code needs a major review and the schedule doesn't permit this at the moment.

I've put my modified version of PngChunk::readRawProfile() which includes the fixes that I've mentioned. This seems to fix the 6 POCs provided in #428

1197 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ for i in $(ls ~/Downloads/*.png|sort); do echo ---- $i ---- ; bin/exiv2 --verbose $i ; echo '' ; done
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png:
Failed to read image data

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png:
Failed to read image data

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png:
Failed to read image data

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png:
Failed to read image data

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png:
Failed to read image data

---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png:
Failed to read image data

1198 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ 
        DataBuf PngChunk::readRawProfile(const DataBuf& text,bool iTXt)
        {
            DataBuf                 info;
            unsigned char           unhex[103]={0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
                0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
                0,0,0,0,0,0,0,0,0,1, 2,3,4,5,6,7,8,9,0,0,
                0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
                0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,10,11,12,
                13,14,15};
            if (text.size_ == 0) {
                return DataBuf();
            }

            if ( iTXt ) {
                info.alloc(text.size_);
                ::memcpy(info.pData_,text.pData_,text.size_);
                return  info;
            }

            const char *sp = (char*)text.pData_+1;
            int pointerPos = 1;

            // Look for newline
            while (*sp != '\n' && pointerPos < (text.size_ - 1))
            {
                sp++;
                pointerPos++;
            }
            if (pointerPos == (text.size_ - 1))
            {
                return DataBuf();
            }

            // Look for length
            while ((*sp == '\0' || *sp == ' ' || *sp == '\n') && pointerPos < (text.size_ - 1))
            {
                sp++;
                pointerPos++;
            }
            if (pointerPos == (text.size_ - 1))
            {
                return DataBuf();
            }

            const char* startOfLength = sp;
            while (*sp != ' ' && *sp != '\n' && pointerPos < (text.size_ - 1))
            {
                sp++;
                pointerPos++;
            }
            const char* endOfLength = sp;

            if (pointerPos == (text.size_ - 1))
            {
                return DataBuf();
            }
            long length = (long) atol(startOfLength);

            // Allocate space

            if (length == 0)
            {
#ifdef DEBUG
                std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: invalid profile length\n";
#endif
            }

            info.alloc(length);

            if (info.size_ != length)
            {
#ifdef DEBUG
                std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: cannot allocate memory\n";
#endif
                return DataBuf();
            }

            // Copy profile, skipping white space and column 1 "=" signs

            unsigned char *dp = (unsigned char*)info.pData_;
            unsigned int nibbles = length * 2;
            if ( (Exiv2::byte*)endOfLength + nibbles > info.pData_+info.size_ ) {
#ifdef DEBUG
            	std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: nibble count overflow\n";
#endif
                return DataBuf();
            }

            for (long i = 0; i < (long) nibbles; i++)
            {
                while (*sp < '0' || (*sp > '9' && *sp < 'a') || *sp > 'f')
                {
                    if (*sp == '\0')
                    {
#ifdef DEBUG
                        std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: ran out of data\n";
#endif
                        return DataBuf();
                    }

                    sp++;
                }

                if (i%2 == 0)
                    *dp = (unsigned char) (16*unhex[(int) *sp++]);
                else
                    (*dp++) += unhex[(int) *sp++];
            }

            return info;

        } // PngChunk::readRawProfile
@piponazo

This comment has been minimized.

Collaborator

piponazo commented Sep 5, 2018

Great! Thanks @clanmills for the help. I'll include your changes and push them to the branch, so that we can see if the CI is also happy with the changes ;)

@piponazo

This comment has been minimized.

Collaborator

piponazo commented Sep 5, 2018

Ups, it seems that those changes break other tests @clanmills. You can see the issues here:
https://travis-ci.org/piponazo/exiv2/builds/424764668

I'll revert the latest commits to make the CI green again.

@piponazo piponazo force-pushed the piponazo:Issue428 branch from 4b56473 to 0221d02 Sep 5, 2018

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 5, 2018

Oh, that's a surprise. The test suite is passing on my machine.

You've done the right thing to revert this. I will revisit this and update you tomorrow.

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 5, 2018

Apologies. I built and tested the wrong code on my machine yesterday. I've reproduced the test errors being reported. Somehow this has broken icc-test.sh. I'll debug and fix it tomorrow.

Curiously, test the ASAN build has revealed something in stringto-test.sh and preview-test.sh and I'll also investigate that tomorrow and if necessary open new issues to address those matters.

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 6, 2018

I know what's wrong with this and I'll fix it later. The option -iC (insert ICC profile) is causing the loss of Exif and IPTC metadata.

1301 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ cp ../test/data/ReaganSmallPng.png ReaganSmallPng.png 
1302 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -pS ReaganSmallPng.png 
STRUCTURE OF PNG FILE: ReaganSmallPng.png
 address | chunk |  length | data                           | checksum
       8 | IHDR  |      13 | ............                   | 0x8cf910c3
      33 | zTXt  |    8648 | Raw profile type exif..x...Wv$ | 0xca41f34d <--- EXIF Metadata
    8693 | zTXt  |     632 | Raw profile type iptc..x..T[.. | 0xa2860459 <--- IPTC Metadata
    9337 | iTXt  |    7117 | XML:com.adobe.xmp.....<?xpacke | 0x2ff025b8 <--- XMP Metadata
   16466 | gAMA  |       4 | ....                           | 0x0bfc6105
   16482 | bKGD  |       6 | ......                         | 0xa0bda793
   16500 | pHYs  |       9 | ...#...#.                      | 0x78a53f76
   16521 | tIME  |       7 | ......#                        | 0xdf7f5bbd
   16540 | zTXt  |     278 | Comment..x.}..n.@....O..5..h.. | 0xdb1dfff5
   16830 | IDAT  |    8192 | x...i.$.u%v....Gdd...U..X.`0.9 | 0x96dc2ed9
   25034 | IDAT  |    8192 | df.."..1L...0...j....`F.&.yf.. | 0xbfeb3575
   33238 | IDAT  |    8192 | K-N.t.ENL.R..q](jm...sN..U.+.. | 0xe249a922
   41442 | IDAT  |    8192 | >..?.Nw..iN......xE....z..[..} | 0x054b9d1e
   49646 | IDAT  |    7066 | ...q.B...2*@..#....T....h..v.. | 0x327f1e3e
   56724 | IEND  |       0 |                                | 0xae426082
1304 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -pC ../test/data/ReaganLargePng.png > ReaganSmallPng.icc 
1305 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ dir Re*
-rw-r--r--+ 1 rmills staff 1.6M Sep  6 09:54 ReaganSmallPng.icc  <--- Extracted ICC profile
-rw-r--r--+ 1 rmills staff  56K Sep  6 09:52 ReaganSmallPng.png
1306 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -iC --verbose ReaganSmallPng.png 
File 1/1: ReaganSmallPng.png
1307 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ bin/exiv2 -pS ReaganSmallPng.png 
STRUCTURE OF PNG FILE: ReaganSmallPng.png
 address | chunk |  length | data                           | checksum
       8 | IHDR  |      13 | ............                   | 0x8cf910c3
      33 | iCCP  | 1159185 | ICC PROFILE..x...uP.[..9@.HB.D | 0x4a90a7d7 <--- Inserted ICC profile
                                                                         <---  lost EXIF and IPTC
 1159230 | iTXt  |    7117 | XML:com.adobe.xmp.....<?xpacke | 0x2ff025b8 <--- XMP Metadata 
 1166359 | gAMA  |       4 | ....                           | 0x0bfc6105
 1166375 | bKGD  |       6 | ......                         | 0xa0bda793
 1166393 | pHYs  |       9 | ...#...#.                      | 0x78a53f76
 1166414 | tIME  |       7 | ......#                        | 0xdf7f5bbd
 1166433 | zTXt  |     278 | Comment..x.}..n.@....O..5..h.. | 0xdb1dfff5
 1166723 | IDAT  |    8192 | x...i.$.u%v....Gdd...U..X.`0.9 | 0x96dc2ed9
 1174927 | IDAT  |    8192 | df.."..1L...0...j....`F.&.yf.. | 0xbfeb3575
 1183131 | IDAT  |    8192 | K-N.t.ENL.R..q](jm...sN..U.+.. | 0xe249a922
 1191335 | IDAT  |    8192 | >..?.Nw..iN......xE....z..[..} | 0x054b9d1e
 1199539 | IDAT  |    7066 | ...q.B...2*@..#....T....h..v.. | 0x327f1e3e
 1206617 | IEND  |       0 |                                | 0xae426082
1308 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ 
@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 7, 2018

I'm making steady progress with this. More difficult than expected.

I've identified what's causing stringto-test to fail (when built for ASAN) and I'll open a different issue (with patch).

I want to continue painting the summer house today and hope to finish this issue this evening.

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 7, 2018

More challenges:

  1. There's another ASAN address issue in src/iptc.cpp int IptcParser::decode()
    I don't believe I have time to work on that until Sunday afternoon.

  2. There's also something incorrect about CMake/setting the ASAN switches with:

$ cmake .. -G Xcode -DEXIV2_TEAM_USE_SANITIZERS=On

If I build without -DEXIV2_TEAM_USE_SANITIZERS=On, I can enable them from the Xcode gui.

I've found this article (although I don't know if this is true) https://stackoverflow.com/questions/44320465/whats-the-proper-way-to-enable-addresssanitizer-in-cmake-that-works-in-xcode I suspect we'll need something similar to the following which was added to src/CMakeLists.txt:

1624 rmills@rmillsmm:~/gnu/github/exiv2/exiv2 $ nl -b a src/CMakeLists.txt | grep -i xcode 
   315	        XCODE_ATTRIBUTE_GCC_GENERATE_DEBUGGING_SYMBOLS[variant=Debug] "YES"
1625 rmills@rmillsmm:~/gnu/github/exiv2/exiv2 $ 
@piponazo

This comment has been minimized.

Collaborator

piponazo commented Sep 7, 2018

@clanmills we added the XCODE_ATTRIBUTE_GCC_GENERATE_DEBUGGING_SYMBOLS configuration into src/CMakeLists.txtfew months ago.

If the investigation you are doing is going to take long, I propose to merge the branch in the current status so that we have at least few fixes for 0.27 RC1. We can investigate the other issues later.

@clanmills

This comment has been minimized.

Collaborator

clanmills commented Sep 7, 2018

Oh, I know you added XCODE_ATTRIBUTE_GCC_GENERATE_DEBUGGING_SYMBOLS. My suspicion is that we need something similar to enable ASAN to build/debug correctly from the Xcode IDE.

I'll fix the ASAN issues in IptcParser::decode() and PngChunk::readRawProfile() and get #428 closed.

I'll open a new issue about stringto-test.cpp.

@clanmills

I've reviewed PngChunk::readRawProfile() again. My previous comments stand. The code is galloping along a buffer without checking for the end.

I don't know the Specification for this code. From the comments, this has been specified by ImageMagick. It seems that an Exif (&Iptc) binary metadata block is encoded as hex nibbles which are decoded. If the decoding fails, the chunk is returned as raw binary. These raw chunks can contain an ICC profile or XMP/xml.

        DataBuf PngChunk::readRawProfile(const DataBuf& text,bool iTXt)
        {
            DataBuf                 info;
            unsigned char           unhex[103]={0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
                0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
                0,0,0,0,0,0,0,0,0,1, 2,3,4,5,6,7,8,9,0,0,
                0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
                0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,10,11,12,
                13,14,15};
            if (text.size_ == 0) {
                return DataBuf();
            }

            if ( iTXt ) {
                info.alloc(text.size_);
                ::memcpy(info.pData_,text.pData_,text.size_);
                return  info;
            }

            const char *sp  = (char*) text.pData_+1;          // current byte (space pointer)
            const char *eot = (char*) text.pData_+text.size_; // end of text

            // Look for newline
            while (*sp != '\n' && sp < eot )
            {
                sp++;
                if ( sp == eot )
                {
                    return DataBuf();
                }
            }
            sp++ ; // step over '\n'

            // Look for length
            while ( (*sp == '\0' || *sp == ' ' || *sp == '\n') && sp < eot )
            {
                sp++;
                if (sp == eot )
                {
                    return DataBuf();
                }
            }

            const char* startOfLength = sp;
            while ( ('0' <= *sp && *sp <= '9') && sp < eot)
            {
                sp++;
                if (sp == eot )
                {
                    return DataBuf();
                }
            }
            sp++ ; // step over '\n'

            long length = (long) atol(startOfLength);

            // Allocate space
            if (length == 0)
            {
#ifdef DEBUG
                std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: invalid profile length\n";
#endif
            }
            info.alloc(length);
            if (info.size_ != length)
            {
#ifdef DEBUG
                std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: cannot allocate memory\n";
#endif
                return DataBuf();
            }

            // Copy profile, skipping white space and column 1 "=" signs

            unsigned char *dp = (unsigned char*)info.pData_; // decode pointer
            unsigned int nibbles = length * 2;

            for (long i = 0; i < (long) nibbles; i++)
            {
                while (*sp < '0' || (*sp > '9' && *sp < 'a') || *sp > 'f')
                {
                    if (*sp == '\0')
                    {
#ifdef DEBUG
                        std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: ran out of data\n";
#endif
                        return DataBuf();
                    }

                    sp++;
                }

                if (i%2 == 0)
                    *dp = (unsigned char) (16*unhex[(int) *sp++]);
                else
                    (*dp++) += unhex[(int) *sp++];
            }

            return info;

        } // PngChunk::readRawProfile

@piponazo piponazo force-pushed the piponazo:Issue428 branch from ba73423 to 767fda9 Sep 10, 2018

@piponazo piponazo force-pushed the piponazo:Issue428 branch from 767fda9 to cfc7a1c Sep 11, 2018

@piponazo piponazo requested a review from clanmills Sep 11, 2018

@clanmills

Good team-work here, Luis.

@piponazo

This comment has been minimized.

Collaborator

piponazo commented Sep 11, 2018

Thanks for your help! 👍

@piponazo piponazo merged commit 7d32da8 into Exiv2:master Sep 11, 2018

4 checks passed

codecov/patch 94.44% of diff hit (target 62.02%)
Details
codecov/project 62.09% (+0.07%) compared to dce05ac
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@piponazo piponazo deleted the piponazo:Issue428 branch Sep 11, 2018

@piponazo piponazo added this to In Progress in v0.27 Sep 11, 2018

@piponazo piponazo moved this from In Progress to Done in v0.27 Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment