Skip to content

Commit c5721e1

Browse files
committed
More strict parsing of identities.
- We no longer return true when paring the string "invalid". Yes, technically we *parsed* successfully. However, most callers would not consider parsing the invalid identity a success for their puspose, they assume that if true is returned, we have a real identity of some kind. (But perhaps one that is the "unknown" kind.) This was true of all existing call sites. So this was basicaly a bug trap and/or security issue. If a new caller wants to handle the string "invalid", they can do it themselves. - Assume that the relay is running the latest protocol, so treat an unknown prefix as failure / invalid. Do not try to continue. This doesn't impact this opensource code, since the relay is not currently open source.
1 parent 6cf5378 commit c5721e1

File tree

1 file changed

+49
-37
lines changed

1 file changed

+49
-37
lines changed

src/steamnetworkingsockets/steamnetworkingsockets_shared.cpp

Lines changed: 49 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -334,8 +334,11 @@ STEAMNETWORKINGSOCKETS_INTERFACE bool SteamAPI_SteamNetworkingIdentity_ParseStri
334334
if ( pszStr == nullptr || *pszStr == '\0' )
335335
return false;
336336

337-
if ( V_strcmp( pszStr, "invalid" ) == 0 )
338-
return true; // Specifically parsed as invalid is considered "success"!
337+
// NOTE: Reversing this decision. 99% of use cases, we really want the function to return
338+
// false unless the identity is valid. The 1% of cases that want to allow this can
339+
// specifically check for this string.
340+
// if ( V_strcmp( pszStr, "invalid" ) == 0 )
341+
// return true; // Specifically parsed as invalid is considered "success"!
339342

340343
size_t sizeofData = sizeofIdentity - sizeofHeader;
341344

@@ -395,43 +398,52 @@ STEAMNETWORKINGSOCKETS_INTERFACE bool SteamAPI_SteamNetworkingIdentity_ParseStri
395398
return pIdentity->SetGenericBytes( tmp, nBytes );
396399
}
397400

398-
// Unknown prefix. But does it looks like it is a string
399-
// of the form <prefix>:data ? We assume that we will only
400-
// ever use prefixes from a restricted character set, and we
401-
// won't ever make them too long.
402-
int cchPrefix = 0;
403-
do
404-
{
405-
// Invalid type prefix or end of string?
406-
// Note: lowercase ONLY. Identifiers are case sensitive (we need this to be true
407-
// because we want to be able to hash them and compare them as dumb bytes), so
408-
// any use of uppercase letters is really asking for big problems.
409-
char c = pszStr[cchPrefix];
410-
if ( ( c < 'a' || c > 'z' )
411-
&& ( c < '0' || c > '9' )
412-
&& c != '_'
413-
) {
414-
return false;
415-
}
401+
// Unknown prefix.
402+
// The relays should always be running the latest code. No client should
403+
// be using a protocol newer than a relay.
404+
#ifdef IS_STEAMDATAGRAMROUTER
405+
return false;
406+
#else
407+
408+
// Does it looks like it is a string
409+
// of the form <prefix>:data ? We assume that we will only
410+
// ever use prefixes from a restricted character set, and we
411+
// won't ever make them too long.
412+
int cchPrefix = 0;
413+
do
414+
{
415+
// Invalid type prefix or end of string?
416+
// Note: lowercase ONLY. Identifiers are case sensitive (we need this to be true
417+
// because we want to be able to hash them and compare them as dumb bytes), so
418+
// any use of uppercase letters is really asking for big problems.
419+
char c = pszStr[cchPrefix];
420+
if ( ( c < 'a' || c > 'z' )
421+
&& ( c < '0' || c > '9' )
422+
&& c != '_'
423+
) {
424+
return false;
425+
}
416426

417-
// Char is OK to be in the prefix, move on
418-
++cchPrefix;
419-
if ( cchPrefix > 16 )
427+
// Char is OK to be in the prefix, move on
428+
++cchPrefix;
429+
if ( cchPrefix > 16 )
430+
return false;
431+
} while ( pszStr[cchPrefix] != ':' );
432+
433+
// OK, as far as we can tell, it might be valid --- unless it's too long
434+
int cbSize = V_strlen(pszStr)+1;
435+
if ( cbSize > SteamNetworkingIdentity::k_cchMaxString )
436+
return false;
437+
if ( (size_t)cbSize > sizeofData )
420438
return false;
421-
} while ( pszStr[cchPrefix] != ':' );
422439

423-
// OK, as far as we can tell, it might be valid --- unless it's too long
424-
int cbSize = V_strlen(pszStr)+1;
425-
if ( cbSize > SteamNetworkingIdentity::k_cchMaxString )
426-
return false;
427-
if ( (size_t)cbSize > sizeofData )
428-
return false;
440+
// Just save the exact raw string we were asked to "parse". We don't
441+
// really understand it, but for many purposes just using the string
442+
// as an identifier will work fine!
443+
pIdentity->m_eType = k_ESteamNetworkingIdentityType_UnknownType;
444+
pIdentity->m_cbSize = cbSize;
445+
memcpy( pIdentity->m_szUnknownRawString, pszStr, cbSize );
429446

430-
// Just save the exact raw string we were asked to "parse". We don't
431-
// really understand it, but for many purposes just using the string
432-
// as an identifier will work fine!
433-
pIdentity->m_eType = k_ESteamNetworkingIdentityType_UnknownType;
434-
pIdentity->m_cbSize = cbSize;
435-
memcpy( pIdentity->m_szUnknownRawString, pszStr, cbSize );
436-
return true;
447+
return true;
448+
#endif
437449
}

0 commit comments

Comments
 (0)