strstr etc

Server/GameServer/Game/Struct/StructPlayer.cpp

@@ -5573,6 +5573,68 @@ void StructPlayer::SetDialogTitle( const char *szString, int type )
 	m_strSpecialDialogMenu.clear();
 }
 
+// --- Fraun security fix: NPC dialog trigger validation -------------------------------
+// The client echoes dialog triggers back to the server, which then runs them through
+// LUA()->RunString() (see onDialog in GameMessage.cpp). Validation used to be a substring
+// test (strstr), so a crafted / man-in-the-middled TS_CS_DIALOG packet could embed a
+// legitimate trigger substring and append arbitrary Lua, e.g.
+//     on_channel_set(1) os.execute("...")
+// Because the Lua VM is opened with luaL_openlibs (os/io included), that is full remote
+// code execution on the server host. The validators below now constrain the trigger to
+// exactly the shapes the real client can produce.
+//
+// A "special" dialog menu is a parameterised trigger: the server hands out a prefix
+// (e.g. "on_channel_set", "warp_to_secret_dungeon") and the client completes it with a
+// numeric argument list -> "prefix(<numbers>)" (client builds it as "%s(%s)" / "%s()" in
+// SUIInputNumberWnd / SGameInterface). This helper accepts ONLY that shape: the trigger
+// must start with the prefix and the remainder may contain nothing but a single
+// parenthesised list of digits / signs / dots / commas / whitespace. That lets the client
+// pick a number while making it impossible to chain extra Lua statements.
+static bool IsSafeSpecialDialogTrigger( const char * szTrigger, const char * szPrefix )
+{
+	if( !szTrigger || !szPrefix || !*szPrefix )
+		return false;
+
+	const size_t nPrefixLen = strlen( szPrefix );
+
+	// Must START with the offered prefix (not merely contain it).
+	if( strncmp( szTrigger, szPrefix, nPrefixLen ) != 0 )
+		return false;
+
+	const char * p = szTrigger + nPrefixLen;
+
+	while( *p == ' ' || *p == '\t' ) ++p;		// optional whitespace before '('
+
+	if( *p == '\0' )
+		return true;							// bare prefix, e.g. "exit_indun"
+
+	if( *p != '(' )
+		return false;							// anything other than an arg list -> reject
+	++p;
+
+	// Argument list: numbers only. No quotes, letters, ';', '=', '[', a second '(' etc.,
+	// so no further Lua statement can be smuggled in.
+	while( *p && *p != ')' )
+	{
+		const char c = *p;
+		const bool bAllowed =
+			( c >= '0' && c <= '9' ) ||
+			c == ',' || c == '.' || c == '-' || c == '+' || c == ' ' || c == '\t';
+		if( !bAllowed )
+			return false;
+		++p;
+	}
+
+	if( *p != ')' )
+		return false;							// unterminated argument list
+	++p;
+
+	while( *p == ' ' || *p == '\t' ) ++p;		// optional trailing whitespace
+
+	return *p == '\0';							// nothing may follow the ')'
+}
+// -------------------------------------------------------------------------------------
+
 void StructPlayer::SetFixedDialogTrigger( const char *szString )
 {
 	m_strFixedDialogTrigger = szString;
@@ -5580,7 +5642,11 @@ void StructPlayer::SetFixedDialogTrigger( const char *szString )
 
 bool StructPlayer::IsFixedDialogTrigger( const char * szTrigger )
 {
-	if( m_strFixedDialogTrigger.size() && strstr( szTrigger, m_strFixedDialogTrigger.c_str() ) )
+	// Fraun security fix: the fixed trigger is a complete, server-generated string that the
+	// client echoes back verbatim (e.g. "recall_feather( x, y, layer )" -> SUINotifyWnd sends
+	// it back with "%s"), so it must match EXACTLY. strstr() let a crafted packet keep the
+	// trigger as a prefix and append Lua after it.
+	if( m_strFixedDialogTrigger.size() && !strcmp( szTrigger, m_strFixedDialogTrigger.c_str() ) )
 		return true;
 
 	return false;
@@ -5614,7 +5680,8 @@ void StructPlayer::SetSpecialDialogMenu( const char * szString )
 
 bool StructPlayer::IsSpecialDialogMenu( const char * szString )
 {
-	return( m_strSpecialDialogMenu.size() && strstr( szString, m_strSpecialDialogMenu.c_str() ) );
+	// Fraun security fix: was strstr() (substring) -> now require "<prefix>(<numeric args>)".
+	return( m_strSpecialDialogMenu.size() && IsSafeSpecialDialogTrigger( szString, m_strSpecialDialogMenu.c_str() ) );
 }
 
 void StructPlayer::ClearSpecialDialogMenu()
@@ -5769,7 +5836,8 @@ bool StructPlayer::IsValidTrigger( const char *szTrigger )
 		if( !strcmp( strTriggerList[idx].c_str(), szTrigger ) ) return true;
 	}
 
-	if( m_strSpecialDialogMenu.size() && strstr( szTrigger, m_strSpecialDialogMenu.c_str() ) )
+	// Fraun security fix: was strstr() (substring) -> now require "<prefix>(<numeric args>)".
+	if( m_strSpecialDialogMenu.size() && IsSafeSpecialDialogTrigger( szTrigger, m_strSpecialDialogMenu.c_str() ) )
 		return true;
 
 	return false;