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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance security documentation #11770

Merged
merged 20 commits into from Jan 30, 2018

Conversation

Projects
None yet
7 participants
@felixrieseberg
Copy link
Member

felixrieseberg commented Jan 29, 2018

This is a fairly big PR, but bear 馃惢 with me here. I've promised for months now that I'd update our security documentation with concrete examples, explanations, and information that is good enough for an Electron newcomer. Previously, our security checklist assumed that you already know what experimentalCanvasFeatures are and simply tells you not to enable them.

This PR hopes to do better. It retains the checklist in its short format, but adds a section with a short introduction, a Why? and a How? for each and every item on the checklist. Also, every single item comes with an actionable, concrete code example.

Closes #11098

@zeke
Copy link
Member

zeke left a comment

Wow, this is impressive. Thanks for taking the time to write it up, @felixrieseberg. I just left a few little comments, but overall this is looking great.

As I read this, I wondered what Electron's defaults are for some of this stuff. It might be good to comment on that where it makes sense, like "such-and-such is disabled/enabled by default, so if you haven't changed the default in your app, you're golden."

## Only Display Secure Content

Any resources not included with your application should be loaded using a
secure protocol like `HTTPS`.

This comment has been minimized.

@zeke

zeke Jan 29, 2018

Member

Maybe add something explicit here about NOT using HTTP?


`HTTPS` has three main benefits:

1) It authenticates the remote server, ensuring that the host is actually who

This comment has been minimized.

@zeke

zeke Jan 29, 2018

Member

who(m) 馃馃

## Disable Node Integration for Remote Content

It is paramount that you disable Node integration in any renderer
(`BrowserWindow`, `BrowserView`, or `WebView`) that loads remote content. The

This comment has been minimized.

@zeke

zeke Jan 29, 2018

Member

Might be handy to turn these references into links to the API docs for each.

felixrieseberg added some commits Jan 29, 2018

@felixrieseberg

This comment has been minimized.

Copy link
Member Author

felixrieseberg commented Jan 29, 2018

@zeke Thank you! All solid recommendations, I added them all.

One question: The linter complains about our use of eval() 馃檲. How do we tell it that in this special case, we're doing the right thing?

332:8:    eval can be harmful.
332:22:   eval can be harmful.

Any resources not included with your application should be loaded using a
secure protocol like `HTTPS`. In other words, do not use insecure protocols
like

This comment has been minimized.

@liliakai
connect to.
2) It ensures data integrity, asserting that the data was not modified while in
transit between your application and the host.
3) It encryps the traffic between your user and the destination host, making it

This comment has been minimized.

@liliakai
2) It ensures data integrity, asserting that the data was not modified while in
transit between your application and the host.
3) It encryps the traffic between your user and the destination host, making it
more difficult to eavesdropping on the information sent between your app and

This comment has been minimized.

@liliakai

liliakai Jan 29, 2018

eavesdropping

* [WebViews: Verify the options and params of all `<webview>` tags](#verify-webview-options-before-creation)


## Only Display Secure Content

This comment has been minimized.

@liliakai

liliakai Jan 29, 2018

Do we want to generalize a bit and mention secure/insecure protocols here? e.g., wss:// for websockets.

their power is usually limited to messing with the website that they are
executed on. However, in a renderer process with Node.js integration enabled,
an XSS attack becomes a whole different class of attack: A so-called "Remote
Code Execution" (RCE) attack. Disabling Node.js integration limits the power

This comment has been minimized.

@liliakai

liliakai Jan 29, 2018

nit: "Node.js integration" and "Node integration" are used interchangeably throughout this document. Can there be only one? 鈿旓笍


The `eval()` method has precisely one mission: To evaluate a series of
characters as JavaScript and execute it. It is a required method whenever you
need to evaluate code that is known ahead of time. While legitimate use cases

This comment has been minimized.

@liliakai

liliakai Jan 29, 2018

code that is not known ahead of time

webPreferences: {
webSecurity: false
}
})

This comment has been minimized.

@liliakai

liliakai Jan 29, 2018

missing closing code fence

}
})
```js

This comment has been minimized.

@TiagoDanin

TiagoDanin Jan 29, 2018

Member

Remove the ```js here

image

@@ -100,3 +554,7 @@ app.on('web-contents-created', (event, contents) => {

Again, this list merely minimizes the risk, it does not remove it. If your goal
is to display a website, a browser will be a more secure option.

[browser-window]: (../api/browser-window)

This comment has been minimized.

@TiagoDanin

TiagoDanin Jan 29, 2018

Member

Incorrect paths (browser-window, browser-view and web-view)

@zeke

This comment has been minimized.

Copy link
Member

zeke commented Jan 29, 2018

The linter complains about our use of eval() 馃檲. How do we tell it that in this special case, we're doing the right thing?

Eek. Yeah. Throwing out ideas:

1: // eslint-disable-next-line or // eslint-disable-line
2: Change the code block to not be js
3: Remove the snippet entirely, and instead link to an external doc about how to disable eval

@@ -7,3 +7,6 @@ To report a security issue, email [electron@github.com](mailto:electron@github.c
The Electron team will send a response indicating the next steps in handling your report. After the initial reply to your report, the security team will keep you informed of the progress towards a fix and full announcement, and may ask for additional information or guidance.

This comment has been minimized.

@diracdeltas

diracdeltas Jan 29, 2018

Contributor

It would be really useful to have a time limit here, like "The team will respond in 48 hours to confirm that someone has seen your message. If you do not get a response, do X."

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

That would be useful, but no such policy exists right now and it would need to go through the maintainers group, so no need to block this PR on it. @diracdeltas you may want to open a separate issue for that suggestion.

* [WebViews: Verify the options and params of all `<webview>` tags](#verify-webview-options-before-creation)


## Only Display Secure Content

This comment has been minimized.

@diracdeltas

diracdeltas Jan 29, 2018

Contributor

Display should be generalized to Load or something since there's plenty of resources (js files for instance) that don't necessarily get displayed

@ckerr
Copy link
Member

ckerr left a comment

Felix, this is excellent work. Thank you so much for doing this.

I added a lot of suggestions inline, mostly because I'm very enthusiastic about this PR and fell into lint mode. 馃槃


With that in mind, be aware that displaying arbitrary content from untrusted
With that in mind, be aware that displaying arbitrary content from un-trusted

This comment has been minimized.

@ckerr
* [Enable context isolation in all renderers that display remote content](#enable-context-isolation-for-remote-content)
* [Use `ses.setPermissionRequestHandler()` in all sessions that load remote content](#handle-session-permission-requests-from-remote-content)
* [Do not disable `webSecurity`](#do-not-disable-websecurity)
* [Define a `Content-Security-Policy`](#define-a-content-security-policy)
, and use restrictive rules (i.e. `script-src 'self'`)

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

fix commasplice by removing ',' from ", and use restrictive rules"

, and use restrictive rules (i.e. `script-src 'self'`)
* [Override and disable `eval`](https://github.com/nylas/N1/blob/0abc5d5defcdb057120d726b271933425b75b415/static/index.js#L6-L8)
* [Override and disable `eval`](#override-and-disable)

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

suggest more explanatory tag #override-and-disable-eval?

it claims to be. When loading a resource from an `HTTPS` host, it prevents
an attacker from impersonating that host, thus ensuring that the computer
your app's users are connecting to is actually the host you wanted them to
connect to.

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

This block is the same idea repeated three times.

"It authenticates the remote server, ensuring your app connects to the correct host instead of an impersonator."


`HTTPS` has three main benefits:

1) It authenticates the remote server, ensuring that the host is actually whom

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

s/whom/who/

See the section on [only displaying secure content](#only-display-secure-content)
for more details, but simply put, loading content over `HTTPS` assures the
authenticity and integrity of the loaded resources while encrypting the traffic
itself.

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

Move "See the section.." to the end of this paragraph


_Recommendation is Electron's default_

Blink is the name of the rendering engine behind Chromium. Similarly to

This comment has been minimized.

@ckerr
security features.

Legitimate use cases for this property exist in testing cases, but generally
speaking, `webSecurity` should never be disabled in any production application.

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

repetition of "generally speaking". Fourth occurrence of "legitimate use case."

Better: "Do not disable webSecurity in production applications."


Disabling `webSecurity` will disable the same-origin policy as well as
implicitly setting the `allowRunningInsecureContent` property to `true`. In
other words, it allows the execution of insecure code from different domains.

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

Third occurrence of "as well as"
s/as well as implicitly setting/and set/

Third occurrence of "In other words".
"This allows insecure code from different domains to be executed."

If you do not need popups, you are better off not allowing the creation of
new [`BrowserWindows`](browser-window) by default. This follows the principle
of the minimally required access: Websites that you do not know to need popups
should not have the ability to create new popups.

This comment has been minimized.

@ckerr

ckerr Jan 29, 2018

Member

s/ by default//

s/of the minimally/of minimally/

Awkward last sentence.
"...access: Don't let a website create new popups unless you know it needs that feature."

felixrieseberg added some commits Jan 30, 2018

@felixrieseberg

This comment has been minimized.

Copy link
Member Author

felixrieseberg commented Jan 30, 2018

@ckerr @liliakai @TiagoDanin @diracdeltas: Thank you so much for the awesome feedback. 100% agreed with and thankful for all of it, I implemented it all.

@felixrieseberg

This comment has been minimized.

Copy link
Member Author

felixrieseberg commented Jan 30, 2018

Alright, all relevant tests pass. Jenkins is still sad, but that seems entirely unrelated.

@poiru

poiru approved these changes Jan 30, 2018

Copy link
Member

poiru left a comment

馃憦

JavaScript on your website.

After this, you can grant additional permissions for specific hosts. For example,
if you are opening a BrowserWindow pointed at `https://my-website.com/", you can

This comment has been minimized.

@poiru

poiru Jan 30, 2018

Member

" should be `

API to remotely loaded content.

In the following example preload script, the later loaded website will have
access to a `window.readConfig()` method, but no Node.js features.

This comment has been minimized.

@poiru

poiru Jan 30, 2018

Member

I'd mention something about exposing most restricted API possible and to be extra careful about sanitizing arguments etc.

I'd also mention that it's really difficult to make this approach entirely secure because remote content can modify what e.g. Function.prototype.call does even in the context of the preload script. I think we should strongly suggest using context isolation and message passing instead (with the same caveat about exposing the most restricted API etc.).

@zeke

zeke approved these changes Jan 30, 2018

@ckerr ckerr merged commit 67196bd into master Jan 30, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@ckerr ckerr deleted the security-documentation branch Jan 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.