mirror of
https://github.com/ddnet/ddnet.git
synced 2024-11-14 03:58:18 +00:00
Merge #6068
6068: Fix client crash when unpacking a sixup packet r=def- a=Robyt3 The client crashes when trying to unpack a packet that has the sixup flag set, as `CNetClient` does not pass pointers for the output parameters `pSecurityToken` and `pResponseToken` to `CNetBase::UnpackPacket`. Since the client does not handle sixup packets, checks are added to return an error and ignore the packet instead of crashing due to a null pointer access. This was found by fuzzing the data returned by `net_udp_recv` with radamsa. ``` ==6200==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f8d0fb8ba56 bp 0x7ffcbf67c7f0 sp 0x7ffcbf67c7a8 T0) ==6200==The signal is caused by a WRITE memory access. ==6200==Hint: address points to the zero page. 0 0x7f8d0fb8ba56 (/lib/x86_64-linux-gnu/libc.so.6+0xc4a56) 1 0x563a7e250fbe in mem_copy src/base/system.cpp:208 2 0x563a7e1bc6b6 in CNetBase::UnpackPacket(unsigned char*, int, CNetPacketConstruct*, bool&, int*, int*) src/engine/shared/network.cpp:263 3 0x563a7e1bf57e in CNetClient::Recv(CNetChunk*) src/engine/shared/network_client.cpp:100 4 0x563a7cfa76a2 in CClient::PumpNetwork() src/engine/client/client.cpp:2546 5 0x563a7cfb7cf6 in CClient::Update() src/engine/client/client.cpp:2838 6 0x563a7cfcfe47 in CClient::Run() src/engine/client/client.cpp:3214 7 0x563a7d04c631 in main src/engine/client/client.cpp:4702 8 0x7f8d0faf0d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 9 0x7f8d0faf0e3f in __libc_start_main_impl ../csu/libc-start.c:392 10 0x563a7cb28754 in _start (build-asan/DDNet+0x2472754) ==8315==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f4accfe5a56 bp 0x7ffcf1318530 sp 0x7ffcf13184e8 T0) ==8315==The signal is caused by a WRITE memory access. ==8315==Hint: address points to the zero page. 0 0x7f4accfe5a56 (/lib/x86_64-linux-gnu/libc.so.6+0xc4a56) 1 0x560413603200 in mem_copy src/base/system.cpp:208 2 0x56041356d9c7 in CNetBase::UnpackPacket(unsigned char*, int, CNetPacketConstruct*, bool&, int*, int*) src/engine/shared/network.cpp:224 3 0x5604135717c0 in CNetClient::Recv(CNetChunk*) src/engine/shared/network_client.cpp:104 4 0x5604123597e2 in CClient::PumpNetwork() src/engine/client/client.cpp:2546 5 0x560412369e36 in CClient::Update() src/engine/client/client.cpp:2838 6 0x560412381f87 in CClient::Run() src/engine/client/client.cpp:3214 7 0x5604123fe771 in main src/engine/client/client.cpp:4702 8 0x7f4accf4ad8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 9 0x7f4accf4ae3f in __libc_start_main_impl ../csu/libc-start.c:392 10 0x560411eda894 in _start (build-asan/DDNet+0x2472894) ``` ## Checklist - [X] Tested the change ingame - [ ] Provided screenshots if it is a visual change - [ ] Tested in combination with possibly related configuration options - [ ] Written a unit test (especially base/) or added coverage to integration test - [X] Considered possible null pointers and out of bounds array indexing - [X] Changed no physics that affect existing maps - [X] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional) Co-authored-by: Robert Müller <robytemueller@gmail.com>
This commit is contained in:
commit
913ccb054e
|
@ -215,6 +215,8 @@ int CNetBase::UnpackPacket(unsigned char *pBuffer, int Size, CNetPacketConstruct
|
||||||
if(pPacket->m_Flags & NET_PACKETFLAG_CONNLESS)
|
if(pPacket->m_Flags & NET_PACKETFLAG_CONNLESS)
|
||||||
{
|
{
|
||||||
Sixup = (pBuffer[0] & 0x3) == 1;
|
Sixup = (pBuffer[0] & 0x3) == 1;
|
||||||
|
if(Sixup && (pSecurityToken == nullptr || pResponseToken == nullptr))
|
||||||
|
return -1;
|
||||||
int Offset = Sixup ? 9 : 6;
|
int Offset = Sixup ? 9 : 6;
|
||||||
if(Size < Offset)
|
if(Size < Offset)
|
||||||
return -1;
|
return -1;
|
||||||
|
@ -241,6 +243,8 @@ int CNetBase::UnpackPacket(unsigned char *pBuffer, int Size, CNetPacketConstruct
|
||||||
{
|
{
|
||||||
if(pPacket->m_Flags & NET_PACKETFLAG_UNUSED)
|
if(pPacket->m_Flags & NET_PACKETFLAG_UNUSED)
|
||||||
Sixup = true;
|
Sixup = true;
|
||||||
|
if(Sixup && (pSecurityToken == nullptr || pResponseToken == nullptr))
|
||||||
|
return -1;
|
||||||
int DataStart = Sixup ? 7 : NET_PACKETHEADERSIZE;
|
int DataStart = Sixup ? 7 : NET_PACKETHEADERSIZE;
|
||||||
if(Size < DataStart)
|
if(Size < DataStart)
|
||||||
return -1;
|
return -1;
|
||||||
|
|
Loading…
Reference in a new issue