Previsualisation broken

[kovidgoyal]

Anyway I just wanted to point out the security implications of allowing access to local content. How you solve is up to you.

[KevinH]

Understood and thanks.

If users are worried, they can just disable javascript for a book until we fix this in the next release.

I think I going to try to deny all custom protocols in acceptNavigationRequest, and for file:/// protocols, make sure that will be in the current epub folder, or in our internal mathjax location or in our current user user css location, otherwise reject them.

Thanks again,

KevinH

[KevinH]

But that may not work, according to what I just read in the docs, local resource urls are not passed through acceptNavigationRequests. So your way of registering our own protocol may be the only way to get to filter all url requests!

[Turtle91]

Just wondering... is Sigil in the business of providing security against evil epub makers, or is that the purview of the publisher? Do other epub software all have custom URL schemes??

If this is something Sigil developers have been doing in the background all along, then color me that much more impressed! Otherwise, in my not-so-knowledgeable opinion, it seems like you may be biting off a huge chunk in trying to correct a(nother) Qt bug???

[KevinH]

Ever since we moved to QWebEngine, we have used settings to try to prevent bad epubs from doing bad things. This includes carefully unpacking zip archives that have been maliciously crafted. We allowed the user to disable book level javascript. We tried to prevent book level javascript from accessing remote resources, and file:/// level urls by disabling them in settings, etc.

So right now using Qt 5.12.x on Windows and macOS,we should be reasonably safe. But a recent change in Qt 5.15 will require us to re-enable javascript accessing file:/// urls. As Kovid, rightly pointed out, that is less safe than what we had before, and that we should do something about that.

The options appear to be limited.

1. We can change Sigil to create and internally use our own url scheme/protocol, as Calibre does. And in that way filter out any bad accesseses

2. Or we can create a QWebInterceptor class that would pretty much do the same thing but without requiring a new scheme and registering it.

But I had hoped for a simpler solution using acceptNavigationRequest but it is too limited to do what we need.

So yes, once javascript was allowed in epub3, and we had the tools to separate book javascript from Sigil javascript, we have tried to protect the user where we could.

This is just an unexpected Qt 5.15 change, that has made security harder not easier for our case.

No worries for current Sigil on Qt 5.12.x, just something we will have to fix for advanced Arch Linux users who have already moved to Qt 5.15.

[Turtle91]

Very interesting. Thanks for the explanation!

[KevinH]

And to come full circle, using and registering our own protocol as calibre has done is probably the easiest path forward but will require help for how we use mathjax.

[eschwartz]

Ugh, this is just amazing in all the bad ways.

Thanks Diap for pointing me at this. I can certainly backport the relevant sigil/pageedit commits to my Arch package. But before I do so -- given the discussion that's happened here AFTER your PM, is this still something you'd recommend doing? If you're going to write a custom protocol very soon, is that something I should be waiting for instead?

(Javascript should be disabled by default, so users who enable it will not be walking into this security hole by accident. So it seems reasonable to make things work for most users. Maybe I could print a post_upgrade message warning that qt has broken the security hardening if you've enabled javascript, and it should only be enabled if you trust the book.)

[DiapDealer]

If you're willing, I would say go ahead and backport the one-liner so Sigil is useable again for the time being. I'm not really sure when we'll get a new release with the permanent fix ready. It could be a little while yet. The post upgrade message warning sounds like a good idea too.

[KevinH]

Yes, any real fix is going to take some time to develop especially for the multi-mainwindow macOS version since I want to force even the new protocol to be inside the host ebook folder to prevent just moving the problem to the new protocol.

So please consider the one-line change for Arch as a stop gap until a our next release is made (and that may be quite a while yet).

[eschwartz]

Fixed in [community], allowing for (~1 hour) time for your currently configured mirror to sync the changes. Affected users can run the following commands to ensure they have the fixed versions:

# for sigil
sudo pacman -Syu "sigil>=1.2.1-2"
# for pageedit
sudo pacman -Syu "pageedit>=1.2.0-2"

[roger64]

Took note of it and feeling pampered like a first class citizen.

Congratulation for such a nicely driven inquiry.

Thank you all for this extraordinary unity of purpose.


and indeed I did get this warning when the update arrived

[...]
( 9/11) mise à jour de pageedit                    [######################] 100%
Warning: Qt5 5.15 has broken the security sandbox for ebooks with javascript.
If you've enabled javascript support, be aware that untrusted EPUBs can now
exfiltrate user data.  See
https://www.mobileread.com/forums/sh...d.php?t=330486
(10/11) mise à jour de sigil                       [######################] 100%
Warning: Qt5 5.15 has broken the security sandbox for ebooks with javascript.
If you've enabled javascript support, be aware that untrusted EPUBs can now
exfiltrate user data.  See
https://www.mobileread.com/forums/sh...d.php?t=330486
[...]

I also just disabled javascript for ePubs (see screenshot)

[KevinH]

After looking at this and studying different approaches, I think I am going to implement a QWebEngineUrlRequestInterceptor that will simply screen all local urls to block any that do not target to something in the epub, or something in the local copies of mathjax or user css style sheets.

This approach should be faster as creating our own custom scheme protocal will need to double buffer (double read/write a file) to send replies while still allowing the main thread to access them.

So allowing localcontent can access file urls to be true and screening out any sneaky/malicious attempts seems the most straight forward approach.

[KevinH]

Okay,
I have just pushed an experimental fix that should help protect against bad file: based urls coming from a maliciously crafted epub/javascript.

It sets a QWebEngineUrlRequestUnterceptor in main.cpp that will examine every requestedURL and if it uses the file: scheme, it will only allow it if it points into one of the following 3 folders:

- the current epub main folder
- the mathjax main folder
- the user's Sigil preferences location

Any other file: destination will be blocked.

I have kept it as simple as possible and enabled a bunch of debug information.

I have no idea if it will work on Windows (due to file separator concerns) and Linux but there should be enough info in the debug output to help track down and fix any issues.

So if anyone has access to a Windows build or a Linux build, I would appreciate the feedback, especially for Qt 5.12.x and Qt 5.15.

Note: This fix depends on setting access to local file content to true (ie includes DiapDealer's fix for this issue).

[eschwartz]

For starters, it's typoed.

/home/eschwartz/git/Sigil/src/main.cpp:549:39: error: ‘class QWebEngineProfile’ has no member named ‘setURLRequestInterceptor’; did you mean ‘setUrlRequestInterceptor’?
  549 |  QWebEngineProfile::defaultProfile()->setURLRequestInterceptor(urlint);
      |                                       ^~~~~~~~~~~~~~~~~~~~~~~~
      |                                       setUrlRequestInterceptor