Skip to content

JS: add jQuery writes to href/src as sinks in js/client-side-unvaledated-url-redirection #6632

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

Closed
wants to merge 10 commits into from

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Sep 7, 2021

Inspired by a question on the Security lab Slack.

A quick run on LGTM shows that the sink is somewhat common.

An evaluation looks fine.
Two new results that look like TPs.

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Sep 7, 2021
@github-actions github-actions bot added the JS label Sep 7, 2021
@erik-krogh erik-krogh added JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis no-change-note-required This PR does not need a change note and removed Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Sep 7, 2021
@erik-krogh erik-krogh marked this pull request as ready for review September 7, 2021 10:15
@erik-krogh erik-krogh requested a review from a team as a code owner September 7, 2021 10:15
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Hm, the idea is to flag links where the javascript: scheme can be injected, right?

I'd rather avoid flagging links that simply have a user-controlled hostname, as it's quite common to allow that (you can do it right here on GitHub, for example). One of the two new results is of that type and I wouldn't consider that a vulnerability.

*/
class JQueryHrefSink extends Sink {
JQueryHrefSink() {
exists(string prop | prop = DOM::getAPropertyNameInterpretedAsJavaScriptUrl() |
Copy link
Contributor

Choose a reason for hiding this comment

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

AttributeDefinition should be used here. The jQuery model contributes to that class, and if it's a problem with jQuery it should be a problem regardless of how you assign to the attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about AttributeDefinition, it already has support for the pattern I was looking for.

We already used AttributeDefinition in the query, but only for a very limited set of attribute names.
Which is weird, because in ReactAttributeWriteUrlSink we just use all the property names from DOM::getAPropertyNameInterpretedAsJavaScriptUrl().

I'll try to make a DOMAttributeWriteUrlSink that behaves similar to ReactAttributeWriteUrlSink, and see what the result from that looks like.

One of the two new results is of that type and I wouldn't consider that a vulnerability.

That looks like a missing sanitizer node/edge to me, and not a problem with the sink.
I can take a look at that after I get results back from DOMAttributeWriteUrlSink.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AttributeDefinition should be used here. The jQuery model contributes to that class, and if it's a problem with jQuery it should be a problem regardless of how you assign to the attribute.

And the new evaluation looks exactly like the old evaluation.
I'll look into a sanitizer node/edge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll look into a sanitizer node/edge.

I think the problem is that some sinks are vulnerable to a user-controlled hostname (leading to simple redirection) while others require user-controlled scheme (leading to xss).

I think it's worth looking at a "deep" fix rather than just squeezing an extra sink in here. I think we should try to address these two issues:

  • There is no query with dedicated "untrusted URL scheme" sink. This could be the XSS query or a new query.
  • The src attribute on script tags leads to XSS for untrusted hostnames, but is modelled as client-side url redirection because we can reuse the sanitizer. This is causing confusion however, so it would be nicer if it was in the XSS query.

So I'm wondering if we should add the necessary flow labels to DomBasedXss and just pay the price. We already use TaintedUrlSuffix - maybe that can be reused.

@erik-krogh erik-krogh marked this pull request as draft September 9, 2021 16:38
@erik-krogh
Copy link
Contributor Author

I think it's worth looking at a "deep" fix rather than just squeezing an extra sink in here.

I gave a "deep" fix a go.

I now got 3 flow labels:

  • TaintedUrlSuffix: a URL where the attacker only controls a suffix.
  • Taint: a tainted value where the attacker controls part of the value.
  • PrefixLabel: a tainted value where the attacker controls the prefix

It seems to work OK.
An evaluation shows OK results.
There are some new FPs, but they are FPs for other reasons.
And there is almost no performance penalty.

@erik-krogh erik-krogh marked this pull request as ready for review December 2, 2021 16:03
@erik-krogh
Copy link
Contributor Author

#8304 replaces this PR.

@erik-krogh erik-krogh closed this Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants