Skip to content

Ignore client race-finished notifications on server - #5881

Merged
Alayan-stk-2 merged 2 commits into
supertuxkart:masterfrom
acts-1631:fix-forged-race-finish
Sep 22, 2026
Merged

Alayan-stk-2 merged 2 commits into
supertuxkart:masterfrom
acts-1631:fix-forged-race-finish

Conversation

@acts-1631

Copy link
Copy Markdown
Contributor

GameEventsProtocol::notifyEvent handles GE_KART_FINISHED_RACE by calling the decoder that updates World::getWorld()->getKart(kart_id) and marks that kart as finished. There was no check that the event came from the server, so a modified client could send this notification directly. That could let a client forge race results, finish another kart early, or provide an invalid kart ID to the decoder.

On server instances, this change rejects GE_KART_FINISHED_RACE events received from clients and logs the sender. Clients still process the event because it is a server-to-client notification.

@Alayan-stk-2

Copy link
Copy Markdown
Member

I'm not particularly familiar with the networking code, but the two kartFinishedRace function within the very same code file indicate quite clearly that one should only be called for clients and the others for servers ; while the GE_STARTUP_BOOST handling of the server case indicates that servers may be on the receiving end of Game Event data.

So the fix in isolation appears sensible, however it opens some additional questions:

  • Could a modified client send wrong data to another client through this channel?
  • Wouldn't the logic of potential fake notifications from a modified client being read and applied by the server also apply to some other message types?

For example, the reset ball directly calls sw->handleResetBallFromServer after only checking that there is an actual soccer world, so if the server reads GE message from a client, it seems a client could modify server state that way. The called function doesn't handle the case either, but arguably the nicer solution would be making it impossible to call it at all if NetworkConfig indicates a server... That's kind of the issue with classes, state gets everywhere, we can't simply get a soccer world without those functions and a compile-time guarantee that a server will never try to call those.

Now, this practically asks to build a client that can tell the server a goal has been scored on demand and see what happens.

Only startup boost requests are sent from clients. Reject other\ngame-event types before they can modify server-side game state, and\nvalidate the startup-boost payload length.
@acts-1631

Copy link
Copy Markdown
Contributor Author

Thanks, that was a good catch. I checked the other GameEventsProtocol cases as well.

GE_STARTUP_BOOST is the only event sent from a client. The other event types are server notifications, and there is no client-to-client relay for this protocol. I updated the branch so the server rejects every other game-event type before it reaches the mode-specific handlers. This covers forged finish, goal, ball reset, FFA/CTF score, and check-line messages.

I also added a length check for the remaining startup-boost request. I tested this with a local soccer server and a modified client sending GE_RESET_BALL; the server received it and logged that it was ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants