multitheftauto/mtasa-blue

Add event onPlayerElementDataThreshold

Closed this issue · 8 comments

Is your feature request related to a problem? Please describe.

There is currently no protection against clients spamming setElementData
if a player sets random element data on all elements very quickly it can cause a lot of network usage and lag the server, sometimes even crash it when there are many elements

Describe the solution you'd like

Add a new event onPlayerElementDataThreshold that works like onPlayerTriggerEventThreshold but for setElementData
it should be triggered when a player spams too often in a short time

Describe alternatives you've considered

/

Additional context

While spamming setElementData can be monitored using onElementDataChange, this method is cumbersome and inefficient
adding the proposed onPlayerElementDataThreshold event would provide a much simpler and more reliable solution for detecting and handling such cases

Security Policy

  • I have read and understood the Security Policy and this issue is not about a cheat or security vulnerability.

If a cheater is able to set element data on all elements, then something is very wrong with your server’s security.

If you mean the client side with the sync argument set to true to flood the server with packets, there’s also a solution for that now. You can use the clientChangesPolicy argument in setElementData and set it to "deny", so the client cannot modify that element data from their side.

Try to keep as much as possible on the server side. If you have element data that absolutely must be synchronized, you could consider using syncMode as subscriber, so it’s only synced with those who really need it.

However, if you absolutely must synchronize element data across the entire server, then such an event might be a reasonable solution - but I’m not sure. Some people might use setElementData in onClientRender and what then? Of course, doing that with sync set to true is extreme stupidity, but such cases exist. In that situation, sending the event on the server side could probably produce false positives. It’s also hard to define a rational threshold for this type of event.

If a cheater is able to set element data on all elements, then something is very wrong with your server’s security.

If you mean the client side with the sync argument set to true to flood the server with packets, there’s also a solution for that now. You can use the clientChangesPolicy argument in setElementData and set it to "deny", so the client cannot modify that element data from their side.

Try to keep as much as possible on the server side. If you have element data that absolutely must be synchronized, you could consider using syncMode as subscriber, so it’s only synced with those who really need it.

However, if you absolutely must synchronize element data across the entire server, then such an event might be a reasonable solution - but I’m not sure. Some people might use setElementData in onClientRender and what then? Of course, doing that with sync set to true is extreme stupidity, but such cases exist. In that situation, sending the event on the server side could probably produce false positives. It’s also hard to define a rational threshold for this type of event.

It’s not necessary for the data to already exist, a cheater can simply create a new one
Exapmle:

local elements = { "vehicle", "player", "object" --[[and more]] }
for _, v in ipairs(elements) do
    for _, element in ipairs(getElementsByType(v)) do 
        setElementData(element, randomString(), true, true)
    end
end

And this would result in the same situation i described above

I understand your point about setElementData into onClientRender, I think it might be helpful if we added a note in the Script Security Guidelines for example we could clarify the correct and safe way of using setElementData, so scripters don’t accidentally create vulnerabilities in their resources.

If a cheater is able to set element data on all elements, then something is very wrong with your server’s security.
If you mean the client side with the sync argument set to true to flood the server with packets, there’s also a solution for that now. You can use the clientChangesPolicy argument in setElementData and set it to "deny", so the client cannot modify that element data from their side.
Try to keep as much as possible on the server side. If you have element data that absolutely must be synchronized, you could consider using syncMode as subscriber, so it’s only synced with those who really need it.
However, if you absolutely must synchronize element data across the entire server, then such an event might be a reasonable solution - but I’m not sure. Some people might use setElementData in onClientRender and what then? Of course, doing that with sync set to true is extreme stupidity, but such cases exist. In that situation, sending the event on the server side could probably produce false positives. It’s also hard to define a rational threshold for this type of event.

It’s not necessary for the data to already exist, a cheater can simply create a new one Exapmle:

local elements = { "vehicle", "player", "object" --[[and more]] }
for _, v in ipairs(elements) do
for _, element in ipairs(getElementsByType(v)) do
setElementData(element, randomString(), true, true)
end
end
And this would result in the same situation i described above

I understand your point about setElementData into onClientRender, I think it might be helpful if we added a note in the Script Security Guidelines for example we could clarify the correct and safe way of using setElementData, so scripters don’t accidentally create vulnerabilities in their resources.

Hmm, you’re right. A cheater can create any element data and send it to the server without any consequences. In that case, maybe we should consider two solutions:

  • An option in mtaserver.conf to completely disable client-side element data synchronization. That is, if this option is turned off, packets from the client will simply be ignored.

  • An onElementDataCreated event or something similar, which would be triggered if the server receives an element data key that doesn’t already exist and was created via a client-side sync packet. Although I’m not sure if this is a good idea, because it could lead to event flooding, either intentionally by cheaters or through careless use of setElementData.

That’s exactly why I suggested the idea of an onPlayerElementDataThreshold event
But I also think your second option could be a good solution as well.

https://wiki.multitheftauto.com/wiki/Server_mtaserver.conf#elementdata_whitelisted

Just set it to "1" and you're all good. Also, follow FileEX's advice.

https://wiki.multitheftauto.com/wiki/Server_mtaserver.conf#elementdata_whitelisted

Just set it to "1" and you're all good. Also, follow FileEX's advice.

But does this also work on element data that doesn’t exist on the server side?
It seems to me this only applies to modifications of already existing element data

Yes, it works just like that. If the data doesn't exist on the server-side, the client will not be able to send the packet.

The client will only be able to, if it's previously set on server-side AND clientChangesPolicy is set to "allow".

Yes, it works just like that. If the data doesn't exist on the server-side, the client will not be able to send the packet.

The client will only be able to, if it's previously set on server-side AND clientChangesPolicy is set to "allow".

I didn’t notice that part and i actually thought it worked the same way as FileEX described thanks for clarifying