Validate snapshot size and member variables and demo snapshots

Add `CSnapshot::IsValid` to check if a snapshot unpacked from a snapshot delta or demo is valid:
- ensure number of items and data size are not negative
- ensure that the actual size of the snapshot matches the size derived from its member variables
- ensure item offsets are within the valid range
- ensure item sizes are not negative

Add `CSnapshot::TotalSize` and `CSnapshot::OffsetSize` utility functions.

Minor improvements to related error messages.

Fixes buffer overflow:

```
==47744==ERROR: AddressSanitizer: global-buffer-overflow on address 0x558618e3767f at pc 0x558614b9bdfb bp 0x7ffe58a32cd0 sp 0x7ffe58a32cc0
READ of size 4 at 0x558618e3767f thread T0
    0x558614b9bdfa in CSnapshotItem::Type() const src/engine/shared/snapshot.h:16
    0x558615c3c911 in CSnapshot::GetItemType(int) const src/engine/shared/snapshot.cpp:29
    0x558614aebaba in CClient::UnpackAndValidateSnapshot(CSnapshot*, CSnapshot*) src/engine/client/client.cpp:2264
    0x558614af87cb in CClient::OnDemoPlayerSnapshot(void*, int) src/engine/client/client.cpp:2598
    0x558615b9db1a in CDemoPlayer::DoTick() src/engine/shared/demo.cpp:659
    0x558615babd3f in CDemoPlayer::Update(bool) src/engine/shared/demo.cpp:1007
    0x558614afb08b in CClient::Update() src/engine/client/client.cpp:2686
    0x558614b1d9eb in CClient::Run() src/engine/client/client.cpp:3296
    0x558614b8e64f in main src/engine/client/client.cpp:4761
```

And fixes a buffer overflow that manifests itself as an internal ASan error:

```
=================================================================
==4755==AddressSanitizer CHECK failed: ../../../../src/libsanitizer/asan/asan_descriptions.cc:79 "((0 && "Address is not in memory and not in shadow?")) != (0)" (0x0, 0x0)
    0x7f0bf5f368be in AsanCheckFailed ../../../../src/libsanitizer/asan/asan_rtl.cc:72
    0x7f0bf5f54eee in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) ../../../../src/libsanitizer/sanitizer_common/sanitizer_termination.cc:77
    0x7f0bf5e4cb6f in GetShadowKind ../../../../src/libsanitizer/asan/asan_descriptions.cc:79
    0x7f0bf5e4cb6f in __asan::GetShadowAddressInformation(unsigned long, __asan::ShadowAddressDescription*) ../../../../src/libsanitizer/asan/asan_descriptions.cc:95
    0x7f0bf5e4cb6f in __asan::GetShadowAddressInformation(unsigned long, __asan::ShadowAddressDescription*) ../../../../src/libsanitizer/asan/asan_descriptions.cc:92
    0x7f0bf5e4e386 in __asan::AddressDescription::AddressDescription(unsigned long, unsigned long, bool) ../../../../src/libsanitizer/asan/asan_descriptions.cc:440
    0x7f0bf5e50e94 in __asan::ErrorGeneric::ErrorGeneric(unsigned int, unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long) ../../../../src/libsanitizer/asan/asan_errors.cc:380
    0x7f0bf5f35f4d in __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) ../../../../src/libsanitizer/asan/asan_report.cc:460
    0x7f0bf5e86f5e in __interceptor_memset ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:762
    0x558234873f1d in mem_zero src/base/system.cpp:213
    0x55823481fc27 in CSnapshotBuilder::NewItem(int, int, int) src/engine/shared/snapshot.cpp:675
    0x55823481be65 in CSnapshotDelta::UnpackDelta(CSnapshot*, CSnapshot*, void const*, int) src/engine/shared/snapshot.cpp:380
    0x558234776641 in CDemoPlayer::DoTick() src/engine/shared/demo.cpp:631
    0x5582347861a9 in CDemoPlayer::Update(bool) src/engine/shared/demo.cpp:1007
    0x5582336d4c7d in CClient::Update() src/engine/client/client.cpp:2695
    0x5582336f75dd in CClient::Run() src/engine/client/client.cpp:3305
    0x558233768241 in main src/engine/client/client.cpp:4770
```
This commit is contained in:
Robert Müller 2022-07-23 00:01:46 +02:00
parent 05dae670f1
commit 3863d41623
4 changed files with 76 additions and 29 deletions

View file

@ -2053,7 +2053,12 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy)
const int SnapSize = m_SnapshotDelta.UnpackDelta(pDeltaShot, pTmpBuffer3, pDeltaData, DeltaSize);
if(SnapSize < 0)
{
dbg_msg("client", "delta unpack failed!=%d", SnapSize);
dbg_msg("client", "delta unpack failed. error=%d", SnapSize);
return;
}
if(!pTmpBuffer3->IsValid(SnapSize))
{
dbg_msg("client", "snapshot invalid. SnapSize=%d, DeltaSize=%d", SnapSize, DeltaSize);
return;
}
@ -2097,7 +2102,7 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy)
const int AltSnapSize = UnpackAndValidateSnapshot(pTmpBuffer3, pAltSnapBuffer);
if(AltSnapSize < 0)
{
dbg_msg("client", "unpack snapshot and validate failed!=%d", AltSnapSize);
dbg_msg("client", "unpack snapshot and validate failed. error=%d", AltSnapSize);
return;
}
@ -2581,26 +2586,22 @@ void CClient::OnDemoPlayerSnapshot(void *pData, int Size)
{
// update ticks, they could have changed
const CDemoPlayer::CPlaybackInfo *pInfo = m_DemoPlayer.Info();
CSnapshotStorage::CHolder *pTemp;
m_aCurGameTick[g_Config.m_ClDummy] = pInfo->m_Info.m_CurrentTick;
m_aPrevGameTick[g_Config.m_ClDummy] = pInfo->m_PreviousTick;
// handle snapshots
pTemp = m_aapSnapshots[g_Config.m_ClDummy][SNAP_PREV];
m_aapSnapshots[g_Config.m_ClDummy][SNAP_PREV] = m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT];
m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT] = pTemp;
mem_copy(m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]->m_pSnap, pData, Size);
// create a verified and unpacked snapshot
unsigned char aAltSnapBuffer[CSnapshot::MAX_SIZE];
CSnapshot *pAltSnapBuffer = (CSnapshot *)aAltSnapBuffer;
const int AltSnapSize = UnpackAndValidateSnapshot((CSnapshot *)pData, pAltSnapBuffer);
if(AltSnapSize < 0)
{
dbg_msg("client", "unpack snapshot and validate failed!=%d", AltSnapSize);
dbg_msg("client", "unpack snapshot and validate failed. error=%d", AltSnapSize);
return;
}
// handle snapshots after validation
std::swap(m_aapSnapshots[g_Config.m_ClDummy][SNAP_PREV], m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]);
mem_copy(m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]->m_pSnap, pData, Size);
mem_copy(m_aapSnapshots[g_Config.m_ClDummy][SNAP_CURRENT]->m_pAltSnap, pAltSnapBuffer, AltSnapSize);
GameClient()->OnNewSnapshot();

View file

@ -614,20 +614,10 @@ void CDemoPlayer::DoTick()
{
// process delta snapshot
static char s_aNewsnap[CSnapshot::MAX_SIZE];
CSnapshot *pNewsnap = (CSnapshot *)s_aNewsnap;
DataSize = m_pSnapshotDelta->UnpackDelta((CSnapshot *)m_aLastSnapshotData, pNewsnap, s_aData, DataSize);
GotSnapshot = 1;
DataSize = m_pSnapshotDelta->UnpackDelta((CSnapshot *)m_aLastSnapshotData, (CSnapshot *)s_aNewsnap, s_aData, DataSize);
if(DataSize >= 0)
{
if(m_pListener)
m_pListener->OnDemoPlayerSnapshot(s_aNewsnap, DataSize);
m_LastSnapshotDataSize = DataSize;
mem_copy(m_aLastSnapshotData, s_aNewsnap, DataSize);
}
else
if(DataSize < 0)
{
if(m_pConsole)
{
@ -636,16 +626,47 @@ void CDemoPlayer::DoTick()
m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "demo_player", aBuf);
}
}
else if(!pNewsnap->IsValid(DataSize))
{
if(m_pConsole)
{
char aBuf[256];
str_format(aBuf, sizeof(aBuf), "snapshot delta invalid. DataSize=%d", DataSize);
m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "demo_player", aBuf);
}
}
else
{
if(m_pListener)
m_pListener->OnDemoPlayerSnapshot(s_aNewsnap, DataSize);
m_LastSnapshotDataSize = DataSize;
mem_copy(m_aLastSnapshotData, s_aNewsnap, DataSize);
GotSnapshot = 1;
}
}
else if(ChunkType == CHUNKTYPE_SNAPSHOT)
{
// process full snapshot
GotSnapshot = 1;
CSnapshot *pSnap = (CSnapshot *)s_aData;
if(!pSnap->IsValid(DataSize))
{
if(m_pConsole)
{
char aBuf[256];
str_format(aBuf, sizeof(aBuf), "snapshot invalid. DataSize=%d", DataSize);
m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "demo_player", aBuf);
}
}
else
{
GotSnapshot = 1;
m_LastSnapshotDataSize = DataSize;
mem_copy(m_aLastSnapshotData, s_aData, DataSize);
if(m_pListener)
m_pListener->OnDemoPlayerSnapshot(s_aData, DataSize);
m_LastSnapshotDataSize = DataSize;
mem_copy(m_aLastSnapshotData, s_aData, DataSize);
if(m_pListener)
m_pListener->OnDemoPlayerSnapshot(s_aData, DataSize);
}
}
else
{

View file

@ -122,6 +122,26 @@ void CSnapshot::DebugDump()
}
}
bool CSnapshot::IsValid(size_t ActualSize) const
{
// validate total size
if(ActualSize < sizeof(CSnapshot) || m_NumItems < 0 || m_DataSize < 0 || ActualSize != TotalSize())
return false;
// validate item offsets
const int *pOffsets = Offsets();
for(int Index = 0; Index < m_NumItems; Index++)
if(pOffsets[Index] < 0 || pOffsets[Index] > m_DataSize)
return false;
// validate item sizes
for(int Index = 0; Index < m_NumItems; Index++)
if(GetItemSize(Index) < 0) // the offsets must be validated before using this
return false;
return true;
}
// CSnapshotDelta
struct CItemList

View file

@ -3,6 +3,7 @@
#ifndef ENGINE_SHARED_SNAPSHOT_H
#define ENGINE_SHARED_SNAPSHOT_H
#include <cstddef>
#include <stdint.h>
// CSnapshot
@ -27,6 +28,9 @@ class CSnapshot
int *Offsets() const { return (int *)(this + 1); }
char *DataStart() const { return (char *)(Offsets() + m_NumItems); }
size_t OffsetSize() const { return sizeof(int) * m_NumItems; }
size_t TotalSize() const { return sizeof(CSnapshot) + OffsetSize() + m_DataSize; }
public:
enum
{
@ -53,6 +57,7 @@ public:
unsigned Crc();
void DebugDump();
bool IsValid(size_t ActualSize) const;
};
// CSnapshotDelta