Lack of state bag rate limiting

[niekschoemaker]

What happened?

Somebody set numerous (100+) statebags on an entity with payloads of around a megabyte each on an entity.
This in turn causes the server to crash because this data has to be networked to all players around them.

Expected result

Instead of a server crash the state bag update should have been rejected

Reproduction steps

1. Have a lot of players in one area
2. Set a state bag on a entity you have ownership over (so create a object)
3. Add statebag with random keys and random enormous payloads onto this entity
4. Wait for server to crash

Importancy

Crash

Area(s)

FXServer

Specific version(s)

Server 7290 Linux & Server 7339 Win32

Additional information

I would suggest a simple rate limiter the same way they are implemented on msgServerEvent would probably be enough to fix any issues regarding this, so a rate limiter for the number of events and one for the size of the payload.

Also having the possibility to cancel the AddStateBagChangeHandler somehow would really be appreciated to be able to add some more custom security regarding this, so being able to validate state bags with custom logic dependant on the server.

I'm not to familiair with the internals on how the rate limiters work exactly with the console context so that's the reason I'm creating an issue instead of a pull request.

[ghost]

Thanks for letting us know. We agree there is an issue and we’re looking into it.

[AvarianKnight]

Heres a temporary server-side fix for people to use during the meantime, tested minimally but should work fine :)

local function isValueTooLarge(val)
    local valType = type(val)
    if valType == "string" then
        -- if you send a string larger then this you're doing something very
        -- wrong
        return val:len() > 500
    elseif valType == "table" then
        -- we shouldn't be sending large data over state bags
        return msgpack.pack_args(val):len() > 5000
    end
    return false
end

local function rejectStateChange(caller, ent, state, key, curVal)
    -- reset the state bag back to its original value, this won't remove the key
    -- from the server but theres no way to remove a key currently anyways until
    -- https://github.com/citizenfx/fivem/pull/2108 is merged or someone
    -- partially applies these changes outside of this PR
    TriggerEvent("StateBagAbuse", caller, ent)
    DropPlayer(caller, "Reliable state bag packet overflow")
    -- we have to execute this after the change handler so we just wait a tick
    -- to set it back to its current value
    SetTimeout(0, function()
        -- server replicates by default, this will set the state back to the
        -- original value
        state[key] = curVal
    end)
end

AddStateBagChangeHandler("", "", function(bagName, key, value, source, replicated)
    -- global state isn't able to be set from the client
    if bagName == "global" then return end
    -- we're the ones that set this data, we don't want to possibly drop the
    -- player for it
    if not replicated then return end
    local ent
    local owner
    local state


    if bagName:find("entity") then
        ent = GetEntityFromStateBagName(bagName)
        owner = NetworkGetEntityOwner(ent)
        state = Entity(ent).state
    else
        ent = GetPlayerFromStateBagName(bagName)
        owner = ent
        state = Player(ent).state
    end

    -- get the current value, the value of the current state wont change until
    -- after the state bag change handler finishes
    local curVal = state[key]
    if type(key) == "string" then
        -- keys should never be above 20 characters long, if it is then reject
        -- and drop the owning player
        if key:len() > 20 then
            rejectStateChange(owner, ent, state, key, curVal)
        end
    end

    if isValueTooLarge(value) then
        rejectStateChange(owner, ent, state, key, curVal)
    end
end)

[mlsx64]

bagName

Does this not create the risk of abusing the check by changing statebags on entities owned by other players? I'm not sure whether there's a different way to convert the reserved/source value to a usable server ID..

[AvarianKnight]

You can't change the state bag if you're not the owner.

[mlsx64]

That clears some things up, I wasn't sure if this was the case while I was trying to work out a fix myself, thanks. 👍