[FEATURE]: Built-in sync validation checks which worth adding

[NexiusTailer]

Feature request

SA-MP doesn't have built-in serverside or clientside anticheat and it could be justified to put this on scripters, but it also doesn't have some critical validity checks which is not about anticheat algorithms, it's rather about the basic principles of security and stability of the server, and here it is:

Regular sync data for blocking

1. Block any passenger sync if reported seatid is a driver seat (0)
2. Block any unoccupied sync if playerid updates a vehicle from passenger seat, but he seats on a different passenger seat in the same vehicle
3. Block any bullet sync if lagcompmode is disabled
4. Block any bullet sync if reported hitid is the same as vehicleid the player sitting in (player shoots his own car), or hitid is another player sitting in the same car with playerid

---

RPC data for blocking

1. Block any incoming RequestClass and RequestSpawn RPCs if playerid is already spawned
2. Block any incoming RequestClass RPCs if reported classid isn't created on the server (so it isn't valid)
3. Block any incoming SCMEvent RPCs (EnterExitModShop part of it) if reported enterexit is 1 (enter) but player was already in modshop
4. Block any incoming SCMEvent RPCs (EnterExitModShop part of it) if reported enterexit is 0 (exit) but player was not in modshop
5. Block any incoming SCMEvent RPCs (EnterExitModShop part of it) if it was sent outside of the reported vehicle (playerid isn't a driver of this vehicleid)
6. Block any incoming Death RPCs if playerid is already dead
7. Block any incoming EnterVehicle RPCs if playerid is not in onfoot player state
8. Block any incoming ExitVehicle RPCs if playerid is not in any vehicle or he is in different vehicle
9. Block any incoming InteriorChange RPCs if reported newinteriorid is equal to oldinteriorid
10. Block any incoming MenuSelect and MenuQuit RPCs (OnPlayerSelectedMenuRow and OnPlayerExitedMenu) if no any menu is shown for player (including when player has already responded to the menu and now it should've been closed) or reported row (for RPC MenuSelect) isn't valid
11. Block any incoming DialogResponse RPCs if reported listitem isn't valid
12. Block any incoming ClickTextDraw RPCs if reported clicked textdraw id isn't clickable or isn't shown for player
13. Block any incoming GiveTakeDamage RPCs (both give and take) if bodypart is invalid
14. Block any incoming GiveTakeDamage RPCs (GiveDamage part of it) if reported damagedid (victim) or weaponid is invalid
15. Block any incoming ActorDamage RPCs (OnPlayerGiveDamageActor) if amount of damage less than 0.0 (negative values) or bodypart is invalid
16. Block any incoming EnterEditObject RPCs (OnPlayerSelectObject) if playerid is not in selection mode (SelectObject isn't set or it should've been cancelled), or if reported modelid is not equal to the actual object model
17. Block any incoming EditAttachedObject RPCs (OnPlayerEditAttachedObject) if reported modelid is not equal to the actual attached object model
18. Block any incoming EditObject RPCs (OnPlayerEditObject) if playerid is not in edit mode for that object (EditObject is set for other objectid to edit)
19. Block any incoming EditAttachedObject RPCs (OnPlayerEditAttachedObject) if reported boneid is invalid
20. Block any incoming EditObject RPCs (OnPlayerEditObject) if reported response type is invalid
21. Block any incoming ClientCheckResponse RPCs (OnClientCheckResponse) if no client checks were sent (or they were sent for different type/actionid comparing to the response)

---

RPC data for checking

1. [DialogResponse IRPC] Convert 'response' internally to boolean and report only 1 / 0 to the final pawn callback (now it can be sent with any other value)
2. [GiveTakeDamage IRPC] (TakeDamage part of it) Change 'issuerid' to 65535 if playerid is equal to issuerid
3. [Death IRPC] Change 'killerid' to 65535 if playerid is equal to killerid

Contact information

Nexius#2253

Additional information

This should be still relevant stuff moved from previous CBT repo

[NexiusTailer]

Another thing: although omp server filters all invalid float data (NaNs, etc) very well and where it's even possible, but it does this rather strangely when it used inside the main synchronizations and it "corrects" incorrect values replacing them with 0.0 instead of blocking the whole sync packet.

The following sync types can be considered problematic having current invalid float values corrections:

1. onfoot sync if reported position or velocity are invalid (NaN/infinity/etc)
2. driver sync if reported position or velocity are invalid
3. passenger sync if reported position is invalid
4. spectating sync if reported position is invalid
5. unoccupied sync if reported position or velocity are invalid
6. trailer sync if reported position or velocity are invalid

I.e. any sync which sends player/vehicle main movement data for others.

In the case of samp server, when trying to send NaN, for example, to the player or his vehicle position, such packet will simply be blocked and if he continues to send it many times, he will become afk for other people for a while. At the same time, on omp server these values will be changed to 0.0, so that the player will be fake teleported to zero coordinates (on foot, with his vehicle or other vehicle/trailer he's sending an update for), which can be once again used as some loophole on those servers that have very weak anti-teleport or no anti-teleport at all. Not to mention the fact that this is generally a significant difference in behavior with samp server in cases where both of them can filter such invalid values by default.

[AmyrAhmady]

Most of the ones in first comment is done, only those that are already done due to how open.mp is written or causing problems are stayed out For stuff in second comment, it should be discussed

[NexiusTailer]

Judging by the commits published and visible from discord, the following validation checks were considered as possibly causing problems or maybe a little misunderstood and as a result, such conclusions were drawn. Anyway I would like to describe them more clearly if this can help to better understand their degree of reliability (because I doubt it was fixed till the current build before, so I think it was skipped from a list for some reasons above):

Block any unoccupied sync if playerid updates a vehicle from passenger seat, but he seats on a different passenger seat in the same vehicle

The source of the problem is when a player sends passenger sync with n seat (let's assume it's seatid 3) but unoccupied sync is sent with a different seat (let's assume it's seatid 2). Any problems could appear if we would check seatid from GetPlayerVehicleSeat which is < 0 (player is not in vehicle) and unoccupied sync with valid passenger seat, as any such cases might occur when player just left unoccupied vehicle. That's not our case because proposed checks should validate if both seats are valid passenger seats and they don't match.

---

Block any incoming SCMEvent RPCs (EnterExitModShop part of it) if it was sent outside of the reported vehicle (playerid isn't a driver of this vehicleid)

This issue were checked exactly on omp server and it was possible to call this RPC not being as this vehicle driver. I think it should be double-checked as there was no any commits related to SCMEvent validation.

---

Block any incoming MenuSelect and MenuQuit RPCs (OnPlayerSelectedMenuRow and OnPlayerExitedMenu) if no any menu is shown for player (including when player has already responded to the menu and now it should've been closed) or reported row (for RPC MenuSelect) isn't valid

Both omp and samp server blocks these RPCs called when no menu was shown at all before for that player, but if any menu was shown and closed by him or by the server, this menu could be spoofed later like it is still shown for that player.

---

Block any incoming EnterEditObject RPCs (OnPlayerSelectObject) if playerid is not in selection mode (SelectObject isn't set or it should've been cancelled) Block any incoming EditObject RPCs (OnPlayerEditObject) if playerid is not in edit mode for that object (EditObject is set for other objectid to edit)

Exactly the same case for EnterEditObject RPC like with menus just above. One thing I can add about EditObject RPC is that there were also no validations if I started edit object ID 5, but send spoofed data for object ID 16. It just checks if I'm in edit mode without being tied to a specific object that the server allowed me to edit.

---

Block any incoming EditAttachedObject RPCs (OnPlayerEditAttachedObject) if reported boneid is invalid Block any incoming EditObject RPCs (OnPlayerEditObject) if reported response type is invalid

This was definitely checked on omp and those parameters was successfully spoofed with invalid values. Also didn't appeared in the commits history, so I think was skipped for some reason.

Well, these seem to be all the main things that I suspect have been left out for reasons that are not very clear for me now. As for the rest, have no questions yet.

[AmyrAhmady]

Last 3 are done, either in new commits or were already done in open.mp

but about first and second ones, somehow I missed the first 😄 , and second one will be done

[AmyrAhmady]

These are all done now in build 6 (except NaN values, which like I said before will be discussed in team's chat to see what's the best approach) Thanks for reporting these detailed issues, helped us a lot 👍🏻