search

FooSoft / yomichan

Japanese pop-up dictionary extension for Chrome and Firefox.
https://foosoft.net/projects/yomichan
Other
1.07k stars 226 forks source link

Extension security #353

Closed toasted-nutbread closed 4 years ago

toasted-nutbread commented 4 years ago

I was recently going through Mozilla's security best practices for extensions page and noticed a few things that may affect Yomichan.

siikamiika commented 4 years ago

Don’t inject moz-extension paths directly

Agreed. It's good that the <iframe> isn't injected before text is scanned, though. I also think that Chrome is not affected by this, because they use the same ID in Chrome Web Store (other than fingerprinting by the fact of having Yomichan installed). What about other identifying data (however small) such as class or data-yomichan-theme?

toasted-nutbread commented 4 years ago

What about other identifying data (however small) such as class or data-yomichan-theme?

I don't see that as being as big of a deal, since it's not truly unique per user. Unfortunately, Mozilla's page doesn't really describe how to hide the assets. They mention it's possible to use data urls or srcdoc, but the host webpage can read both. If the iframe is treated as a empty temp frame which Yomichan subsequently injects scripts into, then Yomichan would need a way of correlating that iframe element to the extension frame ID. I ran into this same issue while trying to implement better popup iframe support, but I didn't find a good solution. At least not without making assumptions on how some of the web extension APIs work which is not covered in the documentation.

I also mentioned this briefly in #292, but a closed shadow DOM may help with this, as scripts won't be able to read the DOM. This could also help isolate the Yomichan styles. (There are potentially issues with closed being able to be bypassed, but we may not have this issue since the context of the web extension scripts is more secure.)

Also: it does seem possible to manually call postMessage from the host webpage on the iframe to trigger the iframe APIs. This may be able to be improved by changing targetOrigin to something other than '*', but this will take some experimentation.

siikamiika commented 4 years ago

They mention it's possible to use data urls or srcdoc, but the host webpage can read both.

I guess a way to protect yourself against non-targeted fingerprinting could be to include a random value inside the srcdoc or data url each time, but it would still be quite easy to identify the usage of Yomichan. Still better than a unique ID, though.

I also mentioned this briefly in #292, but a closed shadow DOM may help with this, as scripts won't be able to read the DOM. This could also help isolate the Yomichan styles. (There are potentially issues with closed being able to be bypassed, but we may not have this issue since the context of the web extension scripts is more secure.)

Certainly looks like shadow DOM could be a solution for this issue if it doesn't mess up focus handling on different browsers.

Also: it does seem possible to manually call postMessage from the host webpage on the iframe to trigger the iframe APIs. This may be able to be improved by changing targetOrigin to something other than '*', but this will take some experimentation.

Scary, that could open the browser for exploitation if there was a vulnerable integration in Yomichan that can be accessed with postMessage.

siikamiika commented 4 years ago

Also: it does seem possible to manually call postMessage from the host webpage on the iframe to trigger the iframe APIs. This may be able to be improved by changing targetOrigin to something other than '*', but this will take some experimentation.

Scary, that could open the browser for exploitation if there was a vulnerable integration in Yomichan that can be accessed with postMessage.

Another solution could be a CSRF style token that Backend provides.

toasted-nutbread commented 4 years ago

Re: eslint-plugin-no-unsanitized: #352 / ecdb476a3a4f6adcb5a88ae2693b609b31c096d1

toasted-nutbread commented 4 years ago

I have committed 912d59d3dfefde5738b4244d74d944795edcc02c, which hides the iframe's URL from fingerprinting. Based on some stuff I found while looking into https://github.com/FooSoft/yomichan/pull/364#issuecomment-586663700.

toasted-nutbread commented 4 years ago

I have also committed created a PR #367, which defers the injection of the /fg/css/client.css stylesheet until after the popup iframe is added to the document. This prevents the detection of Yomichan's presence using CSS rules testing (before the popup is spawned of course).

There is currently an issue with about:blank pages, so this may or may not work.

toasted-nutbread commented 4 years ago

Origin is now checked on posted messages: aee16c443195ff8ab2b0f5f5e8551e44895d48a1.

A unique token is now used for posting messages to the float.html page: 0f46e3a093e7f0c07ad310d8c17e2582bdfd2741

There are two remaining places which could use some modifications: https://github.com/FooSoft/yomichan/blob/0f46e3a093e7f0c07ad310d8c17e2582bdfd2741/ext/fg/js/float.js#L71-L77

These messages are posted to the parent window, so the host webpage would also be able to see these messages. Not only does that tell the host page that Yomichan is doing something, it could also potentially interfere with the host page if it expects messages of the same format.

There is also this: (7d9d45ae10302582ce7431bd72ec4f8604dc5e65) https://github.com/FooSoft/yomichan/blob/0f46e3a093e7f0c07ad310d8c17e2582bdfd2741/ext/bg/js/search.js#L250

Which may have to be changed if the other messages are changed.

siikamiika commented 4 years ago

Awesome, that should patch the attack vector when the other three places are handled too.

There's a related issue in https://github.com/FooSoft/yomichan/pull/364, I had to add checks like this https://github.com/FooSoft/yomichan/blob/0f6c62ab0a0420a13911e410a588419868725914/ext/fg/js/frontend.js#L218 to make sure that the iframe popup doesn't trigger the same method on itself infinitely.

It might be solved by switching to the PopupProxy class but I'm not sure.

Another is to use an ID like in https://github.com/FooSoft/yomichan/pull/366 and reject its own ID, but there has to be some kind of reliable communication from the iframe popup to its host Frontend first.

siikamiika commented 4 years ago

While reviewing https://github.com/FooSoft/yomichan/pull/380, I got interested in the inner workings of Handlebars and found that there are different kinds of ways to compile a template: https://github.com/wycats/handlebars.js/blob/master/docs/compiler-api.md https://handlebarsjs.com/api-reference/compilation.html#handlebars-precompile-template-options

Yomichan currently uses Handlebars.compile, but there is also Handlebars.precompile which does the same thing the template compile script used to do. Handlebars.precompile doesn't use the Function constructor: Handlebars.precompile set compile flag to recursive compileChildren calls use Function.apply only if asObject is true

So, since Anki runs QtWebEngine, it could also use the template once it's been precompiled. It would probably require a fork of Handlebars removing support for Handlebars.compile. This is still missing the step of getting Handlebars into Anki without triggering extension store detectors, because the precompiled templates don't work as standalone functions without the host library.

If the above seems impossible, it could also be possible to turn the AST generated with Handlebars.parse into HTML within Yomichan without doing any eval, but that could be a huge task and also worth upstreaming.

I also found that Handlebars is supposed to be somewhat compatible with Mustache, and while Mustache doesn't use eval (https://github.com/janl/mustache.js/issues/207), it also doesn't support helper functions or nested objects for input.

toasted-nutbread commented 4 years ago

Not sure to what extent this will affect Yomichan, but apparently a v3 of the extension manifest is eventually coming to Chrome. https://developer.chrome.com/extensions/migrating_to_manifest_v3

Noteworthy points:

Are you currently using chrome.tabs.executeScript({code: '...'}), eval(), or new Function() in background contexts or content scripts?

  • Move all external code (JS, Wasm, CSS) into your extension bundle.
  • Update script and style references to load resources from the extension bundle.
  • Use chrome.runtime.getURL() to build resource URLs at runtime.

Potential issues with Handlebars.js.

Are you currently using chrome.runtime.getBackgroundPage(), chrome.extension.getBackgroundPage(), chrome.extension.getExtensionTabs(), or chrome.extension.getViews()?

  • Migrate to passing messages between other contexts and the background service worker.

This is already planned.

CORS related changes for content scripts. See: https://www.chromium.org/Home/chromium-security/extension-content-script-fetches

This shouldn't affect Yomichan, but #428 was having a similar CORS issue.

toasted-nutbread commented 4 years ago

Closing as most security concerns have been addressed, except the Handlebars eval issue, which is large enough to be a separate issue. The one other potential "security" issue is that window.postMessage is used for iframe parent resolution, but it's not so much of a "security" issue as it is a "potential to mess with the underlying website" issue.