Message sizes: fix integer overflow in send path, clarify limits

Add k_cbMaxSteamNetworkingSocketsMessageSizeSend_Internal to accommodate
the P2PMessageHeader prepended by ISteamNetworkingMessages, so callers can
use the full advertised message size. Add GetEffectiveRecvMaxMessageSize()
on connections owned by a messages endpoint to match. Move size checks to
API entry points (SpewWarning + k_EResultInvalidParam) and demote the
innermost check to an Assert against the internal limit.

This addresses issue #382.

I don't believe there is a real security vuln here in the threat model
where the local process is trusted.  A malicious remote peer cannot
trigger this overflow.

Author: zpostfacto

src/steamnetworkingsockets/clientlib/csteamnetworkingmessages.cpp

@@ -3,22 +3,14 @@
 #include "csteamnetworkingmessages.h"
 #include "csteamnetworkingsockets.h"
 #include "steamnetworkingsockets_p2p.h"
+#include "../steamnetworkingsockets_internal.h"
 
 // memdbgon must be the last include file in a .cpp file!!!
 #include "tier0/memdbgon.h"
 
 // Make sure we're enabled
 #ifdef STEAMNETWORKINGSOCKETS_ENABLE_STEAMNETWORKINGMESSAGES
 
-#pragma pack(push,1)
-struct P2PMessageHeader
-{
-	uint8 m_nFlags;
-	int m_nToChannel;
-};
-#pragma pack(pop)
-COMPILE_TIME_ASSERT( sizeof(P2PMessageHeader) == 5 );
-
 // FIXME TODO:
 // * Need to clear P2P error when we start connecting or get a successful result
 // * When we get P2P error callback from steam, need to flow that back up through to the session
@@ -29,6 +21,18 @@ COMPILE_TIME_ASSERT( sizeof(P2PMessageHeader) == 5 );
 // Put everything in a namespace, so we don't violate the one definition rule
 namespace SteamNetworkingSocketsLib {
 
+// Messages sent using the ISteamnNetworkingMessages interface are prepended
+// internally by a small internal header
+#pragma pack(push,1)
+struct P2PMessageHeader
+{
+	uint8 m_nFlags;
+	int m_nToChannel;
+};
+#pragma pack(pop)
+COMPILE_TIME_ASSERT( sizeof(P2PMessageHeader) == 5 );
+COMPILE_TIME_ASSERT( k_cbMaxSteamNetworkingSocketsMessageSizeSend + sizeof(P2PMessageHeader) <= k_cbMaxSteamNetworkingSocketsMessageSizeSend_Internal );
+
 const SteamNetworkingMicroseconds k_usecSteamNetworkingP2PSessionIdleTimeout = 3*60*k_nMillion;
 const int k_ESteamNetConnectionEnd_P2P_SessionClosed = k_ESteamNetConnectionEnd_App_Min + 1;
 const int k_ESteamNetConnectionEnd_P2P_SessionIdleTimeout = k_ESteamNetConnectionEnd_App_Min + 2;
@@ -347,7 +351,7 @@ void CMessagesEndPointSession::ClearActiveConnection()
 CSteamNetworkingMessages::Channel::Channel()
 {
 	m_queueRecvMessages.m_pRequiredLock = &g_lockAllRecvMessageQueues;
-		
+
 }
 
 CSteamNetworkingMessages::Channel::~Channel()
@@ -418,8 +422,15 @@ EResult CSteamNetworkingMessages::SendMessageToUser( const SteamNetworkingIdenti
 {
 	if ( identityRemote.IsInvalid() )
 	{
-		AssertMsg( false, "Identity isn't valid for Messages sessions." );
-		return k_EResultFail;
+		SpewWarning( "Identity %s isn't valid for Messages sessions.", SteamNetworkingIdentityRender( identityRemote ).c_str() );
+		return k_EResultInvalidParam;
+	}
+
+	// Message too big?
+	if ( cubData > (unsigned)k_cbMaxSteamNetworkingSocketsMessageSizeSend )
+	{
+		SpewWarning( "Message size %u is too big.  Max is %d", cubData, k_cbMaxSteamNetworkingSocketsMessageSizeSend );
+		return k_EResultInvalidParam;
 	}
 
 	SteamNetworkingGlobalLock scopeLock( "SendMessageToUser" ); // !SPEED! Can we avoid this?
@@ -486,7 +497,7 @@ EResult CSteamNetworkingMessages::SendMessageToUser( const SteamNetworkingIdenti
 		nSendFlags = k_nSteamNetworkingSend_Reliable;
 
 	// Allocate a message, and put our header in front.
-	int cbSend = cubData + sizeof(P2PMessageHeader);
+	int cbSend = (int)cubData + (int)sizeof(P2PMessageHeader);
 	CSteamNetworkingMessage *pMsg = (CSteamNetworkingMessage *)m_steamNetworkingSockets.m_pSteamNetworkingUtils->AllocateMessage( cbSend );
 	if ( !pMsg )
 	{

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp

@@ -579,7 +579,7 @@ CSteamNetworkConnectionBase::CSteamNetworkConnectionBase( CSteamNetworkingSocket
 	m_bConnectionInitiatedRemotely = false;
 	m_pTransport = nullptr;
 	m_nSupressStateChangeCallbacks = 0;
- 
+
 	// Initialize configuration using parent interface for now.
 	m_connectionConfig.Init( &m_pSteamNetworkingSocketsInterface->m_connectionConfig );
 
@@ -1400,7 +1400,7 @@ ESteamNetConnectionEnd CSteamNetworkConnectionBase::RecvCryptoHandshake(
 	m_sCryptRemote = msgSessionInfo.info();
 
 	// If they presented a signature, it must be valid
-	const CertAuthScope *pCACertAuthScope = nullptr; 
+	const CertAuthScope *pCACertAuthScope = nullptr;
 	if ( msgCert.has_ca_signature() )
 	{
 
@@ -1599,7 +1599,7 @@ ESteamNetConnectionEnd CSteamNetworkConnectionBase::FinishCryptoHandshake( bool
 		SetCryptoCipherList();
 	}
 	Assert( m_msgCryptLocal.ciphers_size() > 0 );
-	
+
 	// Find a mutually-acceptable cipher
 	Assert( m_eNegotiatedCipher == k_ESteamNetworkingSocketsCipher_INVALID );
 	m_eNegotiatedCipher = k_ESteamNetworkingSocketsCipher_INVALID;
@@ -2027,6 +2027,13 @@ EResult CSteamNetworkConnectionBase::APISendMessageToConnection( const void *pDa
 			return k_EResultNoConnection;
 	}
 
+	// Check message size
+	if ( cbData > (unsigned)k_cbMaxSteamNetworkingSocketsMessageSizeSend )
+	{
+		SpewWarning( "Message size %u is too big.  Max is %d", cbData, k_cbMaxSteamNetworkingSocketsMessageSizeSend );
+		return k_EResultInvalidParam;
+	}
+
 	// Fill out a message object
 	CSteamNetworkingMessage *pMsg = CSteamNetworkingMessage::New( cbData );
 	if ( !pMsg )
@@ -2083,16 +2090,27 @@ int64 CSteamNetworkConnectionBase::APISendMessageToConnection( CSteamNetworkingM
 			return -k_EResultNoConnection;
 	}
 
+	// Message too big?
+	if ( (unsigned)pMsg->m_cbSize > (unsigned)k_cbMaxSteamNetworkingSocketsMessageSizeSend )
+	{
+		SpewWarning( "Message size %u is too big.  Max is %d", pMsg->m_cbSize, k_cbMaxSteamNetworkingSocketsMessageSizeSend );
+		pMsg->Release();
+		return -k_EResultInvalidParam;
+	}
+
 	return _APISendMessageToConnection( pMsg, usecNow, pbThinkImmediately );
 }
 
 int64 CSteamNetworkConnectionBase::_APISendMessageToConnection( CSteamNetworkingMessage *pMsg, SteamNetworkingMicroseconds usecNow, bool *pbThinkImmediately )
 {
 
 	// Message too big?
-	if ( pMsg->m_cbSize > k_cbMaxSteamNetworkingSocketsMessageSizeSend )
+	// NOTE - we should have detected this higher up in the call stack.
+	// Also, use unsigned math here just in case we have a bug and the size is
+	// actually negative somehow
+	if ( (unsigned)pMsg->m_cbSize > (unsigned)k_cbMaxSteamNetworkingSocketsMessageSizeSend_Internal )
 	{
-		AssertMsg2( false, "Message size %d is too big.  Max is %d", pMsg->m_cbSize, k_cbMaxSteamNetworkingSocketsMessageSizeSend );
+		AssertMsg2( false, "Message size %d is too big.  Max internal size is %d", pMsg->m_cbSize, k_cbMaxSteamNetworkingSocketsMessageSizeSend_Internal );
 		pMsg->Release();
 		return -k_EResultInvalidParam;
 	}
@@ -2137,7 +2155,7 @@ int CSteamNetworkConnectionBase::APIReceiveMessages( SteamNetworkingMessage_t **
 	m_pLock->AssertHeldByCurrentThread();
 
 	g_lockAllRecvMessageQueues.lock();
-	
+
 	int result = m_queueRecvMessages.RemoveMessages( ppOutMessages, nMaxMessages );
 	g_lockAllRecvMessageQueues.unlock();
 
@@ -2204,7 +2222,7 @@ bool CSteamNetworkConnectionBase::DecryptDataChunk( uint16 nWireSeqNum, int cbPa
 
 		// Restore the IV to the base value
 		*(uint64 *)&m_cryptIVRecv.m_buf -= LittleQWord( ctx.m_nPktNum );
-	
+
 		// Did decryption fail?
 		if ( !bDecryptOK ) {
 
@@ -2646,10 +2664,11 @@ CSteamNetworkingMessage *CSteamNetworkConnectionBase::AllocateNewRecvMessage( ui
 	//
 
 	// Max message size
-	if ( (uint32)m_connectionConfig.RecvMaxMessageSize.Get() < cbSize )
+	const int cbRecvMaxMessageSize = GetEffectiveRecvMaxMessageSize();
+	if ( (unsigned)cbRecvMaxMessageSize < cbSize )
 	{
-		SpewMsg( "[%s] recv message of size %u too large for limit of %d.\n", GetDescription(), cbSize, m_connectionConfig.RecvMaxMessageSize.Get() );
-		ConnectionState_ProblemDetectedLocally( k_ESteamNetConnectionEnd_Misc_InternalError, "Failed to allocate a buffer of size %u (limit is %d).", cbSize, m_connectionConfig.RecvMaxMessageSize.Get() );
+		SpewMsg( "[%s] recv message of size %u too large for limit of %d.\n", GetDescription(), cbSize, cbRecvMaxMessageSize );
+		ConnectionState_ProblemDetectedLocally( k_ESteamNetConnectionEnd_Misc_InternalError, "Failed to allocate a buffer of size %u (limit is %d).", cbSize, cbRecvMaxMessageSize );
 		return nullptr;
 	}
 
@@ -3702,7 +3721,7 @@ SteamNetworkingMicroseconds CSteamNetworkConnectionBase::ThinkConnection_ClientC
 	Assert( !m_bConnectionInitiatedRemotely );
 
 	// Default behaviour for client periodically sending connect requests
-	
+
 	// Ask transport if it's ready
 	if ( !m_pTransport || !m_pTransport->BCanSendEndToEndConnectRequest() )
 		return usecNow + k_nMillion/20; // Nope, check back in just a bit.

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h

@@ -91,7 +91,7 @@ struct RemoteConnectionKey_t
 	}
 };
 
-/// Base class for connection-type-specific context structure 
+/// Base class for connection-type-specific context structure
 struct SendPacketContext_t
 {
 	inline SendPacketContext_t( SteamNetworkingMicroseconds usecNow, const char *pszReason ) : m_usecNow( usecNow ), m_pszReason( pszReason ) {}
@@ -353,7 +353,7 @@ struct ConnectionScopeLock : ScopeLock<ConnectionLock>
 /// Abstract interface for a connection to a remote host over any underlying
 /// transport.  Most of the common functionality for implementing reliable
 /// connections on top of unreliable datagrams, connection quality measurement,
-/// etc is implemented here. 
+/// etc is implemented here.
 class CSteamNetworkConnectionBase : public ILockableThinker< ConnectionLock >
 {
 public:
@@ -678,6 +678,18 @@ class CSteamNetworkConnectionBase : public ILockableThinker< ConnectionLock >
 	static constexpr CMessagesEndPointSession *m_pMessagesEndPointSessionOwner = nullptr;
 	#endif
 
+	/// Get the effective value of the RecvMaxMessageSize value.
+	/// Connections used by the ISteamNetworkingMessages interface need a little extra space
+	int GetEffectiveRecvMaxMessageSize() const
+	{
+		int cbResult = m_connectionConfig.RecvMaxMessageSize.Get();
+		if ( m_pMessagesEndPointSessionOwner )
+		{
+			cbResult += k_cbMaxSteamNetworkingSocketsMessageSizeSend_Internal - k_cbMaxSteamNetworkingSocketsMessageSizeSend;
+		}
+		return cbResult;
+	}
+
 	/// Time when we would like to send our next connection diagnostics
 	/// update.  This is initialized the first time we enter the "connecting"
 	/// state and we are on a platform that wants those updates.
@@ -799,7 +811,7 @@ class CSteamNetworkConnectionBase : public ILockableThinker< ConnectionLock >
 	// combined with the packet number so that the effective IV is
 	// unique per packet.  We use a 96-bit IV, which is what TLS uses
 	// (RFC5288), what NIST recommends (https://dl.acm.org/citation.cfm?id=2206251),
-	// and what makes GCM the most efficient. 
+	// and what makes GCM the most efficient.
 	AutoWipeFixedSizeBuffer<12> m_cryptIVSend;
 	AutoWipeFixedSizeBuffer<12> m_cryptIVRecv;
 
@@ -972,11 +984,11 @@ class CConnectionTransport
 	virtual bool SendDataPacket( SteamNetworkingMicroseconds usecNow ) = 0;
 
 	/// Connection will call this to ask the transport to surround the
-	/// "chunk" with the appropriate framing, and route it to the 
-	/// appropriate host.  A "chunk" might contain a mix of reliable 
-	/// and unreliable data.  We use the same framing for data 
-	/// payloads for all connection types.  Return value is 
-	/// the number of bytes written to the network layer, UDP/IP 
+	/// "chunk" with the appropriate framing, and route it to the
+	/// appropriate host.  A "chunk" might contain a mix of reliable
+	/// and unreliable data.  We use the same framing for data
+	/// payloads for all connection types.  Return value is
+	/// the number of bytes written to the network layer, UDP/IP
 	/// header is not included.
 	///
 	/// ctx is whatever the transport passed to SNP_SendPacket, if the
@@ -1116,7 +1128,7 @@ extern CUtlHashMap<int, CSteamNetworkPollGroup *, std::equal_to<int>, Identity<i
 // All of the tables above are projected by the same lock, since we expect to only access it briefly
 struct TableLock : Lock<RecursiveTimedMutexImpl> {
 	TableLock() : Lock<RecursiveTimedMutexImpl>( "table", LockDebugInfo::k_nFlag_Table, LockDebugInfo::k_nOrder_ObjectOrTable ) {}
-}; 
+};
 using TableScopeLock = ScopeLock<TableLock>;
 extern TableLock g_tables_lock;
 

src/steamnetworkingsockets/steamnetworkingsockets_internal.h

@@ -287,10 +287,6 @@ const int k_cbSteamNetworkingSocketsTypicalMaxPlaintextPayloadSend = k_cbSteamNe
 const int k_cbSteamNetworkingSocketsMaxEncryptedPayloadRecv = k_cbSteamNetworkingSocketsMaxUDPMsgLen;
 const int k_cbSteamNetworkingSocketsMaxPlaintextPayloadRecv = k_cbSteamNetworkingSocketsMaxUDPMsgLen;
 
-/// Max value that RecvMaxMessageSize can be set to.
-const int k_cbMaxMessageSizeRecv_Limit = k_cbMaxSteamNetworkingSocketsMessageSizeSend*2;
-COMPILE_TIME_ASSERT( k_cbMaxMessageSizeRecv_Limit >= k_cbMaxSteamNetworkingSocketsMessageSizeSend*2 );
-
 /// If we have a cert that is going to expire in <N seconds, try to renew it
 const int k_nSecCertExpirySeekRenew = 3600*2;
 
@@ -303,6 +299,11 @@ COMPILE_TIME_ASSERT( k_cbSteamNetworkingSocketsMaxEncryptedPayloadSend + 50 < k_
 /// Min size of raw UDP message.
 const int k_nMinSteamDatagramUDPMsgLen = 5;
 
+/// Max message size we will actually send, internally.  This is a bit of a kludge because the ISteamNetworkingMessages
+/// API prepends messages with a small header, and we want users of that API to be able to use the full message size,
+/// without having to know about this internal header.
+const int k_cbMaxSteamNetworkingSocketsMessageSizeSend_Internal = k_cbMaxSteamNetworkingSocketsMessageSizeSend + 16;
+
 /// When sending a stats message, what sort of reply is requested by the calling code?
 enum EStatsReplyRequest
 {
@@ -870,7 +871,7 @@ struct ConnectionConfig
 	ConfigValue<int32> SendBufferSize;
 	ConfigValue<int32> RecvBufferSize;
 	ConfigValue<int32> RecvBufferMessages;
-	ConfigValue<int32> RecvMaxMessageSize;
+	ConfigValue<int32> RecvMaxMessageSize; // NOTE - use GetEffectiveRecvMaxMessageSize()!
 	ConfigValue<int32> RecvMaxSegmentsPerPacket;
 	ConfigValue<int32> SendRateMin;
 	ConfigValue<int32> SendRateMax;