nahsra / antisamy

a library for performing fast, configurable cleansing of HTML coming from untrusted sources
BSD 3-Clause "New" or "Revised" License
185 stars 89 forks source link

CVE-2022-24891 #252

Closed onemoreflag closed 1 year ago

onemoreflag commented 1 year ago

Please refer to:https://github.com/ESAPI/esapi-java-legacy/blob/develop/documentation/ESAPI-security-bulletin8.pdf Code that needs to be fixed

kwwall commented 1 year ago

@onemoreflag - I don't think there's anything to fix in AntiSamy. ESAPI had a slightly different (and very behind) AntiSamy policy file than an of the AntiSamy policy files that come with AntiSamy itself. The problem was a malformed regular expression that was originally correct but someone added a '~' at the end and it inadvertently changed the meaning of the regex. I see something similar on line 25 of tinymce.xml, but that line is commented out and thus should be harmless.

This it is not clear what code or policy file you think needs to be fixed. If you find it, please be more specific and it it is a security vulnerability, please practice responsible disclosure by following these steps outlined by the AntiSamy development team rather than reporting it as a GitHub issue. Failure to do that could put other projects that use AntiSamy at risk.

Thanks in advance for your cooperation.

kwwall commented 1 year ago

@onemoreflag - One additional thing... if you have found an Software Composition Analysis tool that is reporting this against AntiSamy (rather than against older versions of ESAPI, for which I am one of the project co-leads), the SCA is likely at fault. Please let the AntiSamy team know if this is being flagged by some SCA tool so that the AntiSamy team can contest it as a false positive. Thanks.

onemoreflag commented 1 year ago

@kwwall My project uses Antisamy, So I'm more concerned about whether it has security holes. You can confirm if there is a problem, I will also dig into whether the same vulnerability exists Hope it doesn't affect Antisamy

kwwall commented 1 year ago

@onemoreflag - Ultimately, since some customization of AntiSamy policy rules is generally expected, only you can test your own specific policy files for the sort of typos that were found in ESAPI. (It's much less common that ESAPI users tweak their AntiSamy policy files as our documentation doesn't call that out as an option. In fact, in my observation over the past 10+ years, I would say it is rare.)

There is sufficient information provided in ESAPI Security Bulletin 8 for you to construct your own specific tests against javascript: URLs, but based just on its name, it wouldn't surprise me one bit if at least the "antisamy-anythinggoes.xml" policy file allows that.

Ultimately, security is responsibility of those who are using the dependencies as library providers, no matter how conscientious or how skilled they are at AppSec, can't do it all. Clients of libraries must be using them as intended (which hopefully is sufficiently and accurately documented). Anytime there are configurations that may be tweaked for a given library, that goes double, because library providers cannot possibly test all possible configuration variations.

That said, if you do find a vulnerability in AntiSamy, please report if via these steps outlined by the AntiSamy development team rather than reporting it as a GitHub issue. As I mentioned previously, failure to do that could put other projects that use AntiSamy at risk.

onemoreflag commented 1 year ago

@kwwall ok

kwwall commented 1 year ago

@onemoreflag - I finally got around to testing all 6 of the sample AntiSamy policy files under https://github.com/nahsra/antisamy/tree/main/src/main/resources using the same test that I did to confirm that CVE-2022-24891 had been remediated for ESAPI. For each of the 6 Antisamy policy files, I used the respective policy file along with the SAX scanner to scan the tainted input string of:

String taintedInput = "<a href=\"javascript:alert(1)\">This is safe from XSS. Trust us!</a>";

to return a CleanResults object and then examined the cleansed output of CleanResults.getCleanHTML(). In all cases, the javascript: URL was stripped out. There were two variations of cleansed output, depending on the specific policy used for the scanning.

The 2 policy files "antisamy-slashdot.xml" and ""antisamy-tinymce.xml" both returned This is safe from XSS. Trust us! as the cleansed string result, whereas the other 4 policy files ("antisamy.xml", "antisamy-anythinggoes.xml", "antisamy-ebay.xml", and "antisamy-myspace.xml") all returned <a>This is safe from XSS. Trust us!</a> as the cleansed string. I'm no sure the reason for the difference, but my guess is that "antisamy-slashdot.xml" and ""antisamy-tinymce.xml" don't allow anchor tags at all.

But the important thing here is that in ALL CASES, all of the Antisamy sample policy files returned safe output, i.e., no JavaScript was returned in the cleansed results returned by CleanResults.getCleanHTML().

@davewichers and @spassarop - I don't think there is anything further to do here. I suggest that you close this issue without any code changes to AntiSamy.

davewichers commented 1 year ago

Thanks @kwwall and @spassarop for all your research here. Per your recommendation I'm closing this. I guess this means I can release a new release that upgrades the one vulnerable dependency that exists in the current release.