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

Rejected: vuln(NSWG-ECO-483): zombie #442

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@lirantal
Copy link
Member

commented Nov 23, 2018

  • WIP as there's an issue I'm trying to figure out with the maintainer before requesting a CVE: assaf/zombie#1169 (comment)
  • In this report I've opted-in for flagging all future releases as vulnerable as well because unlike unmaintained packages which are unlikely to be vulnerable, this package is maintained and can easily just release a new minor without a security fix which will get the CVE to not be applied. /cc @nodejs/security-wg maybe we should better settle the policy on this? and also WDYT on this specific case.

@lirantal lirantal self-assigned this Nov 23, 2018

@assaf

This comment has been minimized.

Copy link

commented Nov 24, 2018

Zombie is “full-stack testing using Node.js”. It’s fit for purpose: use it in your test environment, to run test suites, against your application code.

Zombie is not designed to test adversaries (eg pen tests, web scraping).

If you are testing your application code, that implies components and services you trust. After all, you will be exposing your users to those same URLs!

Since test suites should be fast, isolated, and repeatable, that implies control over the resources used in your tests.

There are various tools that allow you to capture, verify, block, and modify resources. I personally use nock, replay (disclosure: also mine), and sinon. But Zombie will work with many other tools like that.

Following the UNIX philosophy that "tools should do one thing, and do it well,” Zombie does not intend to duplicate features from nock/replay/sinon/etc, but rather be used in combination with these tools

The changes you are suggesting are for offl label use. Zombie does not support the use cases of “untrusted crawled pages.”

TL;DR If you’re using Zombie as intended, as part of a test enviornment that follows best practices, then you have high trust/control over the resources Zombie is using. If you are using Zombie for off label use, please don’t.

@lirantal

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2018

Hi Assaf,

Thank you for jumping in on this thread. I know that your time is valuable and completely appreciate that you're able to set time to discuss with us the security issue that was brought to our attention.

Your comment above about zombie being used for local testing purposes makes sense, however I couldn't find any reference for that stated in the project's README, especially not as strong as you've pointed it out here. The README actually uses a generic reference to outside hosts such as:

// We're going to make requests to http://example.com/signup
// Which will be routed to our test server localhost:3000
Browser.localhost('example.com', 3000);

Which doesn't help in implying by any means that the library should only be used for trusted systems.

Furthermore, in my opinion one can find a good similarity between zombie and jsdom, yet the latter has made it very clear in the docs about the dangers of executing scripts (https://github.com/jsdom/jsdom#executing-scripts), as well as opted-in to be secure by default by requiring you to turn on a flag that will enable executing scripts:

To enable executing scripts inside the page, you can use the runScripts: "dangerously" option:

const dom = new JSDOM(`<body>
  <script>document.body.appendChild(document.createElement("hr"));</script>
</body>`, { runScripts: "dangerously" });

In summary, I believe that taking a route like jsdom will be a good way to mitigate the vulnerability: providing information about the security implications of using this library on none-trusted websites and resources, along with a secure default to not executing scripts .

I will be happy to hear your thoughts on this as well as see if anyone else from the working group has other ideas about approaching this.

@assaf

This comment has been minimized.

Copy link

commented Nov 25, 2018

The README actually uses a generic reference to outside hosts such as:

I suggest you read the docs in full. This feature is the exact of opposite of what you just described. From the README:

Allows you to make requests against a named domain and HTTP/S port, and will route it to the test server running on localhost and unprivileged port.

For example, if you want to call your application "example.com", and redirect traffic from port 80 to the test server that's listening on port 3000, you can do this:

-- https://www.npmjs.com/package/zombie#browserlocalhosthost-port

This feature is designed for the sole purpose of helping you test your application code, which is running on your machine, and so is bound to a non-privileged port on the loopback adapter (https://en.wikipedia.org/wiki/Localhost).

Tests are supposed to run in an isolated environment and produce repeatable results (ie no network connection). Resources are either served locally (the feature above), or captured and replayed (nock, replay, sinon, etc).

I'm not sure where the confusion is coming from. The docs use a variant of "test" 20 times, open with an example of a Mocha BDD test suite, have a section dedicated to assertions, and the npm package has 8 keywords related to testing.

Since that doesn't seem clear enough, we'll accept contributions to further clarify that Zombie is "for testing client-side JavaScript code".


This is especially important if zombiejs is used for building a crawler.
-- https://hackerone.com/reports/389583

DO NOT use Zombie for building crawlers.

Zombie was not designed for building crawlers. It is not a supported use case, and so was never factored into the design, and as such may break in unexpected and undesirable ways.


If you want to build a web crawler, there are other Node libraries that support this use case, please use those instead.

Because Zombie is a tool for application testing and not for web crawlers, we cannot accept any contribution that's designed specifically for web crawling. That would conflict with the mission and scope of the project.


TL;DR Please do not use Zombie for building web crawlers, it was not designed for that. Use it as intended for application testing. Please read the docs first, and if you find the docs lacking, contribute to make them better.

@mcollina

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

IMHO this is not a security vulnerability. It should not be categorized as such, and I'm a bit surprised this is getting open here. Zombie.js is a testing tool.

The overall theme of the README is how much Zombie.js is like a browser for testing purposes, and I can see how it could generate confusion in a user. I would recommend to put a warning notice at the top of Zombie.js README that states the following:

Crawling untrusted web pages with Zombie.js is not safe.

@MarcinHoppe

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

I also think this would be enough to clarify potential security problems coming from using the library outside of the nominal use case.

@lirantal

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

Thank you Assaf for addressing all of those points. I think you have made a great job at explaining the risks in the comment and it will be great to see that making it to the README for the sake of users safety.

I would like to clarify that the example with localhost routing is an added feature on the library, but nothing stops a user to use it as follows, which allows connecting to remote websites and testing other's people application code. hence it's not strict to testing only your own hosted code:

const browser = new Browser();
browser.visit('https://nodejs.org', function(a,b) {})

Another point I want to also raise with the rest of the wg members as this was brought up by both @assaf and @mcollina - the fact that a library or tool is meant for testing purposes is not a criteria for us to vet it off from security vulnerabilities. For example, I can find other cases where we issued path traversal vulnerabilities for http servers, where theoretically a maintainer or anyone else can claim that they are only meant for development purposes.

In the same context of addressing items by their "testing" purposes - we agreed in the last collab summit that Node's debugger defaulting to listen on 0.0.0.0 is a security vulnerability that deserves a CVE, even though you could argue that this practice is only used for development or testing purposes.

Furthemore, I gave jsdom as a good example for a tool that is also only intended for use in the context of testing (as opposed to crawling) and they mention this security hazard in their docs as well as default to not evaluating javascript code in the DOM.

@mcollina as to opening this issue - the report was opened 4 months ago and the maintainer was contacted back then already. I totally understand how such an e-mail invite can be missed or the maintainer is too busy, which is why I went ahead and opened a public issue in the repository's github to ask for more eyes on the issue so we can discuss and triage together with the maintainers. I'm still unconvinced that this doesn't quantify as a security vulnerability but I do have doubts which is why I opened the issue here with the report in mind as context so we can discuss openly and I can get input from other peers.

In my first comment (#442 (comment)) I suggested the same docs update on the README to clarify the risk. I'm not yet sure however about still executing javascript code in the Node context. Maybe a middle-ground where if the browsing host used by Zombie is not localhost then it will default to not parse scripts?

@mcollina

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Another point I want to also raise with the rest of the wg members as this was brought up by both @assaf and @mcollina - the fact that a library or tool is meant for testing purposes is not a criteria for us to vet it off from security vulnerabilities. For example, I can find other cases where we issued path traversal vulnerabilities for http servers, where theoretically a maintainer or anyone else can claim that they are only meant for development purposes.

I respectfully disagree. As an example, Node.js net and http listen by default on 0.0.0.0. This could be intended as a security vulnerability using the same criteria.
I think each tool/library should have its own hazards clearly spelled out in the README (this is not the case for Zombie), and security should be vetted against those hazards/defined use cases.

Furthemore, I gave jsdom as a good example for a tool that is also only intended for use in the context of testing (as opposed to crawling) and they mention this security hazard in their docs as well as default to not evaluating javascript code in the DOM.

I do not understand why the same approach could not be taken by Zombie.js.


I think a good fix for Zombie could be to add an unsafe flag that enable browsing of things on the web:

const browser = new Browser({ unsafe: true });
browser.visit('https://nodejs.org', function(a,b) {})
@lirantal

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

I do not understand why the same approach could not be taken by Zombie.js.

We're in agreement. It was the first thing I suggested. @assaf what do you think?

the fact that a library or tool is meant for testing purposes is not a criteria for us to vet it off from security vulnerabilities.

I'm not saying it was good or bad practice, but that it wasn't something we decide according to and I discussed with @MarcinHoppe about putting in some triage guidelines as part of his new doc and this is a good example for one of them. We should definitely have your voice in this @mcollina and your input.

@mcollina

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

+1, keep me in the loop. I had recently a few similar case on my modules.

@assaf

This comment has been minimized.

Copy link

commented Nov 27, 2018

@mcollina thanks. I went ahead and added this warning to the README.

but nothing stops a user to use it as follows, which allows connecting to remote websites and testing other's people application code.

Are we discussing the downloading of resources from the internet and executing them in the Node context? nbswg-eco-477?

the fact that a library or tool is meant for testing purposes is not a criteria for us to vet it off from security vulnerabilities.

Makes sense. But the specific use case here is testing your application code — the code you expect end-users to be running on their machines — so there's a reasonable expectation of trust. And yes, your application code often includes 3rd party libraries obtained via npm install, there's no perfect solution.

Furthemore, I gave jsdom as a good example for a tool that is also only intended for use in the context of testing

Citation?

According to https://github.com/jsdom/jsdom#readme (emphasis is mine):

In general, the goal of the project is to emulate enough of a subset of a web browser to be useful for testing and scraping real-world web applications

JSDOM and Zombie do not have the same security concerns.

Maybe a middle-ground where if the browsing host used by Zombie is not localhost

You’re describing Docker.

the report was opened 4 months ago and the maintainer was contacted back then already

FYI If it’s an email from an unknown sender that uses keywords like “security” or “vulnerability”, I get those daily, too many, usually spam, and the occasional phishing attempt.

@jbreckman

This comment has been minimized.

Copy link

commented Nov 27, 2018

I'm just going to chime in that my initial response to reading this conversation was that I agreed with @assaf and @mcollina and that of course this package should be executing javascript by default. However, upon further digging, it looks like there actually is a real vulnerability here.

It seems that somehow require is making its way into the javascript that is being evaluated.
I thought this wasn't possible when using vm.createContext(), but the report (https://hackerone.com/reports/389583) indicates that this is allowed.

There is zero reason for script evaluation inside of a virtual DOM to have access to the outer "require" packages in the host application. After playing around, it looks like this is actually a nodejs vulnerability.

I created vm.js on my local and ran the following:

const vm = require('vm');
const result = vm.runInNewContext(`
  (function() {
    var c = 'constructor';
    var require = require=this[c][c]('return process')().mainModule.require;
    var fs = require('fs');
    return String(fs.readFileSync('/home/joshbreckman/Documents/vm.js'));
  })()
`);

console.log(result);

It returned the contents of the script. vm.runInNewContext is supposed to insulate the runner from these sorts of attack vectors - when in reality any script executed in a new context can get full access to everything on your filesystem (that node has access to).

@assaf

This comment has been minimized.

Copy link

commented Nov 27, 2018

when in reality any script executed in a new context can get full access to everything on your filesystem

Nobody is disputing this.

Your test suite is going to have a lot of access to a lot of thing. To start with, most common pattern we see nowadays is developers building their applications using npm modules (Webpack, Rollup, Browserify, etc), and so immediately you are inheriting all the risks from using npm install.

If you are testing your app, you have the same exposure here as using npm install, and there's no technical way to reduce it.

If you are "testing" some foreign website that you have no trust relationship with, this is called "web scraping", and please don't do that with Zombie because it's not designed for that use case, see warning in the README.

@jbreckman

This comment has been minimized.

Copy link

commented Nov 27, 2018

Nobody is disputing this.

@assaf This is actually news to me. I was under the impression that vm.runInNewContext protected your outside code from any malicious inside code. We were very surprised to learn that there are ways to escape the sandbox and poison the outer VM or to require new packages.

Many websites have external snippets that they include in their page. Google analytics, localytics, etc. Obviously you should trust any external scripts you are loading onto your page, but most people examine those in terms of clientside attacks. It's very surprising to me that any of these third party snippets could potentially get read access to my entire server codebase and my passwords/config if I used Zombie.

I don't think this is a Zombie problem - I think it's a nodejs problem. vm.runInNewContext should not allow any way to escape the sandbox. That's its entire point.

@jbreckman

This comment has been minimized.

Copy link

commented Nov 27, 2018

Have you looked at vm2? It looks like it solves most of the vulnerabilities that vm faces: https://github.com/patriksimek/vm2

Obviously out of memory or infinite loop attacks will still be a thing, but those seem less of a problem with Zombie given that Zombie is primarily a dev testing tool.

@lirantal

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

We were very surprised to learn that there are ways to escape the sandbox and poison the outer VM or to require new packages.

@jbreckman indeed. I recommend @bengl's presentation about why the VM module is not a security sandbox: https://vimeo.com/287707873

--

@assaf @mcollina to the point of the matter - if the claim is that it’s a test tool, then I’m not entirely convinced it’s a good enough claim for the vulnerability presented. I can provide references to past reports about grunt or uglify-js which we can easily put in this “test” or “build” category.

I've pinged the WG for more input on this as well. More importantly, I’d like to take out of this thread a rough criteria or guideline as to why it would or would not constitute as a vulnerability.

@lirantal

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

Getting back to this - in view of no interest, or at least no seeming consensus reached by the parties involved I'll close this issue and PR (and pinged Reed from H1 to see how to turn the report to informational). No CVE was requested so we don't need to revert anything from an external resources perspective.

The addition of the security disclosure to Zombie's project page is a sufficient level of communication for possible security vulnerabilities.

I appreciate everyone's input on the topic and especially @assaf's time and effort to explain through-out this issue the rationale behind the exploit possibility. Thanks!


In retrospective I would like to bring up a couple of action items that we can learn from this:

  1. Should we have guidelines regarding the nature of packages and how they are used for when we consider them to be vulnerable? i.e: should we handle disclosures for a path traversal in a local static web server that is obviously targeted at developers use). Perhaps we should consider an informative level of reports? This is already supported in H1 so maybe no real action to do on that.
  2. As a guideline, we should probably always hold-off on requesting a CVE until the PR is approved and merged. It will allow for more time to gather input and surface a resolution we've made before we actually bump it up to a CVE level awareness.

I'll open an issue for the above points so we can discuss but I wanted to put it here too so everyone on the thread will easily see it for transparency.

@lirantal lirantal changed the title vuln(NSWG-ECO-483): zombie Rejected: vuln(NSWG-ECO-483): zombie Jan 1, 2019

@lirantal lirantal closed this Jan 1, 2019

@mcollina

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Should we have guidelines regarding the nature of packages and how they are used for when we consider them to be vulnerable? i.e: should we handle disclosures for a path traversal in a local static web server that is obviously targeted at developers use). Perhaps we should consider an informative level of reports? This is already supported in H1 so maybe no real action to do on that.

I think so, some guidelines about this should be useful. Overall it's definitely fine for a module author to say "this is not a vulnerability because it's not how this should be used" - it should be up to them to decide this according to a document threat model somewhere in their docs.

handle disclosures for a path traversal in a local static web server that is obviously targeted at developers use

Actually this could be a vulnerability. Node.js by default listen to 0.0.0.0, which means that the full disk would be accessible from anybody on the same network.

@lirantal lirantal deleted the lirantal-vuln-483 branch May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.