Improving Plugin Security

Development
1

Experimenting with the plugin system, I had a few thoughts that I'd like to discuss with the developers:

Is node integration needed for plugins?
Currently, both the main plugin instance and the content scripts have access to the require function and all of nodejs's built-in modules, including fs. I think this is an unnecessary risk, as non-malicious plugins should have no need for this feature.

For the window, in which the main plugin code is running, getting rid of it seems straightforward: we could simply turn off the nodeIntegration flag, and move the sandbox proxy code, which depends on require, to a preload script. I think this could be done while preserving the plugin API. What do you guys think?

Content scripts are more tricky, on the other hand. I suppose the most bulletproof way would be to only run them in iframes. For CodeMirror scripts, I think this would mean wrapping the entire editor into a sandbox, but for markdown plugins, I'm not really sure. Maybe they could run in the iframe where the rendered markup is displayed? Either way, this would be a much bigger task than the main plugin.

Another idea I had, was to put the content scripts into a wrapper which overwrites all possible ways to reference require with undefined (i.e. require, window.require, self.require etc). This has a ton of edge cases and potential ways to get around it, so it will never be as secure as an iframe, but it would require minimal changes.

Plugins crashing Joplin
Prompted by this bug report, I have done some experiments, and it seems that almost any exception coming from CodeMirror content scripts will crash Joplin. Exceptions thrown at the very top of the module are caught, but CM plugins usually rely on registering callbacks, and errors coming from those are especially tricky to catch. Some are caught by CodeMirror, but most of them not. Here's a gist with a minimal content script that can crash joplin every time.

Of course, moving the markdown editor to an iframe would solve this issue as well. But, an easier solution would be to add an error boundary around the editor. The problem with this is that error boundaries can't catch exceptions coming from setTimeout and the like, but CodeMirror does call plugin-defined functions from timeouts in a number of cases (e.g. this one came from a timeout too).

One other idea I had was that maybe the CodeMirror object passed to the plugins could be wrapped in a Proxy, which wraps any callback passed to CM in a try-catch. This too has a bunch of edge cases to deal with, for example sometimes callbacks return callbacks or even objects of callbacks.

2

We give access to native modules because it's needed for various plugins. There's more info about the security policy there: Plugin repository? - #20 by laurent

As for content scripts, it's always been a trade off - we give lower level access to the app, but we know that it can result in them crashing the app. We have various checks here and there to make it more robust and we can add more over time. There's an issue on GitHub about such crash which I'd like to fix at some point.

3

I didn't think about that, then I'm sorry for the ignorant suggestions.

But what do you think about adding the error boundary around the markdown editor? It would mostly fix the issue you mentioned, without much changes.

4

No, you're right to question how it works. The thing is we can't know in advance what plugins developers might want to create and it makes sense to give access to native modules because they might need it. For example, I assume the backup plugin can't work without access to the filesystem.

Could be, but what would the error boundary do? Just display an error instead of the editor?

5

Another option might be to somehow detect abnormal exit and offer to restart in safe mode.

BTW, my resource search plugin relies heavily on both fs and sqlite. Please don't taken them away from me.

1 Like
6

Another option might be to somehow detect abnormal exit and offer to restart in safe mode.

Yes I think we'd need that, even without taking into accounts plugin support. For example Mermaid can freeze the app with some special code, and this is a built-in module.

I guess something that detects the crash, then on next restart we display a message "The app didn't close normally, would you like to start in safe mode?". Safe mode would disable all plugins and perhaps even start the app with a plain text area instead of the regular editors (as they could also have a bug that freeze the app).

7

I thought maybe it could remount the editor without content scripts, so it would still be usable without having to restart Joplin. While also displaying some error message, of course.

8

Maybe worth disabling synchronization as well to avoid corrupting remote storage. It's unlikely but better be safe I think.

9

Yes that would make sense too.

10

Content script and markdown-it plugins I guess, because Mermaid is not a content script but it can crash (or at least freeze) the app too.

11

@mablin7, in terms of plugin security, it's a tricky issue as any restriction we set means certain features will no longer be possible. Perhaps a first step would be to add support for the manifest "permissions" array. It's been there since the beginning but never implemented and with it the plugin could declare what features it's going to need or not. For example the Tab plugin doesn't need filesystem or web access, so knowing that it's safer to install it than, say, the Backup plugin which needs filesystem access.

Another possible security related project would be to create some tool that checks the plugin repo for malware. For example it could automatically download all plugins and build them, to check that the builds are reproducible. With that info, it could also try to scan each plugin source code for potential malware, check what modules or functions it's calling, vet its dependencies, etc. Such tool would provide valuable info on the plugins, and we could even give a safety rating to each plugin.

2 Likes
12

Do you think implementing permissions could work as a GSoC project? It seems like a really interesting challenge, though I'm not sure about the details yet. What sorts of permissions do you think should be distinguished? Filesystem is one for sure, also probably execution of external programs. Maybe file acces could be even more granular, by, for example, only allowing specific directories.

I think internet access permission would be nice too, though it might be trickier to make it work, since there are so many ways JS can access the network (XmlHttpRequests, fetch, adding DOM elements which load things from the web, navigating to a new page, submitting forms, etc., and all of these in content scripts as well).

13

Seems like a big change with an equally big potential for introducing bugs.
Personally I think as long as users are able to recover from plugins crashing Joplin, it's not a big deal

14

In this case a safe mode or an option to disable a specific plugin at startup would help not crashing Joplin at start.

15

Do you think implementing permissions could work as a GSoC project?

Maybe, but I guess the issue is that we don't know much about all this at this point. What can or cannot be done and we don't have a clear spec either as it's never been discussed in the forum. So there's a lot of unknown so it's hard to tell if it's a good project. Perhaps investigate a bit further and see if that seems realistic as a two months project?

Are there other projects you might be interested in?

16

Sure thing! I'll come up with a list of requirements by tomorrow.

Yeah! I'm also looking into the kanban board plugin. I don't yet have a full plan for that either, but in the next day or two I'll be working on that as well. I hope to post a reviewable proposal draft on it by the end of this week, and also, maybe, about the plugin permissions if I have enough time and it ends up looking doable.

2 Likes
17

Here is the list of requirements I came up with. Let me know what you guys think! Is this a good approach? Or are the changes too big in comparison to the added security? After all, a malicious developer could still just create a plugin which seems to have a good reason to access the user's files and then do whatever it wants, so this still can't replace inspecting plugins. On the other hand, having permissions can allow auditors to focus on the plugins, which require special privileges.

I've also added some rough estimates in italics on how much time each point would take. In a final proposal, these would be more concrete of course.

Plugin Permissions Requirements

  • permissions key in plugin manifest
    • List of permissions
    • Each permission can be a string ("FILE_SYSTEM")
    • or an object with additional settings
      {
  "name": "NETWORK",
  "domains": ["example.com"]
}
  • PluginService (~1-2 days)
    • Load permissions key from manifest
    • Store the plugin's permissions in settings
    • Permissions are considered granted if the plugin is enabled
  • Specific permissions to be implemented (only considering desktop)
    • Network permission
      • Allows plugin code to access the internet
      • Main plugin window (~1-2 days)
        • Set Content Security Policy header by adding a meta tag when opening the window in PluginRunner.
        • By default it only allows loading the two script tags necessary for running the plugin.
        • Specific URLs can be whitelisted in the domains section of the permission object
      • Content Scripts (~2-3 days -- make sure everything still works)
        • Set Content Security Policy header on the whole joplin app, only allowing necessary URLs like the plugin repo and the sync target.
        • To keep things simple, content scripts have no network access, regardless of permission settings.
      • The problem with this approach is that it cannot be controlled on a per-plugin basis. If one plugin has permission to access example.com, all of them can.
    • File system permission
      • Allows access to node native modules, including fs
      • Main plugin window (~3-4 days)
        • Disable nodeIntegration by default
        • Enable only for plugins which have this permission
        • Move sandbox proxy to a preload script
      • To keep things simple, content scripts have no file system access, regardless of permission settings
      • CodeMirror content scripts
        • Create wrapper around Editor.tsx which displays the editor in an iframe. (~2 weeks)
          • This is needed because the window object can be accessed through the CodeMirror instance, and thus content scripts could execute code in the main window, even from an isolated context
        • Alternatively, the content scripts could be run in an isolated context, and be provided with a proxy of the CodeMirror object, which doesn't allow access to the window object, instead of the real one1
      • MarkdownIt plugins
        • Create a wrapper which runs the plugin functions in an isolated context, only allowing access to the MarkdownIt object, not the window 1
          • As far as I can tell, it's not possible to get the window object through the MarkdownIt object
        • Assets are already rendered in an iframe, just have to ensure CSP is set for that one as well
  • GUI (~1 week)
    • When browsing installable plugins, display a list of required permissions for each plugin
    • When installing a plugin with permissions for the first time display a warning, that installing automatically grants the required permissions
    • Also display permissions for already installed plugins
    • In the Advanced section, show a list of whitelisted URLs
  • 1 Both of these would require a ContentScriptRunner service running on the main thread, which creates the contexts and handles the communication with them through the contextBridge (~2 weeks)
  • Automatic tests (~2-3 weeks)
    • Using a test framework (like spectron) run integration tests on the desktop app as a whole, testing the security related features
    • Example test cases
      • Trying to access network without permission from content scripts
      • Trying to inject JS into the main frame
      • Trying to run require in different places (content scripts, callbacks, etc.)

Further development

  • List of allowed directories or globs for plugins with file access
  • Permission to run an external program
18

Impressive work. And this is all I can say as I have no opinion on how or if this should be implemented at all.

Oh, one more thing -- not sure if any of your existing items cover this (does not seem so) but maybe there should be a separate permission for calling external programs via child_process?

19

Thanks!

I mentioned at the bottom, under further development as "Permission to run an external program"

20

I feel there's three main parts in this proposal:

  1. Tighten the main application sandbox by enabling CSP headers and setting up nodeIntegration. I've had a look at both before, and once you turn this on, a lot of things stop working. For example the app loads various JS and CSS files, might have embedded scripts, etc. which these security settings don't like. So to get that working, you might have to change a lot of things around. Perhaps also use bridges so that various parts of the app can communicate (like it's done for the note viewer, which runs in a more secure sandbox). All that in itself is a big task, and it's difficult to evaluate how hard or time consuming it will be.

  2. The second part is to use that initial work to restrict what plugins can do.

  3. Finally setting up Spectron and getting it working with a few tests. Doing this would also be a major achievement and something we really need as GUI stuff tends to be break easily on each release. It would also be useful for security as we could implement the PoC we got over the years, and ensure there's no security regressions on new releases.

To be honest, the more I think about it the more I feel that part 1 and 2 are out of scope for this GSoC. We want it to be about developing external tools, apps or plugins around the app. If the core needs to be tweaked a bit, so be it, but for this project you'd have to change many parts of the app, at a deep level, and that's what I want to avoid for now.

Part 3 is absolutely fine and definitely in the spirit of this GSoC since it's an external tool you can get working independently.

So I would suggest to either do something around Spectron, or consider a different project idea entirely, but keeping in mind it should be some external tool, plugin or app.