Page MenuHomeWildfire Games

Fix remote segfault when spamming chat
ClosedPublic

Authored by elexis on Feb 10 2020, 12:15 AM.

Details

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.Feb 10 2020, 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
245

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

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

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

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

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

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

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
245

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
513

Could also silently deserialize emptystring.

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

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

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

elexis updated the Trac tickets for this revision.Apr 22 2020, 1:44 AM
wraitii requested changes to this revision.May 18 2020, 4:00 PM
wraitii added a subscriber: wraitii.

Thanks for debugging this and thanks for the patch.

I think this can be improved slightly before committing, see inlines.

source/network/NMTCreator.h
223–224

I would suggest putting the if null check here.

245

I would move the check above (cf other inline), it looks cleaner (and would be slightly more performant)

source/ps/CStr.cpp
513

Either remove this or also add it to CStrW imo.

This revision now requires changes to proceed.May 18 2020, 4:00 PM
wraitii updated this revision to Diff 12990.Aug 1 2020, 2:28 PM

Apply my inline comments.

Vulcan added a comment.Aug 1 2020, 2:35 PM

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/2828/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.Aug 1 2020, 5:25 PM
This revision was automatically updated to reflect the committed changes.