fix(network): Prevent buffer overflows for incoming messages in NetPacket

[jbremer]

TODO Fix numerous out of bound reads which may lead to crashes.

[xezon]

I looked at code and I think we can fix and simplify these functions like so:

NetCommandMsg * NetPacket::readFileMessage(UnsignedByte *data, Int &i) {
	NetFileCommandMsg *msg = newInstance(NetFileCommandMsg);

	// file name
	AsciiString filename(reinterpret_cast<const char*>(data)); // data is expected to be zero terminated
	msg->setPortableFilename(filename);	// it's transferred as a portable filename
	i += filename.getByteCount();

	// file size
	UnsignedInt dataLength = 0;
	memcpy(&dataLength, data + i, sizeof(dataLength));
	i += sizeof(dataLength);

	// file data
	UnsignedByte *buf = NEW UnsignedByte[dataLength];
	memcpy(buf, data + i, dataLength);
	i += dataLength;

	msg->setFileData(buf, dataLength);

	return msg;
}

Alternatively I was thinking doing

...
	// file name
	const size_t filenameLength = std::min<size_t>(MAX_PATH, strlen(reinterpret_cast<const char*>(data)));
	AsciiString filename(reinterpret_cast<const char*>(data), filenameLength);
	msg->setPortableFilename(filename);	// it's transferred as a portable filename
	i += filename.getByteCount();
...

but this would perhaps not help against cases where a malicious sender would send a short message that is not 0 terminated. So we do not need this. I think all messages need to be zero terminated before processing to be safe.

[jbremer]

Hi, as you noted yourself this would not prevent out of bounds read (when the packet is not null-terminated) and could therefore still result in a crash, albeit a read access violation rather than a stack buffer overflow. Your code could work if you were to use strnlen(data, MAX_PATH) as opposed to a straight strlen() or AsciiString filename(...);.
Perhaps a cleaner solution than my PR, using the original logic, is replacing the loop with something like strncpy(filename, data, _MAX_PATH-1). Looking at the man page for strncpy it seems like you'll still have to manually terminate that string to be safe, i.e., filename[_MAX_PATH-1] = 0;. None of this is great, but an improvement nonetheless.

[xezon]

There are a few messages that expect null terminated strings, so I think we need to always null terminate incoming packet data, to guarantee that no malicious sender can send non 0 terminated data and cause us some mayhem.

So perhaps this is good:

NetPacket::NetPacket(TransportMessage *msg) {
	init();

	m_packetLen = std::min<Int>(msg->length, MAX_PACKET_SIZE);
	memcpy(m_packet, msg->data, m_packetLen);
	memset(m_packet + m_packetLen, 0, ARRAY_SIZE(m_packet) - m_packetLen);

	m_numCommands = -1;
	m_addr = msg->addr;
	m_port = msg->port;
}

Not sure if it is worth it to memset the whole rest to 0. I think it is zero anyway because of the Game Memory Allocator.

I checked msg->length and its theoretical max size is 1024, according to function Transport::isGeneralsPacket.

[jbremer]

In theory that's fine. Not sure if you care about "zero-copy" but this would add a copy (I assume). Additionally I'd note, just in case, that static const Int MAX_PACKET_SIZE = 476; and as such the buffer overflow would still be present with this null termination in place. (Note that you could both have this null termination logic and strncpy() still returning a string that is not zero-terminated because it's too long, e.g., "A"*280. Not saying you're going to use strnlen/strncpy, just stating it for consideration).

[xezon]

I forgot to mention, m_packet[MAX_PACKET_SIZE] can then become m_packet[MAX_PACKET_SIZE+1]. Do you see any issue with that?

[jbremer]

It would resolve out of bounds access ascii string reading logic (strcpy, strlen, AsciiString, and friends). It would not fix the buffer overflow on its own, because ~400+ bytes (MAX_PACKET_SIZE minus generals headers) is still more than _MAX_PATH which is 260.
With that said, I know there's not so much bandwidth generally speaking (generalsy speaking he he he), but I'd not add an additional copy just to add a zero byte at the end. If you want to do that just make sure the network packet is big enough and do something like msg->data[msg->length] = 0;. Provided MAX_PACKET_SIZE is 476, a valid packet size could be +1 or 512 if you want to round up.

[jbremer]

Edit: having the packet zero-terminated and using AsciiString would actually also resolve the buffer overflow issues yes. Missed that for a moment.

[jbremer]

But then still there's _MAX_PATH for a reason. Throwing longer strings at the Windows API won't do much good. Better to do error handling earlier I think (i.e., throw out packets with too long file paths). In the context of generals any file path (a map name) above, say, 128 bytes is probably already too long.

[Skyaero42]

strncpy(filename, data, _MAX_PATH-1)

This could also be:

strlcpy(filename, data, _MAX_PATH)

which will ensure null termination.

[jbremer]

Ah yes, strlcpy() will work if the src argument is nul-terminated. Which today it is not yet, necessarily, but with one of @xezon 's ideas it could be, if you want it to be. (IMO it's not needed but it can be made that way).

[xezon]

Will this change see additional revisions?

[jbremer]

I can make modifications if you like, but so far my PR seems less intrusive than the other suggestions mentioned above (e.g., by not requiring buf to be nul-terminated).