Skip to content
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

Check for vulnerabilities #391

Closed
digitalethics opened this issue May 27, 2020 · 10 comments
Closed

Check for vulnerabilities #391

digitalethics opened this issue May 27, 2020 · 10 comments

Comments

@digitalethics
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

With 25b4df2, 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.

@johnfactotum johnfactotum changed the title Check for XXE vulnerabilities Check for vulnerabilities May 29, 2020
@digitalethics
Copy link
Author

digitalethics commented May 29, 2020

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
Copy link
Owner

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
Copy link
Owner

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

  • Read the file and send it to the WebView, like maybe as a binary string (this is probably a bad idea)
  • Use a custom URL scheme with a special URL that will resolve to the opened file

@digitalethics
Copy link
Author

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
Copy link

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
Copy link

danielweck commented Jun 8, 2020

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants