JS: Add ElectronShellOpenExternalSink class for Electron framework security

[toufik-airane]

Add ElectronShellOpenExternalSink class to detect untrusted input interpreted by openExternal function call from electron module in Electron framework for Node.js.

Based on the Electron Security checklist:
https://www.electronjs.org/docs/tutorial/security#14-do-not-use-openexternal-with-untrusted-content

[toufik-airane]

Here is an example to test the query.

1. create a file named InsecureOpenExternal.js

const { shell } = require('electron');
const args = process.argv.slice(2)
const url = args[0]

//BAD: `url` rely on untrusted input
shell.openExternal(url)

2. run the next command

npm i -g electron
electron InsecureOpenExternal.js https://google.co

[asgerf]

Hi @toufik-airane 👋

Thanks for the contribution! I was a bit perplexed when you linked to PR #14, which also added some Electron support, but I'm guessing that was a coincidence.

I think it would be better suited for the ClientSideUrlRedirect query, because the attacker must be able to control the URL scheme.

As it stands we'll get FPs from code like:

shell.openExternal('https://google.com?q=' + tainted_value)

This is similar to code execution via the javascript: protocol. We understand code injection is more severe than a mere redirect, but due to the importance of controlling the prefix of the input string, we use the same query to flag both cases.

[toufik-airane]

Dear @asgerf,

My bad, the link to PR #14 was a mistake! 😱
I've updated the class location to ClientSideUrlRedirect. 👍

However, I'm not sure that will succeed to detect the issue. Indeed, when I scan my example with the javascript/ql/src/Security/CWE-601/ClientSideUrlRedirect.ql it doesn't trigger.
I feel that I'm missing something to make it works.

[asgerf]

Thanks! I'll help add some tests on Monday. Most of the JavaScript queries don't consider command-line arguments to be a source of untrusted input, so that's probably why your example wasn't flagged out of the box.

[asgerf]

I've opened a PR against your fork with a test case.

[asgerf]

Well, better to merge the PR like this than leave it hanging indefinitely. I'll add the test in another PR.

Thanks for the contribution @toufik-airane!