Merge pull request #1769 from kevinbackhouse/Fix-GHSA-v5g7-46xf-h728

Safer std::vector indexing

samples/addmoddel.cpp

@@ -100,7 +100,7 @@ try {
     if (prv == 0) throw Exiv2::Error(Exiv2::kerErrorMessage, "Downcast failed");
     rv = Exiv2::URationalValue::AutoPtr(prv);
     // Modify the value directly through the interface of URationalValue
-    rv->value_[2] = std::make_pair(88,77);
+    rv->value_.at(2) = std::make_pair(88,77);
     // Copy the modified value back to the metadatum
     pos->setValue(rv.get());
     std::cout << "Modified key \"" << key

samples/exiv2json.cpp

@@ -74,7 +74,7 @@ bool getToken(std::string& in,Token& token,Exiv2::StringSet* pNS=NULL)
 
     while ( !result && in.length() ) {
         std::string c = in.substr(0,1);
-        char        C = c[0];
+        char        C = c.at(0);
         in            = in.substr(1,std::string::npos);
         if ( in.length() == 0 && C != ']' ) token.n += c;
         if ( C == '/' || C == '[' || C == ':' || C == '.' || C == ']' || in.length() == 0 ) {
@@ -114,7 +114,7 @@ Jzon::Node& addToTree(Jzon::Node& r1,Token token)
 
 Jzon::Node& recursivelyBuildTree(Jzon::Node& root,Tokens& tokens,size_t k)
 {
-    return addToTree( k==0 ? root : recursivelyBuildTree(root,tokens,k-1), tokens[k] );
+    return addToTree( k==0 ? root : recursivelyBuildTree(root,tokens,k-1), tokens.at(k) );
 }
 
 // build the json tree for this key.  return location and discover the name
@@ -126,7 +126,7 @@ Jzon::Node& objectForKey(const std::string& Key,Jzon::Object& root,std::string&
     std::string input  = Key ; // Example: "XMP.xmp.MP.RegionInfo/MPRI:Regions[1]/MPReg:Rectangle"
     while ( getToken(input,token,pNS) ) tokens.push_back(token);
     size_t      l      = tokens.size()-1; // leave leaf name to push()
-    name               = tokens[l].n ;
+    name               = tokens.at(l).n ;
 
     // The second token.  For example: XMP.dc is a namespace
     if ( pNS && tokens.size() > 1 ) pNS->insert(tokens[1].n);

src/actions.cpp

@@ -1031,19 +1031,21 @@ namespace Action {
 
         const Params::PreviewNumbers& numbers = Params::instance().previewNumbers_;
         for (Params::PreviewNumbers::const_iterator n = numbers.begin(); n != numbers.end(); ++n) {
-            if (*n == 0) {
+            size_t num = static_cast<size_t>(*n);
+            if (num == 0) {
                 // Write all previews
-                for (int num = 0; num < static_cast<int>(pvList.size()); ++num) {
-                    writePreviewFile(pvMgr.getPreviewImage(pvList[num]), num + 1);
+                for (num = 0; num < pvList.size(); ++num) {
+                    writePreviewFile(pvMgr.getPreviewImage(pvList[num]), static_cast<int>(num + 1));
                 }
                 break;
             }
-            if (*n > static_cast<int>(pvList.size())) {
+            num--;
+            if (num >= pvList.size()) {
                 std::cerr << path_ << ": " << _("Image does not have preview")
-                          << " " << *n << "\n";
+                          << " " << num + 1 << "\n";
                 continue;
             }
-            writePreviewFile(pvMgr.getPreviewImage(pvList[*n - 1]), *n);
+            writePreviewFile(pvMgr.getPreviewImage(pvList[num]), static_cast<int>(num + 1));
         }
         return 0;
     } // Extract::writePreviews
@@ -1603,7 +1605,7 @@ namespace Action {
             return 0;
         }
         std::string timeStr = md->toString();
-        if (timeStr == "" || timeStr[0] == ' ') {
+        if (timeStr.empty() || timeStr[0] == ' ') {
             std::cerr << path << ": " << _("Timestamp of metadatum with key") << " `"
                       << ek << "' " << _("not set\n");
             return 1;
@@ -2163,7 +2165,7 @@ namespace {
                               << "' " << _("exists. [O]verwrite, [r]ename or [s]kip?")
                               << " ";
                     std::cin >> s;
-                    switch (s[0]) {
+                    switch (s.at(0)) {
                     case 'o':
                     case 'O':
                         go = false;
@@ -2228,7 +2230,7 @@ namespace {
                       << ": " << _("Overwrite") << " `" << path << "'? ";
             std::string s;
             std::cin >> s;
-            if (s[0] != 'y' && s[0] != 'Y') return 1;
+            if (s.at(0) != 'y' && s.at(0) != 'Y') return 1;
         }
         return 0;
     }

src/basicio.cpp

@@ -190,11 +190,11 @@ namespace Exiv2 {
         case opRead:
             // Flush if current mode allows reading, else reopen (in mode "r+b"
             // as in this case we know that we can write to the file)
-            if (openMode_[0] == 'r' || openMode_[1] == '+') reopen = false;
+            if (openMode_.at(0) == 'r' || openMode_.at(1) == '+') reopen = false;
             break;
         case opWrite:
             // Flush if current mode allows writing, else reopen
-            if (openMode_[0] != 'r' || openMode_[1] == '+') reopen = false;
+            if (openMode_.at(0) != 'r' || openMode_.at(1) == '+') reopen = false;
             break;
         case opSeek:
             reopen = false;
@@ -934,7 +934,7 @@ namespace Exiv2 {
     size_t FileIo::size() const
     {
         // Flush and commit only if the file is open for writing
-        if (p_->fp_ != 0 && (p_->openMode_[0] != 'r' || p_->openMode_[1] == '+')) {
+        if (p_->fp_ != 0 && (p_->openMode_.at(0) != 'r' || p_->openMode_.at(1) == '+')) {
             std::fflush(p_->fp_);
 #if defined WIN32 && !defined __CYGWIN__
             // This is required on msvcrt before stat after writing to a file
@@ -2133,7 +2133,7 @@ namespace Exiv2 {
     void HttpIo::HttpImpl::writeRemote(const byte* data, size_t size, long from, long to)
     {
         std::string scriptPath(getEnv(envHTTPPOST));
-        if (scriptPath == "") {
+        if (scriptPath.empty()) {
             throw Error(kerErrorMessage, "Please set the path of the server script to handle http post data to EXIV2_HTTP_POST environmental variable.");
         }
 

src/convert.cpp

@@ -973,7 +973,7 @@ namespace Exiv2 {
             return;
         }
 
-        for (unsigned i = 0; i < value.length(); ++i) {
+        for (size_t i = 0; i < value.length(); ++i) {
             if (value[i] == '.') value[i] = ' ';
         }
         (*exifData_)[to] = value;

src/exiv2.cpp

@@ -1455,8 +1455,8 @@ namespace {
             if (valStart != std::string::npos) {
                 value = parseEscapes(line.substr(valStart, valEnd+1-valStart));
                 std::string::size_type last = value.length()-1;
-                if (   (value[0] == '"' && value[last] == '"')
-                       || (value[0] == '\'' && value[last] == '\'')) {
+                if (   (value.at(0) == '"' && value.at(last) == '"')
+                       || (value.at(0) == '\'' && value.at(last) == '\'')) {
                     value = value.substr(1, value.length()-2);
                 }
             }

src/minoltamn_int.cpp

@@ -2030,7 +2030,7 @@ namespace Exiv2 {
     {
         const TagDetails* td = find(minoltaSonyLensID, lensID);
         std::vector<std::string> tokens = split(td[0].label_,"|");
-        return os << exvGettext(trim(tokens[index-1]).c_str());
+        return os << exvGettext(trim(tokens.at(index-1)).c_str());
     }
 
     static std::ostream& resolveLens0x1c(std::ostream& os, const Value& value,

src/properties.cpp

@@ -2606,7 +2606,7 @@ namespace Exiv2 {
         // If property is a path for a nested property, determines the innermost element
         std::string::size_type i = property.find_last_of('/');
         if (i != std::string::npos) {
-            for (; i != std::string::npos && !isalpha(property[i]); ++i) {}
+            for (; i != std::string::npos && !isalpha(property.at(i)); ++i) {}
             property = property.substr(i);
             i = property.find_first_of(':');
             if (i != std::string::npos) {

src/sigmamn_int.cpp

@@ -126,7 +126,7 @@ namespace Exiv2 {
         std::string v = value.toString();
         std::string::size_type pos = v.find(':');
         if (pos != std::string::npos) {
-            if (v[pos + 1] == ' ') ++pos;
+            if (v.at(pos + 1) == ' ') ++pos;
             v = v.substr(pos + 1);
         }
         return os << v;
@@ -136,7 +136,7 @@ namespace Exiv2 {
                                               const Value& value,
                                               const ExifData*)
     {
-        switch (value.toString()[0]) {
+        switch (value.toString().at(0)) {
         case 'P': os << _("Program"); break;
         case 'A': os << _("Aperture priority"); break;
         case 'S': os << _("Shutter priority"); break;
@@ -150,7 +150,7 @@ namespace Exiv2 {
                                               const Value& value,
                                               const ExifData*)
     {
-        switch (value.toString()[0]) {
+        switch (value.toString().at(0)) {
         case 'A': os << _("Average"); break;
         case 'C': os << _("Center"); break;
         case '8': os << _("8-Segment"); break;

src/tags_int.cpp

@@ -3214,10 +3214,10 @@ namespace Exiv2 {
         }
 
         std::string stringValue = value.toString();
-        if (stringValue[19] == 'Z') {
+        if (stringValue.at(19) == 'Z') {
             stringValue = stringValue.substr(0, 19);
         }
-        for (unsigned int i = 0; i < stringValue.length(); ++i) {
+        for (size_t i = 0; i < stringValue.length(); ++i) {
             if (stringValue[i] == 'T') stringValue[i] = ' ';
             if (stringValue[i] == '-') stringValue[i] = ':';
         }

src/tiffvisitor_int.cpp

@@ -481,7 +481,7 @@ namespace Exiv2 {
             uint.push_back((uint16_t) object->pValue()->toLong(i));
         }
         // Check this is AFInfo2 (ints[0] = bytes in object)
-        if ( ints[0] != object->pValue()->count()*2 ) return ;
+        if ( ints.at(0) != object->pValue()->count()*2 ) return ;
 
         std::string familyGroup(std::string("Exif.") + groupName(object->group()) + ".");
 

src/types.cpp

@@ -606,7 +606,7 @@ namespace Exiv2 {
     bool stringTo<bool>(const std::string& s, bool& ok)
     {
         std::string lcs(s); /* lowercase string */
-        for(unsigned i = 0; i < lcs.length(); i++) {
+        for(size_t i = 0; i < lcs.length(); i++) {
             lcs[i] = std::tolower(s[i]);
         }
         /* handle the same values as xmp sdk */

src/utils.cpp

@@ -60,7 +60,7 @@ namespace Util {
         if (p.length() == 2 && p[1] == ':') return p; // For Windows paths
         std::string::size_type idx = p.find_last_of("\\/");
         if (idx == std::string::npos) return ".";
-        if (idx == 1 && p[0] == '\\' && p[1] == '\\') return p; // For Windows paths
+        if (idx == 1 && p.at(0) == '\\' && p.at(1) == '\\') return p; // For Windows paths
         p = p.substr(0, idx == 0 ? 1 : idx);
         while (   p.length() > 1
                && (p[p.length()-1] == '\\' || p[p.length()-1] == '/')) {
@@ -80,7 +80,7 @@ namespace Util {
         }
         if (p.length() == 2 && p[1] == ':') return ""; // For Windows paths
         std::string::size_type idx = p.find_last_of("\\/");
-        if (idx == 1 && p[0] == '\\' && p[1] == '\\') return ""; // For Windows paths
+        if (idx == 1 && p.at(0) == '\\' && p.at(1) == '\\') return ""; // For Windows paths
         if (idx != std::string::npos) p = p.substr(idx+1);
         if (delsuffix) p = p.substr(0, p.length() - suffix(p).length());
         return p;

src/value.cpp

@@ -491,8 +491,10 @@ namespace Exiv2 {
             std::string::size_type pos = comment.find_first_of(' ');
             std::string name = comment.substr(8, pos-8);
             // Strip quotes (so you can also specify the charset without quotes)
-            if (name[0] == '"') name = name.substr(1);
-            if (name[name.length()-1] == '"') name = name.substr(0, name.length()-1);
+            if (!name.empty()) {
+                if (name[0] == '"') name = name.substr(1);
+                if (name[name.length()-1] == '"') name = name.substr(0, name.length()-1);
+            }
             charsetId = CharsetInfo::charsetIdByName(name);
             if (charsetId == invalidCharsetId) {
 #ifndef SUPPRESS_WARNINGS
@@ -695,6 +697,9 @@ namespace Exiv2 {
         if (buf.length() > 5 && buf.substr(0, 5) == "type=") {
             std::string::size_type pos = buf.find_first_of(' ');
             type = buf.substr(5, pos-5);
+            if (type.empty()) {
+                throw Error(kerInvalidXmpText, type);
+            }
             // Strip quotes (so you can also specify the type without quotes)
             if (type[0] == '"') type = type.substr(1);
             if (type[type.length()-1] == '"') type = type.substr(0, type.length()-1);
@@ -859,6 +864,7 @@ namespace Exiv2 {
 
             std::string::size_type pos = buf.find_first_of(' ');
             lang = buf.substr(5, pos-5);
+            if (lang.empty()) throw Error(kerInvalidLangAltValue, buf);
             // Strip quotes (so you can also specify the language without quotes)
             if (lang[0] == '"') {
                 lang = lang.substr(1);
@@ -869,12 +875,12 @@ namespace Exiv2 {
                 lang = lang.substr(0, lang.length()-1);
             }
 
-            if (lang == "") throw Error(kerInvalidLangAltValue, buf);
+            if (lang.empty()) throw Error(kerInvalidLangAltValue, buf);
 
             // Check language is in the correct format (see https://www.ietf.org/rfc/rfc3066.txt)
             std::string::size_type charPos = lang.find_first_not_of(ALPHA);
             if (charPos != std::string::npos) {
-                if (lang[charPos] != '-' || lang.find_first_not_of(ALPHA_NUM, charPos+1) != std::string::npos)
+                if (lang.at(charPos) != '-' || lang.find_first_not_of(ALPHA_NUM, charPos+1) != std::string::npos)
                     throw Error(kerInvalidLangAltValue, buf);
             }
 

src/xmp.cpp

@@ -495,8 +495,8 @@ namespace Exiv2 {
         bool bNS  = out.find(':') != std::string::npos && !bURI;
 
         // pop trailing ':' on a namespace
-        if ( bNS ) {
-        std::size_t length = out.length();
+        if ( bNS && !out.empty() ) {
+            std::size_t length = out.length();
             if ( out[length-1] == ':' ) out = out.substr(0,length-1);
         }
 

src/xmpsidecar.cpp

@@ -232,7 +232,7 @@ namespace Exiv2 {
         std::string head(reinterpret_cast<const char*>(buf + start), len - start);
         if (head.substr(0, 5)  == "<?xml") {
             // Forward to the next tag
-            for (unsigned i = 5; i < head.size(); ++i) {
+            for (size_t i = 5; i < head.size(); ++i) {
                 if (head[i] == '<') {
                     head = head.substr(i);
                     break;

tests/bugfixes/github/test_issue_ghsa_v5g7_46xf_h728.py

@@ -0,0 +1,18 @@
+# -*- coding: utf-8 -*-
+
+from system_tests import CaseMeta, CopyTmpFiles, path, check_no_ASAN_UBSAN_errors
+
+class Jp2ImageEncodeJp2HeaderOutOfBoundsRead2(metaclass=CaseMeta):
+    """
+    Regression test for the bug described in:
+    https://github.com/Exiv2/exiv2/security/advisories/GHSA-v5g7-46xf-h728
+    """
+    url = "https://github.com/Exiv2/exiv2/security/advisories/GHSA-v5g7-46xf-h728"
+
+    filename = path("$data_path/issue_ghsa_v5g7_46xf_h728_poc.exv")
+    commands = ["$exiv2 $filename"]
+    stdout = [""]
+    stderr = ["""Exiv2 exception in print action for file $filename:
+Invalid XmpText type `'
+"""]
+    retval = [1]