Page MenuHomeWildfire Games

Fix remote segfault when spamming chat
Needs ReviewPublic

Authored by elexis on Mon, Feb 10, 12:15 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Riddler66 reported in the lobby today that sending an excessively large chat message will trigger the host to crash.

Reproduce by starting a host, pressing F9, typing Engine.SendNetworkChat("h".repeat(100000)).

Terminal output:

ERROR: CNetMessage: Corrupt packet (incorrect size)
ERROR: CNetMessage: Corrupt packet (incorrect size)
Segmentation fault (core dumped)

Stacktrace:

#0  CStr8::Deserialize (this=0x7fff7c548c80, buffer=0x1 <error: Cannot access memory at address 0x1>, bufferend=0x7fff7c57a139 "") at ../../../source/ps/CStr.cpp:512
#1  0x00005555555c4512 in CChatMessage::Deserialize (this=0x7fff7c548c70, pos=0xe2713dd79dd5d200 <error: Cannot access memory at address 0xe2713dd79dd5d200>, end=0x7fff7c57a139 "") at ../../../source/network/NetMessages.h:136
#2  0x00005555555d171e in CNetMessageFactory::CreateMessage (pData=0x7fff7c5493f0, dataSize=<optimized out>, scriptInterface=...) at ../../../source/network/NetMessage.cpp:227
#3  0x00005555555e012d in CNetServerWorker::RunStep (this=<optimized out>) at ../../../source/network/NetServer.cpp:544
#4  0x00005555555df8f8 in CNetServerWorker::Run (this=0x55555827ade0) at ../../../source/network/NetServer.cpp:394
#5  0x00005555555ddf8a in CNetServerWorker::RunThread (data=0x7ffff6329320 <_IO_stdfile_1_lock>) at ../../../source/network/NetServer.cpp:379
#6  0x00005555555e935f in std::__invoke_impl<void, void (*)(CNetServerWorker*), CNetServerWorker*> (__f=<error reading variable>, __args=<error reading variable>)
    at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/invoke.h:60
#7  std::__invoke<void (*)(CNetServerWorker*), CNetServerWorker*> (__fn=<error reading variable>, __args=<error reading variable>) at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/bits/invoke.h:95
#8  std::thread::_Invoker<std::tuple<void (*)(CNetServerWorker*), CNetServerWorker*> >::_M_invoke<0ul, 1ul> (this=0xe2713dd79dd5d208) at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/thread:244
#9  std::thread::_Invoker<std::tuple<void (*)(CNetServerWorker*), CNetServerWorker*> >::operator() (this=0xe2713dd79dd5d208) at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/thread:251
#10 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(CNetServerWorker*), CNetServerWorker*> > >::_M_run (this=0xe2713dd79dd5d200)
    at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../include/c++/9.2.0/thread:195
#11 0x00007ffff657eee4 in std::execute_native_thread_routine (__p=0x555558967fb0) at /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80
#12 0x00007ffff63364cf in start_thread () from /usr/lib/libpthread.so.0
#13 0x00007ffff62652d3 in clone () from /usr/lib/libc.so.6

We notice that a packet size error is detected, but that if such a size error is detected, the return value is not read from.
Therefore after detecting a size mismatch, it continues to deserialize the packet and reads the GUID CStr field instead of abandoning.
Therefore we see that the check for that return value is missing in the Deserialize call of NMTCreator.h.

Test Plan

Reproduce the issue using the given steps, examine the stacktrace, find the location of the missing check, try to find similar places, or replace the entire code if there is a patch for that.

Event Timeline

elexis created this revision.Mon, Feb 10, 12:15 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/network/NMTCreator.h
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2015"

source/network/NMTCreator.h
|  75| »   CStr·ToStringRaw()·const;\
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classISerializable{' is invalid C code. Use --std or --language to configure the language.

source/network/NMTCreator.h
|  24| #include·NMT_CREATE_HEADER_NAME
|    | [MAJOR] CPPCheckBear (preprocessorErrorDirective):
|    | No header in #include

source/ps/CStr.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1742/display/redirect

vladislavbelov added inline comments.
source/network/NMTCreator.h
244

Space before \, nullptr instead of NULL. Why that was NULL instead of a correct value?

elexis added inline comments.Mon, Feb 10, 12:36 AM
source/network/NMTCreator.h
244

See summary, it is NULL because CNetMessage::Deserialize returns NULL "CNetMessage: Corrupt packet (incorrect size)".

Stan added a subscriber: Stan.Mon, Feb 10, 7:55 AM

Can't we have an explicit message about message being too big?

vladislavbelov added inline comments.Mon, Feb 10, 1:05 PM
source/network/NMTCreator.h
244

Yes, but why we continue the deserializing if we know that it's impossible?

In D2629#110106, @Stan wrote:

Can't we have an explicit message about message being too big?

There is "CNetMessage: Corrupt packet (incorrect size)" already.

Also "too big" refers to clientside length, but that may not be a too big in the packet received by the server.
The server only notices that the length doesnt match the packet length.
One may apply clientside fixes too, but the server vulnerability must be fixed first.

source/network/NMTCreator.h
244

It continued to deserialize because the check that this patch added had not been added when the code was written (rP122?).
Why the check had not been added to me looks like an oversight (but I can only speculate unless I was hoping to find a more specific reason in the irclogs. To me it seems out of question that this should abandon deserialization after buffer was set to null already.)

(And CNetMessage::Deserialize only deserializes message type and data length, detects failure, returns null, so it does abandon, it just can't abandon the entire packet deserialization because the entire packet deserialization is coded in NMTCreator.h here)

source/ps/CStr.cpp
511

Could also silently deserialize emptystring.

As for consistency, there are two ENSURE in this file already.

vladislavbelov added inline comments.Mon, Feb 10, 7:44 PM
source/network/NMTCreator.h
244

Ok, that seems more reasonable. So in which place we got pos == nullptr?