Security Enhancement: Distance Check for 'esx:onPickup' Event

[Kenshiin13]

Security Enhancement: Distance Check for 'esx:onPickup' Event

Description

This pull request introduces a critical security update for the "esx:onPickup" event to mitigate potential abuses. The update incorporates a distance check mechanism, preventing players from exploiting the event to collect pickups from excessive distances. This measure ensures fair gameplay and maintains the integrity of the event.

Changes Made

• Store serverside pickup coords as a vector, eliminating the need for client-side conversion, and allowing for arithmetic operations on them.
• Implemented a distance check within the existing 'esx:onPickup' event handling code.
• If a player attempts to interact with a pickup beyond a specified distance (5.0 units), a warning is triggered, and the interaction is blocked.

The modified code snippet is as follows:

RegisterNetEvent('esx:onPickup')
AddEventHandler('esx:onPickup', function(pickupId)
    local pickup, xPlayer, success = Core.Pickups[pickupId], ESX.GetPlayerFromId(source)

    if pickup then
        local playerPickupDistance = #(pickup.coords - xPlayer.getCoords(true))
        if(playerPickupDistance > 5.0) then
            print(('[^3WARNING^7] Player Detected Cheating (Out of range pickup): ^5%s^7'):format(xPlayer.getIdentifier()))
            return
        end

        -- Rest of the original event handling code

        if success then
            Core.Pickups[pickupId] = nil
            TriggerClientEvent('esx:removePickup', -1, pickupId)
        end
    end
end)

Reasoning

This security update is vital to prevent potential abuses of the "esx:onPickup" event. The event could potentially be exploited by malicious players to trigger unauthorized pickups using a simple code snippet like this:

RegisterNetEvent("esx:createPickup", function(pickupId)
    TriggerServerEvent("esx:onPickup", pickupId)
end)

By implementing a distance check, the update ensures that players can only interact with pickups within a reasonable range, discouraging unauthorized collection of pickups from afar.

Counterargument Consideration:

It's worth addressing the potential counterargument that the distance check might be unnecessary due to a player's ability to teleport to pickups, thus eliminating the need for this change. However, it's essential to recognize that third-party anticheats are designed precisely to identify players employing teleportation tactics across the map. Additionally, the presence of one form of vulnerability should not overshadow addressing another potential attack vector.

Thank you for considering these enhancements. Your feedback and suggestions are appreciated.

[iSentrie]

this will require decrease in vector z, else player won't be able to pick anything from ground

https://github.com/Kenshiin13/esx_core/blob/aa1ea636e48656d8d928d1b633c7062bf52b872b/%5Bcore%5D/es_extended/client/main.lua#L387-L388

TriggerEvent('esx:createPickup', pickupId, pickup.label, pickup.coords - vector3(0, 0, 0.5), pickup.type, pickup.name, pickup.components, pickup.tintIndex)

however, who don't use ox_inventory, after client restart missing pickups will appear floating in the air, needs an better fix, but for now at least it will work. currently in last version missing pickups wasn't working at all.

[Kenshiin13]

this will require decrease in vector z, else player won't be able to pick anything from ground

Haven't tested that tbh. The pickups spawned properly on the ground when I tested the vulnerability. I can push another commit tho if requested.

[iSentrie]

i tested, that's why i wrote it.
with vector3(0, 0, 1.0) it will spawn on ground, yes, but you won't be normally able to pick it up.
your changes fixes the error, but there still issue to it, so decreasing that vector to vector3(0, 0, 0.5) makes you able to pick the item, with side effect that item is not on the ground anymore and floats in air a little.

[Kenshiin13]

your changes fixes the error

My changes have nothing to do with that.

[iSentrie]

your changes fixes the error

My changes have nothing to do with that.

without your changes missing pickups won't spawn at all (after a client restart), resulting in an error in client console.
you fixed it without knowing it.
it was fixed before by someone and then broken again by someone, now you fixed it again.

[Gellipapa]

@Kenshiin13 Hi! Could you write in to accept coordinates not just the current coordinate?
Example: I need to tell you that I want a pickup points xyz, because currently it only looks at your current coordinate to the best of my knowledge.

[Kenshiin13]

@Kenshiin13 Hi! Could you write in to accept coordinates not just the current coordinate? Example: I need to tell you that I want a pickup points xyz, because currently it only looks at your current coordinate to the best of my knowledge.

I'm sorry, I'm not sure what you mean. Why would we test against anything other than the players current coordinates to determine whether a pickup request ist legitmate or not?

[Gellipapa]

I seem to remember that currently in ESX there is no option to spawn a pickup point to a given coordinate, you can only spawn a point to the player's coordinate. That's what I was thinking about, that I should be able to spawn a pickup point to a custom coordinate.

[Kenshiin13]

I seem to remember that currently in ESX there is no option to spawn a pickup point to a given coordinate, you can only spawn a point to the player's coordinate. That's what I was thinking about, that I should be able to spawn a pickup point to a custom coordinate.

You mean like this 92d6f49?

[Gellipapa]

@Kenshiin13 Yes, thank you.

[Gellipapa]

Thanks for the PR, we will test it soon and if it works we will merge it.

[Gellipapa]

@Kenshiin13 Hi! I tested this pr and i found bug.

• ESX.CreatePickup method first parameter is type (this is not correct name, please fix) and this is name overwrite default lua type checker and i get nil value if i try run this code. (Please rename all type parameter to other name in server/main.lua)

Let's find a solution so that if I don't want to spawn a weapon, I don't have to specify "components" and "tintIdex", because I currently have to specify them because coordinates are included and the parameter number of pieces makes it mandatory to specify them. (!!We have to pay attention to backwards compatibility because it is not easy to do this.)

[Gellipapa]

In the future, please make sure that it is tested, because this is a basic bug that could have been fixed in the first test. Thanks.

[Kenshiin13]

Let's find a solution so that if I don't want to spawn a weapon, I don't have to specify "components" and "tintIdex", because I currently have to specify them because coordinates are included and the parameter number of pieces makes it mandatory to specify them. (!!We have to pay attention to backwards compatibility because it is not easy to do this.)

Well, I am not sure if there is a way to do this without breaking backwards-compatibility.

[Gellipapa]

Let's find a solution so that if I don't want to spawn a weapon, I don't have to specify "components" and "tintIdex", because I currently have to specify them because coordinates are included and the parameter number of pieces makes it mandatory to specify them. (!!We have to pay attention to backwards compatibility because it is not easy to do this.)

Well, I am not sure if there is a way to do this without breaking backwards-compatibility.

If you have any ideas, feel free to write them down, I can't think of anything right now, if we don't have a solution, it will stay as it was. I mean that part will not be changed.

[Kenshiin13]

If you have any ideas, feel free to write them down, I can't think of anything right now, if we don't have a solution, it will stay as it was. I mean that part will not be changed.

Nope, not really. Does that work for you? 1b27475

[Gellipapa]

If you have any ideas, feel free to write them down, I can't think of anything right now, if we don't have a solution, it will stay as it was. I mean that part will not be changed.

Nope, not really. Does that work for you? 1b27475

I will check soon.

[Gellipapa]

Hi! Well done, I was tested and merged.