Suggestion: Force links to be opened in new windows/tabs

[cnsgithub]

Hi,

I am using owasp-java-html-sanitizer-20180219.1 and just identified one odd thing when using the pre-packaged link sanitizer Sanitizers.LINKS. What I was trying to do is just defining a link's target to be a new window/tab by using <a href="http://phishing.site" target="_blank">Link</a>.

However, the sanitizer does not allow the target attribute and discards it resulting in: <a href="https://phishing.site" rel="nofollow">Link</a>

In my opinion, it's more a security risk than it helps in this case. Not only, that it allows for defacement. Just imagine, the linked site was a phishing site looking exactly the same as the embedding site's login page. As target="_self" is the default, the site's content would be replaced by the malicious site.

My suggestion would be to mitigate such attack vectors by adding an extra rule like requireNewTabOnLinks (in analog to requireRelNofollowOnLinks) to the pre-packaged Sanitizers.LINKS.

This is extremely important also because there seems to be no possibility to override this behavior by the developer. If there is one, please let me know.

At the moment, the only thing I can do is to explicitly allow the manual addition of target="_blank" by overriding the default sanitizer: Sanitizers.LINKS.and(new HtmlPolicyBuilder().allowElements("a").allowAttributes("target").matching(true, "_blank").onElements("a").toFactory())

However, that does not protect against malicious input...

So, as this is a security issue, it should be addressed with high priority. Please let me know, if you want me to provide a pull request.

Cheers

[cnsgithub]

Any thoughts?

[jmanico]

This seems like a good suggestion. I’m curious to hear Mike’s thoughts...

On Jul 2, 2018, at 12:36 PM, ╭∩╮ (òÓ,) ╭∩╮ notifications@github.com wrote:

Any thoughts?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[cnsgithub]

It's been four months now...

@jmanico Thanks for your reaction.

Just in case anyone comes across this weakness, I came up with the following workaround that will always add the important target="_blank".

PolicyFactory policy = Sanitizers.LINKS.and(new HtmlPolicyBuilder().allowElements("a").allowAttributes("target").matching(true, "_blank").onElements("a").requireRelsOnLinks("noopener, "noreferrer", "nofollow").toFactory());
String sanitizedHtml = policy.sanitize(unsafeHtml.replace("<a ", "<a target=\"_blank\" "));

[ryan-copperleaf]

As target="_self" is the default, the site's content would be replaced by the malicious site.

This is how most links work. Not really a vulnerability.

If you’re going to add target="_blank", you should also add rel="noreferrer" or at least rel="noopener" so the target of the link doesn’t receive window.opener in modern browsers (which allows it to redirect the original page for a much more convincing phishing attack).

[cnsgithub]

@ryan-copperleaf Thank you very much for pointing that out. :+1: I just adjusted the policy in my previous comment accordingly adding both rel="noopener"and rel="noreferrer" because MS browsers do not support rel="noopener": https://caniuse.com/#search=noopener

Further reading for those being interested in this stuff:

• https://cwe.mitre.org/data/definitions/1022.html
• https://mathiasbynens.github.io/rel-noopener/

[mikesamuel]

You can do this with a custom element policy.

myPolicyBuilder.allowElements(
    (elementName, attrs) -> {
      int targetIndex = attrs.indexOf("target");
      if (targetIndex < 0) {
        attrs.add("target");
        attrs.add("_blank");
      } else {
        attrs.set(targetIndex + 1, "_blank");
      }
      return elementName;
    },
    "a");