From 004f2a2447c3d5d1d178fb5d9e961a1b4c26dc98 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sat, 10 Jul 2021 10:41:53 +0100 Subject: [PATCH 1/3] Regression test for https://github.com/Exiv2/exiv2/security/advisories/GHSA-v5g7-46xf-h728 --- test/data/issue_ghsa_v5g7_46xf_h728_poc.exv | Bin 0 -> 276 bytes .../github/test_issue_ghsa_v5g7_46xf_h728.py | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100755 test/data/issue_ghsa_v5g7_46xf_h728_poc.exv create mode 100644 tests/bugfixes/github/test_issue_ghsa_v5g7_46xf_h728.py diff --git a/test/data/issue_ghsa_v5g7_46xf_h728_poc.exv b/test/data/issue_ghsa_v5g7_46xf_h728_poc.exv new file mode 100755 index 0000000000000000000000000000000000000000..3e13b27a48c737c1b1c137acbb6ee878216505f2 GIT binary patch literal 276 zcmX|7%WA_g5Ol9O_XC7-w&f@yiH&6+YI@5hP{>t@H;HjA30Yu;d|IyloI*dLk(O6?%^^SCA${Znki@pk=# zK9srhTFn8mh(&R@#`y$graGjX^7EnN5yz*$XZ3v_UFMN8M>0#2gv69k8U{gty0f&x zg|eHDDtDO@y1l8jt(8Qh>@W-lgoV;K$M&p&zXkN&pT{($GxjALsg`UtU$7-6qZ#sW Q;}eosL=Rq109h6J4JxT#4gdfE literal 0 HcmV?d00001 diff --git a/tests/bugfixes/github/test_issue_ghsa_v5g7_46xf_h728.py b/tests/bugfixes/github/test_issue_ghsa_v5g7_46xf_h728.py new file mode 100644 index 0000000000..de68afc222 --- /dev/null +++ b/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] From 2e7bb581a234bfb0d0c9e16a1dbf037a8c30681e Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sat, 10 Jul 2021 10:42:24 +0100 Subject: [PATCH 2/3] Check that `type` isn't an empty string. --- src/value.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/value.cpp b/src/value.cpp index b333693b51..caed3328f6 100644 --- a/src/value.cpp +++ b/src/value.cpp @@ -695,6 +695,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); From 76e313745e813f80e8910aceb2210af3ad8cf897 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 11 Jul 2021 12:04:53 +0100 Subject: [PATCH 3/3] Safer std::vector indexing. --- samples/addmoddel.cpp | 2 +- samples/exiv2json.cpp | 6 +++--- src/actions.cpp | 20 +++++++++++--------- src/basicio.cpp | 8 ++++---- src/convert.cpp | 2 +- src/exiv2.cpp | 4 ++-- src/minoltamn_int.cpp | 2 +- src/properties.cpp | 2 +- src/sigmamn_int.cpp | 6 +++--- src/tags_int.cpp | 4 ++-- src/tiffvisitor_int.cpp | 2 +- src/types.cpp | 2 +- src/utils.cpp | 4 ++-- src/value.cpp | 11 +++++++---- src/xmp.cpp | 4 ++-- src/xmpsidecar.cpp | 2 +- 16 files changed, 43 insertions(+), 38 deletions(-) diff --git a/samples/addmoddel.cpp b/samples/addmoddel.cpp index 44692a77fb..98a112fdcb 100644 --- a/samples/addmoddel.cpp +++ b/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 diff --git a/samples/exiv2json.cpp b/samples/exiv2json.cpp index e6b0db1bbf..2dc15f56e6 100644 --- a/samples/exiv2json.cpp +++ b/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); diff --git a/src/actions.cpp b/src/actions.cpp index 8cfe0fd91c..405e79e634 100644 --- a/src/actions.cpp +++ b/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(*n); + if (num == 0) { // Write all previews - for (int num = 0; num < static_cast(pvList.size()); ++num) { - writePreviewFile(pvMgr.getPreviewImage(pvList[num]), num + 1); + for (num = 0; num < pvList.size(); ++num) { + writePreviewFile(pvMgr.getPreviewImage(pvList[num]), static_cast(num + 1)); } break; } - if (*n > static_cast(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(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; } diff --git a/src/basicio.cpp b/src/basicio.cpp index 5193ae5432..be7d38637a 100644 --- a/src/basicio.cpp +++ b/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."); } diff --git a/src/convert.cpp b/src/convert.cpp index f4d1fdf0a1..427419dd3b 100644 --- a/src/convert.cpp +++ b/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; diff --git a/src/exiv2.cpp b/src/exiv2.cpp index 22ccafc469..1ebb633943 100644 --- a/src/exiv2.cpp +++ b/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); } } diff --git a/src/minoltamn_int.cpp b/src/minoltamn_int.cpp index aef7e96f43..d54be65a5e 100644 --- a/src/minoltamn_int.cpp +++ b/src/minoltamn_int.cpp @@ -2030,7 +2030,7 @@ namespace Exiv2 { { const TagDetails* td = find(minoltaSonyLensID, lensID); std::vector 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, diff --git a/src/properties.cpp b/src/properties.cpp index 7fcfc711df..0d4395dc16 100644 --- a/src/properties.cpp +++ b/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) { diff --git a/src/sigmamn_int.cpp b/src/sigmamn_int.cpp index fdafc4c738..9c1521560d 100644 --- a/src/sigmamn_int.cpp +++ b/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; diff --git a/src/tags_int.cpp b/src/tags_int.cpp index 460b4443d2..1be201006d 100644 --- a/src/tags_int.cpp +++ b/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] = ':'; } diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp index 68ca8597d6..b011b31226 100644 --- a/src/tiffvisitor_int.cpp +++ b/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()) + "."); diff --git a/src/types.cpp b/src/types.cpp index b093487d12..e1eca47abf 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -606,7 +606,7 @@ namespace Exiv2 { bool stringTo(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 */ diff --git a/src/utils.cpp b/src/utils.cpp index f5b72473d6..b68d8a22aa 100644 --- a/src/utils.cpp +++ b/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; diff --git a/src/value.cpp b/src/value.cpp index caed3328f6..739cd46602 100644 --- a/src/value.cpp +++ b/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 @@ -862,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); @@ -872,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); } diff --git a/src/xmp.cpp b/src/xmp.cpp index 9f47a1a8f9..c22bcf240f 100644 --- a/src/xmp.cpp +++ b/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); } diff --git a/src/xmpsidecar.cpp b/src/xmpsidecar.cpp index dd2cd130e4..d9d5e090f8 100644 --- a/src/xmpsidecar.cpp +++ b/src/xmpsidecar.cpp @@ -232,7 +232,7 @@ namespace Exiv2 { std::string head(reinterpret_cast(buf + start), len - start); if (head.substr(0, 5) == "