diff --git a/change_log.md b/change_log.md index 5c7f079a..561ef6f5 100644 --- a/change_log.md +++ b/change_log.md @@ -2,6 +2,25 @@ Most recent at top. * Next release + * The filter on text kept inside `style`, `script`, `iframe` and other + literal-content elements now examines a possible tag prefix before + looking for its closing `>`. A long run of `<` characters before one + `>` made it repeatedly scan and copy the same suffix, taking quadratic + time. Removing a tag now also removes every immediately preceding `<`, + rather than letting `<<img` turn into `` through as text, and that `>` could belong to an end tag diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java index 49c6f8ec..cb3add7c 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java @@ -50,8 +50,10 @@ class ElementAndAttributePolicyBasedSanitizerPolicy implements HtmlSanitizer.Policy, TagBalancingHtmlStreamEventReceiver.TextSuppressionPolicy, + TagBalancingHtmlStreamEventReceiver.OpenTagOutputPolicy, HtmlChangeReporter.AttributelessSkipPolicy, - HtmlChangeReporter.DroppedTextSource { + HtmlChangeReporter.DroppedTextSource, + HtmlChangeReporter.DiscardedAttributeSource { final Map elAndAttrPolicies; final Set allowedTextContainers; /** @@ -114,6 +116,11 @@ class ElementAndAttributePolicyBasedSanitizerPolicy /** Told about filtered literal content; null while nobody is listening. */ private @Nullable HtmlStreamRenderer.DroppedTextListener droppedTextListener; + /** Told exactly which input attributes an attribute policy rejects. */ + private @Nullable HtmlChangeReporter.DiscardedAttributeListener + discardedAttributeListener; + /** The output name, if any, produced by the most recent open-tag call. */ + private transient @Nullable String outputElementNameForLastOpenTag; ElementAndAttributePolicyBasedSanitizerPolicy( HtmlStreamEventReceiver out, @@ -151,6 +158,8 @@ public void openDocument() { inKeptCdataElement = false; keptCdataElementName = null; droppedTextListener = null; + discardedAttributeListener = null; + outputElementNameForLastOpenTag = null; skippedLastTagAsAttributeless = false; openElementStack.clear(); skipTextBeforeOpen.clear(); @@ -173,6 +182,7 @@ public void closeDocument() { skipText = true; inKeptCdataElement = false; keptCdataElementName = null; + outputElementNameForLastOpenTag = null; out.closeDocument(); } @@ -181,6 +191,15 @@ public void reportDroppedTextTo( this.droppedTextListener = listener; } + public void reportDiscardedAttributesTo( + @Nullable HtmlChangeReporter.DiscardedAttributeListener listener) { + this.discardedAttributeListener = listener; + } + + public @Nullable String outputElementNameForLastOpenTag() { + return outputElementNameForLastOpenTag; + } + public void text(String textChunk) { if (!skipText) { // The renderer emits the text of a kept literal-content element as it @@ -247,18 +266,34 @@ private String stripTags(String text, String elementName) { while (i < len) { int tagStart = text.indexOf('<', i); if (tagStart < 0) { break; } - int tagEnd = text.indexOf('>', tagStart + 1); - if (tagEnd < 0) { break; } // No '<' from here on starts a tag. - String trimmed = text.substring(tagStart + 1, tagEnd).trim(); - boolean isEndTag = trimmed.startsWith("/"); - String tagName = tagNameOf(trimmed, isEndTag); - if (tagName == null) { + int nameStart = skipTrimSpace(text, tagStart + 1); + if (nameStart == len) { break; } + boolean isEndTag = text.charAt(nameStart) == '/'; + if (isEndTag) { + nameStart = skipTrimSpace(text, nameStart + 1); + if (nameStart == len) { break; } + } + if (!Character.isLetter(text.charAt(nameStart))) { // Not a tag: "", "", "<3" and the like. The '<' is text, - // and the scan resumes right after it: the '>' found above may end - // a tag that starts inside the span, as in "< ". + // and the scan resumes right after it. In particular, do not search + // for a '>' until the prefix is known to open a tag: repeatedly + // searching the same suffix makes a long run of '<' quadratic. i = tagStart + 1; continue; } + int tagEnd = text.indexOf('>', nameStart + 1); + if (tagEnd < 0) { break; } // No '<' from here on starts a tag. + int bodyEnd = tagEnd; + while (bodyEnd > nameStart && text.charAt(bodyEnd - 1) <= ' ') { + --bodyEnd; + } + int nameEnd = nameStart + 1; + while (nameEnd < bodyEnd + && !isRegexWhitespace(text.charAt(nameEnd))) { + ++nameEnd; + } + String tagName = HtmlLexer.canonicalElementName( + text.substring(nameStart, nameEnd)); int kind = isEndTag ? END_TAG : START_TAG; int[] tag = { tagStart, tagEnd + 1, -1, kind }; if (kind == START_TAG) { @@ -286,11 +321,13 @@ private String stripTags(String text, String elementName) { int dropStart = tag[TAG_START]; pos = tag[KIND] == END_TAG || tag[MATCH_END] < 0 ? tag[TAG_END] : tag[MATCH_END]; - // A '<' that the dropped tag followed would meet what follows the tag. + // Any '<'s that the dropped tag followed would meet what follows the + // tag. Take the whole adjacent run, lest "<<img" become "= 0 && result.charAt(last) == '<') { + while (last >= 0 && result.charAt(last) == '<') { result.setLength(last); dropStart -= 1; + --last; } reportDroppedText(elementName, text, dropStart, pos); } @@ -325,18 +362,26 @@ private void reportDroppedText( /** The kinds of record. */ private static final int START_TAG = 0, END_TAG = 1; - /** - * The canonical name of the tag whose trimmed content between the angle - * brackets is {@code trimmed}, or null if it is not a tag. A tag name - * starts with a letter; whitespace between {@code <} or {@code attrs) { + outputElementNameForLastOpenTag = null; ElementAndAttributePolicies policies = elAndAttrPolicies.get(elementName); String adjustedElementName = applyPolicies(elementName, attrs, policies); skippedLastTagAsAttributeless = false; @@ -377,7 +423,7 @@ public boolean skippedLastTagAsAttributeless() { return skippedLastTagAsAttributeless; } - static final @Nullable String applyPolicies( + private @Nullable String applyPolicies( String elementName, List attrs, ElementAndAttributePolicies policies) { String adjustedElementName; @@ -389,12 +435,14 @@ public boolean skippedLastTagAsAttributeless() { = policies.attrPolicies.get(name); if (attrPolicy == null) { attrsIt.remove(); - attrsIt.next(); + String value = attrsIt.next(); + reportDiscardedAttribute(name, value); attrsIt.remove(); } else { String value = attrsIt.next(); String adjustedValue = attrPolicy.apply(elementName, name, value); if (adjustedValue == null) { + reportDiscardedAttribute(name, value); attrsIt.remove(); attrsIt.previous(); attrsIt.remove(); @@ -419,6 +467,13 @@ public boolean skippedLastTagAsAttributeless() { return adjustedElementName; } + /** Reports an attribute-policy rejection without invoking user code. */ + private void reportDiscardedAttribute(String name, String value) { + if (discardedAttributeListener != null) { + discardedAttributeListener.discardedAttribute(name, value); + } + } + public void closeTag(String elementName) { int n = openElementStack.size(); for (int i = n; i > 0;) { @@ -445,6 +500,7 @@ public void closeTag(String elementName) { void writeOpenTag( ElementAndAttributePolicies policies, String adjustedElementName, List attrs) { + outputElementNameForLastOpenTag = adjustedElementName; if (!HtmlTextEscapingMode.isVoidElement(adjustedElementName)) { push(policies.elementName, adjustedElementName); // A kept element is the container for the text inside it. It is judged diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java index d63ffb3f..c5e6b2d8 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java @@ -28,11 +28,13 @@ package org.owasp.html; import java.util.ArrayList; +import java.util.BitSet; import java.util.List; import javax.annotation.Nullable; import org.owasp.html.TagBalancingHtmlStreamEventReceiver.TextSuppressionPolicy; +import org.owasp.html.TagBalancingHtmlStreamEventReceiver.OpenTagOutputPolicy; /** * Sits between the HTML parser, the policy, and the renderer so that it @@ -110,10 +112,23 @@ void reportDroppedTextTo( @Nullable HtmlStreamRenderer.DroppedTextListener listener); } + /** Receives exact input attributes rejected by an attribute policy. */ + interface DiscardedAttributeListener { + void discardedAttribute(String name, String value); + } + + /** Implemented by a policy that can identify attribute-policy rejections. */ + interface DiscardedAttributeSource { + /** Sends rejections to {@code listener}, or to nobody when null. */ + void reportDiscardedAttributesTo( + @Nullable DiscardedAttributeListener listener); + } + private static final class InputChannel implements HtmlSanitizer.Policy, TagBalancingHtmlStreamEventReceiver.NestingLimitListener, TextSuppressionPolicy, + OpenTagOutputPolicy, HtmlStreamRenderer.DroppedTextListener { HtmlStreamEventReceiver policy; final OutputChannel output; @@ -121,6 +136,8 @@ private static final class InputChannel final HtmlChangeListener listener; /** Alternating element names and text, gathered before user callbacks. */ final List pendingDroppedText = new ArrayList<>(); + /** Output name produced in response to the most recent input start tag. */ + private @Nullable String outputElementNameForLastOpenTag; InputChannel( OutputChannel output, HtmlChangeListener listener, @@ -160,12 +177,21 @@ public void droppedText(String elementName, String text) { pendingDroppedText.add(text); } + public @Nullable String outputElementNameForLastOpenTag() { + return outputElementNameForLastOpenTag; + } + public void openDocument() { pendingDroppedText.clear(); + outputElementNameForLastOpenTag = null; policy.openDocument(); if (policy instanceof DroppedTextSource) { ((DroppedTextSource) policy).reportDroppedTextTo(this); } + if (policy instanceof DiscardedAttributeSource) { + ((DiscardedAttributeSource) policy) + .reportDiscardedAttributesTo(output); + } // The renderer decides on its own to drop literal content it cannot // emit, so it has to tell us; any other receiver keeps that to itself. // Bound once the renderer has opened the document, which forgets any @@ -181,16 +207,19 @@ public void closeDocument() { if (policy instanceof DroppedTextSource) { ((DroppedTextSource) policy).reportDroppedTextTo(null); } + if (policy instanceof DiscardedAttributeSource) { + ((DiscardedAttributeSource) policy) + .reportDiscardedAttributesTo(null); + } output.listenForDroppedText(null); dispatchDroppedText(); } public void openTag(String elementName, List attrs) { output.openedElementName = null; - output.expectedAttrs.clear(); // Copied before the policy runs: it removes rejected attributes from // attrs in place, and their values are wanted for the report. - output.expectedAttrs.addAll(attrs); + output.expectAttributes(attrs); policy.openTag(elementName, attrs); { // Gather the notification details to avoid any problems with the @@ -201,6 +230,7 @@ public void openTag(String elementName, List attrs) { // name is not compared with the input name: an ElementPolicy may // rename the element, and a renamed element was kept, not dropped. boolean discarded = output.openedElementName == null; + outputElementNameForLastOpenTag = output.openedElementName; output.openedElementName = null; // Attributes go unreported with a tag the policy rejected: the tag // report covers them. Not so when the policy allowed the element and @@ -210,13 +240,11 @@ public void openTag(String elementName, List attrs) { || (policy instanceof AttributelessSkipPolicy && ((AttributelessSkipPolicy) policy) .skippedLastTagAsAttributeless()); - int nDiscarded = attrsRejectedOnTheirOwn - ? output.expectedAttrs.size() / 2 - : 0; - String[] discardedAttrs = nDiscarded != 0 - ? output.expectedAttrs.toArray(new String[nDiscarded * 2]) + String[] discardedAttrs = attrsRejectedOnTheirOwn + ? output.discardedAttributes() : ZERO_STRINGS; - output.expectedAttrs.clear(); + int nDiscarded = discardedAttrs.length / 2; + output.clearExpectedAttributes(); // Dispatch notifications to the listener, under the input name, // which is the one the listener can relate to what came in. if (discarded) { @@ -263,7 +291,8 @@ private void dispatchDroppedText() { private static final String[] ZERO_STRINGS = new String[0]; } - private static final class OutputChannel implements HtmlStreamEventReceiver { + private static final class OutputChannel + implements HtmlStreamEventReceiver, DiscardedAttributeListener { private final HtmlStreamEventReceiver renderer; /** * The name of the tag the policy has opened in response to the tag being @@ -271,20 +300,66 @@ private static final class OutputChannel implements HtmlStreamEventReceiver { */ String openedElementName; /** - * The attributes on the tag being opened, as name and value pairs, that - * have not turned up in the output yet. A list rather than a map: a + * The attributes on the tag being opened, as original name and value + * pairs. A list rather than a map: a * name repeated on one tag is two attributes, and HTML forbids that, so * the sanitizer keeps the first and drops the rest. Collapsing the copies * here would leave the surviving one accounting for all of them, and the * drops would go unreported. The values ride along so that the drops can * be reported with them. */ - List expectedAttrs = new ArrayList<>(); + final List expectedAttrs = new ArrayList<>(); + /** Input pairs an attribute policy explicitly rejected. */ + final BitSet rejectedAttrs = new BitSet(); + /** Input pairs accounted for by attributes the policy emitted. */ + final BitSet emittedAttrs = new BitSet(); OutputChannel(HtmlStreamEventReceiver renderer) { this.renderer = renderer; } + /** Starts accounting for the attributes on one input start tag. */ + void expectAttributes(List attrs) { + expectedAttrs.clear(); + expectedAttrs.addAll(attrs); + rejectedAttrs.clear(); + emittedAttrs.clear(); + } + + public void discardedAttribute(String name, String value) { + for (int i = 0, n = expectedAttrs.size() / 2; i < n; ++i) { + int pair = i * 2; + if (!rejectedAttrs.get(i) && !emittedAttrs.get(i) + && name.equals(expectedAttrs.get(pair)) + && value.equals(expectedAttrs.get(pair + 1))) { + rejectedAttrs.set(i); + return; + } + } + } + + /** Returns original pairs not accounted for by emitted attributes. */ + String[] discardedAttributes() { + int n = expectedAttrs.size() / 2; + int nDiscarded = n - emittedAttrs.cardinality(); + if (nDiscarded == 0) { return InputChannel.ZERO_STRINGS; } + String[] discarded = new String[nDiscarded * 2]; + int out = 0; + for (int i = 0; i < n; ++i) { + if (!emittedAttrs.get(i)) { + discarded[out++] = expectedAttrs.get(i * 2); + discarded[out++] = expectedAttrs.get(i * 2 + 1); + } + } + return discarded; + } + + void clearExpectedAttributes() { + expectedAttrs.clear(); + rejectedAttrs.clear(); + emittedAttrs.clear(); + } + /** * Has the renderer report dropped literal content to {@code listener}, * or to nobody when null, if it is one that can. The library's own @@ -315,16 +390,17 @@ public void openTag(String elementName, List attrs) { for (int i = 0, n = attrs.size(); i < n; i += 2) { // Accounts for one copy of the name, so repeats the policy dropped // stay behind to be reported. - removeFirstNamed(expectedAttrs, attrs.get(i)); + markFirstEmitted(attrs.get(i)); } renderer.openTag(elementName, attrs); } - /** Removes the first pair in pairs whose name is name, if there is one. */ - private static void removeFirstNamed(List pairs, String name) { - for (int i = 0, n = pairs.size(); i < n; i += 2) { - if (name.equals(pairs.get(i))) { - pairs.subList(i, i + 2).clear(); + /** Accounts for the first eligible input copy of an emitted name. */ + private void markFirstEmitted(String name) { + for (int i = 0, n = expectedAttrs.size() / 2; i < n; ++i) { + if (!rejectedAttrs.get(i) && !emittedAttrs.get(i) + && name.equals(expectedAttrs.get(i * 2))) { + emittedAttrs.set(i); return; } } diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java index feeb7ad2..4b948eae 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java @@ -32,6 +32,8 @@ import java.util.BitSet; import java.util.List; +import javax.annotation.Nullable; + import org.owasp.html.HtmlElementTables.HtmlElementNames; /** @@ -47,12 +49,19 @@ public class TagBalancingHtmlStreamEventReceiver private final HtmlStreamEventReceiver underlying; private int nestingLimit = Integer.MAX_VALUE; private final IntVector openElements = new IntVector(); + /** + * The element each entry in {@link #openElements} became after policy + * application, or {@link #NO_OUTPUT_ELEMENT} when the policy dropped it. + * When the receiver below cannot report that, the input name is used. + */ + private final IntVector outputElements = new IntVector(); private final IntVector toResumeInReverse = new IntVector(); private static final HtmlElementTables METADATA = HtmlElementTables.get(); private static final int UNRECOGNIZED_TAG = METADATA.indexForName(HtmlElementNames.CUSTOM_ELEMENT_NAME); private static final int A_TAG = METADATA.indexForName("a"); private static final int BODY_TAG = METADATA.indexForName("body"); + private static final int NO_OUTPUT_ELEMENT = -1; /** * Elements on entering which a browser puts a marker on its list of * active formatting elements, so that an {@code a} opened inside one of @@ -105,6 +114,20 @@ interface TextSuppressionPolicy { boolean suppressesTextWhenDropped(String canonElementName); } + /** + * Implemented by a policy that can identify the element, if any, emitted + * by its most recent {@link HtmlStreamEventReceiver#openTag} call. The + * balancer uses the output name when applying the formatting-marker rule + * for nested links: a marker the policy dropped cannot affect how a browser + * parses the sanitized output. + */ + interface OpenTagOutputPolicy { + /** + * @return the canonical emitted name, or null if no element was emitted. + */ + @Nullable String outputElementNameForLastOpenTag(); + } + /** * How many elements whose content the policy would suppress -- {@code *