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 15fd2c5309df060412a064ac8a78d16efb7ff172..e9317c399211c029f50679c4cf2571591afe8385 100644 (file)
--- 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 fe0c22b21e55f41f96da565f24be1bb26df480d1..20fd16120439ecba674e4466009c73bb9e0435cf 100644 (file)
--- 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;
 }