Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add draft of ecosystem triage guidelines #508
Add draft of ecosystem triage guidelines #508
Changes from all commits
55e16fe
5196040
33bc5e0
3fc5088
bfa9b5a
6b20aec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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 wronglytext
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 likehtmlAlert
orrawConcat
.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 thanhtmlAlert
so I think this can be left to the maintainer to decide what might work best with his current APIThere was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 theobvious way
which I assume isNode.appendChild
?Does it introduce new security concerns compared to the native DOM
Node.appendChild
?There was a problem hiding this comment.
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.
This API has multiple unsanitized and unescaped script sinks, but it isn't a vulnerability and doesn't need remediation.
There was a problem hiding this comment.
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:In my opinion the answer is yes, because it is clearly spelled out in the documentation, including explicit mention of XSS risk.
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.
There was a problem hiding this comment.
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 :)There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.