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.

Sign up for GitHub

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

Add draft of ecosystem triage guidelines #508

Merged
merged 6 commits into from
Jul 8, 2019
Merged

Add draft of ecosystem triage guidelines #508

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
55e16fe
Add draft of ecosystem triage guidelines
Apr 3, 2019
5196040
Remove unnecessary line
Apr 5, 2019
33bc5e0
Merge branch 'master' into triage-guidelines
lirantal Apr 6, 2019
3fc5088
Add paragraph about threat models
MarcinHoppe Jun 27, 2019
bfa9b5a
Merge branch 'master' into triage-guidelines
lirantal Jun 28, 2019
6b20aec
Merge branch 'master' into triage-guidelines
MarcinHoppe Jul 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions processes/third_party_triage_guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Node.js Ecosystem Triage Guidelines

## Introduction

The purpose of this document is to bring the Node.js Ecosystem Triage team to a common understanding of how to triage and disclose security vulnerabilities reported through the Node.js third-party modules program hosted on HackerOne (https://hackerone.com/nodejs-ecosystem).

This is a living document and is expected to evolve over time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


## Why are guidelines needed?

The Node.js Ecosystem Triage team consists of volunteers with varying level of time, energy and vulnerability management experience. Documenting best practices and common understanding of core concerns when triaging issues will allow the team to provide more consistent service both for security researchers and NPM package maintainers.

Guidelines will also be a useful resource in onboarding new triage team members.

## What counts as a vulnerability?

HackerOne gives the following definition of a vulnerability:

```
A software bug that would allow an attacker to perform an action in violation of an expressed security policy. A bug that enables escalated access or privilege is a vulnerability. Design flaws and failures to adhere to security best practices may qualify as vulnerabilities. (...)
```

Any bug that is directly exploitable by attackers is a vulnerability.

Special case to consider are defects in libraries: if a documented or obvious way to use a library leads to an exploitable vulnerability in the correct and safe calling code, then those defects are also vulnerabilities. Some APIs are unsafe to use and are not vulnerabilities if they are clearly marked this way and if safe alternatives exist. An excellent example of this is dangerouslySetInnerHTML in React.
lirantal marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@ronperris ronperris Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@ronperris ronperris Apr 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


## What is the root cause of the vulnerability?

There are cases where the submitter notices a vulnerability in a parent package but the root cause of the vulnerability can be traced to one of the downstream dependencies. An example of this was an XSS vulnerability in the serve package that could be traced to lack of proper HTML escaping in the downstream serve-handler package. In cases such as this one, it is important to work with submitter and package maintainers to determine where a vulnerability in question is best remediated.

## Does package maintainer have to acknowledge the vulnerability?

If the triage team can contact the maintainer, then getting them to acknowledge the vulnerability before proceeding is strongly encouraged. If it was not possible to make contact, the responsibility for making a decision is on the triage team.

## Which vulnerabilities need to be disclosed?

Vulnerabilities that require users of a package to take an action, most often upgrade of the package, need to be publicly disclosed.

## What about resolution verification?

Sometimes vulnerability resolutions are partial and do not fully resolve the underlying problem. It is encouraged to facilitate collaboration between the submitter and package maintainer to verify if the resolution is full and does not lead to further security problems.

# References

HackerOne Disclosure Guidelines
https://www.hackerone.com/disclosure-guidelines

CVE Counting Rules
https://drive.google.com/file/d/1edCzKfsds79S6z411EJ5qQa-c6z2Bc4A/view