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

Please Review Vulnerability Escalation Procedures #405

Open
jbreckman opened this Issue Sep 14, 2018 · 23 comments

Comments

Projects
None yet
7 participants
@jbreckman
Copy link

jbreckman commented Sep 14, 2018

Hi there,

Let me begin by saying how much we love the nodejs community and how proactive you guys are in making sure our development environment is safe. The fact that github now warns us about packages with defects is really remarkable.

When we logged in yesterday morning we got the big warning:

We found a potential security vulnerability in one of your dependencies.
A dependency defined in server/package.json has known security vulnerabilities and should be updated.

Needless to say, this raised alarms and we dropped what we were doing to investigate and get a fix out that patched the vulnerability. At a glance in github, it looked like this was a XSS vulnerability in the exceljs package, so we knew we had to fix it very quickly.

After digging, we were linked to https://nvd.nist.gov/vuln/detail/CVE-2018-16459 which linked to https://hackerone.com/reports/356809

Unfortunately, this seems to be where we noticed that things had taken a wrong turn somewhere. Upon further introspection, it seemed super odd that a package such as exceljs could possibly introduce an XSS vulnerability. There would be almost no reason to run exceljs inside of a browser -- it is almost exclusively a nodejs package... how could it possibly introduce cross-site scripting?

The VERY thorough ticket included this code snippet:

'use strict'
/*global console*/
const Excel = require('exceljs')
const http = require('http')
const port = 8080

const workbook = new Excel.Workbook()
const filename = 'testsheet.xlsx'

function createHTML(worksheet) {
    let __html = `
    <table>
        <tr>
            <td>${worksheet.getCell('A1').value}</td>
            <td>${worksheet.getCell('A2').value}</td>
            <td>${worksheet.getCell('A3').value}</td>
        </tr>
        <tr>
            <td>${worksheet.getCell('B1').value}</td>
            <td>${worksheet.getCell('B2').value}</td>
            <td>${worksheet.getCell('B3').value}</td>
        </tr>
    </table>
    `

    return __html
}

const requestHandler = (request, response) => {
    workbook.xlsx.readFile(filename)
        .then(worksheets => {
            worksheets.eachSheet(function(worksheet, sheetId) {
                response.writeHeader(200, {
                    "Content-Type": "text/html"
                })
                response.write(createHTML(worksheet))
                response.end()
            });
        });
}

const server = http.createServer(requestHandler)

server.listen(port, (err) => {
    if (err) {
        return console.log(err)
    }
    console.log(`server is listening on ${port}`)
})

The report is 100% correct. If you run that code and put HTML/javascript into your excel file, you will end up executing that script in a browser. Unfortunately, the vulnerability does not exist within the exceljs package, it was introduced in the code sample provided.

Most engineers should know that the correct way to make sure you are protected from XSS is to sanitize everything going into your HTML. There are a LOT of ways to do this. Jade has a number of ways of escaping (td= a1). Handlebars escapes all values unless you explicitly use three {{{a1}}}. AngularJS does this automatically, unless you protect against it. React escapes everything unless you call dangerouslySetInnerHTML (which is really great since it drives home how dangerous this is). In fact, because of XSS concerns, it is always recommended to not ever format HTML like this yourself.

An identical ticket could be filed against the nodejs package fs:

'use strict'
/*global console*/
const Excel = require('fs')
const http = require('http')
const port = 8080

const filename = 'testsheet.xlsx'

function createHTML(fileContents) {
    let __html = `
    <table>
        <tr>
            <td>${fileContents}</td>
        </tr>
    </table>
    `

    return __html
}

const requestHandler = (request, response) => {
    var fileContents = String(fs.readFileSync(filename));    
    response.writeHeader(200, {
        "Content-Type": "text/html"
    })
    response.write(fileContents)
    response.end()
}

const server = http.createServer(requestHandler)

server.listen(port, (err) => {
    if (err) {
        return console.log(err)
    }
    console.log(`server is listening on ${port}`)
})

We tried to raise this issue, and @lirantal said that he agreed the ticket was "debatable". I very strongly disagree. If this package is debatable, then 90% of packages on npm are also debatable.

exceljs/exceljs#608 (comment)
exceljs/exceljs@9066cd8

The community as a whole would be more secure if they used specific packages that explicitly protected against script injection when formatting HTML. If we trust or expect all of our packages to protect against injection then one of them will get it wrong and we will possibly be exposed.

We really love an appreciate the security warnings, but they have to be properly vetted. If we start alerting and filing unnecessary CVEs, then the warnings start to mean less and will start to be ignored.

So this ticket is mostly asking how this "vulnerability" slipped through the system and what can be done to prevent these sorts of "vulnerabilities" from popping up again.

@vdeturckheim

This comment has been minimized.

Copy link
Member

vdeturckheim commented Sep 17, 2018

Hey @jbreckman ,
Thanks a lot for openning this ticket.

This is a very valid discussion to have. The vulnerability management program will soon be one year old. We planned to take a look back at what happened during this year in the next few months in order to see what can be improved.

So far, we don't have much guidelines to define what is an security issue. It is the people from the triage team who generaly take this decision.

I guess the best solution to that issue is start writing better process on our side. BUT, currently, the Working Group is massively undersized. Most of the work happens in the triage team within which @lirantal does a tremendous job on a daily basis.

So my question is, do you have time to spend with us doing this? If so, we would be more than happy to welcome you to the WG and work with you to make this whole process better. WDYT?

This invitation is actually valid for anyone ready this message. The Working Group needs more hands, it could be you!

@jbreckman

This comment has been minimized.

Copy link
Author

jbreckman commented Sep 17, 2018

Hey @vdeturckheim,

I'm not saying no, but I'm not saying yes. I'm a pretty busy guy, but I could probably squeeze a few hours a week in.

Is there a document somewhere that indicates how vulnerabilities are triaged and escalated? I looked and couldn't find a clear incoming vulnerability escalation document. For example, how many people in the security-wg looked at this vulnerability before it escalated into a CVE?

I'd like to help, but I also don't want to just be another cog in a security wheel pushing stuff along without really looking at it. From the outside, this is what I see:

  1. The initial report was made by a self described "bug bounty hunter". In my opinion, they got a bit carried away trying to add to their bug count and reported a non-vulnerability.
  2. The initial ticket was filed. I'm not sure if @lirantal looked at the code, but he definitely tried to reproduce the error. The package maintainer was invited to participate.
  3. The package maintainer appears to realize this is a non-issue but seems to want to please the official nodejs security working group by making a change to his package:

My first concern is that exceljs is, for the most part, content agnostic and many installations using it will rely on this so if we were to modify the return value of the .value property it would break a lot of systems.

  1. His concerns are ignored.
  2. @lirantal files a pull request (presumably to get the CVE out?) #395
  3. This pull request itself contains no details of what the vulnerability is, besides the fact that it is "XSS". It has no description and is quickly approved with no discussion.
  4. @yonjah sees this ticket and echoes my sentiments exactly: #395 (comment) . @lirantal seems to agree, but ignores this concern.
  5. The CVE is filed. This gets registered with the government.
  6. Github shows a big warning message, I'm assuming based on the CVE?
  7. My engineer tries to do an emergency production release to fix the vulnerability that github notified us about.

I'm not sure what more people in the WG would have done to help mitigate this. The way I see it, there were 4 points when this should have been nipped in the bud:

  1. When the report was initially filed, the responder should have looked at the code carefully and dismissed the ticket.
  2. The package maintainer brought up some valid concerns, although maybe not as vocally as he should have. Those were ignored.
  3. The ticket was approved without investigating the vulnerability.
  4. @yonjah commented, pointing out how ludicrous the ticket was and was mostly ignored.

If just adding eyes would help, I'd consider helping... but I feel like this is also a process question and I'm not sure I have the energy to join a group and fight for major process change myself.

@jbreckman

This comment has been minimized.

Copy link
Author

jbreckman commented Sep 17, 2018

On a whim, I was clicking around to bl4de's hackerone profile and see that pretty much all of his reported "vulnerabilities" are complete nonsense.

https://hackerone.com/bl4de?sort_type=latest_disclosable_activity_at&filter=type%3Apublic&page=1

There are 9 "XSS" vulnerabilities there that should have all been shut down.

@yonjah

This comment has been minimized.

Copy link
Contributor

yonjah commented Sep 17, 2018

We really love an appreciate the security warnings, but they have to be properly vetted. If we start alerting and filing unnecessary CVEs, then the warnings start to mean less and will start to be ignored.

@jbreckman I couldn't agree more and this was my intention on raising this concern. But I don't think @lirantal ignored my comment. I didn't make it clear enough or pushed on this point too much on my initial comment and unfortunately been a bit too busy to keep chasing that issue.

This invitation is actually valid for anyone ready this message. The Working Group needs more hands, it could be you!

@vdeturckheim node.js security initiatives and this working group are doing amazing job I'd be happy to help where I can.

@bnb

This comment has been minimized.

Copy link
Member

bnb commented Sep 17, 2018

@vdeturckheim not sure if there's still spaces on the schedule, but perhaps defining criteria is something we could have a session on at the Collaborator's Summit in Vancouver next month?

@jbreckman

This comment has been minimized.

Copy link
Author

jbreckman commented Sep 17, 2018

@yonjah maybe "ignore" was the wrong word. He definitely responded and seemed to understand, but the CVE ended up being filed anyway. It just seemed like there was a lot of momentum moving the "issue" towards a CVE.

@lirantal

This comment has been minimized.

Copy link
Member

lirantal commented Sep 17, 2018

Hey @jbreckman 👋,

It was my mistake to request a CVE instead of resolving the issue as informative (as I've done with several other reports on the same topic in the past). Apologies for the issues this may have caused for you out of human error. I will take better care in the future.

I do not believe I or bl4de ignored the maintainers' concerns. Please refer to the rest of their message that points out a workaround they suggested about adding another method for a specific content encoding context. Based on that we have resolved the report.

I'm also not sure why you have concluded that I've ignored him when I felt I was completely agreeing with him.

Thanks for engaging - I want to make sure your concerns are addressed. As others have pointed we are a small volunteer team and are always understaffed.

@jbreckman

This comment has been minimized.

Copy link
Author

jbreckman commented Sep 17, 2018

Mr. @lirantal!

I really hope I'm not coming off as too much of a jerk in these messages. It is totally not my intention. I just want what's best for nodejs, and will do what I can to help.

With regards to the package maintainer, I'm not sure you appreciate how intimidating a message from the "Official NodeJS Security Working Group" is. A maintainer can either find a way to appease the request or fight back - and it is probably easier to just find a way to appease. Some people are less stubborn than me :)

My big problem right now is that no one seems to be saying "The package maintainer should not have gotten involved and no changes should have been made to the package."

Because of this, a not-that-great HTML escaping function was added to exceljs, which has now made that package less secure. By default, engineers should assume that their template/frontend package should escape the values, but now there is a perceived notion of security and if someone makes the mistake of using .html from this package, their project will now be at risk.

If you look at https://hackerone.com/reports/309367 - it looks like your gut instincts were right, but bl4de pressured you into moving forward with it. This is another ticket that should have been immediately closed.

This one didn't result in a CVE, but is clearly just bad sample code (again): https://hackerone.com/reports/317125

Originally I thought all of bl4de's submissions were totally bogus, but upon further inspection a lot of the code samples were from the package itself. It's actually really hard to see and I totally see how you'd make that mistake (see this as a sample https://hackerone.com/reports/309648) Code samples from the package showing the vulnerability are obviously great to see - but at least 3 of these tickets have code sample vulnerabilities from a testbed app that bl4de wrote himself.

Anyway, I love what you guys are doing. We love security over here and really talk about it all the time. I'd love to get involved in some way but am not sure the best way to be effective. I got an invite to the public slack group and have been a fly on the wall for most of the day :)

Josh

@vdeturckheim

This comment has been minimized.

Copy link
Member

vdeturckheim commented Sep 18, 2018

@yonjah \o/ I'm looking forward to working with you then

@bnb good question. We'll start something at WG level and might try to find some time during the summit (or the conf) to discuss it!

@yonjah

This comment has been minimized.

Copy link
Contributor

yonjah commented Sep 25, 2018

@vdeturckheim let me know how I can help.

This is not 100% related but I noticed a few issues where no warnings are shown for known vulnerabilities.
For example hapijs/cryptiles#34 was fixed some time ago and has a CVE assigned but since I guess the entire process was done outside the working group it is not shown when running npm audit (but github will send a warning if you commit a json-lock file that includes the affected version) I assume github is scaning he CVE database automatically so it would be great to integrate this feature into the WG database somehow.

I also noticed some other packages that maintainers were reported vulnerabilities by some random user (usually publicly in github issues) but don't seem to act on the report in anyway. what should be done in this case ?

@vdeturckheim

This comment has been minimized.

Copy link
Member

vdeturckheim commented Sep 27, 2018

@yonjah I believe @MarcinHoppe started to work on a document. I'll check what the status is.

You point 2 intersting things: how do you think we can find a way to learn about these vulnerabilities handled outside of the WG?

@yonjah

This comment has been minimized.

Copy link
Contributor

yonjah commented Oct 1, 2018

@vdeturckheim with my second point, I wan't actually thinking about how the WG could learn about those issues (even though it can be an interesting problem to tackle) but more on once an individual is aware of such an issue what is the best way to raise the WG awareness to it.

Going through the bug bounty seems a bit off since it is an issue which was originally reported by someone else.
So what is the responsible way to disclose such issues to the WG

@lirantal

This comment has been minimized.

Copy link
Member

lirantal commented Oct 2, 2018

if I understand you correctly @yonjah there are several ways to report an issue beyond just H1:

  • if the issue has already been fixed and you want to report after the fact, it's possible to just open a PR for the report with the issue details and we can help to properly format and fill it with you
  • another way to report is to e-mail us at security-ecosystem@nodejs.org (more info here: https://nodejs.org/en/security)
@yonjah

This comment has been minimized.

Copy link
Contributor

yonjah commented Oct 3, 2018

Thanks @lirantal.
This is actually an issue with an npm package so I guess I should e-mail security-ecosystem@nodejs.org
I'll try to do it later on today.

@yonjah

This comment has been minimized.

Copy link
Contributor

yonjah commented Oct 17, 2018

@lirantal I finally got the time to send the vulin report and got the following automated response from hackerOne -

You are receiving this message because Node.js third-party modules uses HackerOne to receive security vulnerability reports. Before Node.js third-party modules can review your vulnerability report, you must complete your submission on HackerOne.

ACTION REQUIRED: Submit Vulnerability Report.

is this intentional ?

@lirantal

This comment has been minimized.

Copy link
Member

lirantal commented Oct 18, 2018

/cc @reedloden

I think that could be happening because the program is also currently paused due to some reports we need to triage. Added Reed to help clarify.

@reedloden

This comment has been minimized.

Copy link
Member

reedloden commented Oct 18, 2018

@yonjah, I take it that you sent an e-mail to security-ecosystem@nodejs.org, correct?

If so, you're using HackerOne's email forwarding feature to submit. What that does is take the e-mail and store it as a draft report. In order to actually finalize the submission and send it to the team for review, you need to click the link in the e-mail, sign-in to or create a HackerOne account, and finish the report submission process.

Does that make sense? Basically, you still need to login to the HackerOne website and complete the report before the team can see it.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Oct 18, 2018

@reedloden We missed some of our response targets for ecosystem triage, what happens in H1 when that occurs?

@yonjah

This comment has been minimized.

Copy link
Contributor

yonjah commented Oct 22, 2018

@reedloden Yes.
But I sent the e-mail hopping I won't have to go through H1 process.

Mostly because it's already known issue so applying for a bounty feels a bit off
But also because I was hoping to avoid any H1 hassle (I think I should have an account some where)

It feels a bit weird that reports are required to go through H1.
So is it possible you have a few valid reports that weren't finalized and so unavailable for the team ?

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Oct 22, 2018

Sorry it feels weird, but from our point of view they have to go somewhere, and that somewhere has to be private. I don't think you have to apply for a bounty.

So is it possible you have a few valid reports that weren't finalized and so unavailable for the team

That is possible. :-) Just 1, actually, that we still have to triage, and a few pending disclosure. Our triage time targets are a bit aggressive, but we aspire to them.

@reedloden

This comment has been minimized.

Copy link
Member

reedloden commented Oct 23, 2018

@yonjah The Node.js Ecosystem security project doesn't offer bounties at all. It just uses HackerOne to manage incoming reports.

@yonjah

This comment has been minimized.

Copy link
Contributor

yonjah commented Oct 24, 2018

@sam-github @reedloden thanks for your replies.
I'll try to complete the H1 report then

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Oct 24, 2018

@yonjah Thank you. I know its a bit more work for you, but it makes things easier for us, and will also have some advantages for you. It will give you a private communication channel where we can discuss the report (the conversation will eventually become public when the vuln is public), and you'll be able to easily monitor the progress.

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.