From ab063847e6e893740749029a04cce1f6b7345ed5 Mon Sep 17 00:00:00 2001 From: Mike Lockwood Date: Wed, 12 Nov 2014 14:20:06 -0800 Subject: 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 --- media/mtp/MtpProperty.cpp | 93 ++++++++++++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 38 deletions(-) (limited to 'media/mtp/MtpProperty.cpp') 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]); } -- cgit v1.1