From: Moritz Bunkus <moritz@bunkus.org>
Date: Sat, 20 Dec 2014 15:54:28 +0000 (+0100)
Subject: KaxBlock::ReadData(): use safe/checked memory access
X-Git-Tag: release-1.4.2~8
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4457e70466dcc77984202a05525428383cb74fe3;p=libmatroska

KaxBlock::ReadData(): use safe/checked memory access

Unchecked reading from memory locations works for cases in which all the
data is valid, but if it isn't then this leads to invalid memory access
and segmentation faults.

See https://trac.bunkus.org/ticket/1096
---

diff --git a/ChangeLog b/ChangeLog
index 15fd2c5..e9317c3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2014-12-20  Moritz Bunkus  <moritz@bunkus.org>
+
+        * KaxBlock::ReadData(): fixed several instances of unchecked
+        memory access leading to invalid memory access/segmentation faults
+        with invalid or broken data inside block groups/simple blocks.
+
 2014-12-19  Moritz Bunkus  <moritz@bunkus.org>
 
         * KaxBlock::ReadInternalHead(): fixed a off-by-one buffer overflow
diff --git a/src/KaxBlock.cpp b/src/KaxBlock.cpp
index fe0c22b..20fd161 100644
--- a/src/KaxBlock.cpp
+++ b/src/KaxBlock.cpp
@@ -30,11 +30,14 @@
   \version \$Id: KaxBlock.cpp 1265 2007-01-14 17:20:35Z mosu $
   \author Steve Lhomme     <robux4 @ users.sf.net>
   \author Julien Coloos    <suiryc @ users.sf.net>
+  \author Moritz Bunkus <moritz@bunkus.org>
 */
 #include <cassert>
 
 //#include <streams.h>
 
+#include "ebml/MemReadIOCallback.h"
+#include "ebml/SafeReadIOCallback.h"
 #include "matroska/KaxBlock.h"
 #include "matroska/KaxContexts.h"
 #include "matroska/KaxBlockData.h"
@@ -449,50 +452,56 @@ filepos_t KaxInternalBlock::ReadData(IOCallback & input, ScopeMode ReadFully)
 
   FirstFrameLocation = input.getFilePointer(); // will be updated accordingly below
 
+  SetValueIsSet(false);
+
+  try {
   if (ReadFully == SCOPE_ALL_DATA) {
     Result = EbmlBinary::ReadData(input, ReadFully);
-    binary *cursor = EbmlBinary::GetBuffer();
+    if (Result != GetSize())
+      throw SafeReadIOCallback::EndOfStreamX(GetSize() - Result);
+
+    binary *BufferStart = EbmlBinary::GetBuffer();
+
+    SafeReadIOCallback Mem(*this);
     uint8 BlockHeadSize = 4;
 
     // update internal values
-    TrackNumber = *cursor++;
+    TrackNumber = Mem.GetUInt8();
     if ((TrackNumber & 0x80) == 0) {
       // there is extra data
       if ((TrackNumber & 0x40) == 0) {
         // We don't support track numbers that large !
-        return Result;
+        throw SafeReadIOCallback::EndOfStreamX(0);
       }
       TrackNumber = (TrackNumber & 0x3F) << 8;
-      TrackNumber += *cursor++;
+      TrackNumber += Mem.GetUInt8();
       BlockHeadSize++;
     } else {
       TrackNumber &= 0x7F;
     }
 
-    big_int16 b16;
-    b16.Eval(cursor);
-    LocalTimecode = int16(b16);
+    LocalTimecode = int16(Mem.GetUInt16BE());
     bLocalTimecodeUsed = true;
-    cursor += 2;
 
+    uint8 Flags = Mem.GetUInt8();
     if (EbmlId(*this) == EBML_ID(KaxSimpleBlock)) {
-      bIsKeyframe = (*cursor & 0x80) != 0;
-      bIsDiscardable = (*cursor & 0x01) != 0;
+      bIsKeyframe = (Flags & 0x80) != 0;
+      bIsDiscardable = (Flags & 0x01) != 0;
     }
-    mInvisible = (*cursor & 0x08) >> 3;
-    mLacing = LacingType((*cursor++ & 0x06) >> 1);
+    mInvisible = (Flags & 0x08) >> 3;
+    mLacing = LacingType((Flags & 0x06) >> 1);
 
     // put all Frames in the list
     if (mLacing == LACING_NONE) {
-      FirstFrameLocation += cursor - EbmlBinary::GetBuffer();
-      DataBuffer * soloFrame = new DataBuffer(cursor, GetSize() - BlockHeadSize);
+      FirstFrameLocation += Mem.GetPosition();
+      DataBuffer * soloFrame = new DataBuffer(BufferStart + Mem.GetPosition(), GetSize() - BlockHeadSize);
       myBuffers.push_back(soloFrame);
       SizeList.resize(1);
       SizeList[0] = GetSize() - BlockHeadSize;
     } else {
       // read the number of frames in the lace
       uint32 LastBufferSize = GetSize() - BlockHeadSize - 1; // 1 for number of frame
-      uint8 FrameNum = *cursor++; // number of frames in the lace - 1
+      uint8 FrameNum = Mem.GetUInt8(); // number of frames in the lace - 1
       // read the list of frame sizes
       uint8 Index;
       int32 FrameSize;
@@ -506,10 +515,12 @@ filepos_t KaxInternalBlock::ReadData(IOCallback & input, ScopeMode ReadFully)
           for (Index=0; Index<FrameNum; Index++) {
             // get the size of the frame
             FrameSize = 0;
+            uint8 Value;
             do {
-              FrameSize += uint8(*cursor);
+              Value = Mem.GetUInt8();
+              FrameSize += Value;
               LastBufferSize--;
-            } while (*cursor++ == 0xFF);
+            } while (Value == 0xFF);
             SizeList[Index] = FrameSize;
             LastBufferSize -= FrameSize;
           }
@@ -517,17 +528,17 @@ filepos_t KaxInternalBlock::ReadData(IOCallback & input, ScopeMode ReadFully)
           break;
         case LACING_EBML:
           SizeRead = LastBufferSize;
-          FrameSize = ReadCodedSizeValue(cursor, SizeRead, SizeUnknown);
+          FrameSize = ReadCodedSizeValue(BufferStart + Mem.GetPosition(), SizeRead, SizeUnknown);
           SizeList[0] = FrameSize;
-          cursor += SizeRead;
+          Mem.Skip(SizeRead);
           LastBufferSize -= FrameSize + SizeRead;
 
           for (Index=1; Index<FrameNum; Index++) {
             // get the size of the frame
             SizeRead = LastBufferSize;
-            FrameSize += ReadCodedSizeSignedValue(cursor, SizeRead, SizeUnknown);
+            FrameSize += ReadCodedSizeSignedValue(BufferStart + Mem.GetPosition(), SizeRead, SizeUnknown);
             SizeList[Index] = FrameSize;
-            cursor += SizeRead;
+            Mem.Skip(SizeRead);
             LastBufferSize -= FrameSize + SizeRead;
           }
           if (Index <= FrameNum) // Safety check if FrameNum == 0
@@ -543,19 +554,34 @@ filepos_t KaxInternalBlock::ReadData(IOCallback & input, ScopeMode ReadFully)
           assert(0);
       }
 
-      FirstFrameLocation += cursor - EbmlBinary::GetBuffer();
+      FirstFrameLocation += Mem.GetPosition();
 
       for (Index=0; Index<=FrameNum; Index++) {
-        DataBuffer * lacedFrame = new DataBuffer(cursor, SizeList[Index]);
+        DataBuffer * lacedFrame = new DataBuffer(BufferStart + Mem.GetPosition(), SizeList[Index]);
         myBuffers.push_back(lacedFrame);
-        cursor += SizeList[Index];
+        Mem.Skip(SizeList[Index]);
       }
     }
+
+    binary *BufferEnd = BufferStart + GetSize();
+    size_t NumFrames  = myBuffers.size();
+
+    // Sanity checks for frame pointers and boundaries.
+    for (size_t Index = 0; Index < NumFrames; ++Index) {
+      binary *FrameStart  = myBuffers[Index]->Buffer();
+      binary *FrameEnd    = FrameStart + myBuffers[Index]->Size();
+      binary *ExpectedEnd = (Index + 1) < NumFrames ? myBuffers[Index + 1]->Buffer() : BufferEnd;
+
+      if ((FrameStart < BufferStart) || (FrameEnd > BufferEnd) || (FrameEnd != ExpectedEnd))
+        throw SafeReadIOCallback::EndOfStreamX(0);
+    }
+
     SetValueIsSet();
-  }
-  else if (ReadFully == SCOPE_PARTIAL_DATA) {
+  } else if (ReadFully == SCOPE_PARTIAL_DATA) {
     binary _TempHead[5];
     Result = input.read(_TempHead, 5);
+    if (Result != 5)
+      throw SafeReadIOCallback::EndOfStreamX(0);
     binary *cursor = _TempHead;
     binary *_tmpBuf;
     uint8 BlockHeadSize = 4;
@@ -670,6 +696,21 @@ filepos_t KaxInternalBlock::ReadData(IOCallback & input, ScopeMode ReadFully)
     Result = GetSize();
   }
 
+  } catch (SafeReadIOCallback::EndOfStreamX &) {
+    SetValueIsSet(false);
+
+    std::memset(EbmlBinary::GetBuffer(), 0, GetSize());
+    myBuffers.clear();
+    SizeList.clear();
+    Timecode           = 0;
+    LocalTimecode      = 0;
+    TrackNumber        = 0;
+    bLocalTimecodeUsed = false;
+    FirstFrameLocation = 0;
+
+    return 0;
+  }
+
   return Result;
 }