I didn’t expect you to move the update to the main site before all the reported bugs are fixed – seems like I have to hurry up with my userscript fixes 
3 Likes
Sorry about that, I was hoping turning on the Script Compatibility Mode setting would be a good enough solution
Super cool! The API design is really ergonomic. One thing that might be nice is if the global registration could be a dictionary keyed on the script version. That way, instead of initing a separate instance every time the script is included, your library could automatically just use a single instance for each used version that is shared among all scripts using that version.
Also, I’m not sure that your usage of unsafeWindow is secure. Your script does use grant none, so if the script is directly installed instead of included, there is no issue since the script is directly injected into the document in that case and window and unsafeWindow I think are the same object. However, if the script is included in another script that does not use grant none, then I think it may introduce a privilege escalation vulnerability that would allow any untrusted javascript running on the page to access the sandbox containing the userscript. (I’m not sure exactly how script managers handle inclusion in this case. If they just inject the grant none script directly into the page the same as if it were a standalone script, then there is again no issue and the usage of unsafeWindow could be replaced by window in your script. However, if an included script gets run in the same sandbox as the script including it, this would create a security issue.) Specifically, any untrusted script in the page could get access to the global prototype of object for the sandbox from the object that you set as a property of unsafewindow. Even if you clear the prototype of that object, I think there would still be an issue since you would actually need to recursively clear the prototypes of all objects that are descendants of it and I don’t think you can do that with function objects. Also, even if you were able to fix all of that, I’m not sure whether there are other dangerous properties. There are also security gotchas with even just reading properties of unsafeWindow. For this reason, I would recommend avoiding it entirely. As far as I know, the only secure way to expose a function that the page can call is to have the trusted privileged script insert an untrusted unprivileged script into the document which exposes the methods and then communicates back to the privileged script using event listeners or postMessage. (I.E., your trusted library in the sandbox has a function called myfun, you inject an untrusted script which exposes a function also called myfun which dispatches an event called call_myfun. The trusted library listens for this event and calls the actual implementation when it receives the event. You could alternatively implement this approach using postMessage, but postMessage has security gotchas of its own so I would recommend the mentioned events approach instead.)
I have it currently set up so that wkItemInfo is only set up once and all scripts use the same object – except if a newer version appears afterwards, then it replaces the current wkItemInfo, resulting in multiple versions running at once (which should not happen if all scripts @require the newest version).
Yes, that would be problematic if the script would run on any uncontrolled webpage – but since it is only intended for WaniKani, shouldn’t this be alright? The only scripts running on WK should come from WK, which I think makes them trustworthy, and maybe other userscripts?
My mistake.
It’s mainly the userscripts I don’t trust. It’s not a totally hypothetical concern, since for example, the community mnemonics userscript had a vulnerability which would allow anyone on the internet (not just the script author) to execute any malicious javascript they want in the browsers of all users of the script.
(For anyone reading this who is unfamiliar with what happened with community mnemonics, I say “had a vulnerability” in the preceding sentence in the sense that it had a vulnerability until the script authors remotely disabled the server the script used, at which point, the vulnerability was still present in the code, but it could not be exploited because the script wouldn’t be able to connect to the server with the mnemonics. As far as I know, there is not currently any deployed and working version/fork of the community mnemonics userscript for this reason, and if that has changed since I last checked, I would not install it due to the repeated and likely still unresolved difficulties different script authors have had writing a version that has a reasonable assurance of security.)
If it’s too much work to implement a fully secure method of sharing the library for privileged userscripts, maybe it would make sense to have each privileged userscript use it’s own isolated copy that isn’t published to unsafeWindow? I haven’t benchmarked it, but after looking at the code some more, my guess is that most of the performance impact from having it included multiple times is from parsing the javascript when the page loads, rather than init or handling observer callbacks. And time spent parsing isn’t affected by your current setup vs having isolated copies.
I think it would be easier for any hypothetical hacker to create their own WK userscript with elevated permissions, instead of trying to exploit WK Item Info Injector to gain these permissions. It would probably take at least several months until someone notices the malicious code in their script.
Or the hacker could create a script that “accidentally” runs on https://www.wanikani.com/account/subscription, and “accidentally” sends all the entered payment information to their own server. That does not even require any elevated permissions.
I still don’t understand why there seem to be so many difficulties to remove the XSS vulnerability from this script. The received unsafe text could just be parsed manually for the few expected tags, and then these tags can be created by the script using .textContent instead of .innerHTML, so that it cannot be interpreted as code. I do the same thing in my WaniKani Old Mnemonics userscript.
I’m a bit curious how this can be exploited? I’m not too surprised that it is possible, but I wasn’t able to find an explanation on the internet.
That would probably be the most practicable solution to your suggested issue, because even if I were to drop the support for @require and instead insist that the end user has to install WK Item Injector themselves (similar to WK Open Framework), the problem would just shift to the moment the privileged userscript registers its callbacks to wkItemInfo (or an imposter of it).
I don’t especially like the idea of multiple instances of WK Item info Injector running at the same time. I don’t know if the order of the injected items would stay consistent in this case, and the injection of the DOM elements would be more spaced out over time, which might cause the browser to do reflows in between, increasing the performance impact.
I was also thinking about enclosing the whole script in a string and only evaluating it if necessary (wkItemInfo not defined or older version), but this seemed like going a bit too far just for performance optimization. And I don’t know if it would actually be faster.
I don’t think it is possible to achieve “full security”. After addressing this issue, the next vulnerability you could bring up is that I’m not sanitizing the values I read from jStorage or from the DOM, and this could probably go on forever.
When I decided to use unsafeWindow in my script, I did so because I was of the opinion that the risk of hackers trying to exploit such a niche script was low enough so that it is justifiable to trust all scripts running on WK. Especially because a hacker would have so many easier ways to get what they want.
Somehow I missed that you were using jQuery heavily, which obsoletes what I was originally advocating for. It appears you are able to access $ in the global scope without explicitly using unsafeWindow to access it, so that probably means that your library is being injected into the page and therefore is running unprivileged so maybe you can remove unsafeWindow from it entirely? (It also means that it would probably be too much work/overhead to get it to run inside the script sandbox without using unsafeWindow.)
That of course then shifts the issue to how scripts that use the library can do so securely, which I believe you alluded to. I looked at the sandbox feature grants that these scripts actually use and it looks like all of them could be replaced by shims that provide similar functionality without needing to run in a sandbox, which would allow a switch to grant none and eliminate all the potential issues we have been discussing. The only potential problem I thinkis if any of the resource declarations are for resources with restricted CORs, but it looks like raw.githubusercontent.com at least has permissive CORS headers. Also, note that wkof has support for resource fetching and caching built in (subject to standard CORS limitations) so that might be a good replacement for @resource.
I agree it’s probably not worth it. And as you said, I’m not even sure it would improve performance, since I don’t know how using eval would affect the caching that javascript engines do after parsing/compiling. If you do decide to something like this, you could use wkof’s resource fetching and caching.
Also, unrelated, but the single biggest impact on page load times I’ve seen in general comes from wkof blocking (waiting) on network requests completing in some cases where it doesn’t need to (like to read settings), blocking on updating its cache in indexeddb when it could defer the cache update to when the page is idle, and storing large lists of things in single indexeddb rows instead of breaking them up, which makes it harder to update without blocking for a long time.
The issue is they wanted mnemonics to be able to contain not only text but also certain other types of html tags. And to save time, instead of writing a parser which parses the mnemonic and creates html dom elements subject to whatever security rules you want (like whitelisting attribute names and ensuring anything that is supposed to be text is set using textContent) they wanted to use regex replacement on the raw html string directly.
The exploit has to be specific to the application. Basically, having the prototype lets you override inherited properties for all preexisting and future instances. You can then override these properties with getters/setters so that the the attacker obtains access to the instance object whenever the property is accessed or set. The attacker can also of course access any objects reachable from these instances e.g. obj1.obj2.obj3 and their full prototype chains. This, in turn, lets you get access to even more objects and their properties which you can repeat until you get access to an object where changing it lets you do something dangerous.
As an example, getting access to the window object lets you do pretty much anything the userscript itself is permitted to do. Some of my scripts used to call window.__defineGetter__ in order to listen for when certain javascript objects like jQuery and wkEnableAudio load. So the attack would look like: get prototype of object → override __defineGetter__ → wait fortrusted script to call it for window → get reference to window. IIRC, all my scripts use grant none so there is no harm in this happening with them, but one can imagine a privileged script doing this too. (This example isn’t a perfect real world example since __defineGetter__ is deprecated in favor of Object.defineProperty and it is harder for an attacker to get access to Object than Object.prototype. The main point is that it is difficult to reason about for a script author to be able to try to defend against this and the ideal solution is not to allow lower trust code to access any trusted objects at all.)
For the point about the relative ease of exploiting the library from an unprivileged userscript posted by an attacker vs an attacker directly doing something bad in a privileged userscript they post, I think my main concern is a userscript which accidentally allows XSS, like the community mnemonics script accidentally did. In that case, an attacker could have attacked without needing to convince people to install their attack script at all which makes things easier for them, but would not gain any special script permissions available to scripts in sandboxes. In that case, being able to elevate by exploiting a privileged script might be useful to them. I do agree that WaniKani is somewhat niche and it might not be worth an attacker’s time to target WaniKani users who use certain scripts.
Congrats, React is the bomb 
I wouldn’t call 8 calls to $.jStorage “heavy usage”, but yes, WK Item Info Injector uses jQuery for reading the session storage.
No, @required scripts are just prepended (Tampermonkey, Violentmonkey) or appended (Greasemonkey) to the main userscript, so the effect is exactly the same as copy-pasting the library code into the actual userscript. It’s just that in Tampermonkey and Violentmonkey, objects defined in the page’s global scope can be accessed with or without unsafeWindow:
unsafeWindow.$.jStorage.get('currentItem');
works, and
$.jStorage.get('currentItem');
also works, but
window.$.jStorage.get('currentItem');
does not work.
I don’t intend to introduce a wkof dependency into WK Item Info Injector.
But I don’t think an attacker needs WK Item Info Injector to do this. In Tampermonkey and Violentmonkey, objects in the userscript are by default affected by changes to the object prototype in the page. As a test, I use this userscript:
// ==UserScript==
// @name Security Test
// @version 1
// @match https://www.wanikani.com/kanji/*
// @grant GM.xmlHttpRequest
// @grant GM_xmlhttpRequest
// ==/UserScript==
"use strict"
let inSandbox = (typeof unsafeWindow === `object` ? unsafeWindow : window) !== window;
console.log(`Security Test runs in ${inSandbox ? `sandbox` : `page`}`);
let o = {};
setTimeout(() => console.log(o.toString()), 5000);
and then visit a WK kanji page. Before the 5 seconds of the timeout are over, I enter into the browser console:
({}).__proto__.toString = () => `modified`;
The result of o.toString() in the userscript is always modified, no matter if it is privileged or not (by switching to @grant none).
Greasemonkey has a tighter sandbox, and as far as I can tell, it currently does not even distinguish between scripts with @grant none and more privileged scripts. Even if I write
unsafeWindow.securityTest = {f: o => console.log(o)};
into a @grant none script, the browser console tells me that the object securityTest is restricted.
Thart’s really interesting. I looked into it some more and it appears that in older versions of certain script managers like Tampermonkey, things did work like I said with scripts that used grants running separately with their own separate prototype, etc. However, it appears that at some point browser vendors made changes that prevented script managers from offering unsafeWindow for userscripts with this kind of sandboxing due to security concerns along the lines of what I mentioned. Maybe to preserve compatibility, some script managers are now injecting even grant using scripts into the page’s javascript context with a communication mechanism that proxies calls to gm_* functions back to the script manager? (Presumably something like postMessage or dispatching events.) If true, that means one should probably assume that anything in the page may be able to use that same communications mechanism directly without needing gm_* proxy functions. So presumably, any grants for one script could in principal be used by all javascript on the page if one is willing to reverse engineer the communications mechanism that the script manager uses. So I guess that would be another reason to want everything to be grant none.
I think this is correct. And looking at the code of the injected script, it seems that the GM_* functions are even in the global window object for a short time:
(function(that) {
((context,fapply,console)=>{
with (context) {
(module=>{
"use strict";
try {
fapply(module, context, [, , context.CDATA, context.uneval, context.define, context.module, context.exports, context, context, context.unsafeWindow, context.console, context.cloneInto, context.exportFunction, context.createObjectIn, context.GM, context.GM_info, context.GM_xmlhttpRequest]);
} catch (e) {
if (e.message && e.stack) {
console.error("ERROR: Execution of script 'Security Test' failed! " + e.message);
console.log(e.stack.replace(/(\\(eval at )?<anonymous>[: ]?)|([\s.]*at Object.tms_[\s\S.]*)/g, ""));
} else {
console.error(e);
}
}
}
)(async function tms_a6797c23_123e_543b_32564d87e1a(context, fapply, CDATA, uneval, define, module, exports, window, globalThis, unsafeWindow, console, cloneInto, exportFunction, createObjectIn, GM, GM_info, GM_xmlhttpRequest) {
// ==UserScript==
// @name Security Test
// @version 1
// @match https://www.wanikani.com/kanji/*
// @grant GM.xmlHttpRequest
// @grant GM_xmlhttpRequest
// ==/UserScript==
"use strict"
let inSandbox = (typeof unsafeWindow === `object` ? unsafeWindow : window) !== window;
console.log(`Security Test runs in ${inSandbox ? `sandbox` : `page`}`);
let script = document.createElement(`script`);
script.textContent = `({}).__proto__.toString = () => 'modified';`;
document.body.append(script);
let o = {};
console.log(o.toString());
})
}
}
)(that.context, that.fapply, that.console);
}
)((()=>{
const k = "<here was the randomly generated variable name>",
r = this[k];
delete this[k];
return r;
}
)())
The four lines right at the end seem to retrieve the userscript context (that includes GM_xmlhttpRequest) from this[k], where this is the window object. But k is randomly generated, so you cannot use the window.__defineSetter__ trick to intercept the short time it is available.
Anyway, I agree that @grant none will always be a bit safer, but I still think that it is justifiable to assume everything running on the WaniKani page is safe. Aside from the Community Mnemonics script, I can’t think of any userscript that uses data generated by other users, so I don’t think XSS attacks have to be expected. And I especially don’t think that adding wkItemInfo to the global window object adds a notable security vulnerability. Of course, wkItemInfo could be replaced by a malicious imposter object that feeds the privileged script with wrong data, but the same thing also applies to wkof.