Addon-registered slash commands can break action buttons

[Meorawr]

Addons that register slash commands can trigger a problematic interaction between the macro execution system and the talents interface resulting in action buttons becoming unusable via keybinds.

Test case

This issue only affects the Retail client. A video is attached demonstrating the issue, reproduced on a Restoration Shaman with Riptide, Ghost Wolf, and Earth Shield on the first three slots of the main action bar and the /loi macro described below as the last slot on the same bar. Note there is a pause in the video for a few seconds while a massive taint log gets written out.

https://github.com/Stanzilla/WoWUIBugs/assets/287102/cc58f9eb-6f9b-44d5-b867-d7c54e2afb32

Preconditions

• You will need to set up your main action bar such the first spell on the bar that is acquired through talents. Using a Restoration Shaman for example, Riptide (at the base of the Restoration tree) is a suitable option.
• You will need to set up your main action bar with any number of additional spells after the first slot.
• You will need to be on an English client. Other client locales can reproduce the issue, but you'll need to translate the /loi command used in the macro below appropriately.
• You will need to create the following macro to activate a talent loadout and place in any slot on your action bars:

/loi 1

Steps

1. Run the following command: /run SlashCmdList.BREAKTHEUI = nop SLASH_BREAKTHEUI1 = "/breaktheui"
2. Use the macro.
3. Open the Talents window.
4. Hover over the spell learned through talents that is present on your action bar, such that it begins to pulse a yellow indicator.
5. Close the talents panel.
6. Cycle your current action bar page up and down one time.
7. Attempt to use any action via a keybind in the second slot onwards on the main action bar. The usage will fail with a blocked action error.

Explanation

When using any macro the chat system will attempt to import (via ChatFrame_ImportAllListsToHash) any newly defined slash commands present in the SlashCmdList global table to an internal hash table (hash_SlashCmdList).

Because we've registered a slash command and the first action we've taken with the chat system is to execute a macro this will pick up the taint from our pending command registration and propagates it through the remainder of the command execution.

6/21 14:51:55.891  Execution tainted by *** ForceTaint_Strong *** while reading SLASH_BREAKTHEUI1 - Interface/FrameXML/ChatFrame.lua:2729 ChatFrame_ImportListToHash()
6/21 14:51:55.891      Interface/FrameXML/ChatFrame.lua:2777 ChatFrame_ImportAllListsToHash()
6/21 14:51:55.891      Interface/FrameXML/ChatFrame.lua:5227 ChatEdit_ParseText()
6/21 14:51:55.891      Interface/FrameXML/ChatFrame.lua:4899 ChatEdit_SendText()
6/21 14:51:55.891      Interface/FrameXML/ChatFrame.lua:3143
6/21 14:51:55.891      UseAction()
6/21 14:51:55.891      Interface/FrameXML/SecureTemplates.lua:364 handler()
6/21 14:51:55.891      Interface/FrameXML/SecureTemplates.lua:690 PerformAction()
6/21 14:51:55.891      Interface/FrameXML/SecureTemplates.lua:704 OnActionButtonClick()
6/21 14:51:55.891      Interface/FrameXML/SecureTemplates.lua:746 SecureActionButton_OnClick()
6/21 14:51:55.891      Interface/FrameXML/ActionButton.lua:93 TryUseActionButton()
6/21 14:51:55.891      Interface/FrameXML/ActionButton.lua:128 ActionButtonDown()
6/21 14:51:55.891      ACTIONBUTTON12:2

This taint carries through into the execution of the class talent helper functions for switching the loadout index. This taints a significant amount of state owned by the talents UI - far too much stuff to list.

6/21 14:51:55.946  Global variable isConfigReadyToApply tainted by *** ForceTaint_Strong *** - Interface/AddOns/Blizzard_ClassTalentUI/Blizzard_ClassTalentTalentsTab.lua:978 Frame:LoadConfigInternal()
6/21 14:51:55.946      Interface/AddOns/Blizzard_ClassTalentUI/Blizzard_ClassTalentTalentsTab.lua:1459 Frame:LoadConfigByPredicate()
6/21 14:51:55.946      Interface/AddOns/Blizzard_ClassTalentUI/Blizzard_ClassTalentTalentsTab.lua:1487 Frame:LoadConfigByIndex()
6/21 14:51:55.946      Interface/FrameXML/ClassTalentHelper.lua:24 SwitchToLoadoutByIndex()
6/21 14:51:55.946      Interface/FrameXML/ClassTalentHelper.lua:53 ?()
6/21 14:51:55.946      Interface/FrameXML/ChatFrame.lua:5241 ChatEdit_ParseText()
6/21 14:51:55.946      Interface/FrameXML/ChatFrame.lua:4899 ChatEdit_SendText()
6/21 14:51:55.946      Interface/FrameXML/ChatFrame.lua:3143
6/21 14:51:55.946      UseAction()
6/21 14:51:55.946      Interface/FrameXML/SecureTemplates.lua:364 handler()
6/21 14:51:55.946      Interface/FrameXML/SecureTemplates.lua:690 PerformAction()
6/21 14:51:55.946      Interface/FrameXML/SecureTemplates.lua:704 OnActionButtonClick()
6/21 14:51:55.946      Interface/FrameXML/SecureTemplates.lua:746 SecureActionButton_OnClick()
6/21 14:51:55.946      Interface/FrameXML/ActionButton.lua:93 TryUseActionButton()
6/21 14:51:55.946      Interface/FrameXML/ActionButton.lua:128 ActionButtonDown()
6/21 14:51:55.946      ACTIONBUTTON12:2

When hovering over the learned spell in the talent panel, a number of the previously tainted fields are read and taint execution. The interaction itself triggers a glow on the action button for the hovered spell, and with it this taints the ON_BAR_HIGHLIGHT_MARKS global.

6/21 14:52:01.561  Global variable ON_BAR_HIGHLIGHT_MARKS tainted by *** ForceTaint_Strong *** - Interface/FrameXML/ActionButton.lua:51 ClearOnBarHighlightMarks()
6/21 14:52:01.561      Interface/AddOns/Blizzard_ClassTalentUI/Blizzard_ClassTalentButtonTemplates.lua:30 Button:ShowActionBarHighlights()
6/21 14:52:01.561      Interface/AddOns/Blizzard_ClassTalentUI/Blizzard_ClassTalentButtonTemplates.lua:145

When changing the action bar page the action bar controller triggers a full update of all buttons on the bar. This reads the tainted ON_BAR_HIGHLIGHT_MARKS global, taints execution, and critically propagates this to the assignment of .action fields (plus potentially more) on all subsequent action buttons that get updated on the same bar.

6/21 14:52:05.191  Execution tainted by *** ForceTaint_Strong *** while reading ON_BAR_HIGHLIGHT_MARKS - Interface/FrameXML/ActionButton.lua:55 GetOnBarHighlightMark()
6/21 14:52:05.191      Interface/FrameXML/ActionButton.lua:562 ActionButton1:UpdateSpellHighlightMark()
6/21 14:52:05.191      Interface/FrameXML/ActionButton.lua:470 ActionButton1:Update()
6/21 14:52:05.191      Interface/FrameXML/ActionButton.lua:442 ActionButton1:UpdateAction()
6/21 14:52:05.191      Interface/FrameXML/ActionBarController.lua:174 ActionBarController_ResetToDefault()
6/21 14:52:05.191      Interface/FrameXML/ActionBarController.lua:159 ActionBarController_UpdateAll()
6/21 14:52:05.191      Interface/FrameXML/ActionBarController.lua:66
6/21 14:52:05.191      ChangeActionBarPage()
6/21 14:52:05.191      Interface/FrameXML/ActionButton.lua:178 ActionBar_PageDown()
6/21 14:52:05.191      Interface/FrameXML/MainMenuBar.lua:341
6/21 14:52:05.191  Global variable action tainted by *** ForceTaint_Strong *** - Interface/FrameXML/ActionButton.lua:437 ActionButton2:UpdateAction()
6/21 14:52:05.191      Interface/FrameXML/ActionBarController.lua:174 ActionBarController_ResetToDefault()
6/21 14:52:05.191      Interface/FrameXML/ActionBarController.lua:159 ActionBarController_UpdateAll()
6/21 14:52:05.191      Interface/FrameXML/ActionBarController.lua:66
6/21 14:52:05.191      ChangeActionBarPage()
6/21 14:52:05.191      Interface/FrameXML/ActionButton.lua:178 ActionBar_PageDown()
6/21 14:52:05.191      Interface/FrameXML/MainMenuBar.lua:341

When using a keybinding to activate the spell, the tainted .action field is read while updating flyouts which taints execution and leads to the UseAction call being blocked.

6/21 14:52:05.847  Execution tainted by *** ForceTaint_Strong *** while reading action - Interface/FrameXML/ActionButton.lua:1262 ActionButton2:UpdateFlyout()
6/21 14:52:05.847      Interface/FrameXML/ActionButton.lua:1332 ActionButton2:SetButtonState()
6/21 14:52:05.847      Interface/FrameXML/ActionButton.lua:125 ActionButtonDown()
6/21 14:52:05.847      ACTIONBUTTON2:2

6/21 14:52:05.847  Interface/FrameXML/ActionButton.lua:1262 UpdateFlyout()
6/21 14:52:05.847  An action was blocked because of taint from *** ForceTaint_Strong *** - UseAction()
6/21 14:52:05.847      Interface/FrameXML/SecureTemplates.lua:364 handler()
6/21 14:52:05.847      Interface/FrameXML/SecureTemplates.lua:690 PerformAction()
6/21 14:52:05.847      Interface/FrameXML/SecureTemplates.lua:704 OnActionButtonClick()
6/21 14:52:05.847      Interface/FrameXML/SecureTemplates.lua:746 SecureActionButton_OnClick()
6/21 14:52:05.847      Interface/FrameXML/ActionButton.lua:93 TryUseActionButton()
6/21 14:52:05.847      Interface/FrameXML/ActionButton.lua:128 ActionButtonDown()
6/21 14:52:05.847      ACTIONBUTTON2:2

Mitigations

A suitable fix would be to just band-aid the command import process with a securecall barrier as shown below. There's numerous call sites to ChatFrame_ImportAllListsToHash which can spuriously taint while processing pending command registrations. With this barrier the insertion into the hash_SlashCmdList table remains insecure, but the act of importing those commands doesn't taint random interactions with the chat system.

diff --git a/Interface/FrameXML/ChatFrame.lua b/Interface/FrameXML/ChatFrame.lua
index ca7bf300..6b4e820f 100644
--- a/Interface/FrameXML/ChatFrame.lua
+++ b/Interface/FrameXML/ChatFrame.lua
@@ -2774,7 +2774,7 @@ end

 function ChatFrame_ImportAllListsToHash()
    ChatFrame_ImportListToHash(SecureCmdList, hash_SecureCmdList);
-   ChatFrame_ImportListToHash(SlashCmdList, hash_SlashCmdList);
+   securecallfunction(ChatFrame_ImportListToHash, SlashCmdList, hash_SlashCmdList);
    ChatFrame_ImportListToHash(ChatTypeInfo);
 end

Additionally, a similar barrier within ActionBarActionButtonMixin:UpdateSpellHighlightMark would be wise as there are other issues with the Talents interface in general that can trivially taint the ON_BAR_HIGHLIGHT_MARKS global - such as any addons calling the ClassTalentHelper functions. As this function is used purely for visual decoration, there should be no security risk with this change.

diff --git a/Interface/FrameXML/ActionButton.lua b/Interface/FrameXML/ActionButton.lua
index 8b52a08a..61c5bbb6 100644
--- a/Interface/FrameXML/ActionButton.lua
+++ b/Interface/FrameXML/ActionButton.lua
@@ -466,7 +466,7 @@ end

 function ActionBarActionButtonMixin:UpdateSpellHighlightMark()
    if ( self.SpellHighlightTexture and self.SpellHighlightAnim ) then
-       SharedActionButton_RefreshSpellHighlight(self, GetOnBarHighlightMark(self.action));
+       SharedActionButton_RefreshSpellHighlight(self, securecallfunction(GetOnBarHighlightMark, self.action));
    end
 end

[Voopie]

Сan't reproduce it in 10.1.5.50130 If I did everything right, of course

https://github.com/Stanzilla/WoWUIBugs/assets/1324849/aca73380-7ce2-45d3-acde-80dc472b6a85

[Meorawr]

I'm still able to reproduce it 10.1.5.50130.

[Meorawr]

I believe the issue you're having reproducing this is that you're on a non-English client; in some localizations the talent loadout commands like /loi are fully localized without their English equivalents kept working:

https://www.townlong-yak.com/framexml/live/GlobalStrings.lua/GB#15684 https://www.townlong-yak.com/framexml/live/GlobalStrings.lua/RU#15684

If I change my client locale to French (for which /loi is /ic) the issue can't be reproduced unless I also update the macro to translate the command appropriately.

[Voopie]

Yeah, you're right, sorry for bad information

[MysticalOS]

Been a while, any update on this one?

[Meorawr]

Appears to be fixed in 10.2.7 and 1.15.3. Wasn't resolved in Cata, but I'd assume will be there in 4.4.1.