search

mkafrin / PolyZone

PolyZone is a FiveM mod to define zones of different shapes and test whether a point is inside or outside of the zone
MIT License
199 stars 192 forks source link

Event data amplification concerns #54

Open Ekinoxx0 opened 2 years ago

Ekinoxx0 commented 2 years ago

Hi,

as dbub pointed at in a discord conversion, having an event that resend all data to all clients from a single event without any verification is bad pratices. Example : Some cheats just spam the event with massive chunk of data, the server just send it through all clients leading to network overflow.

Possible solutions :

I would have made a PR proposing some solution but I don't really understand what this event is for. (event from reading the original commit description i'm not sure why this feature exist in the first place) The resources I read before making this PR, useful for context :

An different solution would be to create server-side checks for PolyZone, it would actually be useful to implement PolyZone server-side now that we have full knowledge of the entities position on Onesync.

The goal of this issue is not to shame the implementation or usage of this resource, just to open a discussion about modifications on this useful resource.

mkafrin commented 2 years ago

Hmm, thanks for bringing it up. Unfortunate that some people felt enough concern to say it in discord but not post here. I don't use the CFX discord so I appreciate you mentioning it here.

To give a bit of background on why this was added: The server I work on (that a lot of PZ feature requests come from) wanted a way to trigger an event for people inside a particular zone. PZ was not able to be implemented server-side at the time and so a forwarding mechanism was required. Frankly, this was added over a year ago and I have never seen this exploited (and as you're probably aware, many very large servers use it). But I agree it could be exploited and agree we should avoid that.

I wouldn't want to remove it, because it's been useful for us on the server I work with (and I'm sure for others). I think the best way forward is probably just server-side validation. I have recently wanted to implement PZ server-side (shouldn't be too many changes) and so that might be the best option, though I would need to think about the best way to efficiently keep track of all the players.

On one hand, just keeping a list of what players are in what zones through a check of all players every second or so would work, and would make triggering zone events very cheap, but feels heavy running all the time when it might not be used often. Alternatively, it could just check and confirm what players are in the zone when a zone event was triggered, and that would avoid the cost most of the time, but would make triggering zone events a lot more expensive. I lean towards the latter, since zone events are not triggered often (at least on our server), but I'd like to hear your thoughts @Ekinoxx0 .

mkafrin commented 2 years ago

@Ekinoxx0 Any thoughts on the above response?

Ekinoxx0 commented 2 years ago

I think the better way to do it is just to give the choice to developers :) You could leave this server-side event system here for compatibility and allow it to be activated/desactived (I would suggest disabled by default until used)

I my head, it would be better to generalize the current PolyZone code so it could be run server side AND client side, then do checks wherever you need, then allow zone one by one to be "client-side driven" by events with a lot of checks (ratelimiter, whitelisted zones names, maximum table size in character count). But this could be hard to implement properly without affecting too much on performance.

All of this would probably need to be actually implemented to really see the result on different sized servers.

Korioz commented 2 years ago

Hi 👋, the complete implementation of PolyZone from server-side would be a great idea but need some work, for now i suggest that you delete the part that register the event as networked so we are still able to call it from server-side in future releases, this is not a tiny exploit as this allow even the sending of any arguments the client want to any clients.