Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event data amplification concerns #54

Open
Ekinoxx0 opened this issue Apr 10, 2022 · 4 comments
Open

Event data amplification concerns #54

Ekinoxx0 opened this issue Apr 10, 2022 · 4 comments

Comments

@Ekinoxx0
Copy link

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 :

  • Remove this feature if it doesn't see a lot of use
  • Replace this feature with preconfigured data vars
  • Verify for a valid data schema
  • Limit the sending of the event to certains clients in certains conditions only

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
Copy link
Owner

mkafrin commented Apr 11, 2022

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
Copy link
Owner

mkafrin commented Apr 16, 2022

@Ekinoxx0 Any thoughts on the above response?

@Ekinoxx0
Copy link
Author

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
Copy link

Korioz commented May 3, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants