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/MtpServer.cpp | 97 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 18 deletions(-) (limited to 'media/mtp/MtpServer.cpp') diff --git a/media/mtp/MtpServer.cpp b/media/mtp/MtpServer.cpp index aa43967..931a09d 100644 --- a/media/mtp/MtpServer.cpp +++ b/media/mtp/MtpServer.cpp @@ -495,6 +495,9 @@ MtpResponseCode MtpServer::doOpenSession() { mResponse.setParameter(1, mSessionID); return MTP_RESPONSE_SESSION_ALREADY_OPEN; } + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; + mSessionID = mRequest.getParameter(1); mSessionOpen = true; @@ -529,6 +532,9 @@ MtpResponseCode MtpServer::doGetStorageInfo() { if (!mSessionOpen) return MTP_RESPONSE_SESSION_NOT_OPEN; + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; + MtpStorageID id = mRequest.getParameter(1); MtpStorage* storage = getStorage(id); if (!storage) @@ -550,6 +556,8 @@ MtpResponseCode MtpServer::doGetStorageInfo() { MtpResponseCode MtpServer::doGetObjectPropsSupported() { if (!mSessionOpen) return MTP_RESPONSE_SESSION_NOT_OPEN; + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectFormat format = mRequest.getParameter(1); MtpObjectPropertyList* properties = mDatabase->getSupportedObjectProperties(format); mData.putAUInt16(properties); @@ -560,6 +568,8 @@ MtpResponseCode MtpServer::doGetObjectPropsSupported() { MtpResponseCode MtpServer::doGetObjectHandles() { if (!mSessionOpen) return MTP_RESPONSE_SESSION_NOT_OPEN; + if (mRequest.getParameterCount() < 3) + return MTP_RESPONSE_INVALID_PARAMETER; MtpStorageID storageID = mRequest.getParameter(1); // 0xFFFFFFFF for all storage MtpObjectFormat format = mRequest.getParameter(2); // 0 for all formats MtpObjectHandle parent = mRequest.getParameter(3); // 0xFFFFFFFF for objects with no parent @@ -577,6 +587,8 @@ MtpResponseCode MtpServer::doGetObjectHandles() { MtpResponseCode MtpServer::doGetNumObjects() { if (!mSessionOpen) return MTP_RESPONSE_SESSION_NOT_OPEN; + if (mRequest.getParameterCount() < 3) + return MTP_RESPONSE_INVALID_PARAMETER; MtpStorageID storageID = mRequest.getParameter(1); // 0xFFFFFFFF for all storage MtpObjectFormat format = mRequest.getParameter(2); // 0 for all formats MtpObjectHandle parent = mRequest.getParameter(3); // 0xFFFFFFFF for objects with no parent @@ -599,6 +611,8 @@ MtpResponseCode MtpServer::doGetObjectReferences() { return MTP_RESPONSE_SESSION_NOT_OPEN; if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); // FIXME - check for invalid object handle @@ -617,9 +631,13 @@ MtpResponseCode MtpServer::doSetObjectReferences() { return MTP_RESPONSE_SESSION_NOT_OPEN; if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpStorageID handle = mRequest.getParameter(1); MtpObjectHandleList* references = mData.getAUInt32(); + if (!references) + return MTP_RESPONSE_INVALID_PARAMETER; MtpResponseCode result = mDatabase->setObjectReferences(handle, references); delete references; return result; @@ -628,6 +646,8 @@ MtpResponseCode MtpServer::doSetObjectReferences() { MtpResponseCode MtpServer::doGetObjectPropValue() { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 2) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); MtpObjectProperty property = mRequest.getParameter(2); ALOGV("GetObjectPropValue %d %s\n", handle, @@ -639,6 +659,8 @@ MtpResponseCode MtpServer::doGetObjectPropValue() { MtpResponseCode MtpServer::doSetObjectPropValue() { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 2) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); MtpObjectProperty property = mRequest.getParameter(2); ALOGV("SetObjectPropValue %d %s\n", handle, @@ -648,6 +670,8 @@ MtpResponseCode MtpServer::doSetObjectPropValue() { } MtpResponseCode MtpServer::doGetDevicePropValue() { + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpDeviceProperty property = mRequest.getParameter(1); ALOGV("GetDevicePropValue %s\n", MtpDebug::getDevicePropCodeName(property)); @@ -656,6 +680,8 @@ MtpResponseCode MtpServer::doGetDevicePropValue() { } MtpResponseCode MtpServer::doSetDevicePropValue() { + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpDeviceProperty property = mRequest.getParameter(1); ALOGV("SetDevicePropValue %s\n", MtpDebug::getDevicePropCodeName(property)); @@ -664,6 +690,8 @@ MtpResponseCode MtpServer::doSetDevicePropValue() { } MtpResponseCode MtpServer::doResetDevicePropValue() { + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpDeviceProperty property = mRequest.getParameter(1); ALOGV("ResetDevicePropValue %s\n", MtpDebug::getDevicePropCodeName(property)); @@ -674,6 +702,8 @@ MtpResponseCode MtpServer::doResetDevicePropValue() { MtpResponseCode MtpServer::doGetObjectPropList() { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 5) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); // use uint32_t so we can support 0xFFFFFFFF @@ -691,6 +721,8 @@ MtpResponseCode MtpServer::doGetObjectPropList() { MtpResponseCode MtpServer::doGetObjectInfo() { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); MtpObjectInfo info(handle); MtpResponseCode result = mDatabase->getObjectInfo(handle, info); @@ -732,6 +764,8 @@ MtpResponseCode MtpServer::doGetObjectInfo() { MtpResponseCode MtpServer::doGetObject() { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); MtpString pathBuf; int64_t fileLength; @@ -765,6 +799,8 @@ MtpResponseCode MtpServer::doGetObject() { } MtpResponseCode MtpServer::doGetThumb() { + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); size_t thumbSize; void* thumb = mDatabase->getThumbnail(handle, thumbSize); @@ -783,6 +819,8 @@ MtpResponseCode MtpServer::doGetThumb() { MtpResponseCode MtpServer::doGetPartialObject(MtpOperationCode operation) { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 4) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); uint64_t offset; uint32_t length; @@ -832,6 +870,11 @@ MtpResponseCode MtpServer::doGetPartialObject(MtpOperationCode operation) { MtpResponseCode MtpServer::doSendObjectInfo() { MtpString path; + uint16_t temp16; + uint32_t temp32; + + if (mRequest.getParameterCount() < 2) + return MTP_RESPONSE_INVALID_PARAMETER; MtpStorageID storageID = mRequest.getParameter(1); MtpStorage* storage = getStorage(storageID); MtpObjectHandle parent = mRequest.getParameter(2); @@ -853,25 +896,29 @@ MtpResponseCode MtpServer::doSendObjectInfo() { } // read only the fields we need - mData.getUInt32(); // storage ID - MtpObjectFormat format = mData.getUInt16(); - mData.getUInt16(); // protection status - mSendObjectFileSize = mData.getUInt32(); - mData.getUInt16(); // thumb format - mData.getUInt32(); // thumb compressed size - mData.getUInt32(); // thumb pix width - mData.getUInt32(); // thumb pix height - mData.getUInt32(); // image pix width - mData.getUInt32(); // image pix height - mData.getUInt32(); // image bit depth - mData.getUInt32(); // parent - uint16_t associationType = mData.getUInt16(); - uint32_t associationDesc = mData.getUInt32(); // association desc - mData.getUInt32(); // sequence number + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // storage ID + if (!mData.getUInt16(temp16)) return MTP_RESPONSE_INVALID_PARAMETER; + MtpObjectFormat format = temp16; + if (!mData.getUInt16(temp16)) return MTP_RESPONSE_INVALID_PARAMETER; // protection status + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; + mSendObjectFileSize = temp32; + if (!mData.getUInt16(temp16)) return MTP_RESPONSE_INVALID_PARAMETER; // thumb format + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // thumb compressed size + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // thumb pix width + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // thumb pix height + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // image pix width + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // image pix height + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // image bit depth + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // parent + if (!mData.getUInt16(temp16)) return MTP_RESPONSE_INVALID_PARAMETER; + uint16_t associationType = temp16; + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; + uint32_t associationDesc = temp32; // association desc + if (!mData.getUInt32(temp32)) return MTP_RESPONSE_INVALID_PARAMETER; // sequence number MtpStringBuffer name, created, modified; - mData.getString(name); // file name - mData.getString(created); // date created - mData.getString(modified); // date modified + if (!mData.getString(name)) return MTP_RESPONSE_INVALID_PARAMETER; // file name + if (!mData.getString(created)) return MTP_RESPONSE_INVALID_PARAMETER; // date created + if (!mData.getString(modified)) return MTP_RESPONSE_INVALID_PARAMETER; // date modified // keywords follow ALOGV("name: %s format: %04X\n", (const char *)name, format); @@ -1066,6 +1113,8 @@ static void deletePath(const char* path) { MtpResponseCode MtpServer::doDeleteObject() { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 2) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); MtpObjectFormat format = mRequest.getParameter(2); // FIXME - support deleting all objects if handle is 0xFFFFFFFF @@ -1087,6 +1136,8 @@ MtpResponseCode MtpServer::doDeleteObject() { } MtpResponseCode MtpServer::doGetObjectPropDesc() { + if (mRequest.getParameterCount() < 2) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectProperty propCode = mRequest.getParameter(1); MtpObjectFormat format = mRequest.getParameter(2); ALOGV("GetObjectPropDesc %s %s\n", MtpDebug::getObjectPropCodeName(propCode), @@ -1100,6 +1151,8 @@ MtpResponseCode MtpServer::doGetObjectPropDesc() { } MtpResponseCode MtpServer::doGetDevicePropDesc() { + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpDeviceProperty propCode = mRequest.getParameter(1); ALOGV("GetDevicePropDesc %s\n", MtpDebug::getDevicePropCodeName(propCode)); MtpProperty* property = mDatabase->getDevicePropertyDesc(propCode); @@ -1113,6 +1166,8 @@ MtpResponseCode MtpServer::doGetDevicePropDesc() { MtpResponseCode MtpServer::doSendPartialObject() { if (!hasStorage()) return MTP_RESPONSE_INVALID_OBJECT_HANDLE; + if (mRequest.getParameterCount() < 4) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); uint64_t offset = mRequest.getParameter(2); uint64_t offset2 = mRequest.getParameter(3); @@ -1180,6 +1235,8 @@ MtpResponseCode MtpServer::doSendPartialObject() { } MtpResponseCode MtpServer::doTruncateObject() { + if (mRequest.getParameterCount() < 3) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); ObjectEdit* edit = getEditObject(handle); if (!edit) { @@ -1199,6 +1256,8 @@ MtpResponseCode MtpServer::doTruncateObject() { } MtpResponseCode MtpServer::doBeginEditObject() { + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); if (getEditObject(handle)) { ALOGE("object already open for edit in doBeginEditObject"); @@ -1223,6 +1282,8 @@ MtpResponseCode MtpServer::doBeginEditObject() { } MtpResponseCode MtpServer::doEndEditObject() { + if (mRequest.getParameterCount() < 1) + return MTP_RESPONSE_INVALID_PARAMETER; MtpObjectHandle handle = mRequest.getParameter(1); ObjectEdit* edit = getEditObject(handle); if (!edit) { -- cgit v1.1