MTP: add strict bounds checking for all incoming packets

Previously we did not sanity check incoming MTP packets,
which could result in crashes due to reading off the edge of a packet.
Now all MTP packet getter functions return a boolean result
(true for OK, false for reading off the edge of the packet)
and we now return errors for malformed packets.

Bug: 18113092
Change-Id: Ic7623ee96f00652bdfb4f66acb16a93db5a1c105

1 files changed, 55 insertions, 38 deletions

diff --git a/media/mtp/MtpProperty.cpp b/media/mtp/MtpProperty.cpp
index c500901..d58e2a4 100644
--- a/media/mtp/MtpProperty.cpp
+++ b/media/mtp/MtpProperty.cpp
@@ -106,15 +106,15 @@ MtpProperty::~MtpProperty() {
         free(mMinimumValue.str);
         free(mMaximumValue.str);
         if (mDefaultArrayValues) {
-            for (int i = 0; i < mDefaultArrayLength; i++)
+            for (uint32_t i = 0; i < mDefaultArrayLength; i++)
                 free(mDefaultArrayValues[i].str);
         }
         if (mCurrentArrayValues) {
-            for (int i = 0; i < mCurrentArrayLength; i++)
+            for (uint32_t i = 0; i < mCurrentArrayLength; i++)
                 free(mCurrentArrayValues[i].str);
         }
         if (mEnumValues) {
-            for (int i = 0; i < mEnumLength; i++)
+            for (uint16_t i = 0; i < mEnumLength; i++)
                 free(mEnumValues[i].str);
         }
     }
@@ -123,11 +123,14 @@ MtpProperty::~MtpProperty() {
     delete[] mEnumValues;
 }
 
-void MtpProperty::read(MtpDataPacket& packet) {
-    mCode = packet.getUInt16();
+bool MtpProperty::read(MtpDataPacket& packet) {
+    uint8_t temp8;
+
+    if (!packet.getUInt16(mCode)) return false;
     bool deviceProp = isDeviceProperty();
-    mType = packet.getUInt16();
-    mWriteable = (packet.getUInt8() == 1);
+    if (!packet.getUInt16(mType)) return false;
+    if (!packet.getUInt8(temp8)) return false;
+    mWriteable = (temp8 == 1);
     switch (mType) {
         case MTP_TYPE_AINT8:
         case MTP_TYPE_AUINT8:
@@ -140,28 +143,36 @@ void MtpProperty::read(MtpDataPacket& packet) {
         case MTP_TYPE_AINT128:
         case MTP_TYPE_AUINT128:
             mDefaultArrayValues = readArrayValues(packet, mDefaultArrayLength);
-            if (deviceProp)
+            if (!mDefaultArrayValues) return false;
+            if (deviceProp) {
                 mCurrentArrayValues = readArrayValues(packet, mCurrentArrayLength);
+                if (!mCurrentArrayValues) return false;
+            }
             break;
         default:
-            readValue(packet, mDefaultValue);
-            if (deviceProp)
-                readValue(packet, mCurrentValue);
+            if (!readValue(packet, mDefaultValue)) return false;
+            if (deviceProp) {
+                if (!readValue(packet, mCurrentValue)) return false;
+            }
     }
-    if (!deviceProp)
-        mGroupCode = packet.getUInt32();
-    mFormFlag = packet.getUInt8();
+    if (!deviceProp) {
+        if (!packet.getUInt32(mGroupCode)) return false;
+    }
+    if (!packet.getUInt8(mFormFlag)) return false;
 
     if (mFormFlag == kFormRange) {
-            readValue(packet, mMinimumValue);
-            readValue(packet, mMaximumValue);
-            readValue(packet, mStepSize);
+            if (!readValue(packet, mMinimumValue)) return false;
+            if (!readValue(packet, mMaximumValue)) return false;
+            if (!readValue(packet, mStepSize)) return false;
     } else if (mFormFlag == kFormEnum) {
-        mEnumLength = packet.getUInt16();
+        if (!packet.getUInt16(mEnumLength)) return false;
         mEnumValues = new MtpPropertyValue[mEnumLength];
-        for (int i = 0; i < mEnumLength; i++)
-            readValue(packet, mEnumValues[i]);
+        for (int i = 0; i < mEnumLength; i++) {
+            if (!readValue(packet, mEnumValues[i])) return false;
+        }
     }
+
+    return true;
 }
 
 void MtpProperty::write(MtpDataPacket& packet) {
@@ -409,57 +420,59 @@ void MtpProperty::print(MtpPropertyValue& value, MtpString& buffer) {
     }
 }
 
-void MtpProperty::readValue(MtpDataPacket& packet, MtpPropertyValue& value) {
+bool MtpProperty::readValue(MtpDataPacket& packet, MtpPropertyValue& value) {
     MtpStringBuffer stringBuffer;
 
     switch (mType) {
         case MTP_TYPE_INT8:
         case MTP_TYPE_AINT8:
-            value.u.i8 = packet.getInt8();
+            if (!packet.getInt8(value.u.i8)) return false;
             break;
         case MTP_TYPE_UINT8:
         case MTP_TYPE_AUINT8:
-            value.u.u8 = packet.getUInt8();
+            if (!packet.getUInt8(value.u.u8)) return false;
             break;
         case MTP_TYPE_INT16:
         case MTP_TYPE_AINT16:
-            value.u.i16 = packet.getInt16();
+            if (!packet.getInt16(value.u.i16)) return false;
             break;
         case MTP_TYPE_UINT16:
         case MTP_TYPE_AUINT16:
-            value.u.u16 = packet.getUInt16();
+            if (!packet.getUInt16(value.u.u16)) return false;
             break;
         case MTP_TYPE_INT32:
         case MTP_TYPE_AINT32:
-            value.u.i32 = packet.getInt32();
+            if (!packet.getInt32(value.u.i32)) return false;
             break;
         case MTP_TYPE_UINT32:
         case MTP_TYPE_AUINT32:
-            value.u.u32 = packet.getUInt32();
+            if (!packet.getUInt32(value.u.u32)) return false;
             break;
         case MTP_TYPE_INT64:
         case MTP_TYPE_AINT64:
-            value.u.i64 = packet.getInt64();
+            if (!packet.getInt64(value.u.i64)) return false;
             break;
         case MTP_TYPE_UINT64:
         case MTP_TYPE_AUINT64:
-            value.u.u64 = packet.getUInt64();
+            if (!packet.getUInt64(value.u.u64)) return false;
             break;
         case MTP_TYPE_INT128:
         case MTP_TYPE_AINT128:
-            packet.getInt128(value.u.i128);
+            if (!packet.getInt128(value.u.i128)) return false;
             break;
         case MTP_TYPE_UINT128:
         case MTP_TYPE_AUINT128:
-            packet.getUInt128(value.u.u128);
+            if (!packet.getUInt128(value.u.u128)) return false;
             break;
         case MTP_TYPE_STR:
-            packet.getString(stringBuffer);
+            if (!packet.getString(stringBuffer)) return false;
             value.str = strdup(stringBuffer);
             break;
         default:
             ALOGE("unknown type %04X in MtpProperty::readValue", mType);
+            return false;
     }
+    return true;
 }
 
 void MtpProperty::writeValue(MtpDataPacket& packet, MtpPropertyValue& value) {
@@ -517,8 +530,9 @@ void MtpProperty::writeValue(MtpDataPacket& packet, MtpPropertyValue& value) {
     }
 }
 
-MtpPropertyValue* MtpProperty::readArrayValues(MtpDataPacket& packet, int& length) {
-    length = packet.getUInt32();
+MtpPropertyValue* MtpProperty::readArrayValues(MtpDataPacket& packet, uint32_t& length) {
+    if (!packet.getUInt32(length)) return NULL;
+
     // Fail if resulting array is over 2GB.  This is because the maximum array
     // size may be less than SIZE_MAX on some platforms.
     if ( CC_UNLIKELY(
@@ -528,14 +542,17 @@ MtpPropertyValue* MtpProperty::readArrayValues(MtpDataPacket& packet, int& lengt
         return NULL;
     }
     MtpPropertyValue* result = new MtpPropertyValue[length];
-    for (int i = 0; i < length; i++)
-        readValue(packet, result[i]);
+    for (uint32_t i = 0; i < length; i++)
+        if (!readValue(packet, result[i])) {
+            delete result;
+            return NULL;
+        }
     return result;
 }
 
-void MtpProperty::writeArrayValues(MtpDataPacket& packet, MtpPropertyValue* values, int length) {
+void MtpProperty::writeArrayValues(MtpDataPacket& packet, MtpPropertyValue* values, uint32_t length) {
     packet.putUInt32(length);
-    for (int i = 0; i < length; i++)
+    for (uint32_t i = 0; i < length; i++)
         writeValue(packet, values[i]);
 }