johnfactotum / foliate

Read e-books in style
https://johnfactotum.github.io/foliate/
GNU General Public License v3.0
6.17k stars 287 forks source link

Check for vulnerabilities #391

Closed digitalethics closed 1 year ago

digitalethics commented 4 years ago

XML external entity injections (XXE) are security vulnerabilities allowing attackers to interfere with the processing of XML data of an application. I'm not sure how reliable these security advisories on Calibre are but I think it is worth looking into and to evaluate whether or not this currently affects or could potentially affect Foliate in the future, and what kind of fixes or mitigation strategies there are.

johnfactotum commented 4 years ago

You don't need injections to read local files in Foliate, because you already have unlimited file access in the WebView. When JavaScript is disabled, which it is by default, you can't use JavaScript at all so there's no access to anything. I suppose we should add some additional restrictions even when JavaScript is enabled.

If you're using Flatpak it doesn't get to read arbitrary files. But there's always the possibility of injection into the main GJS process (this should not happen but there can always be bugs), which would be much, much more serious, and which is why enabling --talk-name=org.freedesktop.Flatpak is a bad idea. Ideally there should be a portal or at least a more granular approach, but AFAIK that's the only way to access host TTS and dictionaries in the Flatpak. Maybe it should be disabled by default since TTS and offline dictionary aren't really essential.

digitalethics commented 4 years ago

I suppose we should add some additional restrictions even when JavaScript is enabled.

Good idea.

Maybe it should be disabled by default since TTS and offline dictionary aren't really essential.

Yes, I think this is probably better as Flatpak users will typically expect sandboxing. Maybe you could add a guide or wiki with step-by-step instructions how users can manually change the Flatpak permissions model (there's even a new app for this) and what packages to install if they want to use TTS and the offline dictionary. We shouldn't think about this as some kind of trade-off between security and usability (similarly security is often played out against privacy) but instead make sure that both are targeted equally.

johnfactotum commented 4 years ago

With https://github.com/johnfactotum/foliate/commit/25b4df2cc5cf045c8c9daff9fb8c3e06653ef0f3, when JavaScript is enabled, it will still be able to read all files (unless you're using Flatpak), but it won't be able to do anything with that data.

digitalethics commented 4 years ago

unless you're using Flatpak

What kind of permissions model do you use for the snap version? Classic or strict confinement? I wish this were more transparent to users but this is rather an issue with how Flatpak permissions and Snap confinements are displayed on snapcraft and flathub.

If a user is using Foliate via their operating system repositories, then I guess there are major differences as well, especially whether an operating system offers per-app confinement via AppArmor or SELinux. Some operating systems enable AppArmor but without per-app confinement enabled if security profiles are not shipped by upstream applications. Do you think adding a security profile to Foliate is possible and would make sense?

johnfactotum commented 4 years ago

Classic or strict confinement? I wish this were more transparent to users but this is rather an issue with how Flatpak permissions and Snap confinements are displayed on snapcraft and flathub.

It uses strict confinement. Classic confinement apps can only be installed with the --classic argument.

johnfactotum commented 4 years ago

Now external resources cannot be loaded, however there's still a risk: with JavaScript enabled, the book can read local files, and append the contents to the URL of a link; when the user clicks the link, it will open in the default browser, which would send the file contents to a remote website. Allowing unlimited file access is just generally a very bad idea.

So the real, proper solution is to set allow-file-access-from-file-urlsto false. To be able to do that, off the top of my head we can maybe

digitalethics commented 4 years ago

I know that thorium-reader has used Snyk's bot but it's unclear to me whether it helped to quicker understand potential vulnerabilities. Would it make sense to test this with Foliate as well?

danielweck commented 4 years ago

https://snyk.io and https://dependabot.com can be pretty useful with most projects' workflow, but personally I tend to rely much more on https://www.npmjs.com/package/npm-check-updates (aka NCU) to manually verify project dependencies at regular intervals via the command line (typically, at every package update, or at the very least once a week for a top-level app project that has a deep package dependency tree).

danielweck commented 4 years ago

PS: I've used https://www.npmjs.com/package/npm-check and https://www.npmjs.com/package/david in the past as well, but frankly as a command line utility, NCU just does what I need.

I do like David's web service though :)

johnfactotum commented 1 year ago

I think this can be closed now. In the gtk4 branch, the WebView no longer has unlimited file system access, and JavaScript is always disabled. And GitHub's CodeQL and dependabot will check for vulnerabilities