From 8ab78004776f9e7fafff557fb9cc1da44108f65a Mon Sep 17 00:00:00 2001
From: Luis Diaz <piponazo@gmail.com>
Date: Tue, 5 Apr 2022 09:58:20 +0200
Subject: [PATCH] Remove 2nd param from Photoshop::isIrb() since it is always
 hardcoded

---
 include/exiv2/jpgimage.hpp   | 10 +++++-----
 src/jpgimage.cpp             |  8 +++++---
 src/psdimage.cpp             |  4 ++--
 unitTests/test_Photoshop.cpp | 21 ++++++++++++---------
 4 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/include/exiv2/jpgimage.hpp b/include/exiv2/jpgimage.hpp
index 9d0e622e..168031cb 100644
--- a/include/exiv2/jpgimage.hpp
+++ b/include/exiv2/jpgimage.hpp
@@ -27,11 +27,11 @@ struct EXIV2API Photoshop {
   static constexpr uint16_t preview_ = 0x040c;                         //!< %Photoshop preview marker
 
   /// @brief Checks an IRB
-  /// @param pPsData        Existing IRB buffer
-  /// @param sizePsData     Size of the IRB buffer
-  /// @return true  if the IRB marker is known and the buffer is big enough to check this;<BR>
-  ///   false otherwise
-  static bool isIrb(const byte* pPsData, size_t sizePsData);
+  /// @param pPsData  Existing IRB buffer. It is expected to be of size 4.
+  /// @return true  if the IRB marker is known
+  /// @todo This should be an implementation detail and not exposed in the API. An attacker could try to pass
+  ///   a smaller buffer or null pointer.
+  static bool isIrb(const byte* pPsData);
 
   /// @brief Validates all IRBs
   /// @param pPsData        Existing IRB buffer
diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp
index efe0050d..0f5ce70e 100644
--- a/src/jpgimage.cpp
+++ b/src/jpgimage.cpp
@@ -35,9 +35,11 @@ static inline bool inRange2(int value, int lo1, int hi1, int lo2, int hi2) {
   return inRange(lo1, value, hi1) || inRange(lo2, value, hi2);
 }
 
-bool Photoshop::isIrb(const byte* pPsData, size_t sizePsData) {
-  if (sizePsData < 4)
+bool Photoshop::isIrb(const byte* pPsData) {
+  if (pPsData == nullptr) {
     return false;
+  }
+  /// \todo check if direct array comparison is faster than a call to memcmp
   return std::any_of(irbId_.begin(), irbId_.end(), [pPsData](auto id) { return memcmp(pPsData, id, 4) == 0; });
 }
 
@@ -69,7 +71,7 @@ int Photoshop::locateIrb(const byte* pPsData, size_t sizePsData, uint16_t psTag,
   std::cerr << "Photoshop::locateIrb: ";
 #endif
   // Data should follow Photoshop format, if not exit
-  while (position <= sizePsData - 12 && isIrb(pPsData + position, 4)) {
+  while (position <= sizePsData - 12 && isIrb(pPsData + position)) {
     const byte* hrd = pPsData + position;
     position += 4;
     uint16_t type = getUShort(pPsData + position, bigEndian);
diff --git a/src/psdimage.cpp b/src/psdimage.cpp
index 7f0c8155..4df8f882 100644
--- a/src/psdimage.cpp
+++ b/src/psdimage.cpp
@@ -175,7 +175,7 @@ void PsdImage::readMetadata() {
       throw Error(ErrorCode::kerNotAnImage, "Photoshop");
     }
 
-    if (!Photoshop::isIrb(buf, 4)) {
+    if (!Photoshop::isIrb(buf)) {
       break;  // bad resource type
     }
     uint16_t resourceId = getUShort(buf + 4, bigEndian);
@@ -413,7 +413,7 @@ void PsdImage::doWriteMetadata(BasicIo& outIo) {
     // read resource type and ID
     uint32_t resourceType = getULong(buf, bigEndian);
 
-    if (!Photoshop::isIrb(buf, 4)) {
+    if (!Photoshop::isIrb(buf)) {
       throw Error(ErrorCode::kerNotAnImage, "Photoshop");  // bad resource type
     }
     uint16_t resourceId = getUShort(buf + 4, bigEndian);
diff --git a/unitTests/test_Photoshop.cpp b/unitTests/test_Photoshop.cpp
index c38a14dc..61c77c8a 100644
--- a/unitTests/test_Photoshop.cpp
+++ b/unitTests/test_Photoshop.cpp
@@ -12,20 +12,23 @@ constexpr std::array validMarkers{"8BIM", "AgHg", "DCSR", "PHUT"};
 
 TEST(Photoshop_isIrb, returnsTrueWithValidMarkers) {
   for (const auto& marker : validMarkers) {
-    ASSERT_TRUE(Photoshop::isIrb(reinterpret_cast<const byte*>(marker), 4));
+    ASSERT_TRUE(Photoshop::isIrb(reinterpret_cast<const byte*>(marker)));
   }
 }
 
 
 TEST(Photoshop_isIrb, returnsFalseWithInvalidMarkers) {
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("7BIM"), 4));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("AGHg"), 4));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("dcsr"), 4));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("LUIS"), 4));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("7BIM")));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("AGHg")));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("dcsr")));
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("LUIS")));
 }
 
-TEST(Photoshop_isIrb, returnsFalseWithInvalidSize) {
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BIM"), 3));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BIM"), 0));
-  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BIM"), 5));
+TEST(Photoshop_isIrb, returnsFalseWithNullPointer) {
+  ASSERT_FALSE(Photoshop::isIrb(nullptr));
+}
+
+/// \note probably this is not safe and we need to remove it
+TEST(Photoshop_isIrb, returnsFalseWithShorterMarker) {
+  ASSERT_FALSE(Photoshop::isIrb(reinterpret_cast<const byte *>("8BI")));
 }