Skip to content

JS: Fix 2 FPs in TargetBlank.ql#4173

Merged
codeql-ci merged 6 commits intogithub:mainfrom
erik-krogh:targetBlankFP
Sep 4, 2020
Merged

JS: Fix 2 FPs in TargetBlank.ql#4173
codeql-ci merged 6 commits intogithub:mainfrom
erik-krogh:targetBlankFP

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Aug 31, 2020

  1. mailto: links (found in an anomalous query result)

mailto links are safe, they open in a email-program / webmail (which should be safe).

  1. links constructed using template system with safe prefix (reported here: LGTM.com - false positive #4169)

Something like <a href="http://google.com/{{payload}}>safe</a> was already considered safe, but something like <a href="foo.bar#{{payload}}">safe</a> was not.
With this PR we use # and ? as marker chars to determine if the payload is actually part of the query/hash part of the URL.
This does remove some additional FPs in our usual benchmarks.
But it is not perfect, as some template expressions might contain ? or # without those chars ending up as a prefix in the URL.

@erik-krogh erik-krogh added the JS label Aug 31, 2020
@erik-krogh erik-krogh marked this pull request as ready for review September 2, 2020 19:36
@erik-krogh erik-krogh requested a review from a team as a code owner September 2, 2020 19:36
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

I have a request for some minor polish. And a discussion:

mailto links are safe, they open in a email-program / webmail (which should be safe).

I am not sure that they are safe. Are we sure that the mailto protocol handlers doesn't preserve the runtime reference to the DOM if the handler implementation somehow knows that the mail client is in another webpage (i.e. webmail)?

I know of security fixes for target=blank links that went to reputable sites such as github.com or google.com, simply because the security model didn't trust external sites, so it doesn't seem like a stretch to distrust arbitrary webmail services.

*/
bindingset[prefix]
string getDelimiterMatchingRegexpWithPrefix(string prefix) {
result = "(?s)" + prefix + "(" + concat("\\Q" + getADelimiter() + "\\E", "|") + ").*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update getDelimiterMatchingRegexp to have the body:

result = getDelimiterMatchingRegexpWithPrefix(".*")

@erik-krogh
Copy link
Contributor Author

I am not sure that they are safe. Are we sure that the mailto protocol handlers doesn't preserve the runtime reference to the DOM if the handler implementation somehow knows that the mail client is in another webpage (i.e. webmail)?

If mailto: opens in a webmail client, then window.opener works exactly the same as if it was an ordinary http:// link.
My thinking was that we could trust gmail.com/outlook.com etc..

so it doesn't seem like a stretch to distrust arbitrary webmail services.

Lets do that - I'll revert the mailto: change.

@esbena
Copy link
Contributor

esbena commented Sep 3, 2020

Thanks. Lets add a change-note and merge this.

@codeql-ci codeql-ci merged commit 58f5189 into github:main Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments