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

Suggest ignoring a vulnerability by the package maintainer #386

Closed
naugtur opened this issue Jul 17, 2020 · 34 comments
Closed

Suggest ignoring a vulnerability by the package maintainer #386

naugtur opened this issue Jul 17, 2020 · 34 comments
Labels
package-maintenance-agenda Agenda items for package-maintenance team

Comments

@naugtur
Copy link

naugtur commented Jul 17, 2020

In a discussion of this: npm/rfcs#18
@wesleytodd suggested I bring it up here to collect feedback on the feature itself, but mostly to ask one thing:

If this can be leveraged for the package maintainers to declare a vulnerability in their own dependency does not affect the security of the package. And if so - how would you want to indicate that as opposed to ignoring the issue for your internal needs of stopping the CI from failing while there's no fix to update to.

@MarcinHoppe
Copy link
Contributor

I think this would be a very useful option to have to increase the signal to noise ratio for vulnerability reports.

@naugtur
Copy link
Author

naugtur commented Jul 17, 2020

Any thoughts on how to do it?
I see two options

  • special option in audit-resolve file
  • a field in package.json

Second option has the appeal of it being crawled by package managers when creating the lock file anyway, so the info could be pulled from there.

@ljharb
Copy link
Member

ljharb commented Jul 17, 2020

I’ve suggested this to npm many times; the challenge/pushback I’ve heard is that, how would you stop a malicious maintainer from hiding a cve report for the purposes of leaving users vulnerable?

@MarcinHoppe
Copy link
Contributor

Malicious maintainer has more direct opportunities for embedding malware in their package, IMO.

@naugtur
Copy link
Author

naugtur commented Jul 18, 2020

And these would not be ignored by default but provides as hint for decision making.
Just a field on audit output.

@darcyclarke darcyclarke added the package-maintenance-agenda Agenda items for package-maintenance team label Jul 22, 2020
@mhdawson
Copy link
Member

Sounds like a good idea. I saw a tweet today from @lirantal mentioning that Snyk is doing something to try an figure that out automatically.

One way I could see that fitting with that suggestion is that a maintainer could use the info from Synck to populate the data that needs to be provided so that all users of the module benefit.

@wesleytodd
Copy link
Member

how would you stop a malicious maintainer from hiding a cve report for the purposes of leaving users vulnerable?

Maybe the question should be phrased as "lazy maintainer". I fully agree that if you have a malicious maintainer you are owned already, but what incentivizes a lazy maintainer to not just ignore all at their level?

That said, I think optimizing for this behavior means we might miss out on the more impactful feature here, which will save "good" maintainers from this burden they have today. So I don't think the answer to this question should block anything as long as we are not making the situation worse.


So the format I think this should move forward is as a spec for the json file. I think the implementation of it in npm will be awesome, but to get broad adoption and enable other tooling ecosystems (yarn, snyk, etc) to build on it we would want an official spec to live somewhere. I think this group is a find place for that to live if @naugtur agrees.

@ljharb
Copy link
Member

ljharb commented Jul 22, 2020

However, if npm and github (the tools who we'd need to respect this ignore setting) don't consider that a worthy tradeoff, then it's not really up to us :-/

@naugtur
Copy link
Author

naugtur commented Jul 22, 2020

There was a suggestion on today's RFC meeting to donate the package for embedders with the JSON schema within to an org. I'm on board with that.

If there's a place to put the tiny schema for the file, I'd love to do it.
Being in control of it was fun and all, but that's not the point.
Looking forward to suggestions.

@naugtur
Copy link
Author

naugtur commented Jul 22, 2020

However, if npm and github (the tools who we'd need to respect this ignore setting) don't consider that a worthy tradeoff, then it's not really up to us :-/

It's up to us to come up with something for them to consider and I believe it's likely to succeed coming from the fine folks here ;)

@ljharb
Copy link
Member

ljharb commented Jul 22, 2020

I certainly hope so; however the reasons I explained are why npm has said no to me on multiple occasions for this very request, so I'm not sure why they'd have a different opinion now.

@naugtur
Copy link
Author

naugtur commented Jul 23, 2020

Both the people involved and the NPM Inc. are different. If we come up with something reasonable, I'm pretty sure it'll get introduced into audit.

There's also an idea in the RFCs to audit licenses, which will require going to package.jsons in the tree and checking something, I think gathering vulnerability resolution recommendations should be doable in the same pass.

@ljharb
Copy link
Member

ljharb commented Jul 23, 2020

npm, inc. has 100% full control over npm audit, so i'm not sure what you mean.

We can try to persuade, and we should, but it's entirely their decision.

@naugtur
Copy link
Author

naugtur commented Jul 23, 2020

I never said we're the decision makers here. It's not something that'd stop me.
I came here to collect some opinions on what'd work and maybe put some of it in my RFC that's older than my now fluently speaking kid. I'm doing it in my rare spare time. I've got other unusual hobbies too 😉

Would you mind sharing the details of what you proposed before and what you think about it nowadays?

@ljharb
Copy link
Member

ljharb commented Jul 23, 2020

I didn't come up with a concrete proposal; I've just had multiple conversations where I wanted, as a maintainer, to be able to say "CVE XXX does not apply to my package", and then ensure that "my package" is never the reason a user sees a warning about that CVE.

@lirantal
Copy link
Member

Glad to join this discussion and indeed @ljharb and me spoke about this as part of his feedback for me about a year back.

@ljharb your feedback from past discussions with npm is weird to me (on their part). I'm referring to:

how would you stop a malicious maintainer from hiding a cve report for the purposes of leaving users vulnerable?

It's a weird statement in the sense that a maintainer could argue the same about a direct vulnerability in their package and claim "it doesnt reproduce/not relevant/etc" but that doesn't hold because the triage process validates the vulnerability and in the same way I expect that if a maintainer wants to "waive off" an indirect vulnerability, they'd need to follow the same and show evidence that there's no code paths/reproducing that works in order to deem a vulnerability as irrelevant.

What I'm trying to say here is that the burden of proof is going to be here on the maintainer, which to me sounds reasonable but also may add more complexity/effort on their part in some occassions.

About this:

However, if npm and github (the tools who we'd need to respect this ignore setting) don't consider that a worthy tradeoff, then it's not really up to us :-/

Last year we had a heated discussion on twitter on this with a bunch of folks and this is essentially what I was saying that this is a bigger ecosystem problem with the way that CVEs work. Meaning to say - even if today a maintainer would've reached out to Snyk to say an indirect CVE is not reachable in their package and Snyk would indeed stop showing it for its users, this solution would still be contained to only those who use Snyk. If you use GitHub or npm you'd still receive the alerts. That's why I have claimed several times that this is a broader issue with the way CVEs work and the complexity they create for indirect packages. You can't dismiss the CVE and you'd need to have the proper tooling/database that says a CVE isn't reachable to dismiss the vuln.

Generally speaking, with regards to what can be done - I think that even if for the Node.js Security WG for example we wanted to allow maintainers to opt-in and dismiss a vulnerability in their indirect package, I see the following complexities:

  1. it wouldn't really help anyone since the actual tooling would need to align: npm audit, github, snyk, etc
  2. who is going to assume the accountability for dismissing based on a maintainer's word? they could be wrong or simply not understanding the involved attack vectors and we'd be putting users in risk. maybe there are super obvious cases but I think those are the 20%, so what do we do about the 80% of cases where deeper security expertise is required?

My feel is that if 20% of the cases are very obvious and straight-forward to maintain and also account for the 80% of the ecosystem noise that will happen then it's still a good investment to focus there as it will ease maintainers and users a lot. Next would be to base on some sort of SAST tool to validate the findings of unreachability for the 80% of cases which are more complex.

@ljharb
Copy link
Member

ljharb commented Jul 25, 2020

I totally agree the real issue is with CVEs and the security industry built up around them, and not with any individual tool.

npm, github, and snyk, at least, would surely be able to align (if they all agreed it's a problem worth solving), and the rest would follow suit. The second point, however, is exactly why my many suggestions over the years to npm for this have not resulted in any changes.

@naugtur
Copy link
Author

naugtur commented Jul 25, 2020

It's midnight and I was catching up on the discussion.
A midnight crazy thought: what if instead of putting the ignore recommendation in the package itself we did something along the model of definitely typed repository with a collection of reports from people or even a voting/rating system where you could form a web of trust and decide you're going to ignore vulns that certain people also ignore?

It'd be fairly easy to add to an audit resolver tool - "3 people you like ignored this vulnerability"

@mhdawson
Copy link
Member

Discussed in the package-maintenance team meeting, we thought it would be good to invite people from npm/github, snyk to our next meeting.

Maybe @MylesBorins, @darcyclarke, @lirantal to see if there is any opportunity to work together on the issue.

@naugtur
Copy link
Author

naugtur commented Jul 28, 2020

maybe @isaacs and @wesleytodd too as they were the ones suggesting to involve this group.

@darcyclarke
Copy link
Member

Apologize I couldn't make it today. I'll make sure I can attend next week.

@dominykas
Copy link
Member

Would it be enough (or at least a good starting point) if the tooling gave a "Developer response" link as part of the audit report printed in the console?

Not sure whether the package.json or the audit.json would be the right place to store that link (I suspect not, as that requires an npm publish), but advisories usually already link back to discussions/PRs, etc - so bubbling that up and making it more prominent could be a step in the right direction?

@naugtur
Copy link
Author

naugtur commented Jul 30, 2020

Audit currently only reads package-lock, installation process involves going through the tree of package.json files, so at least there's battle tested code that does that...

I'd see it as a section in package.json if it's supposed to affect what audit displays

@dominykas
Copy link
Member

dominykas commented Jul 30, 2020

If we're talking about maintainers providing information on vulnerabilities in their package, then audit would have to fetch the package tarball to get the audit.json? And it would have to fetch the packument to read the vuln info from there? Not sure the lock files have space for this information?

Either way, I would think authors should have write access to add this info in the vulnerability db, which the audit tool fetches?

@dominykas dominykas reopened this Jul 30, 2020
@dominykas
Copy link
Member

(sorry, fat finger on the phone)

@mhdawson
Copy link
Member

We should have Darcy at the next meeting to represent npm. I talked to Adam Baldwin who can't make it but he'll catch up with Darcy between now and the next meeting. Still hoping we'll get @lirantal but piing here as I'll be out most of the time befor the next meeting and won't be able to follow up.

@naugtur
Copy link
Author

naugtur commented Sep 11, 2020

Not an ad and I'm not affiliated with snyk beyond having talked to two people working there. And I got magic wands with their logo.

Snyk has already published a feature that detects if the vulnerability is reachable from the app. For some other languages :) I know they're working on doing it in JS too.

Does that affect your opinion?
How does that change the case for people not using tools beyond package manager?

@boneskull
Copy link

It does not affect my opinion until it lands for JS, is open source (we need everybody using naive security checks to use something smarter, and they are not all Snyk users), and works as advertised.

@boneskull
Copy link

Are there "next steps" on this?

@mhdawson
Copy link
Member

I'm talking with a few people to see if they will champion a "collaboration space" focused on agreeing/documenting the path forward. Have not quite closed on that, but hoping that we spin that up as the next step.

@mhdawson
Copy link
Member

Please let us know if you think closing this was not the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package-maintenance-agenda Agenda items for package-maintenance team
Projects
None yet
Development

No branches or pull requests

10 participants