eng-disable CSP_GLOBAL_CHECK not working

[avin-shum]

Describe the bug Applying /* eng-disable CSP_GLOBAL_CHECK */ to the line reported CSP_GLOBAL_CHECK cannot disable the warning.

The line reported has applied CSP already.

To Reproduce Steps to reproduce the behavior:

1. Write an Electron application with CSP like what is mentioned here: https://www.electronjs.org/docs/tutorial/security#csp-http-header
2. Scan with electronegativity
3. Follow the location reported and apply /* eng-disable CSP_GLOBAL_CHECK */
4. Scan again
5. The warning is not dismissed

Expected behavior The warning shall be dismissed

Platform (please complete the following information):

• OS: Win
• Electronegativity version: 1.8.1

Additional context The warning is dismissed if I apply /* eng-disable CSP_JS_CHECK */. So I think the "Check ID" in the scan report is incorrect.

[phosphore]

Hello @avin-shum and thanks for contributing,

As pointed out in the README and in #85, Global Checks can't be disabled using code annotations, since they work on the first pass of Atomic Checks (i.e. making decisions over these). If you really need to, you can use the exclusion flag (-x) and pass -x CSPGlobalCheck to disable the check.

CSP_GLOBAL_CHECK is a Global Check that works by checking if any CSP_JS_CHECK or CSP_HTML_CHECK findings were issued, and then checking their CSP policy strength using https://www.npmjs.com/package/@doyensec/csp-evaluator. If the policy is missing or deemed insecure, a finding is issued.

Are you sure that there are no CSP included in the HTML's meta tags? What's the CSP policy set by your onHeadersReceived callback? Could you provide a reproducible code sample for this?

Cheers,

[avin-shum]

Please note that CSP_JS_CHECK and CSP_HTML_CHECK are not listed in the Wiki. Moreover, the report generated does not contain either one of them.

On the other hand, I can dismiss the warning by applying /* eng-disable CSP_JS_CHECK */ instead. So I think what should be listed in the report is CSP_JS_CHECK instead of CSP_GLOBAL_CHECK.

There are no CSP included in the HTML's meta tag because the HTML is out of this project. That is another story.

[phosphore]

The fact that you can dismiss the warning by applying // eng-disable CSP_JS_CHECK is not looking good. This is not how it should work. CSP_JS_CHECK and CSP_HTML_CHECK are not listed in the wiki because they should never be "touched" by the end users, they are just internal utilities for CSP_GLOBAL_CHECK.

https://github.com/doyensec/electronegativity/blob/master/src/finder/checks/GlobalChecks/CSPGlobalCheck.js#L17-L51

By design, if you are not getting a CSP_GLOBAL_CHECK finding it means that either a secure CSP_JS_CHECK or a secure CSP_HTML_CHECK were also found. If you could provide a sample project to reproduce the issue it would be super helpful.

Thanks again,

[avin-shum]

OK. Thanks for your explanation. I will check my code then.

[avin-shum]

@phosphore

Below is the main.ts I scanned. I got CSP_GLOBAL_CHECK | 22:6 │ Failed to parse CSP, it may not be valid., but I can pass the test using https://www.npmjs.com/package/@doyensec/csp-evaluator

import { app, BrowserWindow, session } from 'electron';

let win: BrowserWindow;

app.on('ready', createWindow);

app.on('activate', () => {
  if (win === null) {
    createWindow();
  }
});

function createWindow() {
  session.defaultSession.webRequest.onHeadersReceived((details, callback) => {
    callback({
      responseHeaders: {
        ...details.responseHeaders,
        'Content-Security-Policy': [
          "script-src 'none'; object-src 'none'; base-uri 'none';",
        ],
      },
    });
  });

  win = new BrowserWindow({
    width: 1024,
    height: 768,
    center: true,
    title: 'xxx',
    icon: __dirname + '../../favicon.ico',
    webPreferences: {
      nodeIntegration: false,
      nodeIntegrationInSubFrames: true,
      preload: __dirname + '/preload.js',
      enableRemoteModule: false,
      disableBlinkFeatures: 'Auxclick',
      contextIsolation: true,
      sandbox: true,
      worldSafeExecuteJavaScript: true,
    },
    show: false,
  });

  win.on('closed', () => {
    win = null;
  });

  app.on('browser-window-created', (e, win) => {
    win.removeMenu();
  });

  win.on('page-title-updated', (evt) => {
    evt.preventDefault();
  });
  win.show();
}

app.on('window-all-closed', () => {
  app.quit();
});

[reZach]

@phosphore I think you accidentally tagged me!

[phosphore]

Hello @avin-shum!

I tried to run the npm version of electronegativity (v1.8.1) against your target file but I only got:

42 check(s) successfully loaded: 6 global, 36 atomic
████████████████████████████████████████ 100% | 1/1
Fetching Electron's new releases, this may take a while...
Updated releases list to v11.2.3!
┌─────────────────────────────────────────┬───────────────────────┬──────────┬──────────────────────────────────────────────────┐
│ Check ID                                │ Affected File         │ Location │ Issue Description                                │
├─────────────────────────────────────────┼───────────────────────┼──────────┼──────────────────────────────────────────────────┤
│ PRELOAD_JS_CHECK                        │ /private/tmp/index.ts │ 34:6     │ Review the use of preload scripts                │
│ *Review Required*                       │                       │          │ https://git.io/JeuMu                             │
│ MEDIUM | FIRM                           │                       │          │                                                  │
├─────────────────────────────────────────┼───────────────────────┼──────────┼──────────────────────────────────────────────────┤
│ LIMIT_NAVIGATION_GLOBAL_CHECK           │ N/A                   │ 0:0      │ Missing navigation limits using .on new-window   │
│ HIGH | CERTAIN                          │                       │          │ and will-navigate events                         │
│                                         │                       │          │ https://git.io/JeuMs                             │
├─────────────────────────────────────────┼───────────────────────┼──────────┼──────────────────────────────────────────────────┤
│ PERMISSION_REQUEST_HANDLER_GLOBAL_CHECK │ N/A                   │ 0:0      │ Missing PermissionRequestHandler to limit        │
│ MEDIUM | CERTAIN                        │                       │          │ specific permissions (e.g. openExternal) in      │
│                                         │                       │          │ response to events from particular origins.      │
│                                         │                       │          │ https://git.io/JeuM0                             │
└─────────────────────────────────────────┴───────────────────────┴──────────┴──────────────────────────────────────────────────┘

So I must be missing something... Let me know if the snippet is incorrect.

@reZach sorry about that!