citizenfx / fivem

The source code for the Cfx.re modification frameworks, such as FiveM, RedM and LibertyM, as well as FXServer.
https://cfx.re/
3.57k stars 2.11k forks source link

State bags set on the server do not respect replicated flag #2438

Open jxckUK opened 7 months ago

jxckUK commented 7 months ago

What happened?

As reported originally back in 2021 by @AvarianKnight here https://forum.cfx.re/t/server-side-entity-state-bags-sync-when-replicate-is-false/2313177 and seems it was never opened here.

I ran into this issue and did some research and found the original report.

I would argue that this is a security issue because currently when scripting developers are setting this value to be false there could be an expectation that its working correctly, therefore data stored with the intent to not replicate in fact is replicating which would be a data security issue.

Expected result

It should respect the replicated flag being set to false and not replicate the state to the clients.

Reproduction steps

  1. Create an entity or player state server side and set the replicated flag to false.
  2. Query the state client side and see that the data is replicated to other players when entering their scope.

There is also a reproduction script in @AvarianKnight's original report.

Importancy

Security issue

Area(s)

OneSync

Specific version(s)

FiveM, Server 7752, Linux

Additional information

No response

tens0rfl0w commented 7 months ago

Simply using:

Player(source).state:set("test", "hi", false)

works as expected and does not replicate the state bag value to any client. (Same for entity state bags)

So there have to be other factors to make this not work as expected.

AvarianKnight commented 7 months ago

iirc this requires you to send a replicated value afterwards and/or re-enter the entities/player scope.

The underlying implementation for state bags doesn't have anything to respect the replication flag sent by the caller after the first call

https://github.com/citizenfx/fivem/blob/b5ed6d642b65e83bbcdc9e64af2b5494f2f1255a/code/components/citizen-scripting-core/src/ResourceScriptFunctions.cpp#L320-L340

https://github.com/citizenfx/fivem/blob/b5ed6d642b65e83bbcdc9e64af2b5494f2f1255a/code/components/citizen-resources-core/src/StateBagComponent.cpp#L175-L178

https://github.com/citizenfx/fivem/blob/b5ed6d642b65e83bbcdc9e64af2b5494f2f1255a/code/components/citizen-resources-core/src/StateBagComponent.cpp#L234-L269

tens0rfl0w commented 7 months ago

Looking at @AvarianKnight's repro from the initial issue: Replication state does seem to get ignored when calling a 'set' operation on entities too fast after init creation.

Simply waiting a few ticks after entity creation correctly applies the replication state to the state bag key.

For example, waiting 100 ticks after calling CreateAutomobile and another 100 ticks after the two 'set' operations then correctly replicates the state bag values: (second wait is just to ensure values got synced to the client) image

(This also works if setting a replicated state bag key on the same entity afterwards.)

AvarianKnight commented 7 months ago

This ties into the original issue of

The underlying implementation for state bags doesn't have anything to respect the replication flag sent by the caller after the first call

When the state bag is initially created it doesn't send the updates to anyone until the client has said that it has the entity created, this will call AddRoutingTarget which will sync all the key/values to the client (without respect for the replication flag)

https://github.com/citizenfx/fivem/blob/b5ed6d642b65e83bbcdc9e64af2b5494f2f1255a/code/components/citizen-server-impl/src/state/ServerGameState.cpp#L1815-L1826

tens0rfl0w commented 7 months ago

Verified exactly what you just described: Replication state is only applied when the client is in scope of the entity/the entity already exists on the client before the 'set' operation is executed.👍🏼Never noticed that before.

jxckUK commented 7 months ago

Thanks both for the much more detailed insight into the issue than I would have been able to provide!

and/or re-enter the entities/player scope.

This particular issue is the one that alerted me to the problem, delaying the set operation in this instance won't provide a valid mitigation (at least for Player state) because the players within a users scope is subject to change at any time.

I assume this doesn't affect states set on the client side which are set to not replicate by default? Or are they also affected by the underlying implementation not respecting replication flags post creation/first call?