bugfix(network): Fix out of bounds memory access for wrapped net commands #1946
Conversation
| UnsignedShort m_commandID; | ||
| UnsignedByte *m_data; | ||
| UnsignedInt m_dataLength; | ||
| UnsignedInt m_totalDataLength; |
There was a problem hiding this comment.
I altered this for sanity reasons as the original name can get confusing when the data length of the data chunk is also being considered.
I did consider further refactoring the NetWrapperCommandMsg but chose to revert those changes for now as they were touching NetPacket which has some waiting PR's to be merged
| m_dataLength = msg->getTotalDataLength(); | ||
| m_data = NEW UnsignedByte[m_dataLength]; // pool[]ify | ||
| m_totalDataLength = msg->getTotalDataLength(); | ||
| m_data = NEW UnsignedByte[m_totalDataLength]; // pool[]ify |
There was a problem hiding this comment.
I was considering if we might want to also validate the total data length, since we can't allocate MAX_UINT of memory for example in a 32bit application.
If there is not a limit considered in the game already, then we should consider something sensible for a single transfer.
There was a problem hiding this comment.
Having a transfer size cap does make sense.
There was a problem hiding this comment.
I guess it's a matter of determining what we would consider our max, the game is still not large address aware so limited to 2/2.5GB of RAM at the moment.
There was a problem hiding this comment.
10 MB would be a reasonable limit for starters I think. But better be a separate change.
There was a problem hiding this comment.
Indeed, leave it for a followup.
Mauller
force-pushed
the
Mauller/fix-filewrapperlist-security
branch
2 times, most recently
from
3d36e1d to
2fe28a3
Compare
|
One thing this transfer mechanism lacks is a way to signal up that there was a problem with the transfer and that it needs to be cancelled. I think the game has a timeout on the transfer screen of 5 minutes but is always good to be able to signal a problem. But a refactor for that would be more considerable and better done when we do some bigger refactoring of netcode. |
Mauller
force-pushed
the
Mauller/fix-filewrapperlist-security
branch
from
2fe28a3 to
1e5ab7f
Compare
|
Tweaked based on feedback. |
Mauller
force-pushed
the
Mauller/fix-filewrapperlist-security
branch
from
1e5ab7f to
17c275e
Compare
Mauller
force-pushed
the
Mauller/fix-filewrapperlist-security
branch
from
17c275e to
c7fc2bd
Compare
|
Tweaked |
|
I don't think this needs replicating since the code is already in core, so should be good to go. |
3 participants
This PR prevents the ability to perform an out of bounds memory access using a well crafted wrapped command.
The Wrapped commands are designed to chunk up a larger data packet, so files can be transfered to another machine such as a map.
This PR adds checks to make sure the chunked data makes sense and fits within the bounds of the allocated memory.