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

jxckUK · 2024-03-27T15:45:35Z

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

Create an entity or player state server side and set the replicated flag to false.
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 · 2024-03-27T19:53:29Z

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 · 2024-03-27T19:59:58Z

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

fivem/code/components/citizen-scripting-core/src/ResourceScriptFunctions.cpp

Lines 320 to 340 in b5ed6d6

    
           fx::ScriptEngine::RegisterNativeHandler("SET_STATE_BAG_VALUE", [](fx::ScriptContext& context) 
        
           { 
        
           	auto bagName = context.CheckArgument<const char*>(0); 
        
           	auto keyName = context.CheckArgument<const char*>(1); 
        
           	auto keyValue = context.CheckArgument<const char*>(2); 
        
           	auto keySize = context.GetArgument<uint32_t>(3); 
        
           	auto replicated = context.GetArgument<bool>(4); 
        
           	auto rm = fx::ResourceManager::GetCurrent(); 
        
           	auto sbac = rm->GetComponent<fx::StateBagComponent>(); 
        
           	if (auto bag = sbac->GetStateBag(bagName); bag) 
        
           	{ 
        
           		bag->SetKey(0, keyName, std::string_view{ keyValue, keySize }, replicated); 
        
           		context.SetResult(true); 
        
           		return; 
        
           	} 
        
           	context.SetResult(false); 
        
           });

fivem/code/components/citizen-resources-core/src/StateBagComponent.cpp

Lines 175 to 178 in b5ed6d6

    
           auto continuation = [thisRef, source, replicated](std::string_view key, std::string_view data) 
        
           { 
        
           	static_cast<StateBagImpl*>(thisRef.get())->SetKeyInternal(source, key, data, replicated); 
        
           };

fivem/code/components/citizen-resources-core/src/StateBagComponent.cpp

Lines 234 to 269 in b5ed6d6

    
           void StateBagImpl::SetKeyInternal(int source, std::string_view key, std::string_view data, bool replicated) 
        
           { 
        
           	{ 
        
           		std::unique_lock _(m_dataMutex); 
        
           		if (auto it = m_data.find(key); it != m_data.end()) 
        
           		{ 
        
           			if (data != it->second) 
        
           			{ 
        
           				it->second = data; 
        
           			} 
        
           			else 
        
           			{ 
        
           				replicated = false; // nothing changed 
        
           			} 
        
           		} 
        
           		else 
        
           		{ 
        
           			m_data.emplace(key, data); 
        
           		} 
        
           	} 
        
           	if (replicated) 
        
           	{ 
        
           		if (m_replicationEnabled) 
        
           		{ 
        
           			SendKeyValueToAllTargets(key, data); 
        
           		} 
        
           		else 
        
           		{ 
        
           			// store it for now 
        
           			std::unique_lock lk(m_replicateDataMutex); 
        
           			m_replicateData.emplace(key, data); 
        
           		} 
        
           	} 
        
           }

tens0rfl0w · 2024-03-27T20:17:09Z

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)

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

AvarianKnight · 2024-03-27T20:22:49Z

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)

fivem/code/components/citizen-server-impl/src/state/ServerGameState.cpp

Lines 1815 to 1826 in b5ed6d6

    
           if (syncData.hasCreated) 
        
           { 
        
           	// Add this player as a routing target to this entity's statebag, if present. 
        
           	// notes: 
        
           	// * this will try to add it every frame, but the statebag will only add it once (std::set). 
        
           	// * will occur on the next update/tick when syncData.hasCreated is true, this'll ensure that it's sent after the client knows about this entity. 
        
           	// TODO: PERF: remove this every-frame call by giving this system a nice and fresh design 
        
           	if (auto stateBag = entity->GetStateBag()) 
        
           	{ 
        
           		stateBag->AddRoutingTarget(slotId); 
        
           	} 
        
           }

tens0rfl0w · 2024-03-27T20:41:51Z

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 · 2024-03-27T21:39:56Z

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?

* Previously, we didn't keep track of the replication state of a sbag key, resulting in sending all sbag data when a client entered the scope of an entity/player (regardless of the replication state). * We now store the replication state of a sbag key and check if we should replicate its data when sending all sbag data to the client. Fixes citizenfx#2438

jxckUK added bug triage Needs a preliminary assessment to determine the urgency and required action labels Mar 27, 2024

tens0rfl0w linked a pull request Apr 2, 2024 that will close this issue

fix(statebag): check for replication state on SendAll #2445

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jxckUK commented Mar 27, 2024

tens0rfl0w commented Mar 27, 2024 •

edited

AvarianKnight commented Mar 27, 2024 •

edited

tens0rfl0w commented Mar 27, 2024 •

edited

AvarianKnight commented Mar 27, 2024

tens0rfl0w commented Mar 27, 2024

jxckUK commented Mar 27, 2024 •

edited

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

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

Comments

jxckUK commented Mar 27, 2024

What happened?

Expected result

Reproduction steps

Importancy

Area(s)

Specific version(s)

Additional information

tens0rfl0w commented Mar 27, 2024 • edited

AvarianKnight commented Mar 27, 2024 • edited

tens0rfl0w commented Mar 27, 2024 • edited

AvarianKnight commented Mar 27, 2024

tens0rfl0w commented Mar 27, 2024

jxckUK commented Mar 27, 2024 • edited

tens0rfl0w commented Mar 27, 2024 •

edited

AvarianKnight commented Mar 27, 2024 •

edited

tens0rfl0w commented Mar 27, 2024 •

edited

jxckUK commented Mar 27, 2024 •

edited