Add draft of ecosystem triage guidelines

[MarcinHoppe]

This document aims at getting all members (present and future) of the Ecosystem Triage Team to a (hopefully) consistent practice of vulnerability triage.

[MarcinHoppe]

@nodejs/security-wg All feedback appreciated.

[yonjah]

Hi @MarcinHoppe this looks great and I can't find any issues with it.
Not being part of the WG I don't know how WG triage process is being handled internally so it might be hard to comment on the process so take my input with a grain of salt.

[sam-github]

Document content is helpful during triage, and too bug reporters, but from the title and introduction, I thought it would actually describe how to do the triage process, but instead its a description of what we would or would not consider a vulnerability.

[sam-github]

I don't know this is worth saying, all our docs evolve over time!

[MarcinHoppe]

This was copied from its original Google Doc format, I will remove this line.

[ronperris]

Some APIs are unsafe to use and are not vulnerabilities if they are clearly marked this way and if safe alternatives exist.

The and right here greatly expands the scope of what is considered a vulnerability that we must triage.

Many libraries are unsafe to use if the developer doesn't read the documentation and the solution is not always to implement an alternative API that prevents unsafe usage.

[yonjah]

@ronperris could you give an example where there is no safe alternative ?

I think that the key here is obvious way to use a library If the library has an obvious use that you expect to be safe it should not introduce unsafe behaviour even if it's documented.
I think noty is an extreme example when it's done wrongly text attribute is expected to only contain text and not parse html mentioning n the docs that is does parse html is almost meaning less.
I can think of a few more hypothetical example think of a sql library that offer a bind parameter -
coolSQL.query('select * from users where user id=?', { bind: [user.id]})
If the documentation for the bind property is `strings that are concatenated into the query please make sure to properly escape and wrap your parameters' it does not make the library safe.
There is just a few common behaviour developers expect. and even those carefully reading the documentation can fail to notice such inconsistencies from common practice.
Also there is a reason why the safe method is common. Even if you know exactly what a library does it is much more easy to make a mistake or forget to set a flag so best practice should require us to always use the more secure option by default and require to explicitly opt-out from the secure path when needed.

With that being said I don't think we have to go to the extreme of naming methods dangerouslySetInnerHTML. If the maintainer is willing to do that, that is great.
But naming a method that set html as html is probably good enough.
I think that maintainers that are anyway not keen to fixing a security issue in their package would find it hard to agree to add a mthod prefixed with unsafe but will be willing to name the method to something more descriptive like htmlAlert or rawConcat.
Also changing the API doesn't mean that you have to change the method name alert(msg, {allowHTML: true}) is as good and maybe even better than htmlAlert so I think this can be left to the maintainer to decide what might work best with his current API

[ronperris]

@yonjah Here one of thousands of example APIs that are script sinks that don't have warnings, safe defaults or documented safe alternatives. http://snapsvg.io/docs/#Element.append

Is this a vulnerability that you expect us to triage and force your prescribed solution on?

[yonjah]

Sorry @ronperris I'm not familiar with snap.svg.
What is the vuln when using Element.append ? What does it does differently to the obvious way which I assume is Node.appendChild ?
Does it introduce new security concerns compared to the native DOM Node.appendChild?

[ronperris]

This is the line in the new policy I have an issue with.

Some APIs are unsafe to use and are not vulnerabilities if they are clearly marked this way and if safe alternatives exist.

This API has multiple unsanitized and unescaped script sinks, but it isn't a vulnerability and doesn't need remediation.

[MarcinHoppe]

I'd rather clarify what may constitute a safe alternative rather than remove this line, because I think this cases are the most interesting to have guidelines for.

To use the Bootbox example, I think its alert method does not classify as a vulnerability. In the context of this guide, I there are two questions that need to be answered:

1. Is the API clearly marked as unsafe?

In my opinion the answer is yes, because it is clearly spelled out in the documentation, including explicit mention of XSS risk.

2. Does a safe alternative exist?

In my opinion the answer is again yes, because the caller can escape the data before passing it to Bootbox alert.

I already had a chat about this with @ronperris but would love some more feedback from @lirantal and @yonjah on this.

I think there is value in keeping this and I would like for us to flesh it out in a way that is reasonably unambiguous and helpful in day-to-day triage activities.

[lirantal]

About the "save alternative" - my point being is what if there is no safe alternative? Which is the case with bootbox if I read correctly from their API. If they move the risk to the user then is it an alternative? You can always add layers of abstractions and transfer risk to other parts, I'm just not sure that clearly means an "alternative". If we come to an agreement that this is what it means then let's make that perfect clear in the guidelines :)

[yonjah]

@MarcinHoppe as I mentioned earlier I still have hope for a fix with code changes so the actual API will default for the more secure implementation. But I understand completely that all WG members has more experience than I do on this specific matter so I rather agree to what you see right.

One thing I would be happy is if you can clarify how you see a report should be made in this instance.
If I have a similar case where there is a potential misuse of a library due lack of documentation.
Does it even make sense to report it to the WG ?
assuming there is a public repository wouldn't it just be as good to open a public issue and maybe even a pull request ? Is there any value in privately disclosing an issue where the only required change is anyway updating the docs ?

I mean I can see the value in having the WG (and in bootbox case even though there was a github issue the documents were only properly updates after the triage ended) but since the WG team is busy as is, maybe it's better to try and handle this kind of issues initially without the WG involvement ?

[MarcinHoppe]

@yonjah I'd like to separate covering the specific Bootbox submission from general guidelines around triage (not submission) as much as possible. I think keeping the discussion on Slack or the original H1 issues would be better and would make it more clear what this PR is about.

[MarcinHoppe]

@lirantal How about phrasing it in terms of intended usage or library (function, method) contract? For example: if library documentation clearly states that the caller is responsible for escaping HTML content rendered by the library in question, it would not qualify as a vulnerability. Would such verbiage make it clearer?

The safe/unsafe alternatives would be left as is to cover cases such as dangerouslySetInnerHTML.

[mhdawson]

LGTM

[lirantal]

Good work @MarcinHoppe !
I would think there is a small issue to fix with regards to Ron's comment and good on my side.

[MarcinHoppe]

@sam-github Do you think we need to have a more technical description of how to triage issues in HackerOne? We could have a step-by-step instruction or a tutorial documented here. So far this has been tribal knowledge shared during onboarding of new triage team members.

[MarcinHoppe]

I added one paragraph to cover a case where packages clearly document their threat models and desired security properties.

@nodejs/security-wg can you give a once-over and chime in if you are comfortable merging this proposal and adopting it?

[lirantal]

Looks ok to me.