From 9069a1d987e79c1da4144e02c8c310b3d00c1e64 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Thu, 8 Feb 2024 22:17:21 +0000 Subject: [PATCH 1/3] Regression test for https://github.com/Exiv2/exiv2/security/advisories/GHSA-crmj-qh74-2r36 --- test/data/issue_ghsa_crmj_qh74_2r36_poc.mov | Bin 0 -> 155969 bytes .../github/test_issue_ghsa_crmj_qh74_2r36.py | 18 ++++++++++++++++++ .../test_regression_allfiles.py | 2 ++ 3 files changed, 20 insertions(+) create mode 100644 test/data/issue_ghsa_crmj_qh74_2r36_poc.mov create mode 100644 tests/bugfixes/github/test_issue_ghsa_crmj_qh74_2r36.py diff --git a/test/data/issue_ghsa_crmj_qh74_2r36_poc.mov b/test/data/issue_ghsa_crmj_qh74_2r36_poc.mov new file mode 100644 index 0000000000000000000000000000000000000000..245cc14bce9aa59bd4cdf5a1c6633adad06ce0d3 GIT binary patch literal 155969 zcmeI5y^a)D5QW=309qtlLPQoJC zkR5n@>UQ_NeLeT5yXjt%Z=}`k?9R^4)>NH3b*k!i(==Cqef7tS-(R(DbG+1Ceyz_< zb3-4WpFe#3Yu6lgYd+Kcv;MxmYTSMId-rZW=zp)h-s=1IduQ+7zkSo`3*GGZKK5~6 zoW72yk8b_)+t1IQJbtO?|NraK{kwPW-?l$r`#=7%$K+n$@OSL){91;1qh z@H^JG4zkJO`kVSP)DKNKssj&%t1??PX~Ic%RN&Pz`Ut)2$I!SAWwiT<6= z&$T#e6>I1xGJ#|`>-X8zUJPnn-r@MufBGXK3|FKE;g zTE)Odc@%w6w%oz4_&5G7&zi;au2Wx9Us7KVb06%dW%q?We-o~RxlZ;QhWuT&TW4s@ zeAN_Z8uV}UZ^cBd?t`DePo}tm{*C@U^#{wV{yy{H%zyjyW>(Lwp`W0i7_7|clcArm zpBC~@$9@{;`=t39`bn{lK|dMt&5U!w;3x1CqbKka*`RvPCHw?_B0ur@m-&;(A4VMU z_R%-!?}YPk>F>C6gqNJV0Y8DC7_7igWCQp~G%vX294GUG@-OBG(|Sv#uWax%@F4aZ z*l%#|zCDxJ5xXU`YH4C3$!cz8~*J8&E_I%e#d-Ou`iLpyENbM@h_XBpP-){ zkIbKNKHKCxN#O$LYfkLJ{C84^ujT7@$S*;cUX3&8C$c5_Nt(}8{7{xdKM6c<`E?dY zzk`3nzy17r$NV7kgK0dbzAWMd^MgY^W`3}rSAd^Pbtw3W`NKgU!qK_cm+t2mm>-Pl zRQ4~^`MFXrC3NJ~bvykX`a1?^Jw9=LZi|O3|GNr)gWrAL=la}*IHA6zzBD~^gT4a% zq!4Ep$P?-B1b>EqbN%9o2gx-~?$e*AKVRL~sMK4n{A!~-3VzF$W1ntSnp3ZBGID&qHezFnogP*`p;3u*G{N!YhW=kE5`cghdeYuLyd3@sf zfNtI_-+!*?Ma+K}d1C_a(*6qnF4{5cpU!Tb3upGzWCm{!_R}KW!#rfpK6$IhZBz%r zPh?B@NgD4feRNrF?C){DPjNmDe!~3VJ~#t@?_*c=LG(dOZ?pIq^<}XxqP~!&ea73qx3SFOso2_DOSG3O6X@-h4* z!WphV|H*NN`jYz67Wd=0luc&z8TBaoJ3+T8oPR)nhyIRYWd|J#{NBf|^mpj*SbCe2 z2hmT^Pa^(=ezJ;BqMvX+L=;CO{?gwuJwNIT!%yHR@Do{JR{ae>iEtHu0zYAY-t}eB zPlTa6=u+?#_zC<3esZa~%>F#{-^_nc`4;>e{yp^vOO*cp<=1T+<&A{yGd#XB*QrO1 z&&&R0X@0z7NBTSHCm}vNt`FF*n-`+LLw_fVqmZA%z5@F@lX+pzKS}yaEnl~CEY3ff z_>t@p@k{VK*rStibc^hx`=PG{{~q*V&aWE$gzLM8eBH+l^(FPCp(xjPwRnJ^MEnc= z1pOrNs}B7H{UojbyE#wv?+0yjO!ZCS-^#6)U*X^I?>qNz%M+$`I{TMqkApshqjwkC z2Yv!S>HUW5a~J0J;3vg79{7o32!7({iDivZS4SU2A9U^jKam;G2hj(G2kz$dV)!@w zTi)RLCH?uxA1qKTPP~2eHU0UMy$bmE;3w#V=!1*&FV1JPea$R>T!o*6{L|6Dq<=~O za^Pd^&x>=gKc9|=iq0X+jl3Rxu$X`0C-9Sfa0dL|$F7`zQmQ8^c-HDYVSk7Hojk8f z^9}St`tuv%2KWtrD+~4YNu`cqzX5#^eNeID^Dq6&$R9==@%GU-=wH&mJRZ3|AlCpadsnb?E#Pm+10#z)&Bzl3ueR^tr%plq4fAy(U!`jYxmo<)5bcrf!PJKzlX4Sri- zt>Ft3O14+~qtNU!|C-O1$lU4c7;}iO~J5OMBy@7sGtosvqm-bimlcF87{)v8K z@te4g4f9n|U1)$mM-OiGxQ+Hq;3u*r{3MO{m3my3V}E{D{X~Dq^p9U!yZ}F$^a*{Q6c4uR%i!Nh-b;N+eYp{CfZyP^P^r2YnEI@XCjy!>cXygjUbAQ65Dflr7N*(|C_QIO?CSKk4F^`PfO_ zOMfR>=T_n-!M9fHSl~DK&HiO&ULF1o|Av1T`g0Y%AN{0Q_b2jsnopyj6zzz9A}(Wn zQm!u*>aD(S%=r+-`3mrx`ICL>AnMC~>`MQV{-r#N{^hKG2>Zp6&d7doy1u$V-4*-> zzrk-=0Q_dZ7=5s)YoQNDI1%j$QeVn8%%7~{b3XsFKkxL?tbShO#o#yi?f0`fu0PNI zP8yHtUlws9i@(gDFn_}Q3G*jLSP%LT9&o-r=iB?b5B^=utLP`fjG(ikpR~>KQgeA% z-Ie|h{T)Re{rP}91^z~Vp8mXI#p?r{e-in_h$G%U`bLPWD4+LvXCCg@(++=!`B>&- zojXup$_%xBN2|wJ{0)6K>2~a2di|gCtI9gA*=O*d{yI6}cZj1x-=LGxbYp*>{dul0 z74J9I@)i0Z`k>Fx9sPOw^MyPMKgnG;xyO@RK|DZ#z5)Ivx7=4s*)P zf6L2uuq*vb`j-yBtZtybtjx#Lzl{1->=&o&t6ChjinWdU3g9>R4So;tRISIrZ}3~b z;^!-vKZ*RofW+&^=!58k{=D4Pa}V$n_=&-a!<8B@rhiHQ(&wL!{w4j(bbamln{XxQ z?#x#)U)A@c7U)v&6ZnbI6Zna2fPNC`vWb1t)pZd4OZgZ5%QT;<)I|nQgD%7V4*NUK z9oXNI87grDerNP;Uq5Ajet~v{f5X2WpfP^}|8C_8leq}+JA=>QcX>|B+h_2nzOF?d zL?1*SOq_@0^DpNkmvN=Wi|Jp|zodW3eATQx%X}5{Rm@k->AS&CqB^FqU&!@cO;613 zZ(n8pd*~xAu7l`bDhBCa(!VU>i_@Jtm##P;LT2#xc=qJ+%jtc6edzCVzCGvLdt607 zK|eu1K|eu1F@7HPtKi?_3-E9FH~f3iUKI0HB|WI3m$ky2$vg_@f6E@8pIQGRGoMoH zlWu5N{2Tw4SF*no_8`%}ci@TeZ^hUSb_Kt|Z}3|d0Ka#=P9yjYej70g^-JeqhxwC< z9elq|rr~^h`j^%70MJjS`p|Xym-H{aJ|LS^x;p3kWcm>vpE5sZzDo1a z?9a15&;C69%YlzfdA~6f`j_w%`j`Elzr};aM~ye2pA_q&1Yb?}Ydn9G&4Vr(?Q`|I z{k-=@j~eIhIsd!VAHu&~yl3lo&OhP$4W1{lzmts<@Vl4?%XaHrx`Kbp4CsTEe8l`9 z^MeCkxcY?i*&-dC`N4F3Rpa@su-EFY&<9-2`gi^pm0;(N6|Gbs)a_q>fBVOuet7=u)z43#y?k`8vC;$K0pRDRV@7}+C)9J74Hs8*-CIb}1FZzLs za6bt^|8BN8lTMg_{r^_AK)x!hSv@=reE4$qVaW*BsF>`n%trEK9Y`@lrE=p8o(JY~L9G literal 0 HcmV?d00001 diff --git a/tests/bugfixes/github/test_issue_ghsa_crmj_qh74_2r36.py b/tests/bugfixes/github/test_issue_ghsa_crmj_qh74_2r36.py new file mode 100644 index 0000000000..704e624e93 --- /dev/null +++ b/tests/bugfixes/github/test_issue_ghsa_crmj_qh74_2r36.py @@ -0,0 +1,18 @@ +# -*- coding: utf-8 -*- + +from system_tests import CaseMeta, CopyTmpFiles, path, check_no_ASAN_UBSAN_errors + +class QuickTimeVideoNikonTagsDecoderOutOfBoundsRead(metaclass=CaseMeta): + """ + Regression test for the bug described in: + https://github.com/Exiv2/exiv2/security/advisories/GHSA-crmj-qh74-2r36 + """ + url = "https://github.com/Exiv2/exiv2/security/advisories/GHSA-crmj-qh74-2r36" + + filename = path("$data_path/issue_ghsa_crmj_qh74_2r36_poc.mov") + commands = ["$exiv2 $filename"] + retval = [1] + stderr = ["""$exiv2_exception_message $filename: +$kerCorruptedMetadata +"""] + stdout = [""] diff --git a/tests/regression_tests/test_regression_allfiles.py b/tests/regression_tests/test_regression_allfiles.py index 426a0c36f4..81be2fbfcf 100644 --- a/tests/regression_tests/test_regression_allfiles.py +++ b/tests/regression_tests/test_regression_allfiles.py @@ -113,6 +113,8 @@ def get_valid_files(data_dir): "issue_2339_poc.tiff", "issue_2352_poc.jpg", "issue_2385_poc.tiff", + "issue_ghsa_crmj_qh74_2r36_poc.mov", + "issue_ghsa_g9xm_7538_mq8w_poc.mov", "issue_ghsa_583f_w9pm_99r2_poc.jp2", "issue_ghsa_7569_phvm_vwc2_poc.jp2", "issue_ghsa_mxw9_qx4c_6m8v_poc.jp2", From 8426b5d72e08b0b940ab36dd9b91ffcc6d34646d Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Thu, 8 Feb 2024 22:19:00 +0000 Subject: [PATCH 2/3] Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65541 Add `recursion_depth` parameter to ensure that the recursion doesn't go too deep. --- include/exiv2/quicktimevideo.hpp | 12 ++++++----- src/quicktimevideo.cpp | 36 +++++++++++++++++++------------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/include/exiv2/quicktimevideo.hpp b/include/exiv2/quicktimevideo.hpp index 3d3d23bd0f..e40e448ca1 100644 --- a/include/exiv2/quicktimevideo.hpp +++ b/include/exiv2/quicktimevideo.hpp @@ -52,7 +52,7 @@ class EXIV2API QuickTimeVideo : public Image { instance after it is passed to this method. Use the Image::io() method to get a temporary reference. */ - explicit QuickTimeVideo(BasicIo::UniquePtr io); + explicit QuickTimeVideo(BasicIo::UniquePtr io, size_t max_recursion_depth = 1000); //@} //! @name Manipulators @@ -71,7 +71,7 @@ class EXIV2API QuickTimeVideo : public Image { @brief Check for a valid tag and decode the block at the current IO position. Calls tagDecoder() or skips to next tag, if required. */ - void decodeBlock(std::string const& entered_from = ""); + void decodeBlock(size_t recursion_depth, std::string const& entered_from = ""); /*! @brief Interpret tag information, and call the respective function to save it in the respective XMP container. Decodes a Tag @@ -80,7 +80,7 @@ class EXIV2API QuickTimeVideo : public Image { @param buf Data buffer which contains tag ID. @param size Size of the data block used to store Tag Information. */ - void tagDecoder(Exiv2::DataBuf& buf, size_t size); + void tagDecoder(Exiv2::DataBuf& buf, size_t size, size_t recursion_depth); private: /*! @@ -123,7 +123,7 @@ class EXIV2API QuickTimeVideo : public Image { @brief Interpret Tag which contain other sub-tags, and save it in the respective XMP container. */ - void multipleEntriesDecoder(); + void multipleEntriesDecoder(size_t recursion_depth); /*! @brief Interpret Sample Description Tag, and save it in the respective XMP container. @@ -140,7 +140,7 @@ class EXIV2API QuickTimeVideo : public Image { in the respective XMP container. @param size Size of the data block used to store Tag Information. */ - void userDataDecoder(size_t size); + void userDataDecoder(size_t size, size_t recursion_depth); /*! @brief Interpret Preview Tag, and save it in the respective XMP container. @@ -202,6 +202,8 @@ class EXIV2API QuickTimeVideo : public Image { //! Variable to store height and width of a video frame. uint64_t height_ = 0; uint64_t width_ = 0; + //! Prevent stack exhaustion due to excessively deep recursion. + const size_t max_recursion_depth_; }; // QuickTimeVideo End diff --git a/src/quicktimevideo.cpp b/src/quicktimevideo.cpp index ee298dd98d..3e524087c5 100644 --- a/src/quicktimevideo.cpp +++ b/src/quicktimevideo.cpp @@ -538,8 +538,11 @@ namespace Exiv2 { using namespace Exiv2::Internal; -QuickTimeVideo::QuickTimeVideo(BasicIo::UniquePtr io) : - Image(ImageType::qtime, mdNone, std::move(io)), timeScale_(1), currentStream_(Null) { +QuickTimeVideo::QuickTimeVideo(BasicIo::UniquePtr io, size_t max_recursion_depth) : + Image(ImageType::qtime, mdNone, std::move(io)), + timeScale_(1), + currentStream_(Null), + max_recursion_depth_(max_recursion_depth) { } // QuickTimeVideo::QuickTimeVideo std::string QuickTimeVideo::mimeType() const { @@ -569,12 +572,14 @@ void QuickTimeVideo::readMetadata() { xmpData_["Xmp.video.MimeType"] = mimeType(); while (continueTraversing_) - decodeBlock(); + decodeBlock(0); xmpData_["Xmp.video.AspectRatio"] = getAspectRatio(width_, height_); } // QuickTimeVideo::readMetadata -void QuickTimeVideo::decodeBlock(std::string const& entered_from) { +void QuickTimeVideo::decodeBlock(size_t recursion_depth, std::string const& entered_from) { + enforce(recursion_depth < max_recursion_depth_, Exiv2::ErrorCode::kerCorruptedMetadata); + const long bufMinSize = 4; DataBuf buf(bufMinSize + 1); uint64_t size = 0; @@ -613,7 +618,7 @@ void QuickTimeVideo::decodeBlock(std::string const& entered_from) { if (newsize > buf.size()) { buf.resize(newsize); } - tagDecoder(buf, newsize); + tagDecoder(buf, newsize, recursion_depth + 1); } // QuickTimeVideo::decodeBlock static std::string readString(BasicIo& io, size_t size) { @@ -624,14 +629,15 @@ static std::string readString(BasicIo& io, size_t size) { return Exiv2::toString(str.data()); } -void QuickTimeVideo::tagDecoder(Exiv2::DataBuf& buf, size_t size) { +void QuickTimeVideo::tagDecoder(Exiv2::DataBuf& buf, size_t size, size_t recursion_depth) { + enforce(recursion_depth < max_recursion_depth_, Exiv2::ErrorCode::kerCorruptedMetadata); assert(buf.size() > 4); if (ignoreList(buf)) discard(size); else if (dataIgnoreList(buf)) { - decodeBlock(Exiv2::toString(buf.data())); + decodeBlock(recursion_depth + 1, Exiv2::toString(buf.data())); } else if (equalsQTimeTag(buf, "ftyp")) fileTypeDecoder(size); @@ -654,10 +660,10 @@ void QuickTimeVideo::tagDecoder(Exiv2::DataBuf& buf, size_t size) { videoHeaderDecoder(size); else if (equalsQTimeTag(buf, "udta")) - userDataDecoder(size); + userDataDecoder(size, recursion_depth + 1); else if (equalsQTimeTag(buf, "dref")) - multipleEntriesDecoder(); + multipleEntriesDecoder(recursion_depth + 1); else if (equalsQTimeTag(buf, "stsd")) sampleDesc(size); @@ -836,7 +842,8 @@ void QuickTimeVideo::CameraTagsDecoder(size_t size_external) { io_->seek(cur_pos + size_external, BasicIo::beg); } // QuickTimeVideo::CameraTagsDecoder -void QuickTimeVideo::userDataDecoder(size_t size_external) { +void QuickTimeVideo::userDataDecoder(size_t size_external, size_t recursion_depth) { + enforce(recursion_depth < max_recursion_depth_, Exiv2::ErrorCode::kerCorruptedMetadata); size_t cur_pos = io_->tell(); const TagVocabulary* td; const TagVocabulary* tv; @@ -866,7 +873,7 @@ void QuickTimeVideo::userDataDecoder(size_t size_external) { break; if (equalsQTimeTag(buf, "DcMD") || equalsQTimeTag(buf, "NCDT")) - userDataDecoder(size - 8); + userDataDecoder(size - 8, recursion_depth + 1); else if (equalsQTimeTag(buf, "NCTG")) NikonTagsDecoder(size - 8); @@ -898,7 +905,7 @@ void QuickTimeVideo::userDataDecoder(size_t size_external) { } else if (td) - tagDecoder(buf, size - 8); + tagDecoder(buf, size - 8, recursion_depth + 1); } io_->seek(cur_pos + size_external, BasicIo::beg); @@ -1269,7 +1276,8 @@ void QuickTimeVideo::imageDescDecoder() { xmpData_["Xmp.video.BitDepth"] = static_cast(buf.read_uint8(0)); } // QuickTimeVideo::imageDescDecoder -void QuickTimeVideo::multipleEntriesDecoder() { +void QuickTimeVideo::multipleEntriesDecoder(size_t recursion_depth) { + enforce(recursion_depth < max_recursion_depth_, Exiv2::ErrorCode::kerCorruptedMetadata); DataBuf buf(4 + 1); io_->readOrThrow(buf.data(), 4); io_->readOrThrow(buf.data(), 4); @@ -1278,7 +1286,7 @@ void QuickTimeVideo::multipleEntriesDecoder() { noOfEntries = buf.read_uint32(0, bigEndian); for (uint32_t i = 0; i < noOfEntries && continueTraversing_; i++) { - decodeBlock(); + decodeBlock(recursion_depth + 1); } } // QuickTimeVideo::multipleEntriesDecoder From a60f0a38ce79dfa711a6272ff32349ceb8185764 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 11 Feb 2024 15:39:42 +0000 Subject: [PATCH 3/3] Throw exception if the recursion goes too deep. --- include/exiv2/bmffimage.hpp | 3 ++- src/bmffimage.cpp | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/include/exiv2/bmffimage.hpp b/include/exiv2/bmffimage.hpp index b634953925..786f57201b 100644 --- a/include/exiv2/bmffimage.hpp +++ b/include/exiv2/bmffimage.hpp @@ -56,7 +56,7 @@ class EXIV2API BmffImage : public Image { @param create Specifies if an existing image should be read (false) or if a new file should be created (true). */ - BmffImage(BasicIo::UniquePtr io, bool create); + BmffImage(BasicIo::UniquePtr io, bool create, size_t max_box_depth = 1000); //@} //@{ @@ -138,6 +138,7 @@ class EXIV2API BmffImage : public Image { uint16_t xmpID_{0}; std::map ilocs_; bool bReadMetadata_{false}; + const size_t max_box_depth_; //@} /*! diff --git a/src/bmffimage.cpp b/src/bmffimage.cpp index 38532dcc7a..67d9cd1427 100644 --- a/src/bmffimage.cpp +++ b/src/bmffimage.cpp @@ -78,8 +78,8 @@ std::string Iloc::toString() const { return Internal::stringFormat("ID = %u from,length = %u,%u", ID_, start_, length_); } -BmffImage::BmffImage(BasicIo::UniquePtr io, bool /* create */) : - Image(ImageType::bmff, mdExif | mdIptc | mdXmp, std::move(io)) { +BmffImage::BmffImage(BasicIo::UniquePtr io, bool /* create */, size_t max_box_depth) : + Image(ImageType::bmff, mdExif | mdIptc | mdXmp, std::move(io)), max_box_depth_(max_box_depth) { } // BmffImage::BmffImage std::string BmffImage::toAscii(uint32_t n) { @@ -237,7 +237,7 @@ uint64_t BmffImage::boxHandler(std::ostream& out /* = std::cout*/, Exiv2::PrintS // never visit a box twice! if (depth == 0) visits_.clear(); - if (visits_.find(address) != visits_.end() || visits_.size() > visits_max_) { + if (visits_.find(address) != visits_.end() || visits_.size() > visits_max_ || depth >= max_box_depth_) { throw Error(ErrorCode::kerCorruptedMetadata); } visits_.insert(address);