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
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 @@
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 @@
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 @@
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::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 @@
}
}
-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 @@
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]);
}