multitheftauto / mtasa-blue

Multi Theft Auto is a game engine that incorporates an extendable network play element into a proprietary commercial single-player game.
https://multitheftauto.com
GNU General Public License v3.0
1.38k stars 424 forks source link

Security enhancement #3669

Open gownosatana opened 3 weeks ago

gownosatana commented 3 weeks ago

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

It's really easy to reproduce any resource root element using getResourceRootElement + getResourceFromName, which is very dangerous and can be exploited by cheaters to fake triggering events

Describe the solution you'd like

Remove completely getResourceRootElement on client side or at least make this function possible to disable in server config

Additional context

triggerServerEvent("some event", getResourceRootElement(getResourceFromName("my-resource")))

Security Policy

TracerDS commented 3 weeks ago

you have the client variable. You can check if client tried to fake the event.

gownosatana commented 3 weeks ago

you have the client variable. You can check if client tried to fake the event.

Im not sure if you know what you're talking about or did read whole post, but I don't mean player that triggers the event but triggering events cross-resource.

TracerDS commented 3 weeks ago

Im not sure if you know what you're talking about or did read whole post, but I don't mean player that triggers the event but triggering events cross-resource.

That in itself is not an issue. Triggering events cross-resource is actually very useful. Especially on client You know you can do the same with root, right?

CrosRoad95 commented 3 weeks ago

First of all "addEventHandler" to handle build in, events from client/server across resources is big nono for me. You should have "addRemoteEventHandler" at least because i have seen in past many times people were doing addEvent("onPlayerJoin", true) in server side code. Also, if resourceA from client can trigger server side event from resourceB is it also big nono for me, i compare it to using javascript fetch("sa-mp.com/someurl") from mtasa.com domain, browser require sa-mp.com to allow requests from mtasa.com - this "cross resource remote triggers" should be configurable and possible to disable.

Remove completely getResourceRootElement

something like: getElementParent(rootElement) -> getElementParent -> getElementChildren and you get your roots back

gownosatana commented 3 weeks ago

Im not sure if you know what you're talking about or did read whole post, but I don't mean player that triggers the event but triggering events cross-resource.

That in itself is not an issue. Triggering events cross-resource is actually very useful. Especially on client You know you can do the same with root, right?

Just read the MTA wiki page about TriggerServerEvent https://wiki.multitheftauto.com/wiki/TriggerServerEvent

resourceRoot can also be used as alternative choice, if addEventHandler is bound to root element, or to resourceRoot when there is need to restrict event to single certain resource (although cheater could still trigger it from different resource, by using getResourceRootElement and passing respective resource root element)

matchingSource = (source == resourceRoot) -- check whether source element (2nd argument in triggerServerEvent) passed from client was resourceRoot```

THIS can be faked

TracerDS commented 3 weeks ago

I know about this note, but does it also include sourceResource and sourceResourceRoot? If not then you can use these to check against source

source -- The player or element the event was attached to
sourceResource -- the resource that called the event
sourceResourceRoot -- the root of the resource that called the event

If cheater can still bypass sourceResource then that is an issue, not crossresource event triggering.

CrosRoad95 commented 3 weeks ago

Remember long time ago i discussed with botder possibility to make resourceA elements are readonly for rest of the resources, probably it wont help much but maybe this idea can be revisited with focus on cheaters.

gownosatana commented 3 weeks ago

I know about this note, but does it also include sourceResource and sourceResourceRoot? If not then you can use these to check against source

source -- The player or element the event was attached to
sourceResource -- the resource that called the event
sourceResourceRoot -- the root of the resource that called the event

If cheater can still bypass sourceResource then that is an issue, not crossresource event triggering.

sourceResource is NOT a variable for events from client side

addEvent('my-event', true)
addEventHandler('my-event', resourceRoot, function()
    print(sourceResourceRoot) -- nil
end)
TracerDS commented 3 weeks ago

well that is an issue then.

gownosatana commented 3 weeks ago

I think that will be enough to add sourceResourceRoot variable for events from client side

TheNormalnij commented 3 weeks ago

I think that will be enough to add sourceResourceRoot variable for events from client side

it doesn't add any security. Any resource can be injected. You should be ready for that.

TracerDS commented 3 weeks ago

Can we do the same thing client does but for sourceResource and sourceResourceRoot?

TheNormalnij commented 3 weeks ago

A server guarantees only client variable.

TracerDS commented 3 weeks ago

Right now, yes. But every client script runs in a resource. It could be retrieved, no?

TheNormalnij commented 3 weeks ago

How?

TracerDS commented 3 weeks ago

I'd assume in the same way client is retrieved. But you dont fetch the player but the resource.

TheNormalnij commented 3 weeks ago

I can be wrong, but a server set client variable by using a client connection socket.

TracerDS commented 3 weeks ago

I can be wrong, but a server set client variable by using a client connection socket.

Yeah it is. I wonder if a value can be assigned to that socket. If so, we could generate uid for each resource for each player. Then get the uid and convert it to a resource ig?

gownosatana commented 3 weeks ago

I can be wrong, but a server set client variable by using a client connection socket.

Yeah it is. I wonder if a value can be assigned to that socket. If so, we could generate uid for each resource for each player. Then get the uid and convert it to a resource ig?

No, it can't be done, resources are just memory and not socket connections like players. It still could be faked with more advanced cheats but simple cheats and lua executors wouldn't be able to fake it. Also not talking only about cheats, executing code with backdoors could make it impossible to impersonate resource. sourceResource and sourceResourceRoot should be added as soon as possible, it's crazy that it's not here yet.