Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/snippets/features.7132.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- (#7132) Gorton's birthday present

When playing full share, when you self destruct a selection with one or more ACUs in it you will self destruct only the ACUs instead.
29 changes: 28 additions & 1 deletion lua/SimCallbacks.lua
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,34 @@ Callbacks.UpdateMarker = SimPing.UpdateMarker

Callbacks.FactionSelection = ScenarioFramework.OnFactionSelect

Callbacks.ToggleSelfDestruct = import("/lua/selfdestruct.lua").ToggleSelfDestruct
---@param data ToggleSelfDestructData
---@param units Unit[]
Callbacks.ToggleSelfDestruct = function(data, units)
-- prevent malformed input
if (not units) or (table.getn(units) == 0) then
return
end

-- prevent abuse
if (not data) or (not data.owner) or (not OkayToMessWithArmy(data.owner)) then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using user-defined Args in the data table cannot be relied upon for security.

When I look into it further, the verification for this function is ending up a bit all over the place:

  1. data.owner is verified with OkayToMessWithArmy in this callback and the import function.
  2. data.owner is confirmed to not be -1 (observer) in the import function. Which I guess prevents observers from self-destructing units in cheat games? But data.owner is GetFocusArmy from UI side, and observers can't select units so they can't trigger the callback with any selection to act upon while also sending an owner of -1.
  3. units is checked with SecureUnits, which loops over units and checks OkayToMessWithArmy(unit.Army) (it also has a part converting entity ids into entities but that isn't relevant here). This is all sim-side info and the units list is from the engine UI code, so it is secure. The issue is the duplication where this OkayToMessWithArmy(unit.Army) is used in your category filter, and the imported function (twice at that).

Overall, the import function has the final results and always checks OkayToMessWithArmy(unit.Army), so it works, but it seems like the rest of the checks can be replaced. data.owner can instead be an early exit using GetCommandSourceArmy or GetFocusArmy sim-side. Checks for OkayToMessWithArmy(unit.Army) can be replaced with SecureUnits in the main callback function replacing the units variable, and then sending it to the imported function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just me pulling the logic from the SelfDestruct function to the boundary, see also lines 35: if data.owner ~= -1 and OkayToMessWithArmy(data.owner) then.

I personally do not think that the units list needs to be checked, since it's the engine that appends it. Unless we also want to guard against hacked clients, where you can append arbitrary units.

Note that I intentionally did not mess with the existing functionality, since that's not what was requested.

return
end

-- moderation rule: if you self destruct with one or more ACUs in the selection when playing full
-- share, then you only self destruct the ACUs. This does not make it impossible to abuse, but it
-- does introduce a simple guardrail.
if ScenarioInfo.Options.Share == "FullShare" then
local commandUnits = EntityCategoryFilterDown(categories.COMMAND, SecureUnits(units))
if table.getn(commandUnits) > 0 then
import("/lua/selfdestruct.lua").ToggleSelfDestruct(data, commandUnits)
return
end
end
Comment on lines +195 to +204

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is trivial to bypass with UI mods splitting the user's order into two callback calls, so why not implement it in the UI layer? Then it's easy to have a handler with proper user feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also trivial to identify that this happened when parsing the replay. Malicious intent is then confirmed, easier to ban in my point of view.

You're free to also implement something in the UI layer, by all means.


-- otherwise just pass it through as usual
import("/lua/selfdestruct.lua").ToggleSelfDestruct(data, units)
Comment thread
Garanas marked this conversation as resolved.
end


Callbacks.MarkerOnScreen = import("/lua/simcameramarkers.lua").MarkerOnScreen

Expand Down
6 changes: 5 additions & 1 deletion lua/selfdestruct.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ local CancelCountdown = CancelCountdown
local ForkThread = ForkThread
local KillThread = KillThread

---@class ToggleSelfDestructData
---@field owner number
---@field noDelay boolean

-- prevent magic numbers
local countdownDuration = 5

Expand All @@ -20,7 +24,7 @@ local function SelfDestructThread(unit)
end

--- Toggles the destruction of the units
---@param data { owner: number, noDelay: boolean }
---@param data ToggleSelfDestructData
---@param units? Unit[]
function ToggleSelfDestruct(data, units)

Expand Down
Loading