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

Consider Electron version and update checks according to new defaults (#23, #58) #66

Merged
merged 6 commits into from
Jul 3, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ $ electronegativity -h
| -r, --relative | show relative path for files |
| -v, --verbose | show the description for the findings |
| -u, --upgrade <current version..target version> | run Electron upgrade checks, eg -u 7..8 to check upgrade from Electron 7 to 8 |
| -e, --electron-version <version> | assume the set Electron version, overriding the detected one, eg -e 7.0.0 |
baltpeter marked this conversation as resolved.
Show resolved Hide resolved
| -h, --help | output usage information |


Expand Down Expand Up @@ -86,7 +87,9 @@ run({
// show relative path for files (optional)
isRelative: false,
// run Electron upgrade checks, eg -u 7..8 to check upgrade from Electron 7 to 8 (optional)
electronUpgrade: '7..8'
electronUpgrade: '7..8',
// assume the set Electron version, overriding the detected one
electronVersion: '5.0.0'
})
.then(result => console.log(result))
.catch(err => console.error(err));
Expand Down
37 changes: 37 additions & 0 deletions defaults.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"0.1.0": {
"allowRunningInsecureContent": false,
"contextIsolation": false,
"experimentalFeatures": false,
"images": true,
"javascript": true,
"nativeWindowOpen": false,
"nodeIntegration": true,
"nodeIntegrationInWorker": false,
"offscreen": false,
"plugins": false,
"sandbox": false,
"textAreasAreResizable": true,
"webSecurity": true,
"webgl": true,
"webviewTag": true
},
"5.0.0": {
"allowRunningInsecureContent": false,
"contextIsolation": false,
"experimentalFeatures": false,
"images": true,
"javascript": true,
"nativeWindowOpen": false,
"nodeIntegration": false,
"nodeIntegrationInSubFrames": false,
"nodeIntegrationInWorker": false,
"offscreen": false,
"plugins": false,
"sandbox": false,
"textAreasAreResizable": true,
"webSecurity": true,
"webgl": true,
"webviewTag": false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally creating something that dynamically confirms if these security flags are set correctly and working as intended would be preferable, but I can see how using getWebPreferences() is a lot easier in our case. I would settle for this defaults file as of now, maybe planning a more comprehensive version for a next release. Things currently missing that could be useful for future checks:

  • enableRemoteModule (Default should be true)
  • safeDialogs (Default should be false)
  • navigateOnDragDrop (Default should be false)
  • spellCheck (Default should be true)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add those manually for now.

As per this, enableRemoteModule will default to false from version 10.0.0, so I will also include a dump from the latest beta of 10. I'll also update the remote module check accordingly.
The other prefs don't seem to have ever had their default changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh. This also revealed a bug that should have been obvious in hindsight. :D

(Lexicographically) sorting the version keys using .sort() of course produces the wrong result as soon as we get into the double digit major versions. I'll correct this to use semver's compare() as the correct compare function instead.

6 changes: 3 additions & 3 deletions src/finder/checks/AtomicChecks/NodeIntegrationJSCheck.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export default class NodeIntegrationJSCheck {
//nodeIntegrationInWorker Boolean (optional) - Whether node integration is enabled in web workers. Default is false
//nodeIntegrationInSubFrames Boolean (optional) - Whether node integration is enabled in in sub-frames such as iframes. Default is false

match(astNode, astHelper, scope){
match(astNode, astHelper, scope, defaults){
if (astNode.type !== 'NewExpression') return null;
if (astNode.callee.name !== 'BrowserWindow' && astNode.callee.name !== 'BrowserView') return null;

Expand All @@ -37,7 +37,7 @@ export default class NodeIntegrationJSCheck {
locations = locations.concat(loc);
}

if (!nodeIntegrationFound) {
if (!nodeIntegrationFound && defaults.nodeIntegration) {
locations.push({ line: astNode.loc.start.line, column: astNode.loc.start.column, id: this.id, description: this.description, shortenedURL: this.shortenedURL, severity: severity.HIGH, confidence: confidence.FIRM, manualReview: false });
}

Expand All @@ -61,7 +61,7 @@ export default class NodeIntegrationJSCheck {
if ((node.key.value === "sandbox" || node.key.name === "sandbox") && isIdentifier) continue;
if ((nodeIntegrationStrings.includes(node.key.value) || nodeIntegrationStrings.includes(node.key.name)) && !isIdentifier) continue;
}

locations.push({
line: node.key.loc.start.line,
column: node.key.loc.start.column,
Expand Down
24 changes: 17 additions & 7 deletions src/finder/finder.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@ import { CHECKS } from './checks/AtomicChecks';
import { sourceTypes } from '../parser/types';
import { ELECTRON_ATOMIC_UPGRADE_CHECKS } from './checks/AtomicChecks/ElectronAtomicUpgradeChecks';
import chalk from 'chalk';
import { gte } from 'semver';

export class Finder {
constructor(customScan, electronUpgrade) {
let candidateChecks = Array.from(CHECKS)
let candidateChecks = Array.from(CHECKS)
if (electronUpgrade) {
const [currentVersion, targetVersion] = electronUpgrade.split('..');
if (currentVersion && targetVersion) {
Object.keys(ELECTRON_ATOMIC_UPGRADE_CHECKS).forEach(versionToCheck => {
if (versionToCheck > currentVersion && versionToCheck <= targetVersion) {
if (versionToCheck > currentVersion && versionToCheck <= targetVersion) {
candidateChecks = candidateChecks.concat(ELECTRON_ATOMIC_UPGRADE_CHECKS[versionToCheck]);
}
})
Expand All @@ -26,7 +27,7 @@ export class Finder {
console.log(chalk.red(`You have an error in your custom checks list. Maybe you misspelt some check names?`));
process.exit(1);
} else {
for (var i = this._enabled_checks.length - 1; i >= 0; i--)
for (var i = this._enabled_checks.length - 1; i >= 0; i--)
if (!customScan.includes(this._enabled_checks[i].name.toLowerCase()))
this._enabled_checks.splice(i, 1);
}
Expand Down Expand Up @@ -58,7 +59,16 @@ export class Finder {
return sample;
}

async find(file, data, type, content, use_only_checks = null) {
async find(file, data, type, content, use_only_checks = null, electron_version = null) {
// If the loader didn't detect the Electron version, assume the first one. Not knowing the version, we have to assume the worst (i.e.
// all options defaulting to insecure values). By always setting the version here, the code in the checkers is simplified as they now
// don't have to handle the case of unknown versions.
if (!electron_version) electron_version = '0.1.0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, I would like to create a coding style standard for the codebase, but as of now I'm inclined towards keeping the camel case for everything, as my guess is that most of the code is in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, of course. Sorry, that's habit from my personal code style.

Btw: While it doesn't cover variable naming, I would definitely recommend considering Prettier if you want to adopt a code style. I've adopted that for all my JS projects a while ago and am really happy with it. As someone who is fairly opinionated about code styling, accepting their style was a little difficult in the beginning but having a consistent style across many projects and not having to worry about that at all (with everything being formatted on save or commit) is definitely worth it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about this tool, thanks for sharing! So you can't rely on a per-project configuration file for the styling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier explicitly only allows very minor per-project configs. You'll have to decide whether you can live with the sometimes controversial choices they have made. :D Personally, I think having a consistent style across most JS projects is worth it but I can totally see that other people might think differently.


const all_defaults = require('../../defaults.json');
const version_of_last_default_change = Object.keys(all_defaults).sort().reverse().find(current_version => gte(electron_version, current_version));
const defaults = all_defaults[version_of_last_default_change];

const checks = this._checks_by_type.get(type).filter((check) => {
if (use_only_checks && !use_only_checks.includes(check.id)) {
return false;
Expand All @@ -75,7 +85,7 @@ export class Finder {
enter: (node) => {
rootData.Scope.updateFunctionScope(rootData.astParser.getNode(node), "enter");
for (const check of checks) {
const matches = check.match(rootData.astParser.getNode(node), rootData.astParser, rootData.Scope);
const matches = check.match(rootData.astParser.getNode(node), rootData.astParser, rootData.Scope, defaults, electron_version);
if (matches) {
for(const m of matches) {
const sample = this.get_sample(fileLines, m.line - 1);
Expand All @@ -93,7 +103,7 @@ export class Finder {
break;
case sourceTypes.HTML:
for (const check of checks) {
const matches = check.match(data, content);
const matches = check.match(data, content, defaults, electron_version);
if(matches){
for(const m of matches) {
const sample = this.get_sample(fileLines, m.line - 1);
Expand All @@ -105,7 +115,7 @@ export class Finder {
break;
case sourceTypes.JSON:
for (const check of checks) {
const matches = await check.match(data);
const matches = await check.match(data, defaults, electron_version);
if (matches) {
for(const m of matches) {
const sample = this.get_sample(fileLines, m.line - 1);
Expand Down
18 changes: 10 additions & 8 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import run from './runner.js';
const VER = require('../package.json').version;

console.log(`
▄▄▄ ▄▄▌ ▄▄▄ .▄▄·▄▄▄▄▄▄▄
▀▄.▀██• ▀▄.▀▐█ ▌•██ ▀▄ █▪
▐▀▀▪██▪ ▐▀▀▪██ ▄▄▐█.▐▀▀▄ ▄█▀▄
▐█▄▄▐█▌▐▐█▄▄▐███▌▐█▌▐█•█▐█▌.▐▌
▀▀▀.▀▀▀ ▀▀▀·▀▀▀ ▀▀▀.▀ ▀▀█▄▀▪
▄▄▄ ▄▄▌ ▄▄▄ .▄▄·▄▄▄▄▄▄▄
▀▄.▀██• ▀▄.▀▐█ ▌•██ ▀▄ █▪
▐▀▀▪██▪ ▐▀▀▪██ ▄▄▐█.▐▀▀▄ ▄█▀▄
▐█▄▄▐█▌▐▐█▄▄▐███▌▐█▌▐█•█▐█▌.▐▌
▀▀▀.▀▀▀ ▀▀▀·▀▀▀ ▀▀▀.▀ ▀▀█▄▀▪
▐ ▄▄▄▄ .▄▄ • ▄▄▄▄▄▄▄▪ ▌ ▐▪▄▄▄▄▄▄· ▄▌
•█▌▐▀▄.▀▐█ ▀ ▐█ ▀•██ ██▪█·██•██ ▐█▪██▌
▐█▐▐▐▀▀▪▄█ ▀█▄█▀▀█▐█.▐█▐█▐█▐█▐█.▐█▌▐█▪
Expand All @@ -33,6 +33,7 @@ program
.option('-r, --relative', 'show relative path for files')
.option('-v, --verbose', 'show the description for the findings')
.option('-u, --upgrade <current version..target version>', 'run Electron upgrade checks, eg -u 7..8')
.option('-e, --electron-version <version>', 'assume the set Electron version, overriding the detected one, eg -e 7.0.0')
.parse(process.argv);

if(!program.input){
Expand Down Expand Up @@ -63,12 +64,13 @@ const forCli = true;

run({
input,
output: program.output,
output: program.output,
isSarif: program.fileFormat === 'sarif',
customScan: program.checks,
severitySet: program.severity,
severitySet: program.severity,
confidenceSet: program.confidence,
isRelative: program.relative,
isVerbose: program.verbose,
electronUpgrade: program.upgrade
electronUpgrade: program.upgrade,
electronVersionOverride: program.electronVersion
}, forCli);
12 changes: 11 additions & 1 deletion src/loader/loader_asar.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ export class LoaderAsar extends Loader {

// returns map filename -> content
load(archive) {
this.archive = archive;

const archived_files = asar.listPackage(archive);
logger.debug(`Files in ASAR archive: ${archived_files}`);

Expand All @@ -23,6 +25,15 @@ export class LoaderAsar extends Loader {
case 'json':
if (f.toLowerCase().indexOf('package.json') < 0)
continue;
else {
try {
const pjson_data = JSON.parse(this.load_buffer(f));
const dependencies = Object.assign({}, pjson_data.devDependencies, pjson_data.dependencies);
if (dependencies.electron) this._electron_version = dependencies.electron;
} catch (e) {
logger.warn(`Couldn't read package.json data in: ${f}`);
}
}
// eslint-disable-next-line no-fallthrough
case 'js':
case 'jsx':
Expand All @@ -39,7 +50,6 @@ export class LoaderAsar extends Loader {
}

logger.debug(`Discovered ${this.list_files.size} files`);
this.archive = archive;
}

load_buffer(filename) {
Expand Down
11 changes: 11 additions & 0 deletions src/loader/loader_directory.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import logger from 'winston';

import { read_file, list_files } from '../util';
import { Loader } from './loader_interface';

Expand All @@ -11,6 +13,15 @@ export class LoaderDirectory extends Loader {

for (const file of files) {
this._loaded.add(file);
if (file.endsWith('package.json')) {
try {
const pjson_data = JSON.parse(this.load_buffer(file));
const dependencies = Object.assign({}, pjson_data.devDependencies, pjson_data.dependencies);
if (dependencies.electron) this._electron_version = dependencies.electron;
} catch (e) {
logger.warn(`Couldn't read package.json data in: ${file}`);
}
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/loader/loader_interface.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
export class Loader {
constructor() {
this._loaded = new Set();
this._electron_version = undefined;
}

get list_files() { return this._loaded; }
get electron_version() { return this._electron_version; }

// eslint-disable-next-line no-unused-vars
load_buffer(filename) {
Expand Down
6 changes: 5 additions & 1 deletion src/runner.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import cliProgress from 'cli-progress';
import Table from 'cli-table3';
import chalk from 'chalk';
import logger from 'winston';

import { LoaderFile, LoaderAsar, LoaderDirectory } from './loader';
import { Parser } from './parser';
Expand Down Expand Up @@ -28,6 +29,9 @@ export default async function run(options, forCli = false) {
}

await loader.load(options.input);
const electron_version = options.electronVersionOverride || loader.electron_version;
if (!electron_version)
logger.warn("Couldn't detect Electron version, assuming v0.1.0. Defaults have probably changed for your actual version, please check manually.");

if (options.severitySet) {
if (!severity.hasOwnProperty(options.severitySet.toUpperCase())) {
Expand Down Expand Up @@ -93,7 +97,7 @@ export default async function run(options, forCli = false) {
}
}

const result = await finder.find(file, data, type, content);
const result = await finder.find(file, data, type, content, null, electron_version);
issues.push(...result);
} catch (error) {
errors.push({ file: file, message: error.message, tolerable: false });
Expand Down
Binary file modified test/file_formats/electron.asar
Binary file not shown.
7 changes: 6 additions & 1 deletion test/test_loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ describe('Loader classes', () => {

it('extracts file from ASAR', () => {
loader.load(test_files.get('asar'));
loader.list_files.size.should.equal(60);
loader.list_files.size.should.equal(61);
});

it('finds Electron version number in package.json', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have some other tests checking e.g. if the minimum v0.1.0 fallback is actually used when the version can't be found, but we can add this a later PR too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree but I'll have to leave this to someone else.

loader.load(test_files.get('asar'));
loader.electron_version.should.equal('6.1.11');
});
}),

Expand Down