[PATCH 2/5] ole32: Check buffer bounds when reading storage properties.
Nikolay Sivov
nsivov at codeweavers.com
Mon Jan 27 00:24:17 CST 2020
Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
---
dlls/ole32/stg_prop.c | 194 +++++++++++++++++++++++++++++++-----------
1 file changed, 144 insertions(+), 50 deletions(-)
diff --git a/dlls/ole32/stg_prop.c b/dlls/ole32/stg_prop.c
index 54904a581b..00ba5ade29 100644
--- a/dlls/ole32/stg_prop.c
+++ b/dlls/ole32/stg_prop.c
@@ -1229,19 +1229,80 @@ static void* WINAPI Allocate_CoTaskMemAlloc(void *this, ULONG size)
return CoTaskMemAlloc(size);
}
-/* FIXME: there isn't any checking whether the read property extends past the
- * end of the buffer.
- */
-static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
- UINT codepage, void* (WINAPI *allocate)(void *this, ULONG size), void *allocate_data)
+struct read_buffer
{
- HRESULT hr = S_OK;
+ BYTE *data;
+ size_t size;
+};
+
+static HRESULT buffer_test_offset(const struct read_buffer *buffer, size_t offset, size_t len)
+{
+ return len > buffer->size || offset > buffer->size - len ? STG_E_READFAULT : S_OK;
+}
+
+static HRESULT buffer_read_uint64(const struct read_buffer *buffer, size_t offset, ULARGE_INTEGER *data)
+{
+ HRESULT hr;
+
+ if (SUCCEEDED(hr = buffer_test_offset(buffer, offset, sizeof(*data))))
+ StorageUtl_ReadULargeInteger(buffer->data, offset, data);
+
+ return hr;
+}
+
+static HRESULT buffer_read_dword(const struct read_buffer *buffer, size_t offset, DWORD *data)
+{
+ HRESULT hr;
+
+ if (SUCCEEDED(hr = buffer_test_offset(buffer, offset, sizeof(*data))))
+ StorageUtl_ReadDWord(buffer->data, offset, data);
+
+ return hr;
+}
+
+static HRESULT buffer_read_word(const struct read_buffer *buffer, size_t offset, WORD *data)
+{
+ HRESULT hr;
+
+ if (SUCCEEDED(hr = buffer_test_offset(buffer, offset, sizeof(*data))))
+ StorageUtl_ReadWord(buffer->data, offset, data);
+
+ return hr;
+}
+
+static HRESULT buffer_read_byte(const struct read_buffer *buffer, size_t offset, BYTE *data)
+{
+ HRESULT hr;
+
+ if (SUCCEEDED(hr = buffer_test_offset(buffer, offset, sizeof(*data))))
+ *data = *(buffer->data + offset);
+
+ return hr;
+}
+
+static HRESULT buffer_read_len(const struct read_buffer *buffer, size_t offset, void *dest, size_t len)
+{
+ HRESULT hr;
+
+ if (SUCCEEDED(hr = buffer_test_offset(buffer, offset, len)))
+ memcpy(dest, buffer->data + offset, len);
+
+ return hr;
+}
+
+static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const struct read_buffer *buffer,
+ size_t offset, UINT codepage, void* (WINAPI *allocate)(void *this, ULONG size), void *allocate_data)
+{
+ HRESULT hr;
DWORD vt;
assert(prop);
- assert(data);
- StorageUtl_ReadDWord(data, 0, &vt);
- data += sizeof(DWORD);
+ assert(buffer->data);
+
+ if (FAILED(hr = buffer_read_dword(buffer, offset, &vt)))
+ return hr;
+
+ offset += sizeof(DWORD);
prop->vt = vt;
switch (prop->vt)
{
@@ -1249,53 +1310,57 @@ static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
case VT_NULL:
break;
case VT_I1:
- prop->u.cVal = *(const char *)data;
+ hr = buffer_read_byte(buffer, offset, (BYTE *)&prop->u.cVal);
TRACE("Read char 0x%x ('%c')\n", prop->u.cVal, prop->u.cVal);
break;
case VT_UI1:
- prop->u.bVal = *data;
+ hr = buffer_read_byte(buffer, offset, &prop->u.bVal);
TRACE("Read byte 0x%x\n", prop->u.bVal);
break;
case VT_BOOL:
- StorageUtl_ReadWord(data, 0, (WORD*)&prop->u.boolVal);
+ hr = buffer_read_word(buffer, offset, (WORD *)&prop->u.boolVal);
TRACE("Read bool %d\n", prop->u.boolVal);
break;
case VT_I2:
- StorageUtl_ReadWord(data, 0, (WORD*)&prop->u.iVal);
+ hr = buffer_read_word(buffer, offset, (WORD *)&prop->u.iVal);
TRACE("Read short %d\n", prop->u.iVal);
break;
case VT_UI2:
- StorageUtl_ReadWord(data, 0, &prop->u.uiVal);
+ hr = buffer_read_word(buffer, offset, &prop->u.uiVal);
TRACE("Read ushort %d\n", prop->u.uiVal);
break;
case VT_INT:
case VT_I4:
- StorageUtl_ReadDWord(data, 0, (DWORD*)&prop->u.lVal);
+ hr = buffer_read_dword(buffer, offset, (DWORD *)&prop->u.lVal);
TRACE("Read long %d\n", prop->u.lVal);
break;
case VT_UINT:
case VT_UI4:
- StorageUtl_ReadDWord(data, 0, &prop->u.ulVal);
+ hr = buffer_read_dword(buffer, offset, &prop->u.ulVal);
TRACE("Read ulong %d\n", prop->u.ulVal);
break;
case VT_I8:
- StorageUtl_ReadULargeInteger(data, 0, (ULARGE_INTEGER *)&prop->u.hVal);
+ hr = buffer_read_uint64(buffer, offset, (ULARGE_INTEGER *)&prop->u.hVal);
TRACE("Read long long %s\n", wine_dbgstr_longlong(prop->u.hVal.QuadPart));
break;
case VT_UI8:
- StorageUtl_ReadULargeInteger(data, 0, &prop->u.uhVal);
+ hr = buffer_read_uint64(buffer, offset, &prop->u.uhVal);
TRACE("Read ulong long %s\n", wine_dbgstr_longlong(prop->u.uhVal.QuadPart));
break;
case VT_R8:
- memcpy(&prop->u.dblVal, data, sizeof(double));
+ hr = buffer_read_len(buffer, offset, &prop->u.dblVal, sizeof(prop->u.dblVal));
TRACE("Read double %f\n", prop->u.dblVal);
break;
case VT_LPSTR:
{
DWORD count;
-
- StorageUtl_ReadDWord(data, 0, &count);
- if (codepage == CP_UNICODE && count % 2)
+
+ if (FAILED(hr = buffer_read_dword(buffer, offset, &count)))
+ break;
+
+ offset += sizeof(DWORD);
+
+ if (codepage == CP_UNICODE && count % sizeof(WCHAR))
{
WARN("Unicode string has odd number of bytes\n");
hr = STG_E_INVALIDHEADER;
@@ -1305,7 +1370,9 @@ static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
prop->u.pszVal = allocate(allocate_data, count);
if (prop->u.pszVal)
{
- memcpy(prop->u.pszVal, data + sizeof(DWORD), count);
+ if (FAILED(hr = buffer_read_len(buffer, offset, prop->u.pszVal, count)))
+ break;
+
/* This is stored in the code page specified in codepage.
* Don't convert it, the caller will just store it as-is.
*/
@@ -1332,8 +1399,12 @@ static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
{
DWORD count, wcount;
- StorageUtl_ReadDWord(data, 0, &count);
- if (codepage == CP_UNICODE && count % 2)
+ if (FAILED(hr = buffer_read_dword(buffer, offset, &count)))
+ break;
+
+ offset += sizeof(DWORD);
+
+ if (codepage == CP_UNICODE && count % sizeof(WCHAR))
{
WARN("Unicode string has odd number of bytes\n");
hr = STG_E_INVALIDHEADER;
@@ -1341,18 +1412,22 @@ static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
else
{
if (codepage == CP_UNICODE)
- wcount = count / 2;
+ wcount = count / sizeof(WCHAR);
else
- wcount = MultiByteToWideChar(codepage, 0, (LPCSTR)(data + sizeof(DWORD)), count, NULL, 0);
+ {
+ if (FAILED(hr = buffer_test_offset(buffer, offset, count)))
+ break;
+ wcount = MultiByteToWideChar(codepage, 0, (LPCSTR)(buffer->data + offset), count, NULL, 0);
+ }
prop->u.bstrVal = SysAllocStringLen(NULL, wcount); /* FIXME: use allocator? */
if (prop->u.bstrVal)
{
if (codepage == CP_UNICODE)
- memcpy(prop->u.bstrVal, data + sizeof(DWORD), count);
+ hr = buffer_read_len(buffer, offset, prop->u.bstrVal, count);
else
- MultiByteToWideChar(codepage, 0, (LPCSTR)(data + sizeof(DWORD)), count, prop->u.bstrVal, wcount);
+ MultiByteToWideChar(codepage, 0, (LPCSTR)(buffer->data + offset), count, prop->u.bstrVal, wcount);
prop->u.bstrVal[wcount - 1] = '\0';
TRACE("Read string value %s\n", debugstr_w(prop->u.bstrVal));
@@ -1366,12 +1441,16 @@ static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
{
DWORD count;
- StorageUtl_ReadDWord(data, 0, &count);
+ if (FAILED(hr = buffer_read_dword(buffer, offset, &count)))
+ break;
+
+ offset += sizeof(DWORD);
+
prop->u.blob.cbSize = count;
prop->u.blob.pBlobData = allocate(allocate_data, count);
if (prop->u.blob.pBlobData)
{
- memcpy(prop->u.blob.pBlobData, data + sizeof(DWORD), count);
+ hr = buffer_read_len(buffer, offset, prop->u.blob.pBlobData, count);
TRACE("Read blob value of size %d\n", count);
}
else
@@ -1382,31 +1461,40 @@ static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
{
DWORD count;
- StorageUtl_ReadDWord(data, 0, &count);
+ if (FAILED(hr = buffer_read_dword(buffer, offset, &count)))
+ break;
+
+ offset += sizeof(DWORD);
+
prop->u.pwszVal = allocate(allocate_data, count * sizeof(WCHAR));
if (prop->u.pwszVal)
{
- memcpy(prop->u.pwszVal, data + sizeof(DWORD),
- count * sizeof(WCHAR));
- /* make sure string is NULL-terminated */
- prop->u.pwszVal[count - 1] = '\0';
- PropertyStorage_ByteSwapString(prop->u.pwszVal, count);
- TRACE("Read string value %s\n", debugstr_w(prop->u.pwszVal));
+ if (SUCCEEDED(hr = buffer_read_len(buffer, offset, prop->u.pwszVal, count * sizeof(WCHAR))))
+ {
+ /* make sure string is NULL-terminated */
+ prop->u.pwszVal[count - 1] = '\0';
+ PropertyStorage_ByteSwapString(prop->u.pwszVal, count);
+ TRACE("Read string value %s\n", debugstr_w(prop->u.pwszVal));
+ }
}
else
hr = STG_E_INSUFFICIENTMEMORY;
break;
}
case VT_FILETIME:
- StorageUtl_ReadULargeInteger(data, 0,
- (ULARGE_INTEGER *)&prop->u.filetime);
+ hr = buffer_read_uint64(buffer, offset, (ULARGE_INTEGER *)&prop->u.filetime);
break;
case VT_CF:
{
DWORD len = 0, tag = 0;
- StorageUtl_ReadDWord(data, 0, &len);
- StorageUtl_ReadDWord(data, 4, &tag);
+ if (SUCCEEDED(hr = buffer_read_dword(buffer, offset, &len)))
+ hr = buffer_read_dword(buffer, offset + sizeof(DWORD), &tag);
+ if (FAILED(hr))
+ break;
+
+ offset += 2 * sizeof(DWORD);
+
if (len > 8)
{
len -= 8;
@@ -1414,7 +1502,7 @@ static HRESULT PropertyStorage_ReadProperty(PROPVARIANT *prop, const BYTE *data,
prop->u.pclipdata->cbSize = len;
prop->u.pclipdata->ulClipFmt = tag;
prop->u.pclipdata->pClipData = allocate(allocate_data, len - sizeof(prop->u.pclipdata->ulClipFmt));
- memcpy(prop->u.pclipdata->pClipData, data+8, len - sizeof(prop->u.pclipdata->ulClipFmt));
+ hr = buffer_read_len(buffer, offset, prop->u.pclipdata->pClipData, len - sizeof(prop->u.pclipdata->ulClipFmt));
}
else
hr = STG_E_INVALIDPARAMETER;
@@ -1522,6 +1610,7 @@ static HRESULT PropertyStorage_ReadSectionHeaderFromStream(IStream *stm,
static HRESULT PropertyStorage_ReadFromStream(PropertyStorage_impl *This)
{
+ struct read_buffer read_buffer;
PROPERTYSETHEADER hdr;
FORMATIDOFFSET fmtOffset;
PROPERTYSECTIONHEADER sectionHdr;
@@ -1617,14 +1706,16 @@ static HRESULT PropertyStorage_ReadFromStream(PropertyStorage_impl *This)
hr = STG_E_INSUFFICIENTMEMORY;
goto end;
}
- hr = IStream_Read(This->stm, buf, sectionHdr.cbSection -
- sizeof(PROPERTYSECTIONHEADER), &count);
+ read_buffer.data = buf;
+ read_buffer.size = sectionHdr.cbSection - sizeof(sectionHdr);
+
+ hr = IStream_Read(This->stm, read_buffer.data, read_buffer.size, &count);
if (FAILED(hr))
goto end;
TRACE("Reading %d properties:\n", sectionHdr.cProperties);
for (i = 0; SUCCEEDED(hr) && i < sectionHdr.cProperties; i++)
{
- PROPERTYIDOFFSET *idOffset = (PROPERTYIDOFFSET *)(buf +
+ PROPERTYIDOFFSET *idOffset = (PROPERTYIDOFFSET *)(read_buffer.data +
i * sizeof(PROPERTYIDOFFSET));
if (idOffset->dwOffset < sizeof(PROPERTYSECTIONHEADER) ||
@@ -1650,9 +1741,9 @@ static HRESULT PropertyStorage_ReadFromStream(PropertyStorage_impl *This)
PROPVARIANT prop;
PropVariantInit(&prop);
- if (SUCCEEDED(PropertyStorage_ReadProperty(&prop,
- buf + idOffset->dwOffset - sizeof(PROPERTYSECTIONHEADER),
- This->codePage, Allocate_CoTaskMemAlloc, NULL)))
+ if (SUCCEEDED(PropertyStorage_ReadProperty(&prop, &read_buffer,
+ idOffset->dwOffset - sizeof(PROPERTYSECTIONHEADER), This->codePage,
+ Allocate_CoTaskMemAlloc, NULL)))
{
TRACE("Read property with ID 0x%08x, type %d\n",
idOffset->propid, prop.vt);
@@ -2960,9 +3051,12 @@ static void* WINAPI Allocate_PMemoryAllocator(void *this, ULONG cbSize)
BOOLEAN WINAPI StgConvertPropertyToVariant(const SERIALIZEDPROPERTYVALUE* prop,
USHORT CodePage, PROPVARIANT* pvar, void* pma)
{
+ struct read_buffer read_buffer;
HRESULT hr;
- hr = PropertyStorage_ReadProperty(pvar, (const BYTE*)prop, CodePage, Allocate_PMemoryAllocator, pma);
+ read_buffer.data = (BYTE *)prop;
+ read_buffer.size = ~(size_t)0;
+ hr = PropertyStorage_ReadProperty(pvar, &read_buffer, 0, CodePage, Allocate_PMemoryAllocator, pma);
if (FAILED(hr))
{
--
2.24.1
More information about the wine-devel
mailing list