mtasa-blue
mtasa-blue copied to clipboard
multitheftauto
Add more utility to deal with blowVehicle abuse
Is your feature request related to a problem? Please describe.
Hello,
With the recent increase of bad people abusing with custom code inside MTA:SA, one of the most used functions to cause mayhem is blowVehicle. Since this is a shared function, clientside execution gets synced with the server, resulting in easy way to blow up all server vehicles.
Describe the solution you'd like
I propose a few ideas to improve the abusive situation by expanding upon blowVehicle:
-
Add an event like onPlayerVehicleBlowThreshold which gets triggered when the client tries to blow up too many vehicles at given time. Source would be player, vehicle the event parameter.
-
Prevent client from using blowVehicle on vehicle elements which are in a different dimension or streamed out. This could be a problem regarding backwards compatibility but could greatly reduce abusive ways if simply looping all vehicle elements.
-
Add an event like onPlayerVehicleBlowUp which gets triggered when client uses blowVehicle. Source is the player, vehicle the parameter. That way server could write its own checks against abusive use and deal with malicious player.
-
Change clientside blowVehicle to be only used if the client is the active syncer of to be blow up vehicle. Again this might be impossible to do due to backwards compatibility reasons but hey I just throw it in.
Describe alternatives you've considered
No response
Additional context
No response
Security Policy
- [X] I have read and understood the Security Policy and this issue is not about a cheat or security vulnerability.
I would like to work on solving the problem, but first we should agree on which solution to choose based on the opinion of the community, so I am waiting for everyone's opinion on this issue.
Imo the syncer check should definitely be there, but it would also be great to have an event for it that you can cancel like onExplosion or onPlayerProjectileCreation. Threshold isn't necessarily a bad idea either, but there might be problems if a client loses connection for a small amount of time (bigger than interval) then they could potentially send multiple vehicle blow packets. Not sure how they are synced exactly, I guess they are their own packets like with explosions for example, so these solutions are possible I think. Good luck with the implementation @Nico8340 ! Nice idea!
Thanks for sharing your opinion @Rilot06!
I'm still waiting for other people's opinions on this. If I were to do this, there would be only one problem - and that would be backwards compatibility, but I think it's a risk that can be ignored at the cost of security, similar to #3391.
Thanks for sharing your opinion @Rilot06!
I'm still waiting for other people's opinions on this. If I were to do this, there would be only one problem - and that would be backwards compatibility, but I think it's a risk that can be ignored at the cost of security, similar to #3391.
Yep, but I don't think many scripts rely on client side blowVehicle, even if they are, they should be fixed immediately because of the security concerns. If the concern of backwards compatibility comes up often with this, you could make the syncer check toggle-able as an mtaserver.conf parameter or the Lua API perhaps?
Even Sphene - which depends on actions being shown as quickly as possible - handles its vehicle explosion logic server-side. For the sake of security let's make it so the server has to validate that the explosion is valid with the ability to cancel it. A syncer check may not be entirely possible as you need to be able to blow up vehicles driven by other players as well.
However, a simple configurable radius check will go a long way. Ideally we'd also verify the underlying cause of the explosions (low hp on vehicle? projectile?) but that may not be as viable right now...
I think we should first wait until #3391 is merged, and then come back to it, since this pr would use the same settings for radius check.
A syncer check may not be entirely possible as you need to be able to blow up vehicles driven by other players as well.
This may be a stupid question, but if there is a driver, doesn't he syncs the vehicle explosion from other synced packets and factors, like collision, other players' RPG projectiles, etc? If not, then you are right, I didn't think about this.
Edit: I may have worded it pretty badly, so here's an example: There are 2 players, A and B, you are player A. If player B is the syncer of a vehicle and you shoot an RPG towards the vehicle, you send a projectile sync packet, that gets to player B. Shouldn't player B's client side vehicle explosion from the synced projectile get synced back to the server since he is the synced?
Edit: I may have worded it pretty badly, so here's an example: There are 2 players, A and B, you are player A. If player B is the syncer of a vehicle and you shoot an RPG towards the vehicle, you send a projectile sync packet, that gets to player B. Shouldn't player B's client side vehicle explosion from the synced projectile get synced back to the server since he is the synced?
That's actually a good question. This should be figured out first.
That's actually a good question. This should be figured out first.
I'm on mobile rn, so I can't check the source code to be sure, but it seems logical to me if ONLY the syncer of a vehicle sends vehicle explosion packets to the server from their client side's explosions based on other factors and things happening, since the client is aware that they are the syncer.
Edit: Talked with @Nico8340 in private, he said he'll check this out from the source code later on before implementing anything.
I think the best way would be for the server developer to be able to disable some client-side functions. Nevertheless, a potential cheater could inject his code and send an explosion packet to the server, but then the server would store information about which client functions are blocked by the server itself and ignore incoming explosions, blowing, etc. synchronization packets.
I think the best way would be for the server developer to be able to disable some client-side functions. Nevertheless, a potential cheater could inject his code and send an explosion packet to the server, but then the server would store information about which client functions are blocked by the server itself and ignore incoming explosions, blowing, etc. synchronization packets.
But AFAIK there is no difference between blowVehicle packet and a simple, natural vehicle explosion packet from low HP for example. Even if there was(/is), a cheater could still send a vehicle explosion packet that looks like a natural explosion from low vehicle HP.
I've done some initial digging and it looks like the explosion itself gets synced. The server then appears to sync that explosion further but will first put the vehicle into a blown state.
So, whoever (most likely the player who shot it) triggered the explosion of the projectile, which triggers the game to explode the vehicle, giving us data about the explosion thereafter seems to be syncing the vehicle blown state and not necessarily the syncer of the vehicle themselves.
Most of this happens in CGame::Packet_ExplosionSync in Server\mods\deathmatch\logic\CGame.cpp
Just a note, but IIRC onVehicleExplode will be triggered in every case (also before the 'vehicle has exploded' packet from a client gets distributed to everyone). The point of 'vehicle blow state' was to prevent a desync bug, where a vehicle would continue to blow up on every frame; and to prevent explode event trigger spam - again, iirc, blow state is not synchronized.
Just so that we are all clear, blowVehicle triggers onExplosion server event. If the serversided event is not cancelled then both onClientExplosion and onClientVehicleExplode will be triggered clientside.
So this type of cheat can be fought with a simple check using onExplosion serverside. You should take into consideration that Helicopters and Planes tend to create a lot of explosions. Also you should consider that some explosions are created for destroying objects (like gas stations) and are also passed via onExplosion.
I've done some initial digging and it looks like the explosion itself gets synced. The server then appears to sync that explosion further but will first put the vehicle into a blown state.
So, whoever (most likely the player who shot it) triggered the explosion of the projectile, which triggers the game to explode the vehicle, giving us data about the explosion thereafter seems to be syncing the vehicle blown state and not necessarily the syncer of the vehicle themselves.
Most of this happens in
CGame::Packet_ExplosionSyncinServer\mods\deathmatch\logic\CGame.cpp
Ohh yeah, from my understanding of this line the sync only happens if you are responsible for the explosion, weird.
Realtalk: What is the point of the blowVehicle function on the client side? A function that can make any vehicle explode regardless of the player's position. It could work well if there were no cheaters, but it makes no sense like this.
Wouldn't any cheater just switch over to setElementHealth once blowVehicle stops being an option or does that function have additional verification that makes it less prone to abuse?
Yeah... In a more secure world, client should not be able to use functions like setElementHealth, setElementPositon, blowVehicle on elements if he aint the explicit syncer of said element.
But this will for sure break backwards compatibility....
Yeah... In a more secure world, client should not be able to use functions like setElementHealth, setElementPositon, blowVehicle on elements if he aint the explicit syncer of said element.
But this will for sure break backwards compatibility....
We had some discussion related to setElementPosition syncing. Sphene relies on it heavily, besides that, this function is used for noclip & ladders system. There was a proposal which wouldn't break backwards compatibility, shortly: a server-side function which would give opportunity for server owner to disable syncing client-side setElementPosition calls per element. By default it would be false (to preserve BWC). You could toggle it on/off when necessary. I'd advice to keep solution consistent with other functions as well. Like here: https://github.com/multitheftauto/mtasa-blue/issues/3245#issuecomment-1817834222, global toggle would bring only issues.
Yeah... In a more secure world, client should not be able to use functions like setElementHealth, setElementPositon, blowVehicle on elements if he aint the explicit syncer of said element. But this will for sure break backwards compatibility....
We had some discussion related to setElementPosition syncing. Sphene relies on it heavily, besides that, this function is used for noclip & ladders system. There was a proposal which wouldn't break backwards compatibility, shortly: a server-side function which would give opportunity for server owner to disable syncing client-side setElementPosition calls per element. By default it would be false (to preserve BWC). You could toggle it on/off when necessary. I'd advice to keep solution consistent with other functions as well. Like here: #3245 (comment), global toggle would bring only issues.
Yeah... In a more secure world, client should not be able to use functions like setElementHealth, setElementPositon, blowVehicle on elements if he aint the explicit syncer of said element. But this will for sure break backwards compatibility....
We had some discussion related to setElementPosition syncing. Sphene relies on it heavily, besides that, this function is used for noclip & ladders system. There was a proposal which wouldn't break backwards compatibility, shortly: a server-side function which would give opportunity for server owner to disable syncing client-side setElementPosition calls per element. By default it would be false (to preserve BWC). You could toggle it on/off when necessary. I'd advice to keep solution consistent with other functions as well. Like here: #3245 (comment), global toggle would bring only issues.
To build upon the proposal. If we were about to add such a server setting, this setting then should include all setElement* functions callable via client which could harm other players. If the client wants to change for example the position of another element hes not syncer of, an event should fire with all necessary information and server could decide on cancelling the sync.
Like onPlayerElementManipulate, source is the player wanting to sync new changes about the element hes not syncer of, parameter would be the function name and element(s) to manipulate.
The same for setPlayer* and setPed* functions as well.
Realtalk: What is the point of the blowVehicle function on the client side? A function that can make any vehicle explode regardless of the player's position. It could work well if there were no cheaters, but it makes no sense like this.
It doesn't matter if there is a blowVehicle function on the client side or not, since it sends the same explosion packet as a natural vehicle explosion from low veh HP or something else. Cheaters can just send a fake vehicle explosion packet if there is no blowVehicle function.
Realtalk: What is the point of the blowVehicle function on the client side? A function that can make any vehicle explode regardless of the player's position. It could work well if there were no cheaters, but it makes no sense like this.
It doesn't matter if there is a blowVehicle function on the client side or not, since it sends the same explosion packet as a natural vehicle explosion from low veh HP or something else. Cheaters can just send a fake vehicle explosion packet if there is no blowVehicle function.
Yes, but it makes a difference whether there is an extra function for this or whether you send a fake packet, but in both cases there is no validation. It is more difficult to send a fake packet.
Yes, but it makes a difference whether there is an extra function for this or whether you send a fake packet, but in both cases there is no validation. It is more difficult to send a fake packet.
It would work against some cheaters, but I don't think it's worth breaking backwards compatibility by removing a function. Many cheats can send fake packets already easily, so it would only make the problem a bit smaller, still wouldn't solve it. Why bother with a half (or less than half) solution that breaks backwards compatibility when there is a way to solve the actual problem?
Yeah... In a more secure world, client should not be able to use functions like setElementHealth, setElementPositon, blowVehicle on elements if he aint the explicit syncer of said element. But this will for sure break backwards compatibility....
We had some discussion related to setElementPosition syncing. Sphene relies on it heavily, besides that, this function is used for noclip & ladders system. There was a proposal which wouldn't break backwards compatibility, shortly: a server-side function which would give opportunity for server owner to disable syncing client-side setElementPosition calls per element. By default it would be false (to preserve BWC). You could toggle it on/off when necessary. I'd advice to keep solution consistent with other functions as well. Like here: #3245 (comment), global toggle would bring only issues.
While Sphene heavily relies on setElementPosition client-side for server-side vehicles over large distances we do so using setElementSyner(element, player, true) (using the persist parameter). This at least makes it so that the player is in fact the syncer of the vehicle. It that sense having syncer checks should be fine and scripters could adapt to actively making use of the persist parameter
In all honesty, we should break backwards-compatibility to fix this:
Proposal: Clientside blowVehicle should only work on local vehicles (createVehicle clientside) - these actually can't even be entered, and are usually for aesthetic purposes or temporary usage (e.g. tuning garage preview). This prevents clients from blowing up serverside vehicles that are synced for everyone. If a developer wants to do it, then it should happen in a serverside script. There is no situation where a developer cannot do blowVehicle serverside for their use-case.
Alternatives: Any other solution to add methods for detecting cheating or strange usage of blowVehicle clientside will just give developers additional headaches and work, in my opinion.
Impact:
The only official/default resource that ever does blowVehicle clientside is race (https://github.com/multitheftauto/mtasa-resources/blob/master/%5Bgamemodes%5D/%5Brace%5D/race/race_client.lua#L594) if the map has "firewater" setting enabled. This could refactored.
Clientside blowVehicle should only work on local vehicles (createVehicle clientside) - these actually can't even be entered, and are usually for aesthetic purposes or temporary usage (e.g. tuning garage preview). This prevents clients from blowing up serverside vehicles that are synced for everyone.
If a cheater can execute Lua function then he can just explode a vehicle without using the function.
Since this discussion came up again recently in discord, the situation remained same if not got worse for server. The fundamental issue here is server trusts the client too much in regards to network sync packets while the server operators cannot really do much other than listening to lua events like onExplosion, onVehicleExplosion and onPlayerProjectileCreation and implementing some artificial threshold.
I would propose a more strict control and ability for servers to disregard certain sync packets by clients.
- Disregard vehicle explosion packets by clients if they are not the active syncer.
- Make onVehicleExplode event cancellable by server to avoid syncing it for all others.