the default onsiteURL regex is not safe, if the url starts with '//', the url can jump out of the origin domain

[onemoreflag]

the default onsiteURL regex:
<regexp name="onsiteURL" value="^(?![\p{L}\p{N}\\\.\#@\$%\+&amp;;\-_~,\?=/!]*(&amp;colon))[\p{L}\p{N}\\\.\#@\$%\+&amp;;\-_~,\?=/!]*"/>

usually，rich text requires the href attribute and the validation rule like this:

<attribute name="href">
			<regexp-list>
				<regexp name="onsiteURL" />
			</regexp-list>
</attribute>

so if developer trust the onsiteURL regex, they will not do any other domain validate, but the onsiteURL regex can bypass by '//' like '//evali.com?params', In this case, phishing attacks may occur. In addition, information leakage may occur due to such as dangling markup attacks.

[davewichers]

I think I understand your concern, but to be super clear can you provide a pull request to AntiSamyTest.java with a failing test case that demonstrates your issue? And a suggested change to the default onsiteURL regex that fixes it, if you have a suggestion?

[onemoreflag]

I think I understand your concern, but to be super clear can you provide a pull request to AntiSamyTest.java with a failing test case that demonstrates your issue? And a suggested change to the default onsiteURL regex that fixes it, if you have a suggestion?

failing test case use the 'antisamy-myspace.xml' policy file:
String dirtyInput = "<a href=\"//kkk.com/stealinfo?a=xxx&b=xxx\"><span style=\"color:red;font-size:100px\">You must click me</span></a><portal src=\"blah";
output: <a href="//kkk.com/stealinfo?a=xxx&amp;b=xxx"><span style="color: red;font-size: 100.0px;">You must click me</span></a>

and maybe this regex can stop someone use '//' jump out the origin domain:
^(/(?![\p{L}\p{N}\\\.\#@\$%\+&;\-_~,\?=/!]*(&colon))[\p{L}\p{N}\\\.\#@\$%\+&;\-_~,\?=!]+)+

but i cant guarantee that this regex will satisfy all requirements maybe you'd better do more test. (*￣︶￣)

[gurshafriri]

👋 @davewichers does this seems like a request forgery vulnerability to you? if so, we (at snyk) would like to add it to our vulnerability database. let me know if you have any further context that would make this not a security issue.

[davewichers]

@gurshafriri - We simply have not had time to research. Based on the fact that most 'security issues' people report turn out to be false positives, I'd suggest holding off for now. We'll get back to you when we know more.

[nahsra]

There's no risk of XSS here, but it is a bypass of the regex to provide a non-host URL, so I think we can can all agree that ideally it should be fixed.

Can you elaborate on what you see as an information leakage risk? I'm not seeing how the attacker can leak anything back to themselves except their own payload URL.

[onemoreflag]

information leakage may occur due to such as dangling markup attacks：https://portswigger.net/web-security/cross-site-scripting/dangling-markup
but it depends on browser version and so on

[onemoreflag]

now i can see that there exists a redirect leak. An attacker may successfully launch a phishing scam and steal user credentials by inject the url.
https://cwe.mitre.org/data/definitions/601.html

[davewichers]

@spassarop - If the new AntiSamy .NET you are working on is using the same regex, then this issue will affect it too. Do you have the time/background to suggest a fix for this issue? Ideally, we'd fix it here first, and you'd use the same solution in your project before it is ever released.

[spassarop]

Regex is the same, so yes, it should be fixed on both. I'll see this in the following days with the provided links and try to come up with a solution.

[spassarop]

Well, after giving this some thought, from scratch, this is my opinion on the issue (open to comments):

Risks

As @nahsra said, there is no apparent XSS risk as no characters to break from the attribute are allowed.

I dug up on the dangling markup attack (I even did one of the PortSwigger labs) to understand how this can be exploited. And to support the previous statement, there must be XSS to exploit this as such, which is not the case and no HTML will be "absorbed" by the query string by trying to leave the attribute open.

On the other side, there is indeed a risk because of potential phishing attacks (as @onemoreflag said) as there is no way of checking by regex that the domain is the same. No information stolen on the parameters, but unsolicited "redirect" to an external site anyway.

Potential solution

I'll break the regex down so I can explain better:

In Java, the regex ends up like this:
^(?![\p{L}\p{N}\\\.\#@\$%\+&;\-_~,\?=\/!]*(&colon))[\p{L}\p{N}\\\.\#@\$%\+&;\-_~,\?=\/!]*
Which can be seen as this:
^(?![ALLOWED_CHARS]*(&colon))[ALLOWED_CHARS]*

Where at the beginning we have the (?!MORE_REGEX) which means negative lookahead, looking that the sub-regex inside does not match. Regardless of it's content, we know it cannot assure that the beginning of the attribute does not have // on it. As at the end of that negative lookahead group there is &colon, we cannot add a restriction for // inside that (it will only be a valid restriction if the text has &colon at the end).

The current regex does not consider spaces of any kind at the beginning, so if we are still on that path, we can safely add the restriction right there, using ^(?!//) and getting the final regex:
^(?!//)(?![\p{L}\p{N}\\\.\#@\$%\+&;\-_~,\?=\/!]*(&colon))[\p{L}\p{N}\\\.\#@\$%\+&;\-_~,\?=\/!]*

That regex passes the test and it seems to me that the initial behavior is preserved. Of course, users cannot use //URL even if it is on the same domain, but that was known from the start. The fix should be adding ^(?!//) on every onsiteURL definition. If this gets approved, I'll add it on AntiSamy .NET.

[davewichers]

@spassarop Thanks for jumping on this so quick, and doing this analysis!

@nahsra - can you review and if you are OK with this, we'll get this fix implemented.

@onemoreflag - Given you raised this issue, what are your thoughts on this proposed fix?

[nahsra]

Makes sense to me! The tests will give us the assurance we need as well.

[davewichers]

@spassarop - Any idea when you might submit a pull request with your proposed change and one or more test cases that fail without the change, but then pass after the change? We are trying to get a release out by end of Nov. as OWASP ESAPI wants to upgrade to a newer AntiSamy that includes the dependency updates that are in the 1.5.11 branch.

[spassarop]

@davewichers - I was waiting for @onemoreflag to state an opinion. Also I didn't know I you wanted me to do make the change and a PR, because I saw you added the test. I think I can do this later today on the same, just tell me on which branch should I work on.

[davewichers]

@spassarop - If you could submit a pull request for this to the branch, that would be great. As to 'the test' I don't recall doing that. But if it was added to master, can you include it in your pull request to the branch?

[spassarop]

@davewichers - I don't really know where it was pushed, this is the commit a526dd1, I've found it above. I'm doing a PR with the change and the test.

[spassarop]

I've also added two tests that didn't fail before, just to make clear that dangling markup attack will fail even when escaping from the tag attribute as the resulting URL (which absorbs and steals the HTML after the URL) is not a valid one.

One test tries to steal more tags (more probable for the attack to fail bypassing AntiSamy) and the other tries to steal just the following tag attribute (less probable to fail, but it does anyway :D).

[davewichers]

This is now fixed in the 1.5.11 branch and will be included in the 1.5.11 release, which should come out sometime this month.

[davewichers]

The commit that fixes this e59963e, has been merged into master and will be included in the release going out today.