JavaScript: Add query for Node.js integration in Electron framework

[bnxi]

This query finds instances of Node.js integration being enabled in WebPreferences object that could result in remote code execution when rendering untrusted web content.

[ghost]

All committers have signed the CLA.

[bnxi]

The Jenkins links are not accessible from internet.

[jbj]

Sorry, we haven't found a good way to enable public access to our Jenkins output.

The QHelp-preview check is failing with the following error:

20:06:46 [2018-09-06 20:06:46] Validating qhelp file ql/javascript/ql/src/Electron/EnablingNodeIntegration.qhelp...
20:06:47 [2018-09-06 20:06:46] [ERROR] ql/javascript/ql/src/Electron/EnablingNodeIntegration.qhelp:37:72: element "webview" not allowed anywhere; expected the element end-tag, text or element "a", "b", "code", "em", "i", "img", "strong", "sub", "sup" or "tt"
20:06:47 [2018-09-06 20:06:46] [ERROR] ql/javascript/ql/src/Electron/EnablingNodeIntegration.qhelp:38:7: The element type "webview" must be terminated by the matching end-tag "</webview>".

It probably needs to be written <webview>, and maybe also add <code> around it for nicer formatting.

The Language-Tests/JavaScript check is failing on EnablingNodeIntegration.qlref with this diff:

22:35:01 --- expected
22:35:01 +++ actual
22:35:01 @@ -1,5 +1,5 @@
22:35:01 -| EnablingNodeIntegration.js:5:28:11:9 | {\\r\\n     ...       } | nodeIntegration property may have been enabled on this object that could result in RCE |
22:35:01 -| EnablingNodeIntegration.js:5:28:11:9 | {\\r\\n     ...       } | nodeIntegrationInWorker property may have been enabled on this object that could result in RCE |
22:35:01 -| EnablingNodeIntegration.js:15:22:20:9 | {\\r\\n     ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
22:35:01 -| EnablingNodeIntegration.js:23:13:27:9 | {\\r\\n     ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
22:35:01 +| EnablingNodeIntegration.js:5:28:11:9 | {\\n      ...       } | nodeIntegration property may have been enabled on this object that could result in RCE |
22:35:01 +| EnablingNodeIntegration.js:5:28:11:9 | {\\n      ...       } | nodeIntegrationInWorker property may have been enabled on this object that could result in RCE |
22:35:01 +| EnablingNodeIntegration.js:15:22:20:9 | {\\n      ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
22:35:01 +| EnablingNodeIntegration.js:23:13:27:9 | {\\n      ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
22:35:01  | EnablingNodeIntegration.js:49:71:49:93 | {nodeIn ... : true} | nodeIntegration property may have been enabled on this object that could result in RCE |

It looks like a Windows/Unix line endings problem. Try to convert all files into Unix line endings. Alternatively, hand-edit the EnablingNodeIntegration.expected file to remove the \\r.

[felicitymay]

Thanks for the changes. It looks as if the Qhelp preview is just complaining about the typo in this line now.

</code>webview</code>

[bnxi]

thanks, will correct it after the code passes Language-Tests/JavaScript test

[jbj]

The diff in the Language-Tests/JavaScript error is now this:

01:46:41  --- expected
01:46:41  +++ actual
01:46:41  @@ -1,5 +1,5 @@
01:46:41  -| EnablingNodeIntegration.js:5:28:11:9 | {\\n     ...       } | nodeIntegration property may have been enabled on this object that could result in RCE |
01:46:41  -| EnablingNodeIntegration.js:5:28:11:9 | {\\n     ...       } | nodeIntegrationInWorker property may have been enabled on this object that could result in RCE |
01:46:41  -| EnablingNodeIntegration.js:15:22:20:9 | {\\n     ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
01:46:41  -| EnablingNodeIntegration.js:23:13:27:9 | {\\n     ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
01:46:41  +| EnablingNodeIntegration.js:5:28:11:9 | {\\n      ...       } | nodeIntegration property may have been enabled on this object that could result in RCE |
01:46:41  +| EnablingNodeIntegration.js:5:28:11:9 | {\\n      ...       } | nodeIntegrationInWorker property may have been enabled on this object that could result in RCE |
01:46:41  +| EnablingNodeIntegration.js:15:22:20:9 | {\\n      ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
01:46:41  +| EnablingNodeIntegration.js:23:13:27:9 | {\\n      ...       } | nodeIntegration is enabled by default in WebPreferences object that could result in RCE |
01:46:41   | EnablingNodeIntegration.js:49:71:49:93 | {nodeIn ... : true} | nodeIntegration property may have been enabled on this object that could result in RCE |

It looks like the expected file has one too few spaces before the ... in the first four lines. This error is hopefully reproducible for you locally with odasa qltest.

[ghost]

@bnxi Thank you for the submission.

I have some stylistic changes that I think you should incorporate, see https://github.com/esben-semmle/ql/commits/NodeIntegration.

Note that I have removed the @precision attribute, since I do not think we want this query by default on LGTM.com yet. You can still enable the query manually in lgtm.yml or by using a custom query pack.

I note that the examples/WebViewNodeIntegration.html file contains a BAD example that is not flagged by the analysis.
We usually only have examples of problems that the analysis can flag. Did you mean to include support for that example as well?

[bnxi]

@esben-semmle thanks for the suggestions and improvements. I had included webview example file as an extra piece of information and it was not intended to be detected by this query. I have removed the example file and reference to it from qlhelp file as well.

[bnxi]

@esben-semmle seems to be EOL issue again, please feel free to abandon this PR and add this query in the next release if you find it useful. I'd like to propose the following queries as well:

1. Warning to enable context isolation when 'preload' property was present in WebPrefrences: https://electronjs.org/docs/tutorial/security#3-enable-context-isolation-for-remote-content
2. Do not use enableBlinkFeatures: https://electronjs.org/docs/tutorial/security#10-do-not-use-enableblinkfeatures

Thanks

[ghost]

@bnxi Thank you for the changes.

One minor nit: can you revert ecd08d4, please? The leading dots are not necessary, and the commit message does not match the change.

seems to be EOL issue again

Not this time. The failure was caused by a temporary internal issue.

please feel free to abandon this PR and add this query in the next release

No need to abandon, you are merging against master, so the feature will land in the next release (1.19) automatically.
The feature freeze for 1.18 was on August 31, when the master branch was forked to rc/1.18.
If you want the feature in 1.18, I need to merge it as a hotfix against rc/1.18.

I'd like to propose the following queries as well: ...

Noted, we will aim for inclusion in 1.19.

[ghost]

Thank you @bnxi , your contribution has been merged.
We will experiment with the query and the two suggested queries, and hopefully make them all a part of the standard LGTM.com query suite soon.