JavaScript: Fix DomMethodCallExpr.interpretsArgumentsAsHTML.#1835
JavaScript: Fix DomMethodCallExpr.interpretsArgumentsAsHTML.#1835semmle-qlci merged 2 commits intogithub:masterfrom
DomMethodCallExpr.interpretsArgumentsAsHTML.#1835Conversation
|
Worth landing IMO.
Let's talk about this offline |
|
How about restricting this to only flagging assignments to a library set of dangerous attribute names (such as "src", "href", "style")? That is what we do for ordinary property assignments such as The "data" property is interesting for an exploratory query, as that property often is read later and used unsanitized in an ordinary XSS sink. |
Good idea in general, although it wouldn't help avoid the false positives in this case; they are all about setting |
|
Updated as suggested. |
| ) and | ||
| // restrict to potentially dangerous attributes | ||
| exists(string attr | attr = "href" or attr = "src" | | ||
| getArgument(argPos-1).getStringValue().toLowerCase() = attr |
There was a problem hiding this comment.
We already have isUrlValuedAttribute which seems to do this more generally, although it currently only works if the element type is known. Maybe we could reuse that?
There was a problem hiding this comment.
Hm, interesting thought. That predicate does rely on knowing what the element type is, though, which in this case we don't. (I'll definitely add action and formaction here, though.)
3 participants
The first argument to
setAttributeis the attribute name, which is generally less sensitive than the second one, so I've changed it. Similar comments apply to the other changes.I've run a dist-compare (internal link) on nightly.slugs. The results are a bit mixed: while the new results on Lucifier look good, the ones on gatsby are false positives. They are, however, in test code, so wouldn't be displayed on LGTM.
I think a proper fix for these false positives would involve flow-labelifying the XSS query.
If we feel uncomfortable with the new results, I'm happy to scrap/postpone this PR.