Map canvas frames and the world map can spread taint around the UI when processing installed click and mouse action handlers, and when refreshing overlay frames. This leads to issues reported in other tickets such as the following:
339
440
In the case of #339, while marked as "fixed" in that specific case only an avenue of taint propagation through edit mode was fixed, rather than the root cause.
With changes made in patch 10.1.5 that restrict the ability for addons to call functions like ScriptRegion:SetPassThroughButtons from insecure code while in-combat, the need for this issue to be resolved increases further, as previously addons could work around the click handler taint by rolling their own click interception logic and preventing propagation to the underlying map canvas.
This issue used to additionally impact map data providers (#114), however that was resolved by making functions that iterate over the registry of data providers do so in such a way that doesn't propagate taint back to callers. A similar solution should be employable for these other aspects of the map infrastructure.
Test cases
For each of these test cases we'll exercise the issue such that the symptom described in #440 is encountered where a blocked action error will occur when opening the encounter journal.
Click handlers
Run the following command: /run WorldMapFrame:AddCanvasClickHandler(print)
Open the world map.
Zoom the world map out one level.
Zoom the world map in one level.
Navigate to a zone on the world map that has a dungeon or raid entrance pin.
Click the dungeon or raid entrance pin.
Close the encounter journal.
Open the encounter journal.
Upon opening the encounter journal - and any more toggling of it thereafter - you'll get a blocked action popup, as shown in the video below.
The following functions should be adjusted to iterate over the contents of their respective tables and to call inner functions securely without propagating taint back to calling code.
It should be noted that there is an additional function (MapCanvasMixin:ProcessCursorHandlers) which behaves similarly to those listed above, however at this time I don't believe this specific function is an issue and it shouldn't need to be modified.
Its only call site is within MapCanvasMixin:OnUpdate where it's the last call made as part of the script handler - as such while execution may taint while processing cursor handlers, it shouldn't spread anywhere else. Additionally, all existing handlers for this function don't modify any state - they simply query it and return a cursor name as a string, so taint propagating between handlers shouldn't be an issue either.
The below patch should accomplish this without too much risk. In the case of click and mouse action handlers, securecallfunction will automatically forward any errors they encounter and and return no results - as such the handling of return values should remain functionally identical to the existing code.
diff --git a/Interface/AddOns/Blizzard_MapCanvas/Blizzard_MapCanvas.lua b/Interface/AddOns/Blizzard_MapCanvas/Blizzard_MapCanvas.lua
index 5692654b..a767abe0 100644
--- a/Interface/AddOns/Blizzard_MapCanvas/Blizzard_MapCanvas.lua
+++ b/Interface/AddOns/Blizzard_MapCanvas/Blizzard_MapCanvas.lua
@@ -1,3 +1,16 @@
+local pairs = pairs;
+local securecallfunction = securecallfunction;
+
+-- Matches a similar function reused in multiple places
+local function EnumerateTaintedKeysTable(tableToIterate)
+ local pairsIterator, enumerateTable, initialIteratorKey = securecallfunction(pairs, tableToIterate);
+ local function IteratorFunction(tbl, key)
+ return securecallfunction(pairsIterator, tbl, key);
+ end
+
+ return IteratorFunction, enumerateTable, initialIteratorKey;
+end
+
MapCanvasMixin = CreateFromMixins(CallbackRegistryMixin);
MapCanvasMixin.MouseAction = { Up = 1, Down = 2, Click = 3 };
@@ -817,7 +830,8 @@ end
-- If the function returns true then handling will stop
-- A priority can optionally be specified, higher priority values will be called first
function MapCanvasMixin:AddCanvasClickHandler(handler, priority)
- table.insert(self.mouseClickHandlers, { handler = handler, priority = priority or 0 });
+ local handlerInfo = setmetatable({ handler = handler, priority = priority or 0 }, { __metatable = false });
+ table.insert(self.mouseClickHandlers, handlerInfo);
table.sort(self.mouseClickHandlers, PrioritySorter);
end
@@ -831,9 +845,9 @@ function MapCanvasMixin:RemoveCanvasClickHandler(handler, priority)
end
function MapCanvasMixin:ProcessCanvasClickHandlers(button, cursorX, cursorY)
- for i, handlerInfo in ipairs(self.mouseClickHandlers) do
- local success, stopChecking = xpcall(handlerInfo.handler, CallErrorHandler, self, button, cursorX, cursorY);
- if success and stopChecking then
+ for i, handlerInfo in EnumerateTaintedKeysTable(self.mouseClickHandlers) do
+ local stopChecking = securecallfunction(handlerInfo.handler, self, button, cursorX, cursorY);
+ if stopChecking then
return true;
end
end
@@ -844,7 +858,8 @@ end
-- If the function returns true then handling will stop
-- A priority can optionally be specified, higher priority values will be called first
function MapCanvasMixin:AddGlobalPinMouseActionHandler(handler, priority)
- table.insert(self.globalPinMouseActionHandlers, { handler = handler, priority = priority or 0 });
+ local handlerInfo = setmetatable({ handler = handler, priority = priority or 0 }, { __metatable = false });
+ table.insert(self.globalPinMouseActionHandlers, handlerInfo);
table.sort(self.globalPinMouseActionHandlers, PrioritySorter);
end
@@ -858,9 +873,9 @@ function MapCanvasMixin:RemoveGlobalPinMouseActionHandler(handler, priority)
end
function MapCanvasMixin:ProcessGlobalPinMouseActionHandlers(mouseAction, button)
- for i, handlerInfo in ipairs(self.globalPinMouseActionHandlers) do
- local success, stopChecking = xpcall(handlerInfo.handler, CallErrorHandler, self, mouseAction, button);
- if success and stopChecking then
+ for i, handlerInfo in EnumerateTaintedKeysTable(self.globalPinMouseActionHandlers) do
+ local stopChecking = securecallfunction(handlerInfo.handler, self, mouseAction, button);
+ if stopChecking then
return true;
end
end
diff --git a/Interface/AddOns/Blizzard_WorldMap/Blizzard_WorldMap.lua b/Interface/AddOns/Blizzard_WorldMap/Blizzard_WorldMap.lua
index 623264bc..e515753e 100644
--- a/Interface/AddOns/Blizzard_WorldMap/Blizzard_WorldMap.lua
+++ b/Interface/AddOns/Blizzard_WorldMap/Blizzard_WorldMap.lua
@@ -281,11 +281,13 @@ function WorldMapMixin:OnHide()
UpdateMicroButtons();
end
+local function RefreshOverlayFrame(i, frame)
+ frame:Refresh();
+end
+
function WorldMapMixin:RefreshOverlayFrames()
if self.overlayFrames then
- for i, frame in ipairs(self.overlayFrames) do
- frame:Refresh();
- end
+ secureexecuterange(self.overlayFrames, RefreshOverlayFrame);
end
end
Map canvas frames and the world map can spread taint around the UI when processing installed click and mouse action handlers, and when refreshing overlay frames. This leads to issues reported in other tickets such as the following:
339
440
In the case of #339, while marked as "fixed" in that specific case only an avenue of taint propagation through edit mode was fixed, rather than the root cause.
With changes made in patch 10.1.5 that restrict the ability for addons to call functions like ScriptRegion:SetPassThroughButtons from insecure code while in-combat, the need for this issue to be resolved increases further, as previously addons could work around the click handler taint by rolling their own click interception logic and preventing propagation to the underlying map canvas.
This issue used to additionally impact map data providers (#114), however that was resolved by making functions that iterate over the registry of data providers do so in such a way that doesn't propagate taint back to callers. A similar solution should be employable for these other aspects of the map infrastructure.
Test cases
For each of these test cases we'll exercise the issue such that the symptom described in #440 is encountered where a blocked action error will occur when opening the encounter journal.
Click handlers
/run WorldMapFrame:AddCanvasClickHandler(print)Upon opening the encounter journal - and any more toggling of it thereafter - you'll get a blocked action popup, as shown in the video below.
https://github.com/Stanzilla/WoWUIBugs/assets/287102/fbd9361f-852a-4915-b334-a02a17582265
Mouse action handlers
/run WorldMapFrame:AddGlobalPinMouseActionHandler(print)As above, same result - toggling the encounter journal will result in a blocked action popup.
https://github.com/Stanzilla/WoWUIBugs/assets/287102/0fc1ce85-5374-4b5c-bb12-ddde1344ffaf
Overlay frames
/run WorldMapFrame:AddOverlayFrame("WorldMapZoneTimerTemplate", "Frame")Astoundingly this test case has the exact same result; toggling the encounter journal will result in a blocked action popup.
https://github.com/Stanzilla/WoWUIBugs/assets/287102/9b8a238f-ecdf-4aa1-9e18-6e6c0051941e
Solution
The following functions should be adjusted to iterate over the contents of their respective tables and to call inner functions securely without propagating taint back to calling code.
It should be noted that there is an additional function (MapCanvasMixin:ProcessCursorHandlers) which behaves similarly to those listed above, however at this time I don't believe this specific function is an issue and it shouldn't need to be modified.
Its only call site is within MapCanvasMixin:OnUpdate where it's the last call made as part of the script handler - as such while execution may taint while processing cursor handlers, it shouldn't spread anywhere else. Additionally, all existing handlers for this function don't modify any state - they simply query it and return a cursor name as a string, so taint propagating between handlers shouldn't be an issue either.
The below patch should accomplish this without too much risk. In the case of click and mouse action handlers, securecallfunction will automatically forward any errors they encounter and and return no results - as such the handling of return values should remain functionally identical to the existing code.