search

supertuxkart / stk-code

The code base of supertuxkart
Other
4.48k stars 1.05k forks source link

Chat message impersonation due to lack of message sanitization by the server #5121

Open mitros123 opened 3 months ago

mitros123 commented 3 months ago

Description

Hello. It is possible to forge the message sent in a lobby by simply setting the sender's name in a message with a modified client. This is of a minor impact, but it may result in a player pretending to be another one, and causing the latter to be kicked due to the former's chat behavior.

The function responsible for sending a chat message is ClientLobby::sendChat() of src/network/protocols/client_lobby.cpp. It looks like the following:

void ClientLobby::sendChat(irr::core::stringw text, KartTeam team)
{
    text = text.trim().removeChars(L"\n\r");
    if (text.size() > 0)
    {
        NetworkString* chat = getNetworkString();
        chat->addUInt8(LobbyProtocol::LE_CHAT);

        core::stringw name;
        PlayerProfile* player = PlayerManager::getCurrentPlayer();
        if (PlayerManager::getCurrentOnlineState() ==
            PlayerProfile::OS_SIGNED_IN)
            name = PlayerManager::getCurrentOnlineProfile()->getUserName();
        else
            name = player->getName();
[...]

        chat->encodeString16(name + L": " + text, 1000/*max_len*/);

        if (team != KART_TEAM_NONE)
            chat->addUInt8(team);

        STKHost::get()->sendToServer(chat, true);
        delete chat;
    }
}   // sendChat

The name of the player is put before the text, encoded and sent. The function that handles the chat is the ClientLobby::handleChat(). That function does not sanitize the contents of the message :

void ClientLobby::handleChat(Event* event)
{
    if (!UserConfigParams::m_lobby_chat)
        return;
    SFXManager::get()->quickSound("plopp");
    core::stringw message;
    event->data().decodeString16(&message);
    Log::info("ClientLobby", "%s", StringUtils::wideToUtf8(message).c_str());
    if (GUIEngine::isNoGraphics())
        return;
    if (message.size() > 0)
    {
        if (GUIEngine::getCurrentScreen() == NetworkingLobby::getInstance())
            NetworkingLobby::getInstance()->addMoreServerInfo(message);
        else if (UserConfigParams::m_race_chat)
            MessageQueue::add(MessageQueue::MT_GENERIC, message);
    }
}   // handleChat

As a result, a modified client (either by static patching or dynamic hooking) that is set to alter the name inside the message, will be able to impersonate another client. Ideally the server should be the one who puts the name before a message, it should not be left to the client.

Steps to reproduce

Simply set a different name in the line

chat->encodeString16(name + L": " + text, 1000/*max_len*/);

Configuration

STK version: From github, commit 84dff44

Additional information

(Part of) stdout.log of server:

[...]
[info   ] ProtocolManager: A class ConnectToServer protocol has been terminated.
Fri Jun 28 21:30:11 2024 [info   ] STKHost: [::1]:61555 has just connected. There are now 1 peers.
Fri Jun 28 21:30:11 2024 [info   ] ServerLobby: Message of type 1 received.
Fri Jun 28 21:30:11 2024 [info   ] ServerLobby: New player tester12 with online id 476395 from [::1]:61555 with SuperTuxKart/git (Windows).
[info   ] ClientLobby: Synchronous message of type 4
[info   ] ClientLobby: Synchronous message of type 6
[info   ] ClientLobby: Synchronous message of type 3
[info   ] ClientLobby: The server accepted the connection.
[info   ] ClientLobby: Synchronous message of type 18
[info   ] ClientLobby: Synchronous message of type 6
[info   ] NetworkTimerSynchronizer: Network timer synchronized, difference: 1ms
Fri Jun 28 21:30:17 2024 [verbose  ] STKHost: Received LAN server query
Fri Jun 28 21:30:20 2024 [info   ] STKHost: 192.168.XXX.XXX:49985 has just connected. There are now 2 peers.
Fri Jun 28 21:30:20 2024 [info   ] ServerLobby: Message of type 1 received.
Fri Jun 28 21:30:20 2024 [info   ] ServerLobby: New player tester3 with online id 476279 from 192.168.XXX.XXX:49985 with SuperTuxKart/git (Linux).
[info   ] ClientLobby: Synchronous message of type 6
Fri Jun 28 21:30:32 2024 [info   ] ServerLobby: Message of type 17 received.
[info   ] ClientLobby: Synchronous message of type 17
[info   ] ClientLobby: tester12: I am the creator of this server
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
Fri Jun 28 21:31:09 2024 [info   ] ServerLobby: Message of type 17 received.
[info   ] ClientLobby: Synchronous message of type 17
[info   ] ClientLobby: tester3: This message was sent by tester3
Fri Jun 28 21:31:29 2024 [info   ] ServerLobby: Message of type 17 received.
[info   ] ClientLobby: Synchronous message of type 17
[info   ] ClientLobby: tester12:This message was also sent by tester3
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
Fri Jun 28 21:32:44 2024 [info   ] ServerLobby: Message of type 17 received.
Fri Jun 28 21:32:44 2024 [error  ] StringUtils: wideToUtf32 error: Invalid UTF-16
[info   ] ClientLobby: Synchronous message of type 17
[info   ] ClientLobby: tester12ퟀ
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
Fri Jun 28 21:33:22 2024 [info   ] ServerLobby: Message of type 17 received.
[info   ] ClientLobby: Synchronous message of type 17
[info   ] ClientLobby: tester12: This message was also sent by tester3, but with the proper space after the colon
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] HTTPRequest: Sending userid=476395&token=************************ to https://online.supertuxkart.net/api/v2/user/poll/
[info   ] ProtocolManager: A class ClientLobby protocol has been terminated.
[info   ] STKHost: Listening has been stopped.
[...]

stk

kimden commented 3 months ago

Ideally the server should be the one who puts the name before a message, it should not be left to the client.

I suppose the username formation is delegated to client because there can be cases (for example, in splitscreen online multiplayer) when the name of the player who types the message cannot be determined uniquely by the server only. So I guess the client still has to send the name, but then the server can check if there's a player in the corresponding peer with such a name.