Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsafe input #110

Closed
Leonya opened this issue Apr 11, 2017 · 12 comments
Closed

Unsafe input #110

Leonya opened this issue Apr 11, 2017 · 12 comments

Comments

@Leonya
Copy link

Leonya commented Apr 11, 2017

With sanitizer configured as follows Sanitizers.FORMATTING.and(Sanitizers.IMAGES).and(Sanitizers.LINKS) the following input is not sanitized correctly and it pops up an alert:

<a href='/"&nbsp;xxx=&#39;'>xxx' onmouseover="alert('oops')" style="position:fixed;left:0;right:0;top:0;bottom:0;z-index:100;background:white"></a>

I have tested versions 20160924 and 20170408, both are affected.

@ChALkeR
Copy link

ChALkeR commented Apr 11, 2017

@Leonya Are you sure that this is the correct way to report this?

@Leonya
Copy link
Author

Leonya commented Apr 11, 2017

@ChALkeR https://github.com/OWASP/java-html-sanitizer/blob/master/docs/attack_review_ground_rules.md — apparently it is. I have contacted @mikesamuel by email since labelling of issues is not available to non-members.

@ChALkeR
Copy link

ChALkeR commented Apr 11, 2017

Sigh.

I am just not happy that this in fact makes the vulnerability that I privately reported public prior to the fix.

I am considering this disclosed, btw.

@ChALkeR
Copy link

ChALkeR commented Apr 11, 2017

@Leonya, the suggested reporting procedure is clearly broken here (as you have already noticed in regard to label being not present), but imo that doesn't mean that this should be immediately dislosed publically.

The procedure got broken in 7ea6673#diff-7924fb07e3115de3a49d641ecc235f90L30 — apparently, this project was hosted elsewhere, which respected the «Private» label previously.

@jmanico
Copy link
Member

jmanico commented Apr 11, 2017 via email

@ChALkeR
Copy link

ChALkeR commented Apr 11, 2017

@Leonya, I wasn't able to reproduce this with pure java-html-sanitizer. Could you provide a full listing of the java testcase file?

@Leonya
Copy link
Author

Leonya commented Apr 11, 2017

I'm really sorry for the false alarm. This is not a bug in java-html-sanitizer after all, it's us doing stupid things with the sanitized output.

@Leonya Leonya closed this as completed Apr 11, 2017
@ChALkeR
Copy link

ChALkeR commented Apr 11, 2017

@jmanico, well, there is one actual issue in java-html-sanitizer mentioned here — you should probably fix your Reporting Vulnerabilites procedure, as that doesn't work now =).

@ChALkeR
Copy link

ChALkeR commented Apr 11, 2017

Note: when providing the actual details above, I was acting under the assumption that this is in fact an issue in java-html-sanitizer, like @Leonya said (as I did not have experience with java-html-sanitizer before, and they had).

If I have had known that this is not an issue in java-html-sanitizer, I probably wouldn't have mentioned those details here.

It is too late to remove that now, though.

@mikesamuel
Copy link
Contributor

I think @Leonya already sorted out the curiosity which prompted this.

I double checked.

Running

    String input = "<a href='/\"&nbsp;xxx=&#39;'>xxx' onmouseover=\"alert('oops')\" style=\"position:fixed;left:0;right:0;top:0;bottom:0;z-index:100;background:white\"></a>";
    assertEquals(
        "TODO",
        Sanitizers.FORMATTING.and(Sanitizers.IMAGES).and(Sanitizers.LINKS)
        .sanitize(input));

I get the output

<a href="/&#34; xxx&#61;&#39;" rel="nofollow">xxx&#39; onmouseover&#61;&#34;alert(&#39;oops&#39;)&#34; style&#61;&#34;position:fixed;left:0;right:0;top:0;bottom:0;z-index:100;background:white&#34;&gt;</a>

which seems to have the payload in the onmouseover shifted to a text node where it should be innocuous.

I loaded that same fragment in a browser via file:///tmp/foo.html, and saw the text xxx' onmouseover="alert('oops')" style="position:fixed;left:0;right:0;top:0;bottom:0;z-index:100;background:white"> linked to file:///%22%C3%82%C2%A0xxx=' so the browser's interpretation of the output seems to be consistent with the sanitizers.

@mikesamuel
Copy link
Contributor

@ChALkeR I think you're right about why the reporting procedure is broken.

The late Google code had a way to mark a bug as private, but Github unfortunately does not.

@Leonya thanks for trying to follow procedure.

@jmanico Should we point people at bugcrowd instead?

mikesamuel added a commit that referenced this issue Apr 12, 2017
Github does not support private issues which makes responsible disclosure tough.  Discussed in issue #110.
@mikesamuel
Copy link
Contributor

I updated instructions with something that's not too terrible while we hash out details.

49f0d1d

now says

Reporting Vulnerabilities

Please report successful attacks with example input via OWASP's bugcrowd queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants