diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 5ec224a..5484335 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -280,6 +280,56 @@ def test_sanitize(self): "", ) + def test_sanitize_attribute_breakout(self): + # XssCleaner.handle_starttag historically emitted url-bearing + # attributes (href, src, background) using raw string + # interpolation: + # + # bt += ' %s="%s"' % (attribute, attrs[attribute]) + # + # HTMLParser decodes character references inside attribute + # values before delegating to handle_starttag, so " arrives + # as a literal `"`. The unescaped interpolation then let that + # quote close the attribute on the way out, injecting sibling + # attributes such as onclick=, onerror=, ... -- a real XSS. + # Route those attributes through quoteattr like every other + # attribute does, and verify with HTMLParser that the cleaned + # output never carries an event-handler attribute. + from html.parser import HTMLParser + + payloads = [ + 'x', + 'x', + 'x', + "x", + ] + event_handlers = ( + "onclick", + "onerror", + "onmouseover", + "onload", + "onfocus", + "onmouseout", + ) + for raw in payloads: + cleaned = XML(raw, sanitize=True).xml() + seen_attrs = [] + + class _Spy(HTMLParser): + def handle_starttag(self, tag, attrs): + seen_attrs.extend(name.lower() for name, _ in attrs) + + handle_startendtag = handle_starttag + + _Spy().feed(cleaned) + for handler in event_handlers: + self.assertNotIn( + handler, + seen_attrs, + "sanitize() leaked %s via attribute-breakout: %r" + % (handler, cleaned), + ) + def test_find(self): a = DIV("A", _class="a") b = SPAN("B", _id="b") diff --git a/yatl/sanitizer.py b/yatl/sanitizer.py index bb1311f..c4ada2f 100644 --- a/yatl/sanitizer.py +++ b/yatl/sanitizer.py @@ -120,7 +120,18 @@ def handle_starttag(self, tag, attrs): for attribute in self.allowed_attributes_here: if attribute in ["href", "src", "background"]: if self.url_is_acceptable(attrs[attribute]): - bt += ' %s="%s"' % (attribute, attrs[attribute]) + # Route URL-bearing attributes through + # quoteattr like every other attribute below. + # HTMLParser decodes character references + # (e.g. " -> ") inside attribute values + # before handing them to us, so naive + # interpolation lets attacker-supplied quotes + # close the attribute and inject sibling + # attributes such as onclick=, onerror=, etc. + bt += " %s=%s" % ( + attribute, + quoteattr(attrs[attribute]), + ) else: bt += " %s=%s" % ( xmlescape(attribute),