oscar-broman / samp-weapon-config

A more consistent and responsive damage system with many new features
Apache License 2.0
91 stars 84 forks source link

Knife "exploit" #238

Closed Zbyss closed 1 year ago

Zbyss commented 2 years ago

Client can spoof OnPlayerGiveDamage with 0.0 knife damage and insta-kill every player on the server. There are no checks if the player even has a knife, has it as an armed weapon, if attacked player is streamed or any delay between knife kills.

Proof: https://streamable.com/tc6vjd (first client has disabled textdraws, but damage given textdraw is also visible there) image

criticalerror99 commented 2 years ago

Yes my server was attacking by SobFoX and by this method. I think it's important to fix this immediately.

NexiusTailer commented 2 years ago

Shouldn't this be limited to range (distance) checks for melee weapons? By this check: https://github.com/oscar-broman/samp-weapon-config/blob/master/weapon-config.inc#L4661-L4670 ProcessDamage, in turn, is called from OnPlayerGiveDamage: https://github.com/oscar-broman/samp-weapon-config/blob/master/weapon-config.inc#L3246

UPD: Forgot about additional knife checks which are applied before ProcessDamage and that's probably the cause: https://github.com/oscar-broman/samp-weapon-config/blob/master/weapon-config.inc#L3165-L3166

So, I think main damage processing with all validations should be moved before this knife-specific condition.

ADRFranklin commented 2 years ago

https://github.com/oscar-broman/samp-weapon-config/blob/6432ea41c877da8c29e9157200ab4f48a62b92d6/weapon-config.inc#L3239

Moving the within streaming distance of player, above the knife stuff, should prevent all players on the server being hit by this. Ofc an additional check should be made to make sure the player doing the damage is within distance of the player as well as actually having a knife.

Zbyss commented 2 years ago

There also needs to be a check on the player state, as currently the player can kill anyone spectating him. https://streamable.com/4l17ed

NexiusTailer commented 2 years ago

@Zbyss Does it have any practical sense? I mean, a spectating person spawns immediately after this kind of "death" after this damage or not? If it is, then this may concern not only knife damage, but any other as well.

Zbyss commented 2 years ago

@NexiusTailer Yes, the spectator spawns like after a regular death. I have no idea if it's relevant also to the other weapons, because they can't be simply spoofed by sending GiveDamage packets. Probably something like a Damager would work, but I have no idea how to test it on a spectator.

NexiusTailer commented 1 year ago

I finally found time to check and fix all currently relevant problems (including unsolved part of this one) and can say that GiveDamage cannot be spoofed for any weapon (including knife damage) for this reason: https://github.com/oscar-broman/samp-weapon-config/blob/9f6f2b7e497e1a8eca6b8facc5ea72cec83b868b/weapon-config.inc#L3223-L3230

^ (this is inside OnPlayerGiveDamage callback)

WC_IsPlayerSpawned, in turn, have this check: https://github.com/oscar-broman/samp-weapon-config/blob/9f6f2b7e497e1a8eca6b8facc5ea72cec83b868b/weapon-config.inc#L1033-L1038

So, if player sends GiveDamage not being in onfoot/driver/passenger state, this will be blocked. However, a cheater can still send TakeDamage being in spectating just like you said before and this will be fixed in coming PR with some other minor improvements.