Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sandbox escape #4553

Closed
2 tasks done
SamMousa opened this issue Jul 27, 2023 · 10 comments
Closed
2 tasks done

Sandbox escape #4553

SamMousa opened this issue Jul 27, 2023 · 10 comments
Labels
⏱ Awaiting Response This ticket hasn't been triaged yet. 🐛 Bug This is a problem with WeakAuras.

Comments

@SamMousa
Copy link

Is there an existing issue for this?

  • I have searched the existing open and closed issues.

Description

While creating a WA that registers a macro I found out there is a sandbox...
This sandbox uses a blacklist in an attempt to keep people inside while still allowing "some" access to global variables.
Because I don't think blacklists are a good approach I set out to escape the sandbox.

Put this in the on init of a WA and you you can escape the sandbox:

local e = DEFAULT_CHAT_FRAME.editBox
ChatEdit_ActivateChat(e)
e:SetText("/run tempG = _G")
ChatEdit_ParseText(e, 1)
local _G = tempG
ChatEdit_ActivateChat(e)
e:SetText("/run tempG = nil")
ChatEdit_ParseText(e, 1)

WeakAuras Version

5.6.0

World of Warcraft Flavor

Wrath of the Lich King Classic

World of Warcraft Region

EU

Tested with only WeakAuras

  • Yes

Lua Error

No error

Reproduction Steps

See issue description

Last Good Version

Never

Screenshots

No response

Export String

No response

@SamMousa SamMousa added the 🐛 Bug This is a problem with WeakAuras. label Jul 27, 2023
@github-actions github-actions bot added the ⏱ Awaiting Response This ticket hasn't been triaged yet. label Jul 27, 2023
@InfusOnWoW
Copy link
Contributor

The "sandbox" is not supposed to be secure, that's not its job.

Though we probably should add DEFAULT_CHAT_FRAME, ChatEdit_ActivateChat to list of blocked globals.

@SamMousa
Copy link
Author

What's the goal then?
Would it not be great if auras could bundle macros? -- if needed via confirmation by the addon.

@InfusOnWoW
Copy link
Contributor

The job is to make certain things hard to do with WA. Such as creating macros.

@krazyito65
Copy link
Contributor

i think another reason is more, its not really WeakAuras job to care about securing the sandbox. WeakAuras tries make it hard to do specific things to deter a majority of people, but obviously the more well versed or interested parties will find ways past it. When something drastic happens, its usually something that needs to be brought up to blizzard for them too not allow addons access (and has in the past).

The main reason (i speculate) the sandbox is not secure is because anything you can do outside of the sandbox you could do already with your own addon. WeakAuras just needs to make it so that 'legit' users dont accidently get tainted by malicious auras/addons that spoof auras.

@Stanzilla
Copy link
Contributor

Because of the nature of how Lua is implemented in WoW, there is no 100% secure sandbox. The goal was always just to make it a little harder for low to medium threat actors. Thank you for your report though, do you have any suggestions on how we can improve?

@SamMousa
Copy link
Author

Use a whitelist, don't do any proxying for globals, just deny all access.

Also probably you should confirm frame scripts stay in the sandbox, I haven't tested those.

The basic lua sandbox is secure, any thing you provide to it is a risk.

@Stanzilla
Copy link
Contributor

Use a whitelist, don't do any proxying for globals, just deny all access.

Also probably you should confirm frame scripts stay in the sandbox, I haven't tested those.

The basic lua sandbox is secure, any thing you provide to it is a risk.

That is pretty impossible to do tbh, there are way too many things for an allow list.

@SamMousa
Copy link
Author

You could wrap each returned function in a closure that first resets its fenv to your proxy, the current issue is that your blacklist only applies to the perimeter. This could break since stuff, but since we're single threaded if you reset it straight after you should be okay

@InfusOnWoW
Copy link
Contributor

That wouldn't even work with the API you asked at the start, in the macro api you set the action to be taken via a normal string, not a lua function. Thus that's not sandboxed at all in your "idea".

It's pretty pointless to discuss the security of something that is not meant to be secure, though so that's all off-topic.

@emptyrivers
Copy link
Contributor

Just so it's clear, the *entire* purpose of the sandbox, such as it is, is to throw a roadblock in the way of script kiddies who don't know what they're doing, to afford our users & Blizzard some time to adjust to the latest stupid gold scam or whatever. Achieving any level of true 'security' against a determined attacker is well beyond the level of effort & time that anyone here has to dedicate to WeakAuras.

Locking down everything to the degree of e.g. secure headers would just make WeakAuras worse in other ways (namely, our users doing wacky things that become popular is a great source of inspiration for good features). It's simply not worth the trade-off, especially when you consider the level of effort required.

InfusOnWoW added a commit to InfusOnWoW/WeakAuras2 that referenced this issue Aug 1, 2023
InfusOnWoW added a commit to InfusOnWoW/WeakAuras2 that referenced this issue Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏱ Awaiting Response This ticket hasn't been triaged yet. 🐛 Bug This is a problem with WeakAuras.
Projects
None yet
Development

No branches or pull requests

5 participants