Thread Tools Display Modes
03/30/17, 10:04 PM   #21
Sounomi
Join Date: Oct 2014
Posts: 40
Here's an example of scene calling causing taint.

Basically I've wanted to modify the fence window and the most logical point for doing so was when it was first opened. But simply redoing the function that handles that causes the inventory fragment to get tainted. I tried everything I could to get around and I don't see what I'm doing wrong. Ultimately I created a simple test add-on which just straight out copies the original code and it still causes the taint.

Code:
local function OnOpened(fence, enableSell, enableLaunder)
    if not IsInGamepadPreferredMode() then
        fence:InitializeModeBar(enableSell, enableLaunder)
        fence.mode = enableSell and ZO_MODE_STORE_SELL_STOLEN or ZO_MODE_STORE_LAUNDER
        SCENE_MANAGER:Show("fence_keyboard")
    end
end

local function OnAddOnLoaded(code, name)
    ZO_PreHook(FENCE_KEYBOARD, "OnOpened", function(...) OnOpened(...); return true end)
end

EVENT_MANAGER:RegisterForEvent("FenceTest", EVENT_ADD_ON_LOADED, OnAddOnLoaded)
I did wind up finding another way to accomplish what I wanted to do but I still worry that whatever's triggering the taint will be a problem for others. For instance, the Awesome Guild Store add-on currently is causing inventory fragment taint as well and it could be due to a similar reason as the above code causes it.


Originally Posted by Rhyono View Post
I at least didn't see it mentioned: how did this change with Homestead for it to suddenly become so prevalent?
My guess is that they tightened up add-on security a bit more as a result to exploits that were mentioned to be used.
  Reply With Quote
03/30/17, 10:08 PM   #22
Rhyono
AddOn Author - Click to view addons
Join Date: Sep 2016
Posts: 659
Originally Posted by Sounomi View Post
Ultimately I created a simple test add-on which just straight out copies the original code and it still causes the taint.
That's because it's very black and white: before it even reads the code (which you said was identical), it's already checked the source (addon) and decided it's tainted.
  Reply With Quote
03/31/17, 02:13 AM   #23
votan
 
votan's Avatar
AddOn Author - Click to view addons
Join Date: Oct 2014
Posts: 577
Originally Posted by Rhyono View Post
That's because it's very black and white: before it even reads the code (which you said was identical), it's already checked the source (addon) and decided it's tainted.
What drives me crazy aren't the obvious dangerous hooks, where one could say: Do /reloadui, when that, when that and boom.

The error occurs after hours, not to say days. /reloadui does fix it. And if it does not occur after disabling an addon, one may was just lucky this time.
Every author (including me) is quite sure (devs are always just quite sure), that one did nothing wrong.
Because everybody tested the own addons before being sure enough to release it.

It looks like it is a timing problem (e.g. from EVENT_MANAGER or ANIM_MANAGER) even ZOS is not aware of, because their code is always trusted anyway. Just a theory.

But if it is a few days of work for ZOS to change the vm, they hopefully get a confirmation to do so.
  Reply With Quote
03/31/17, 02:35 AM   #24
sirinsidiator
 
sirinsidiator's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2014
Posts: 1,566
Originally Posted by ZOS_ChipHilseberg View Post
The top of the callstack would tell you where the closure was created, but it wouldn't show down to the addon triggering it. In the case of the context menu it would give you the ZO_ObjectPool_CreateControl on line 177 in ZO_ObjectPool.lua and that is it. A setting could be an option. A rough guess on storing all of the stacks for all of the stock UI closures is probably somewhere in the 30 MB range. This would require a couple days of VM modifications, but it isn't impossible.
That's still a lot more information than now and it would allow us to narrow down the culprits by searching for addons that call ZO_ObjectPool_CreateControl directly or indirectly. Would still be a lot of work though. If the full stack is "just" 30MB and could be done as a config flag, I think that route would be preferable. These couple of days would save us weeks or even years down the line (every player who has to reload the game due to that issue and every addon author who spends time to try and fix it)

@Sounomi: I haven't touched anything related to scene switches in AwesomeGuildStore since way before the update and it still suddenly pops up for some users, so it's pretty much certain that something changed in the game that causes it now and not before and I have absolutely no idea what and how I am supposed to fix it without taking the whole addon apart...
  Reply With Quote
03/31/17, 07:55 AM   #25
ZOS_ChipHilseberg
ZOS Staff!
Premium Member
Yes this person is from ZeniMax!
Join Date: Oct 2014
Posts: 551
There haven't been any changes made to the security model, so new tainting situations are purely incidental. Since this seems to be focused around the inventory, maybe something changed there that addons were making use of.
  Reply With Quote
03/31/17, 08:42 AM   #26
sirinsidiator
 
sirinsidiator's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2014
Posts: 1,566
Ok. Guess I'll have a look at what has changed between now and then.

A completely different question: Would it make sense and be possible to add a function IsFunctionSecurityContextTainted(function) which returns true if the function/closure has been marked? That way we could show the status in zgoo and similar. Might be useful for debugging as it would allow to check if objects in a pool have tainted functions.
  Reply With Quote
03/31/17, 08:45 AM   #27
Rhyono
AddOn Author - Click to view addons
Join Date: Sep 2016
Posts: 659
Originally Posted by ZOS_ChipHilseberg View Post
Since this seems to be focused around the inventory, maybe something changed there that addons were making use of.
That must be it because I know many addons did not have any code changes (aside from an API bump) post-update and suddenly have this issue.
  Reply With Quote
03/31/17, 08:53 AM   #28
AssemblerManiac
AddOn Author - Click to view addons
Join Date: Jun 2014
Posts: 51
Originally Posted by ZOS_ChipHilseberg View Post
There haven't been any changes made to the security model, so new tainting situations are purely incidental. Since this seems to be focused around the inventory, maybe something changed there that addons were making use of.
I think what's changed is the handling of pool objects.

When objects are put into/taken out of the pool, it seems like some of the properties aren't getting reset. I've been seeing the dividing line showing up RED occasionally when added to a tooltip with the IIfA data, but only after I've looked at stolen items, and it doesn't always happen.

I think this is why doing a drag event from the inventory window randomly shows an error from the IIfA grid (among others). The two are totally unrelated, except that the object pool is being used for creation of controls from virtual versions.

Any of this make sense?

Last edited by AssemblerManiac : 03/31/17 at 09:03 AM.
  Reply With Quote
04/03/17, 06:34 PM   #29
Gamer1986PAN
AddOn Author - Click to view addons
Join Date: Sep 2015
Posts: 87
I am not an Author (even have to search for html and bb-code) but if you are on a way fixing this problem please try to make Circonians FilterIt work again, too. Because if FilterIt is running and you want to add a costume, the banker or any other memento in the Quickslot this error occurs:


Code:
EsoUI/Ingame/Inventory/InventorySlot.lua:2452: attempt to access a private function 'PickupCollectible' from insecure code
stack traceback:
	EsoUI/Ingame/Inventory/InventorySlot.lua:2452: in function '(anonymous)'
	EsoUI/Ingame/Utility/ZO_SlotUtil.lua:14: in function 'RunHandlers'
	(tail call): ?
	(tail call): ?
	ZO_RepairWindowList1Row1_DragStart:4: in function '(main chunk)'
  Reply With Quote
04/04/17, 03:59 AM   #30
Sounomi
Join Date: Oct 2014
Posts: 40
Intentional or not, it seems that something changed with the way certain scenes work. If an add-on loads up a scene before its been used within secure code, it'll be tainted. This clearly didn't happen before or there wouldn't be so many add-ons having problems with the inventory scene/fragment after Homestead.

You can easily see this by pulling up the inventory scene from insecure code after a fresh load with:
Code:
/script SCENE_MANAGER:Show("inventory")
Just doing that will cause secure access errors if you try to do anything uses a protected function, like dragging it or trying to use an item. However if you opened up the inventory via the hotkey before executing that command, everything's fine. Its pretty much the same cause behind the errors with Awesome Guild Store.

This doesn't apply to just the inventory scene either but seemingly all scenes. Say if you do this after a fresh load:
Code:
/script MAIN_MENU_KEYBOARD.categoryBar.m_object:SelectDescriptor(MENU_CATEGORY_MARKET)
You'll get an error about it trying to call a private function. But again if you loaded up the scene via the hotkey before executing that command you won't get that error.

My guess is that before Homestead, scenes like these were initialized ahead of time but after Homestead they're getting initialized when they're first used and if that happens to be due to an add-on bring them up then they get initialized in insecure code and remain tainted.
  Reply With Quote
04/04/17, 04:38 PM   #31
ZOS_ChipHilseberg
ZOS Staff!
Premium Member
Yes this person is from ZeniMax!
Join Date: Oct 2014
Posts: 551
Originally Posted by sirinsidiator View Post
Ok. Guess I'll have a look at what has changed between now and then.

A completely different question: Would it make sense and be possible to add a function IsFunctionSecurityContextTainted(function) which returns true if the function/closure has been marked? That way we could show the status in zgoo and similar. Might be useful for debugging as it would allow to check if objects in a pool have tainted functions.
Should be doable.
  Reply With Quote
04/05/17, 11:16 AM   #32
ZOS_ChipHilseberg
ZOS Staff!
Premium Member
Yes this person is from ZeniMax!
Join Date: Oct 2014
Posts: 551
We added IsTrustedFunction which can tell you if a C function of lua closure is trusted or not.
  Reply With Quote
04/05/17, 11:57 AM   #33
Rhyono
AddOn Author - Click to view addons
Join Date: Sep 2016
Posts: 659
Originally Posted by ZOS_ChipHilseberg View Post
We added IsTrustedFunction which can tell you if a C function of lua closure is trusted or not.
So let's say a function is tainted and we catch it with this: is there some sort of solution as to where we could correct it without /reloadui?
  Reply With Quote
04/05/17, 12:05 PM   #34
sirinsidiator
 
sirinsidiator's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2014
Posts: 1,566
Originally Posted by ZOS_ChipHilseberg View Post
We added IsTrustedFunction which can tell you if a C function of lua closure is trusted or not.
Thanks! Just to clarify, how would this function be used?

For example what would this code print:
Lua Code:
  1. local objects = ZO_Menu.itemPool:GetActiveObjects()
  2. for k, v in pairs(objects) do
  3.     local nameControl = GetControl(v, "Name")
  4.     df("%s: %s", nameControl:GetText(), tostring(IsTrustedFunction(v.OnSelect)))
  5. end
  Reply With Quote
04/16/17, 08:08 AM   #35
sirinsidiator
 
sirinsidiator's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2014
Posts: 1,566
Hi Chip,

Thanks to Rhyono and Sounomi I found (or at least I believe to have found) what causes the problem when AwesomeGuildStore is active. They reported that the error shows up when trying to use an item in the inventory when the guild store is the first thing that is opened after loading the UI, but not when the inventory is opened first.

After some investigation I found that when a state change for the INVENTORY_FRAGMENT occurs, the state change callback calls ZO_InventoryManager:UpdateList which then lazily creates the scroll list rows for the passed inventory type. Because of this, all rows are then tainted and cause the issue at hand.
I don't think I can fix this on the addon side, so I hope you could add some code to fill the inventory row pool in InitializeInventoryList (inventory.lua:176)

e.g. add a new argument prefillCount to ZO_ScrollList_AddDataType
Lua Code:
  1. function ZO_ScrollList_AddDataType(self, typeId, templateName, height, setupCallback, hideCallback, dataTypeSelectSound, resetControlCallback, prefillCount)    
  2.     if(not self.dataTypes[typeId]) then
  3. ...
  4.         UpdateModeFromHeight(self, height)
  5.        
  6.         if(prefillCount) then
  7.             for i = 1, prefillCount do
  8.                 pool:AquireObject()
  9.             end
  10.             pool:ReleaseAllObjects()
  11.         end
  12.     end
  13. end

Lua Code:
  1. local ROW_HEIGHT = 52
  2. local function InitializeInventoryList(inventory)
  3.     local listView = inventory.listView
  4.     if listView then
  5.         ZO_ScrollList_Initialize(listView)
  6.         local prefillCount = math.ceil(listView:GetHeight() / ROW_HEIGHT)
  7.         ZO_ScrollList_AddDataType(listView, inventory.listDataType, inventory.rowTemplate, ROW_HEIGHT, inventory.listSetupCallback, inventory.listHiddenCallback, nil, ZO_InventorySlot_OnPoolReset, prefillCount)
  8.         ZO_ScrollList_AddResizeOnScreenResize(listView)
  9.     end
  10. end

Or just make it a boolean and handle the calculations inside, otherwise a screen resize could become an issue.
InventoryGridView will probably suffer from the same problem even when the pool is filled this way, but I haven't checked how the entries are handled there.
Maybe a way to specify a function for the prefillCount calculation before the pool is created would be helpful.

Last edited by sirinsidiator : 04/16/17 at 08:11 AM.
  Reply With Quote
04/17/17, 07:38 AM   #36
ZOS_ChipHilseberg
ZOS Staff!
Premium Member
Yes this person is from ZeniMax!
Join Date: Oct 2014
Posts: 551
Nice work tracking it down. What is the callstack that is leading to the security error? Is it from the "E" keybind or a right-click menu or something else?
  Reply With Quote
04/17/17, 09:02 AM   #37
Rhyono
AddOn Author - Click to view addons
Join Date: Sep 2016
Posts: 659
Originally Posted by ZOS_ChipHilseberg View Post
Nice work tracking it down. What is the callstack that is leading to the security error? Is it from the "E" keybind or a right-click menu or something else?
Tainting the callstack requires a mere glance at AGS' sell inventory. Even if you don't touch a thing, the keybind and right click menu will be tainted if you now open your inventory and try to do anything.
  Reply With Quote
04/17/17, 03:04 PM   #38
sirinsidiator
 
sirinsidiator's Avatar
AddOn Author - Click to view addons
Join Date: Apr 2014
Posts: 1,566
Originally Posted by ZOS_ChipHilseberg View Post
Nice work tracking it down. What is the callstack that is leading to the security error? Is it from the "E" keybind or a right-click menu or something else?
The callstack for trying to use E:
Code:
EsoUI/Ingame/Inventory/InventorySlot.lua:1020: attempt to access a private function 'UseItem' from insecure code
stack traceback:
	EsoUI/Ingame/Inventory/InventorySlot.lua:1020: in function 'TryUseItem'
	EsoUI/Ingame/Inventory/InventorySlot.lua:1201: in function 'INDEX_ACTION_CALLBACK'
	EsoUI/Ingame/Inventory/InventorySlotActions.lua:96: in function 'ZO_InventorySlotActions:DoPrimaryAction'
	EsoUI/Ingame/Inventory/ItemSlotActionController.lua:31: in function 'callback'
	EsoUI/Libraries/ZO_KeybindStrip/ZO_KeybindStrip.lua:614: in function 'ZO_KeybindStrip:TryHandlingKeybindDown'
	(tail call): ?
	(tail call): ?
For trying to use the context menu:
Code:
EsoUI/Ingame/Inventory/InventorySlot.lua:1020: function expected instead of nil
stack traceback:
	EsoUI/Ingame/Inventory/InventorySlot.lua:1020: in function 'TryUseItem'
	EsoUI/Ingame/Inventory/InventorySlot.lua:1201: in function 'OnSelect'
	EsoUI/Libraries/ZO_ContextMenus/ZO_ContextMenus.lua:419: in function 'ZO_Menu_ClickItem'
	ZO_MenuItem1_MouseUp:4: in function '(main chunk)'
And for double clicking the item:
Code:
EsoUI/Ingame/Inventory/InventorySlot.lua:1020: function expected instead of nil
stack traceback:
	EsoUI/Ingame/Inventory/InventorySlot.lua:1020: in function 'TryUseItem'
	EsoUI/Ingame/Inventory/InventorySlot.lua:1201: in function 'INDEX_ACTION_CALLBACK'
	EsoUI/Ingame/Inventory/InventorySlotActions.lua:96: in function 'ZO_InventorySlotActions:DoPrimaryAction'
	EsoUI/Ingame/Inventory/InventorySlot.lua:1767: in function 'ZO_InventorySlot_DoPrimaryAction'
	ZO_TradingHouseItemPaneSearchResults3Row1_MouseDoubleClick:4: in function '(main chunk)'
Steps to reproduce:
1. Load the UI with AwesomeGuildStore active
2. Open any guild store
3. Switch to the sell tab
4. Close the guild store
5. Open the inventory
6. Try to use a potion or some other item
  Reply With Quote
04/18/17, 02:16 AM   #39
votan
 
votan's Avatar
AddOn Author - Click to view addons
Join Date: Oct 2014
Posts: 577
Originally Posted by sirinsidiator View Post
Steps to reproduce:
1. Load the UI with AwesomeGuildStore active
2. Open any guild store
3. Switch to the sell tab
4. Close the guild store
5. Open the inventory
6. Try to use a potion or some other item
To me it looks like it all raises and falls with the scene/fragment state change system.
Maybe ZOS should give themselves a private function "CallPrivateFunction" (or so) to do a permission check (e.g. the secure render mode) and continue selective sub-calls taint-free, even if triggered by custom code.

/edit: e.g. sub-calls within SCENE_MANAGER:ShowScene() and fragment:Refresh()

a function in C++, which looks like this Lua:
Code:
function CallPrivateFunction(func, ...)
    if (tainted and IsSecureRenderMode()) return false end
    local old_tainted = tainted
    tainted = false
    -- continue untainted
    func(...)
    -- we may got tainted (again)
    tainted = tainted or old_tainted
    return true
end

Last edited by votan : 04/18/17 at 02:38 AM.
  Reply With Quote
04/18/17, 07:43 AM   #40
ZOS_ChipHilseberg
ZOS Staff!
Premium Member
Yes this person is from ZeniMax!
Join Date: Oct 2014
Posts: 551
Votan, I've been toying with that idea for the past week or so. As soon as I can reproduce the bug I'll evaluate how well it applies for this specific situation.
  Reply With Quote

ESOUI » Developer Discussions » General Authoring Discussion » @ZOS: Improve "Access a private function" error message

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off