From add02dc6f9c3f296debd8ff888580f9a69d9e9ed Mon Sep 17 00:00:00 2001 From: Alibek Omarov Date: Thu, 6 Feb 2025 20:54:18 +0300 Subject: [PATCH] engine: fix potential UB in netadr_t --- common/netadr.h | 61 +++++++++++++++---- engine/client/cl_game.c | 2 +- engine/client/cl_main.c | 29 ++++----- engine/common/net_ws.c | 123 ++++++++++++++++++++++---------------- engine/common/net_ws.h | 2 +- engine/server/sv_filter.c | 4 +- 6 files changed, 143 insertions(+), 78 deletions(-) diff --git a/common/netadr.h b/common/netadr.h index 4ce1ef4d34..0281eee569 100644 --- a/common/netadr.h +++ b/common/netadr.h @@ -28,7 +28,17 @@ Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. #define PORT_ANY -1 -typedef enum {NA_LOOPBACK = 1, NA_BROADCAST, NA_IP, NA_IPX, NA_BROADCAST_IPX, NA_IP6, NA_MULTICAST_IP6} netadrtype_t; +typedef enum netadrtype_e +{ + NA_UNDEFINED = 0, + NA_LOOPBACK, + NA_BROADCAST, + NA_IP, + NA_IPX, + NA_BROADCAST_IPX, + NA_IP6, + NA_MULTICAST_IP6 +} netadrtype_t; /* Original Quake-2 structure: @@ -46,29 +56,60 @@ typedef struct #pragma pack( push, 1 ) typedef struct netadr_s { +// the reason we do this evil thing, is that when this struct contains IPv6 +// address the `type` is 2-byte wide, but when it doesn't `type` must 4-byte +// wide _and_ ip6_0 must be zeroed, to keep it binary compatible. +#if XASH_LITTLE_ENDIAN + uint16_t type; + uint8_t ip6_0[2]; +#elif XASH_BIG_ENDIAN + uint8_t ip6_0[2]; + uint16_t type; +#else +#error +#endif + union { // IPv6 struct + uint8_t ip6_1[14]; struct { - uint16_t type6; - uint8_t ip6[16]; - }; - struct - { - uint32_t type; // must be netadrtype_t but will break with short enums union { - uint8_t ip[4]; - uint32_t ip4; // for easier conversions + uint8_t ip[4]; + uint32_t ip4; // for easier conversions }; uint8_t ipx[10]; }; }; - uint16_t port; + uint16_t port; } netadr_t; #pragma pack( pop ) +static inline netadrtype_t NET_NetadrType( const netadr_t *a ) +{ + if( a->type == NA_IP6 || a->type == NA_MULTICAST_IP6 ) + return a->type; + + if( a->ip6_0[0] || a->ip6_0[1] ) + return NA_UNDEFINED; + + return a->type; +} + +static inline void NET_NetadrSetType( netadr_t *a, netadrtype_t type ) +{ + if( type == NA_IP6 || type == NA_MULTICAST_IP6 ) + { + a->type = type; + return; + } + + a->ip6_0[0] = a->ip6_0[1] = 0; + a->type = type; +} + STATIC_CHECK_SIZEOF( netadr_t, 20, 20 ); #endif // NET_ADR_H diff --git a/engine/client/cl_game.c b/engine/client/cl_game.c index 0def052050..5dede3ed0c 100644 --- a/engine/client/cl_game.c +++ b/engine/client/cl_game.c @@ -3427,7 +3427,7 @@ static void GAME_EXPORT NetAPI_SendRequest( int context, int request, int flags, return; } - if( remote_address->type == NA_IPX || remote_address->type == NA_BROADCAST_IPX ) + if( NET_NetadrType( remote_address ) == NA_IPX || NET_NetadrType( remote_address ) == NA_BROADCAST_IPX ) return; // IPX no longer support if( request == NETAPI_REQUEST_SERVERLIST ) diff --git a/engine/client/cl_main.c b/engine/client/cl_main.c index 52cc8619d9..0e564841b9 100644 --- a/engine/client/cl_main.c +++ b/engine/client/cl_main.c @@ -1063,6 +1063,7 @@ static void CL_SendConnectPacket( connprotocol_t proto, int challenge ) const char *key = ID_GetMD5(); netadr_t adr = { 0 }; int input_devices; + netadrtype_t adrtype; protinfo[0] = 0; @@ -1073,14 +1074,16 @@ static void CL_SendConnectPacket( connprotocol_t proto, int challenge ) return; } + adrtype = NET_NetadrType( &adr ); + if( adr.port == 0 ) adr.port = MSG_BigShort( PORT_SERVER ); input_devices = IN_CollectInputDevices(); - IN_LockInputDevices( adr.type != NA_LOOPBACK ? true : false ); + IN_LockInputDevices( adrtype != NA_LOOPBACK ? true : false ); // GoldSrc doesn't need sv_cheats set to 0, it's handled by svc_goldsrc_sendextrainfo // it also doesn't need useragent string - if( adr.type != NA_LOOPBACK && proto != PROTO_GOLDSRC ) + if( adrtype != NA_LOOPBACK && proto != PROTO_GOLDSRC ) { Cvar_SetCheatState(); Cvar_FullSet( "sv_cheats", "0", FCVAR_READ_ONLY | FCVAR_SERVER ); @@ -1218,7 +1221,7 @@ static void CL_CheckForResend( void ) cls.signon = 0; cls.state = ca_connecting; Q_strncpy( cls.servername, "localhost", sizeof( cls.servername )); - cls.serveradr.type = NA_LOOPBACK; + NET_NetadrSetType( &cls.serveradr, NA_LOOPBACK ); cls.legacymode = PROTO_CURRENT; // we don't need a challenge on the localhost @@ -1554,8 +1557,8 @@ static void CL_SendDisconnectMessage( connprotocol_t proto ) MSG_WriteString( &buf, "dropclient\n" ); else MSG_WriteString( &buf, "disconnect" ); - if( !cls.netchan.remote_address.type ) - cls.netchan.remote_address.type = NA_LOOPBACK; + if( NET_NetadrType( &cls.netchan.remote_address ) == NA_UNDEFINED ) + NET_NetadrSetType( &cls.netchan.remote_address, NA_LOOPBACK ); // make sure message will be delivered Netchan_TransmitBits( &cls.netchan, MSG_GetNumBitsWritten( &buf ), MSG_GetData( &buf )); @@ -1724,19 +1727,17 @@ CL_LocalServers_f */ static void CL_LocalServers_f( void ) { - netadr_t adr; - - memset( &adr, 0, sizeof( adr )); + netadr_t adr = { 0 }; Con_Printf( "Scanning for servers on the local network area...\n" ); NET_Config( true, true ); // allow remote // send a broadcast packet - adr.type = NA_BROADCAST; + NET_NetadrSetType( &adr, NA_BROADCAST ); adr.port = MSG_BigShort( PORT_SERVER ); Netchan_OutOfBandPrint( NS_CLIENT, adr, A2A_INFO" %i", PROTOCOL_VERSION ); - adr.type = NA_MULTICAST_IP6; + NET_NetadrSetType( &adr, NA_MULTICAST_IP6 ); Netchan_OutOfBandPrint( NS_CLIENT, adr, A2A_INFO" %i", PROTOCOL_VERSION ); } @@ -2480,18 +2481,18 @@ static void CL_ServerList( netadr_t from, sizebuf_t *msg ) while( MSG_GetNumBitsLeft( msg ) > 8 ) { uint8_t addr[16]; - netadr_t servadr; + netadr_t servadr = { 0 }; - if( from.type6 == NA_IP6 ) // IPv6 master server only sends IPv6 addresses + if( NET_NetadrType( &from ) == NA_IP6 ) // IPv6 master server only sends IPv6 addresses { MSG_ReadBytes( msg, addr, sizeof( addr )); NET_IP6BytesToNetadr( &servadr, addr ); - servadr.type6 = NA_IP6; + NET_NetadrSetType( &servadr, NA_IP6 ); } else { MSG_ReadBytes( msg, servadr.ip, sizeof( servadr.ip )); // 4 bytes for IP - servadr.type = NA_IP; + NET_NetadrSetType( &servadr, NA_IP ); } servadr.port = MSG_ReadShort( msg ); // 2 bytes for Port diff --git a/engine/common/net_ws.c b/engine/common/net_ws.c index 83ac15e172..9f01d729f7 100644 --- a/engine/common/net_ws.c +++ b/engine/common/net_ws.c @@ -151,17 +151,24 @@ static inline qboolean NET_IsSocketValid( int socket ) void NET_NetadrToIP6Bytes( uint8_t *ip6, const netadr_t *adr ) { - memcpy( ip6, adr->ip6, sizeof( adr->ip6 )); + memcpy( &ip6[0], adr->ip6_0, 2 ); + memcpy( &ip6[2], adr->ip6_1, 14 ); } void NET_IP6BytesToNetadr( netadr_t *adr, const uint8_t *ip6 ) { - memcpy( adr->ip6, ip6, sizeof( adr->ip6 )); + memcpy( adr->ip6_0, &ip6[0], 2 ); + memcpy( adr->ip6_1, &ip6[2], 14 ); } static int NET_NetadrIP6Compare( const netadr_t *a, const netadr_t *b ) { - return memcmp( a->ip6, b->ip6, sizeof( a->ip6 )); + uint8_t ip6_a[16], ip6_b[16]; + + NET_NetadrToIP6Bytes( ip6_a, a ); + NET_NetadrToIP6Bytes( ip6_b, b ); + + return memcmp( ip6_a, ip6_b, sizeof( ip6_a )); } /* @@ -171,27 +178,29 @@ NET_NetadrToSockadr */ static void NET_NetadrToSockadr( netadr_t *a, struct sockaddr_storage *s ) { + netadrtype_t type = NET_NetadrType( a ); + memset( s, 0, sizeof( *s )); - if( a->type == NA_BROADCAST ) + if( type == NA_BROADCAST ) { s->ss_family = AF_INET; ((struct sockaddr_in *)s)->sin_port = a->port; ((struct sockaddr_in *)s)->sin_addr.s_addr = INADDR_BROADCAST; } - else if( a->type == NA_IP ) + else if( type == NA_IP ) { s->ss_family = AF_INET; ((struct sockaddr_in *)s)->sin_port = a->port; ((struct sockaddr_in *)s)->sin_addr.s_addr = a->ip4; } - else if( a->type6 == NA_IP6 ) + else if( type == NA_IP6 ) { s->ss_family = AF_INET6; ((struct sockaddr_in6 *)s)->sin6_port = a->port; NET_NetadrToIP6Bytes(((struct sockaddr_in6 *)s)->sin6_addr.s6_addr, a ); } - else if( a->type6 == NA_MULTICAST_IP6 ) + else if( type == NA_MULTICAST_IP6 ) { s->ss_family = AF_INET6; ((struct sockaddr_in6 *)s)->sin6_port = a->port; @@ -208,13 +217,13 @@ static void NET_SockadrToNetadr( const struct sockaddr_storage *s, netadr_t *a ) { if( s->ss_family == AF_INET ) { - a->type = NA_IP; + NET_NetadrSetType( a, NA_IP ); a->ip4 = ((struct sockaddr_in *)s)->sin_addr.s_addr; a->port = ((struct sockaddr_in *)s)->sin_port; } else if( s->ss_family == AF_INET6 ) { - a->type6 = NA_IP6; + NET_NetadrSetType( a, NA_IP6 ); NET_IP6BytesToNetadr( a, ((struct sockaddr_in6 *)s)->sin6_addr.s6_addr ); a->port = ((struct sockaddr_in6 *)s)->sin6_port; } @@ -549,8 +558,8 @@ qboolean NET_StringToFilterAdr( const char *s, netadr_t *adr, uint *prefixlen ) // try to parse as IPv6 first if( ParseIPv6Addr( copy, ip6, NULL, NULL )) { + NET_NetadrSetType( adr, NA_IP6 ); NET_IP6BytesToNetadr( adr, ip6 ); - adr->type6 = NA_IP6; if( !hasCIDR ) *prefixlen = 128; @@ -620,7 +629,7 @@ qboolean NET_StringToFilterAdr( const char *s, netadr_t *adr, uint *prefixlen ) adr->ip4 = ntohl( mask ); } - adr->type = NA_IP; + NET_NetadrSetType( adr, NA_IP ); } return true; @@ -634,10 +643,11 @@ NET_AdrToString const char *NET_AdrToString( const netadr_t a ) { static char s[64]; + netadrtype_t type = NET_NetadrType( &a ); - if( a.type == NA_LOOPBACK ) + if( type == NA_LOOPBACK ) return "loopback"; - if( a.type6 == NA_IP6 || a.type6 == NA_MULTICAST_IP6 ) + if( type == NA_IP6 || type == NA_MULTICAST_IP6 ) { uint8_t ip6[16]; @@ -661,10 +671,11 @@ NET_BaseAdrToString const char *NET_BaseAdrToString( const netadr_t a ) { static char s[64]; + netadrtype_t type = NET_NetadrType( &a ); - if( a.type == NA_LOOPBACK ) + if( type == NA_LOOPBACK ) return "loopback"; - if( a.type6 == NA_IP6 || a.type6 == NA_MULTICAST_IP6 ) + if( type == NA_IP6 || type == NA_MULTICAST_IP6 ) { uint8_t ip6[16]; @@ -689,16 +700,19 @@ Compares without the port */ qboolean NET_CompareBaseAdr( const netadr_t a, const netadr_t b ) { - if( a.type6 != b.type6 ) + netadrtype_t type_a = NET_NetadrType( &a ); + netadrtype_t type_b = NET_NetadrType( &b ); + + if( type_a != type_b ) return false; - if( a.type == NA_LOOPBACK ) + if( type_a == NA_LOOPBACK ) return true; - if( a.type == NA_IP ) + if( type_a == NA_IP ) return a.ip4 == b.ip4; - if( a.type6 == NA_IP6 ) + if( type_a == NA_IP6 ) { if( !NET_NetadrIP6Compare( &a, &b )) return true; @@ -716,13 +730,16 @@ Compare local masks */ qboolean NET_CompareClassBAdr( const netadr_t a, const netadr_t b ) { - if( a.type6 != b.type6 ) + netadrtype_t type_a = NET_NetadrType( &a ); + netadrtype_t type_b = NET_NetadrType( &b ); + + if( type_a != type_b ) return false; - if( a.type == NA_LOOPBACK ) + if( type_a == NA_LOOPBACK ) return true; - if( a.type == NA_IP ) + if( type_a == NA_IP ) { if( a.ip[0] == b.ip[0] && a.ip[1] == b.ip[1] ) return true; @@ -746,10 +763,13 @@ Checks if adr is a part of subnet */ qboolean NET_CompareAdrByMask( const netadr_t a, const netadr_t b, uint prefixlen ) { - if( a.type6 != b.type6 || a.type == NA_LOOPBACK ) + netadrtype_t type_a = NET_NetadrType( &a ); + netadrtype_t type_b = NET_NetadrType( &b ); + + if( type_a != type_b || type_a == NA_LOOPBACK ) return false; - if( a.type == NA_IP ) + if( type_a == NA_IP ) { uint32_t ipa = htonl( a.ip4 ); uint32_t ipb = htonl( b.ip4 ); @@ -757,7 +777,7 @@ qboolean NET_CompareAdrByMask( const netadr_t a, const netadr_t b, uint prefixle if(( ipa & (( 0xFFFFFFFFU ) << ( 32 - prefixlen ))) == ipb ) return true; } - else if( a.type6 == NA_IP6 ) + else if( type_a == NA_IP6 ) { uint16_t a_[8], b_[8]; size_t check = prefixlen / 16; @@ -798,11 +818,13 @@ Check for reserved ip's */ qboolean NET_IsReservedAdr( netadr_t a ) { - if( a.type == NA_LOOPBACK ) + netadrtype_t type_a = NET_NetadrType( &a ); + + if( type_a == NA_LOOPBACK ) return true; // Following checks was imported from GameNetworkingSockets library - if( a.type == NA_IP ) + if( type_a == NA_IP ) { if(( a.ip[0] == 10 ) || // 10.x.x.x is reserved ( a.ip[0] == 127 ) || // 127.x.x.x @@ -814,7 +836,7 @@ qboolean NET_IsReservedAdr( netadr_t a ) } } - if( a.type6 == NA_IP6 ) + if( type_a == NA_IP6 ) { uint8_t ip6[16]; @@ -847,20 +869,23 @@ Compare full address */ qboolean NET_CompareAdr( const netadr_t a, const netadr_t b ) { - if( a.type6 != b.type6 ) + netadrtype_t type_a = NET_NetadrType( &a ); + netadrtype_t type_b = NET_NetadrType( &b ); + + if( type_a != type_b ) return false; - if( a.type == NA_LOOPBACK ) + if( type_a == NA_LOOPBACK ) return true; - if( a.type == NA_IP ) + if( type_a == NA_IP ) { if( a.ip4 == b.ip4 && a.port == b.port ) return true; return false; } - if( a.type6 == NA_IP6 ) + if( type_a == NA_IP6 ) { if( a.port == b.port && !NET_NetadrIP6Compare( &a, &b )) return true; @@ -883,9 +908,13 @@ int NET_CompareAdrSort( const void *_a, const void *_b ) { const netadr_t *a = _a, *b = _b; int porta, portb, portdiff, addrdiff; + netadrtype_t type_a, type_b; + + type_a = NET_NetadrType( a ); + type_b = NET_NetadrType( b ); - if( a->type6 != b->type6 ) - return bound( -1, (int)a->type6 - (int)b->type6, 1 ); + if( type_a != type_b ) + return bound( -1, (int)type_a - (int)type_b, 1 ); porta = ntohs( a->port ); portb = ntohs( b->port ); @@ -896,7 +925,7 @@ int NET_CompareAdrSort( const void *_a, const void *_b ) else portdiff = 0; - switch( a->type6 ) + switch( type_a ) { case NA_IP6: if(( addrdiff = NET_NetadrIP6Compare( a, b ))) @@ -904,14 +933,7 @@ int NET_CompareAdrSort( const void *_a, const void *_b ) // fallthrough case NA_MULTICAST_IP6: return portdiff; - } - - // don't check for full type earlier, as it's value depends on v6 address - if( a->type != b->type ) - return bound( -1, (int)a->type - (int)b->type, 1 ); - switch( a->type ) - { case NA_IP: if(( addrdiff = memcmp( a->ip, b->ip, sizeof( a->ipx )))) return addrdiff; @@ -946,7 +968,7 @@ static qboolean NET_StringToAdrEx( const char *string, netadr_t *adr, int family if( !Q_stricmp( string, "localhost" ) || !Q_stricmp( string, "loopback" )) { - adr->type = NA_LOOPBACK; + NET_NetadrSetType( adr, NA_LOOPBACK ); return true; } @@ -971,7 +993,7 @@ net_gai_state_t NET_StringToAdrNB( const char *string, netadr_t *adr, qboolean v if( !Q_stricmp( string, "localhost" ) || !Q_stricmp( string, "loopback" )) { - adr->type = NA_LOOPBACK; + NET_NetadrSetType( adr, NA_LOOPBACK ); return NET_EAI_OK; } @@ -1017,7 +1039,7 @@ static qboolean NET_GetLoopPacket( netsrc_t sock, netadr_t *from, byte *data, si *length = loop->msgs[i].datalen; memset( from, 0, sizeof( *from )); - from->type = NA_LOOPBACK; + NET_NetadrSetType( from, NA_LOOPBACK ); return true; } @@ -1546,19 +1568,20 @@ void NET_SendPacketEx( netsrc_t sock, size_t length, const void *data, netadr_t int ret; struct sockaddr_storage addr = { 0 }; SOCKET net_socket = 0; + netadrtype_t type = NET_NetadrType( &to ); - if( !net.initialized || to.type == NA_LOOPBACK ) + if( !net.initialized || type == NA_LOOPBACK ) { NET_SendLoopPacket( sock, length, data, to ); return; } - else if( to.type == NA_BROADCAST || to.type == NA_IP ) + else if( type == NA_BROADCAST || type == NA_IP ) { net_socket = net.ip_sockets[sock]; if( !NET_IsSocketValid( net_socket )) return; } - else if( to.type6 == NA_MULTICAST_IP6 || to.type6 == NA_IP6 ) + else if( type == NA_MULTICAST_IP6 || type == NA_IP6 ) { net_socket = net.ip6_sockets[sock]; if( !NET_IsSocketValid( net_socket )) @@ -1566,7 +1589,7 @@ void NET_SendPacketEx( netsrc_t sock, size_t length, const void *data, netadr_t } else { - Host_Error( "%s: bad address type %i (%i)\n", __func__, to.type, to.type6 ); + Host_Error( "%s: bad address type %i (%i, %i)\n", __func__, to.type, to.ip6_0[0], to.ip6_0[1] ); } NET_NetadrToSockadr( &to, &addr ); @@ -1582,7 +1605,7 @@ void NET_SendPacketEx( netsrc_t sock, size_t length, const void *data, netadr_t return; // some PPP links don't allow broadcasts - if( err == WSAEADDRNOTAVAIL && ( to.type == NA_BROADCAST || to.type6 == NA_MULTICAST_IP6 )) + if( err == WSAEADDRNOTAVAIL && ( type == NA_BROADCAST || type == NA_MULTICAST_IP6 )) return; if( Host_IsDedicated( )) diff --git a/engine/common/net_ws.h b/engine/common/net_ws.h index 4825c71230..80b55e3cd0 100644 --- a/engine/common/net_ws.h +++ b/engine/common/net_ws.h @@ -80,7 +80,7 @@ void NET_NetadrToIP6Bytes( uint8_t *ip6, const netadr_t *adr ); static inline qboolean NET_IsLocalAddress( netadr_t adr ) { - return adr.type == NA_LOOPBACK ? true : false; + return NET_NetadrType( &adr ) == NA_LOOPBACK; } #if !XASH_DEDICATED diff --git a/engine/server/sv_filter.c b/engine/server/sv_filter.c index 179d447dee..42aae31d58 100644 --- a/engine/server/sv_filter.c +++ b/engine/server/sv_filter.c @@ -302,7 +302,7 @@ static int SV_FilterToString( char *dest, size_t size, qboolean config, ipfilter static qboolean SV_IPFilterIncludesIPFilter( ipfilter_t *a, ipfilter_t *b ) { - if( a->adr.type6 != b->adr.type6 ) + if( NET_NetadrType( &a->adr ) != NET_NetadrType( &b->adr )) return false; // can't include bigger subnet in small @@ -359,7 +359,7 @@ qboolean SV_CheckIP( netadr_t *adr ) if( entry->endTime && host.realtime > entry->endTime ) continue; // expired - switch( entry->adr.type6 ) + switch( NET_NetadrType( &entry->adr )) { case NA_IP: case NA_IP6: