-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add sandbox
attribute to the iframe generated by Kiwix Serve to prevent top-level navigation breaking out of the iframe (or malicious scripts)
#604
Comments
sandbox
attribute to the iframe generated by Kiwix Serve to prevent top-level navigation breaaking out of the iframe (or malicious scripts)sandbox
attribute to the iframe generated by Kiwix Serve to prevent top-level navigation breaking out of the iframe (or malicious scripts)
Removing
|
Kiwix Serve doesn't use Kiwix JS under the hood, it merely creates an iframe so that it can show its toolbar. But the content can overwrite the iframe in the same way as in Kiwix JS and KJSWL, so there is a vulnerability. If there is no manipulation of DOM content in the iframe, and the whole page is simply recreated (with the iframe) on each page load, then it should be possible to use stronger sandboxing than in KJS. |
The documentation of
Unfortunately, my initial attempts to make the viewer work with only one of |
I am also attaching a tiny ZIM-file demonstrating the problem: naughty.zim.zip |
Thanks @veloman-yunkan . I opened your naughty.zim in v2.4.0 of https://pwa.kiwix.org, which was "hardened" (insofar as is possible) yesterday, and got the following in console.log: It was blocked from breaking out. I agree that adding Another thing that can be done is to inject a CSP into the incoming HTML. I do this currently in Kiwix JS PWA. This is what I inject:
The aim of this is to prevent the iframe contents from accessing the Web at all (it effectively blocks such access). I'm not sure if it's any different from adding the sandbox on the iframe, but until yesterday I was only using this CSP. |
I guess there is nothing more we can do here? Should I close the ticket? |
Well there is one other possibility which might be more robust than setting the sandbox on the iframe. That would be to serve the content coming from the ZIM with a sandbox CSP header. See https://developer.mozilla.org/en-US/docs/web/http/headers/content-security-policy/sandbox . This would not be subject to tampering by the document itself. I am thinking of this for Kiwix JS, as we can set the headers in the Service Worker. I guess this would be a separate issue, as this issue is about the iframe sandbox. |
@veloman-yunkan Any feedback on this? Should we implement it in kiwix-serve? All of this is pretty unclear yet to me (I don't invested enough time) but I'm not in favour of dynamically modifying the HTML at each loading for this reason. |
@kelson42 We don't have to modify the HTML - we can set a HTTP header in our responses. |
Agreed, this is not about modifying HTML, only setting the response header. I haven't tested this solution, but if it works, it would probably be the most robust solution for security (additional to having the iframe sandbox). |
OK, I've now tested this in Kiwix JS PWA code. I removed the iframe sandbox and instead added a Content-Security-Policy response header in the Service Worker (I haven't commited this code yet, as I'm experimenting). I observed the network requests and responses. The results are as below. You can see the response header in top-right box, but you can also see the warning generated by Chromium (bottom pane, gold-coloured text): The question is, is this true or is this just a generic warning? Remember, the iframe no longer has a sandbox attribute (I've double-checked it's no longer there). So how can a document in the iframe alter its own response header which is sent before it is even loaded? It surely can't rewrite the server or the Service Worker. Maybe it can search through the parent elements of the iframe until it finds a div that it can fill with spammy content? The question to decide here is whether setting the headers is more secure than using the sandbox attribute of the iframe, or whether we should have both for good measure. |
And this doesn't prevent @danielzgtg's poc1 ZIM attack (here, an additional iframe has been added by the content in the iframe at the top level, and it has loaded third-party content). |
I can tighten security somewhat by specifying frame-src directive in CSP. In this case, using the meta http-equiv tag in the top-level document, like so:
This at least prevents the poc1.zim from loading the third-party image, but doesn't prevent it creating a top-level iframe: |
Unconditional sandboxing (due to kiwix/libkiwix#906) effectively blocks navigating to external webpages even in a |
We solved that in Kiwix JS by registering a click on the iframe and inspecting it. Will find the code when I get back to a PC. |
OK, relevant code is here: https://github.com/kiwix/kiwix-js/blob/main/www/js/app.js#L1587. Transcribed:
|
There is |
In my experience, FWIW, I think |
I agree for |
@veloman-yunkan @Jaifroid @danielzgtg Can we close that one now? |
Yes. |
As reported by @danielzgtg, please see:
For proof of the vulnerability, simply visit https://library.kiwix.org/content/wiktionary_en_all_nopic_2023-02/A/Wiktionary:Offline, and notice how a script in the latest Wiktionary attempts top-level navigation and breaks out of the iframe.
This should be preventable by adding the sandbox attribute to the iframe, e.g.:
sandbox="allow-same-origin allow-scripts"
Kiwix Serve probably does not need the other directives that KJSWL needs, such as
allow-modals allow-forms allow-popups
, but they may be needed for some Zimit ZIMs, so can be considered. Note that havingallow-same-origin allow-scripts
generates a warning in Chromium browsers:If Kiwix Serve does not need to inject anything into the iframe, then maybe it can do away with
allow-same-origin
, and so harden security further. That is unfortunately not an option for Kiwix JS.The text was updated successfully, but these errors were encountered: