Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <CAB6DpjW0BqVNJqBvSnKuA6bbAEpoEMvK6ef0ALEbhndYVubbCA@mail.gmail.com>
Date: Mon, 16 Jul 2018 15:10:41 +0800
From: Ruikai Liu <lrk700@...il.com>
To: oss-security@...ts.openwall.com
Subject: Integer underflow/overflow in MP4v2 2.0.0

Hi,

Integer underflow and overflow are found in MP4v2 2.0.0, a legacy library
dealing with MP4 media file.

========= Underflow =========

Atom is the basic element of MP4. However there's an integer underflow when
parsing an atom(src/mp4atom.cpp):

 121     uint64_t dataSize = file.ReadUInt32();
 ...
 146     dataSize -= hdrSize;
 ...
 151     if (pos + hdrSize + dataSize > pParentAtom->GetEnd()) {
 ...
 164         // skip to end of atom
 165         dataSize = pParentAtom->GetEnd() - pos - hdrSize;
 166     }

If `dataSize` read from file is less than `hdrSize`, then underflow happens
and it becomes a very large unsigned integer at line 146. Yet the check at
line 151 would still be passed, which results in an corrupted atom with
extremely large size.

========= Overflow =========

`ftyp` is an atom that describes the version info of the MP4 file. It will
allocate memory for compatible brands according to the atom's size:

 54 void MP4FtypAtom::Read()
 55 {
 56     compatibleBrands.SetCount( (m_size - 8) / 4 ); // brands array
fills rest of atom
 57     MP4Atom::Read();
 58 }

 342 void MP4StringProperty::SetCount(uint32_t count)
 343 {
 344     uint32_t oldCount = m_values.Size();
 345
 346     m_values.Resize(count);
 347
 348     for (uint32_t i = oldCount; i < count; i++) {
 349         m_values[i] = NULL;
 350     }
 351 }

`Resize` here is a wrapper of `realloc`:

102         void Resize(MP4ArrayIndex newSize) { \
103             m_numElements = newSize; \
104             m_maxNumElements = newSize; \
105             m_elements = (type*)MP4Realloc(m_elements, \
106                 m_maxNumElements * sizeof(type)); \
107         } \

We notice that an integer overflow could happen when calculating
`m_maxNumElements * sizeof(type)`. So the allocation may return a buffer
smaller than needed, and later operations on the buffer could result in
invalid memory reference, like setting values to be NULL in
`MP4StringProperty::SetCount`. This is the case for 64-bits program which
allows memory allocation for large(~4GB) size.

Things are a little different for 32-bits. In this case `realloc` would
fail and throws an exception:

 74 inline void* MP4Realloc(void* p, uint32_t newSize) {
 75     // workaround library bug
 76     if (p == NULL && newSize == 0) {
 77         return NULL;
 78     }
 79
 80     void* temp = realloc(p, newSize);
 81     if (temp == NULL && newSize > 0) {
 82         throw new PlatformException("malloc
failed",errno,__FILE__,__LINE__,__FUNCTION__);
 83     }
 84     return temp;
 85 }

And the destructor the `MP4StringProperty` would be invoked:

 334 MP4StringProperty::~MP4StringProperty()
 335 {
 336     uint32_t count = GetCount();
 337     for (uint32_t i = 0; i < count; i++) {
 338         MP4Free(m_values[i]);
 339     }
 340 }

But the count here is still the extremly large number we set before, and
the for-loop would certainly have some invalid addresses been freed.

========= POC =========

Here's a very simple POC file:

root@...ian:~# hexdump -Cv c2.mp4
00000000  00 00 00 07 66 74 79 70  6d 70 34 32 41 41 41 41
|....ftypmp42AAAA|
00000010  41 41 41 41 41 41 41 41                           |AAAAAAAA|
00000018

The size of the `ftyp` box is 7(the first 4 bytes), which is smalller than
the header size(8 bytes). Therefore the `dataSize` for this atom would
become -1=0xffffffffffffffff.

This POC file crashes both 32-bits and 64-bits mp4info.

========= Fix =========

For the underflow, we could check if `dataSize >= hdrSize` satisfies:

--- src/mp4atom.cpp     2018-07-16 14:54:33.513635593 +0800
+++ ../mp4v2-2.0.0-orig/src/mp4atom.cpp     2012-05-21 06:11:53.000000000
+0800
@@ -143,9 +143,6 @@
         dataSize = file.GetSize() - pos;
     }

-    if(dataSize < hdrSize) {
-        throw new Exception( "invalid dataSize", __FILE__, __LINE__,
__FUNCTION__ );
-    }
     dataSize -= hdrSize;

     log.verbose1f("\"%s\": type = \"%s\" data-size = %" PRIu64 " (0x%"
PRIx64 ") hdr %u",


For the overflow, we could check the result of the integer multiplication:

--- src/mp4array.h      2018-07-16 15:00:51.333620723 +0800
+++ ../mp4v2-2.0.0-orig-/src/mp4array.h      2012-05-21 06:11:53.000000000
+0800
@@ -102,11 +102,8 @@
         void Resize(MP4ArrayIndex newSize) { \
             m_numElements = newSize; \
             m_maxNumElements = newSize; \
-            uint32_t mul = newSize * sizeof(type); \
-            if(mul / newSize != sizeof(type)) \
-                throw new Exception("multiplication overflow", __FILE__,
__LINE__, __FUNCTION__);\
             m_elements = (type*)MP4Realloc(m_elements, \
-                mul); \
+                m_maxNumElements * sizeof(type)); \
         } \


========= Reference =========

https://code.google.com/archive/p/mp4v2/

-- 
Best regards,

Ruikai Liu

Powered by blists - more mailing lists

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.