xss filter_attributes doesn't work as intended #74

Closed
lemonad opened this Issue Apr 16, 2012 · 6 comments

Comments

Projects
None yet
2 participants

lemonad commented Apr 16, 2012

Looks like there's three problems:

  1. Global leak with out, should be var out = '';

  2. Regexp replace uses $out instead of out.

  3. Regexp is not valid, back references can't be used in character classes.

    Perhaps it's intended to be /\s*[a-z\-]+\s*=\s*(\042|\047)(?:(?!\1).*?)\1/gi.
    However, that results in the following behavior:

 > var s = require('./lib');
 > s.sanitize("<a href=\"javascript:alert('xss')\">some text</a>").xss();
 '<a \'xss\')">some text</a>'

lemonad commented Apr 17, 2012

Oh, I can totally submit a patch if I just get pointed in the right direction in terms of what filter_attributes() was meant to do.

Owner

chriso commented Apr 18, 2012

It's a port of CodeIgniter's xss_clean() helper. My preg to RegExp skills weren't great when I ported it ;)

It looks like the function removes Javascript comments embedded in attribute values before removing common attack vectors like javascript: on this line.

@chriso chriso added a commit that referenced this issue Apr 18, 2012

@chriso chriso Fix obvious bugs in xss, re #74 7abbaa3

chriso closed this in 68ab9d2 Apr 18, 2012

lemonad commented Apr 18, 2012

Thanks for the quick response! It still doesn't look quite right to me: the use of comments seems to have be left over from a previous commit?

@chriso chriso added a commit that referenced this issue Apr 18, 2012

@chriso chriso Fix filter_attributes again, re #74 7d95076
Owner

chriso commented Apr 18, 2012

Yeah rushed it a bit. I also split out the first regexp in to one for each quote so that the lazy .*? doesn't choke on something like href="alert('foo');". I'll write test cases when I get some time

lemonad commented Jun 7, 2012

Looks like this issue is back again, identical to when I reported it. Should I open a new issue or can we reopen this?

Owner

chriso commented Jun 19, 2012

Ok so I've changed the semantics a bit: if an offending attribute is found the whole thing is removed rather than just the evil pattern that was matched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment