Add bounds check to MemIo::seek(). (#944)

- Regression test for missing bounds check in MemIo::seek()
- Add bounds check to MemIo::seek(), this fixes CVE-2019-13504

(cherry picked from commit bd0afe0390439b2c424d881c8c6eb0c5624e31d9)

Additional fixes for 0.27:
- Add fix for the linux variant of MemIo::seek
- Change type of variable from unsigned to signed

src/basicio.cpp

@@ -1291,7 +1291,7 @@ namespace Exiv2 {
 #if defined(_MSC_VER)
     int MemIo::seek( int64_t offset, Position pos )
     {
-        uint64_t newIdx = 0;
+        int64_t newIdx = 0;
 
         switch (pos) {
         case BasicIo::cur: newIdx = p_->idx_ + offset; break;
@@ -1299,7 +1299,15 @@ namespace Exiv2 {
         case BasicIo::end: newIdx = p_->size_ + offset; break;
         }
 
-        p_->idx_ = static_cast<long>(newIdx);   //not very sure about this. need more test!!    - note by Shawn  fly2xj@gmail.com //TODO
+        if (newIdx < 0)
+            return 1;
+
+        if (static_cast<size_t>(newIdx) > p_->size_) {
+            p_->eof_ = true;
+            return 1;
+        }
+
+        p_->idx_ = static_cast<long>(newIdx);
         p_->eof_ = false;
         return 0;
     }
@@ -1314,7 +1322,14 @@ namespace Exiv2 {
         case BasicIo::end: newIdx = p_->size_ + offset; break;
         }
 
-        if (newIdx < 0) return 1;
+        if (newIdx < 0)
+            return 1;
+
+        if (newIdx > p_->size_) {
+            p_->eof_ = true;
+            return 1;
+        }
+
         p_->idx_ = newIdx;
         p_->eof_ = false;
         return 0;

unitTests/CMakeLists.txt

@@ -19,6 +19,7 @@ endforeach()
 
 add_executable(unit_tests mainTestRunner.cpp
     gtestwrapper.h
+    test_basicio.cpp
     test_types.cpp
     test_tiffheader.cpp
     test_futils.cpp

unitTests/test_basicio.cpp

@@ -0,0 +1,77 @@
+#include <exiv2/basicio.hpp>
+#include <gtest/gtest.h>
+
+using namespace Exiv2;
+
+TEST(MemIo, seek_out_of_bounds_00)
+{
+    byte buf[1024];
+    memset(buf, 0, sizeof(buf));
+
+    MemIo io(buf, sizeof(buf));
+    ASSERT_FALSE(io.eof());
+
+    // Regression test for bug reported in https://github.com/Exiv2/exiv2/pull/945
+    // The problem is that MemIo::seek() does not check that the new offset is
+    // in bounds.
+    byte tmp[16];
+    ASSERT_EQ(io.seek(0x10000000, BasicIo::beg), 1);
+    ASSERT_TRUE(io.eof());
+
+    // The seek was invalid, so the offset didn't change and this read still works.
+    ASSERT_EQ(io.read(tmp, sizeof(tmp)), sizeof(tmp));
+}
+
+TEST(MemIo, seek_out_of_bounds_01)
+{
+    byte buf[1024];
+    memset(buf, 0, sizeof(buf));
+
+    MemIo io(buf, sizeof(buf));
+    ASSERT_FALSE(io.eof());
+
+    byte tmp[16];
+
+    // Seek to the end of the file.
+    ASSERT_EQ(io.seek(0, BasicIo::end), 0);
+    ASSERT_EQ(io.read(tmp, sizeof(tmp)), 0);
+
+    // Try to seek past the end of the file.
+    ASSERT_EQ(io.seek(0x10000000, BasicIo::end), 1);
+    ASSERT_TRUE(io.eof());
+    ASSERT_EQ(io.read(tmp, sizeof(tmp)), 0);
+}
+
+TEST(MemIo, seek_out_of_bounds_02)
+{
+    byte buf[1024];
+    memset(buf, 0, sizeof(buf));
+
+    MemIo io(buf, sizeof(buf));
+    ASSERT_FALSE(io.eof());
+
+    byte tmp[16];
+
+    // Try to seek past the end of the file.
+    ASSERT_EQ(io.seek(0x10000000, BasicIo::cur), 1);
+    ASSERT_TRUE(io.eof());
+    // The seek was invalid, so the offset didn't change and this read still works.
+    ASSERT_EQ(io.read(tmp, sizeof(tmp)), sizeof(tmp));
+}
+
+TEST(MemIo, seek_out_of_bounds_03)
+{
+    byte buf[1024];
+    memset(buf, 0, sizeof(buf));
+
+    MemIo io(buf, sizeof(buf));
+    ASSERT_FALSE(io.eof());
+
+    byte tmp[16];
+
+    // Try to seek past the beginning of the file.
+    ASSERT_EQ(io.seek(-0x10000000, BasicIo::cur), 1);
+    ASSERT_FALSE(io.eof());
+    // The seek was invalid, so the offset didn't change and this read still works.
+    ASSERT_EQ(io.read(tmp, sizeof(tmp)), sizeof(tmp));
+}