From dd52b1a8328a07eabe4ccafe78bc54f4795f272a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 25 May 2018 00:39:07 +0200 Subject: [PATCH 1/5] Add helper_functions.cpp/hpp & unit tests - add function which constructs a new std::string from a potentially not null terminated char * - add unit tests --- src/CMakeLists.txt | 1 + src/helper_functions.cpp | 41 +++++++++++++++++++++++++++ src/helper_functions.hpp | 43 +++++++++++++++++++++++++++++ unitTests/CMakeLists.txt | 1 + unitTests/test_helper_functions.cpp | 21 ++++++++++++++ 5 files changed, 107 insertions(+) create mode 100644 src/helper_functions.cpp create mode 100644 src/helper_functions.hpp create mode 100644 unitTests/test_helper_functions.cpp diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index c7fb9ca7..89a7b634 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -17,6 +17,7 @@ set( LIBEXIV2_SRC exif.cpp futils.cpp gifimage.cpp + helper_functions.cpp http.cpp image.cpp ini.cpp diff --git a/src/helper_functions.cpp b/src/helper_functions.cpp new file mode 100644 index 00000000..ba8bd6d4 --- /dev/null +++ b/src/helper_functions.cpp @@ -0,0 +1,41 @@ +// ********************************************************* -*- C++ -*- +/* + * Copyright (C) 2004-2018 Exiv2 authors + * + * This program is part of the Exiv2 distribution. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, 5th Floor, Boston, MA 02110-1301 USA. + */ +/*! + @file helper_functions.cpp + @brief A collection of helper functions + @author Dan Čermák (D4N) + dan.cermak@cgc-instruments.com + @date 25-May-18, D4N: created + */ + +#include "helper_functions.hpp" + +#include + + +std::string string_from_unterminated(const char* data, size_t data_length) +{ + const char* termination = std::find(data, data + data_length, 0); + // if find returned the end iterator => no \0 found + const size_t string_length = termination == data + data_length ? data_length : termination - data; + + return std::string(data, string_length); +} diff --git a/src/helper_functions.hpp b/src/helper_functions.hpp new file mode 100644 index 00000000..d138490b --- /dev/null +++ b/src/helper_functions.hpp @@ -0,0 +1,43 @@ +// ********************************************************* -*- C++ -*- +/* + * Copyright (C) 2004-2018 Exiv2 authors + * + * This program is part of the Exiv2 distribution. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, 5th Floor, Boston, MA 02110-1301 USA. + */ +/*! + @file helper_functions.hpp + @brief A collection of helper functions + @author Dan Čermák (D4N) + dan.cermak@cgc-instruments.com + @date 25-May-18, D4N: created + */ +#ifndef HELPER_FUNCTIONS_HPP +#define HELPER_FUNCTIONS_HPP + +#include + +/*! + @brief Convert a (potentially not null terminated) array into a + std::string. + + Convert a C style string that may or may not be null terminated safely + into a std::string. The string's termination is either set at the first \0 + or after data_length characters. + */ +std::string string_from_unterminated(const char* data, size_t data_length); + +#endif // HELPER_FUNCTIONS_HPP diff --git a/unitTests/CMakeLists.txt b/unitTests/CMakeLists.txt index a8922de4..1778a5aa 100644 --- a/unitTests/CMakeLists.txt +++ b/unitTests/CMakeLists.txt @@ -8,6 +8,7 @@ add_executable(unit_tests mainTestRunner.cpp test_XmpKey.cpp test_DateValue.cpp test_cr2header_int.cpp + test_helper_functions.cpp ) #TODO Use GTest::GTest once we upgrade the minimum CMake version required diff --git a/unitTests/test_helper_functions.cpp b/unitTests/test_helper_functions.cpp new file mode 100644 index 00000000..c47f753e --- /dev/null +++ b/unitTests/test_helper_functions.cpp @@ -0,0 +1,21 @@ +#include "helper_functions.hpp" + +#include "gtestwrapper.h" + +TEST(string_from_unterminated, terminatedArray) +{ + const char data[5] = {'a', 'b', 'c', 0, 'd'}; + const std::string res = string_from_unterminated(data, 5); + + ASSERT_EQ(res.size(), 3); + ASSERT_STREQ(res.c_str(), "abc"); +} + +TEST(string_from_unterminated, unterminatedArray) +{ + const char data[4] = {'a', 'b', 'c', 'd'}; + const std::string res = string_from_unterminated(data, 4); + + ASSERT_EQ(res.size(), 4); + ASSERT_STREQ(res.c_str(), "abcd"); +} From 510560bbd1148b43218ac13565112994e8813d75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 25 May 2018 00:42:20 +0200 Subject: [PATCH 2/5] Prevent out of bounds read in jpgimage.cpp JpegBase::printStructure signature is extracted from an image and can lack the terminating \0, this causes the std::string constructor and strcmp to read beyond the bounds of the allocated array. => Use string_from_unterminated to construct a std::string safely and use it in the subsequent code & use stl functions instead of C functions --- src/jpgimage.cpp | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index d6325aa6..d8757bef 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -1,6 +1,6 @@ // ***************************************************************** -*- C++ -*- /* - * Copyright (C) 2004-2017 Andreas Huggel + * Copyright (C) 2004-2018 Exiv2 authors * * This program is part of the Exiv2 distribution. * @@ -30,6 +30,7 @@ #include "image_int.hpp" #include "error.hpp" #include "futils.hpp" +#include "helper_functions.hpp" #include "enforce.hpp" #ifdef WIN32 @@ -46,6 +47,7 @@ #include // for EOF #include #include +#include // ***************************************************************************** // class member definitions @@ -643,7 +645,7 @@ namespace Exiv2 { // print signature for APPn if (marker >= app0_ && marker <= (app0_ | 0x0F)) { // http://www.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf p75 - const char* signature = (const char*)buf.pData_ + 2; + const std::string signature = string_from_unterminated((const char*)buf.pData_ + 2, buf.size_ - 2); // 728 rmills@rmillsmbp:~/gnu/exiv2/ttt $ exiv2 -pS test/data/exiv2-bug922.jpg // STRUCTURE OF JPEG FILE: test/data/exiv2-bug922.jpg @@ -652,7 +654,7 @@ namespace Exiv2 { // 2 | 0xe1 APP1 | 911 | Exif..MM.*.......%.........#.... // 915 | 0xe1 APP1 | 870 | http://ns.adobe.com/xap/1.0/. 0) { io_->seek(-bufRead, BasicIo::cur); @@ -684,7 +686,7 @@ namespace Exiv2 { bufRead = size; done = !bExtXMP; } - } else if (option == kpsIccProfile && std::strcmp(signature, iccId_) == 0) { + } else if (option == kpsIccProfile && signature.compare(iccId_) == 0) { // extract ICCProfile if (size > 0) { io_->seek(-bufRead, BasicIo::cur); // back to buffer (after marker) @@ -697,7 +699,7 @@ namespace Exiv2 { #endif bufRead = size; } - } else if (option == kpsIptcErase && std::strcmp(signature, "Photoshop 3.0") == 0) { + } else if (option == kpsIptcErase && signature.compare("Photoshop 3.0") == 0) { // delete IPTC data segment from JPEG if (size > 0) { io_->seek(-bufRead, BasicIo::cur); @@ -706,19 +708,29 @@ namespace Exiv2 { } } else if (bPrint) { out << "| " << Internal::binaryToString(buf, size > 32 ? 32 : size, size > 0 ? 2 : 0); - if (std::strcmp(signature, iccId_) == 0) { - int chunk = (int)signature[12]; - int chunks = (int)signature[13]; + if (signature.compare(iccId_) == 0) { + // extract the chunk information from the buffer + // + // the buffer looks like this in this branch + // ICC_PROFILE\0AB + // where A & B are bytes (the variables chunk & chunks) + // + // We cannot extract the variables A and B from the signature string, as they are beyond the + // null termination (and signature ends there). + // => Read the chunk info from the DataBuf directly + enforce(buf.size_ - 2 > 14, "Buffer too small to extract chunk information."); + const int chunk = buf.pData_[2 + 12]; + const int chunks = buf.pData_[2 + 13]; out << Internal::stringFormat(" chunk %d/%d", chunk, chunks); } } // for MPF: http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/MPF.html // for FLIR: http://owl.phy.queensu.ca/~phil/exiftool/TagNames/FLIR.html - bool bFlir = option == kpsRecursive && marker == (app0_ + 1) && std::strcmp(signature, "FLIR") == 0; - bool bExif = option == kpsRecursive && marker == (app0_ + 1) && std::strcmp(signature, "Exif") == 0; - bool bMPF = option == kpsRecursive && marker == (app0_ + 2) && std::strcmp(signature, "MPF") == 0; - bool bPS = option == kpsRecursive && std::strcmp(signature, "Photoshop 3.0") == 0; + bool bFlir = option == kpsRecursive && marker == (app0_ + 1) && signature.compare("FLIR") == 0; + bool bExif = option == kpsRecursive && marker == (app0_ + 1) && signature.compare("Exif") == 0; + bool bMPF = option == kpsRecursive && marker == (app0_ + 2) && signature.compare("MPF") == 0; + bool bPS = option == kpsRecursive && signature.compare("Photoshop 3.0") == 0; if (bFlir || bExif || bMPF || bPS) { // extract Exif data block which is tiff formatted if (size > 0) { @@ -731,7 +743,7 @@ namespace Exiv2 { // copy the data to memory io_->seek(-bufRead, BasicIo::cur); io_->read(exif, size); - uint32_t start = std::strcmp(signature, "Exif") == 0 ? 8 : 6; + uint32_t start = signature.compare("Exif") == 0 ? 8 : 6; uint32_t max = (uint32_t)size - 1; // is this an fff block? From b51b6fc52da6005fe7a6095ed85810d41cdcf9db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 25 May 2018 00:45:42 +0200 Subject: [PATCH 3/5] Prevent an out of bounds read in strstr in JpegBase::printStructure The xmp byte array is not necessarily null terminated => strstr can read beyond the bounds of the allocated array then. Therefore use string_from_unterminated to remedy this issue. Also replace xmp with a std::vector, as stl functions can throw and we don't want a memory leak. --- src/jpgimage.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index d8757bef..20cc2a20 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -658,8 +658,8 @@ namespace Exiv2 { // extract XMP if (size > 0) { io_->seek(-bufRead, BasicIo::cur); - byte* xmp = new byte[size + 1]; - io_->read(xmp, size); + std::vector xmp(size + 1); + io_->read(xmp.data(), size); int start = 0; // http://wwwimages.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf @@ -670,10 +670,11 @@ namespace Exiv2 { // and dumping the XMP in a post read operation similar to kpsIptcErase // for the moment, dumping 'on the fly' is working fine if (!bExtXMP) { - while (xmp[start]) + while (xmp.at(start)) start++; start++; - if (::strstr((char*)xmp + start, "HasExtendedXMP")) { + std::string xmp_from_start = string_from_unterminated((char*)&xmp.at(start), size - start); + if (xmp_from_start.find("HasExtendedXMP", start) != xmp_from_start.npos) { start = size; // ignore this packet, we'll get on the next time around bExtXMP = true; } @@ -681,8 +682,7 @@ namespace Exiv2 { start = 2 + 35 + 32 + 4 + 4; // Adobe Spec, p19 } - out.write((const char*)(xmp + start), size - start); - delete[] xmp; + out.write((const char*)(&xmp.at(start)), size - start); bufRead = size; done = !bExtXMP; } From 088986e5fa51f592ecf526801fa64335c684dbe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 25 May 2018 00:47:50 +0200 Subject: [PATCH 4/5] [unit_tests] Remove a memory leak from test_XmpKey.cpp XmpProperties::registerNs allocates strings on the heap that must be freed manually via XmpProperties::unregisterNs(). => do this in TearDownTestCase() --- unitTests/test_XmpKey.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/unitTests/test_XmpKey.cpp b/unitTests/test_XmpKey.cpp index 2fd503b9..d000b722 100644 --- a/unitTests/test_XmpKey.cpp +++ b/unitTests/test_XmpKey.cpp @@ -24,6 +24,11 @@ public: XmpProperties::registerNs(expectedFamily, expectedPrefix); } + static void TearDownTestCase() + { + XmpProperties::unregisterNs(); + } + void checkValidity(const XmpKey& key) { ASSERT_EQ(expectedKey, key.key()); From a1a9c3d79a9c53ea201de643399ce20560d78271 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Sun, 27 May 2018 11:28:46 +0200 Subject: [PATCH 5/5] Use reinterpret_cast instead of C style cast --- src/jpgimage.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 20cc2a20..d4dd704d 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -645,7 +645,8 @@ namespace Exiv2 { // print signature for APPn if (marker >= app0_ && marker <= (app0_ | 0x0F)) { // http://www.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf p75 - const std::string signature = string_from_unterminated((const char*)buf.pData_ + 2, buf.size_ - 2); + const std::string signature = + string_from_unterminated(reinterpret_cast(buf.pData_ + 2), buf.size_ - 2); // 728 rmills@rmillsmbp:~/gnu/exiv2/ttt $ exiv2 -pS test/data/exiv2-bug922.jpg // STRUCTURE OF JPEG FILE: test/data/exiv2-bug922.jpg @@ -670,10 +671,12 @@ namespace Exiv2 { // and dumping the XMP in a post read operation similar to kpsIptcErase // for the moment, dumping 'on the fly' is working fine if (!bExtXMP) { - while (xmp.at(start)) + while (xmp.at(start)) { start++; + } start++; - std::string xmp_from_start = string_from_unterminated((char*)&xmp.at(start), size - start); + const std::string xmp_from_start = string_from_unterminated( + reinterpret_cast(&xmp.at(start)), size - start); if (xmp_from_start.find("HasExtendedXMP", start) != xmp_from_start.npos) { start = size; // ignore this packet, we'll get on the next time around bExtXMP = true; @@ -682,7 +685,7 @@ namespace Exiv2 { start = 2 + 35 + 32 + 4 + 4; // Adobe Spec, p19 } - out.write((const char*)(&xmp.at(start)), size - start); + out.write(reinterpret_cast(&xmp.at(start)), size - start); bufRead = size; done = !bExtXMP; } @@ -693,7 +696,7 @@ namespace Exiv2 { io_->seek(14 + 2, BasicIo::cur); // step over header DataBuf icc(size - (14 + 2)); io_->read(icc.pData_, icc.size_); - out.write((const char*)icc.pData_, icc.size_); + out.write(reinterpret_cast(icc.pData_), icc.size_); #ifdef DEBUG std::cout << "iccProfile size = " << icc.size_ << std::endl; #endif