Error sanitizing with --> in <script> tag

[jvbrandis]

There seems to be a bug in the sanitizer when sanitizing script tags which contains a HTML comment end tag.

We generate a JSON blob which is inserted into a <script> tag of type application/json, and our HTML policy allows script tags of this type, with text content. However, in certain cases, the JSON contains a value string which looks like a HTML comment end, e.g. "cause --> effect".
(see test case scriptTagWithCommentBlockContainingHtmlCommentEnd)

When this happens, the sanitizer completely drops the entire text contents of the script tag.
This happens regardless of whether the text contents are surrounded by a HTML comment block
(see test case scriptTagWithoutCommentBlockContainingHtmlCommentEnd).
There is also no event fired at the HtmlChangeListener when this happens.

Escaping the JSON string by escaping the right angular solves the problem, however this is no longer valid JSON. (see scriptTagWithCommentBlockContainingEscapedHtmlCommentEnd).

The following code snippet highlights the problem:

package test;

import org.junit.Test;
import org.owasp.html.HtmlPolicyBuilder;
import org.owasp.html.PolicyFactory;

import java.io.IOException;
import java.util.HashSet;

import static java.util.Collections.singletonList;
import static org.junit.Assert.assertEquals;

public class OwaspSanitationTest {


  @Test
  public void scriptTagWithCommentBlockContainingHtmlCommentEnd() throws IOException {
    String html = "<script type=\"application/json\">\n" +
            "<!--\n" +
            "{\"field\":\"-->\"}\n" +
            "// -->\n" +
            "</script>";
    assertEquals(html, createHtmlSanitizer().sanitize(html));
  }
  @Test
  public void scriptTagWithCommentBlockContainingEscapedHtmlCommentEnd() throws IOException {
    String html = "<script type=\"application/json\">\n" +
            "<!--\n" +
            "{\"field\":\"--\\>\"}\n" +
            "// -->\n" +
            "</script>";
    assertEquals(html, createHtmlSanitizer().sanitize(html));
  }

  @Test
  public void scriptTagWithoutCommentBlockContainingHtmlCommentEnd() throws IOException {
    String html = "<script type=\"application/json\">\n" +
            "{\"field\":\"-->\"}\n" +
            "</script>";
    assertEquals(html, createHtmlSanitizer().sanitize(html));
  }

  private PolicyFactory createHtmlSanitizer() {
    return new HtmlPolicyBuilder()
            //allow scripts of type application/json
            .allowElements((elementName, attrs) -> {
              int typeIndex = attrs.indexOf("type");
              if (typeIndex < 0 || attrs.size() < typeIndex + 1 || !attrs.get(typeIndex + 1).equalsIgnoreCase("application/json"))
                return null;
              return elementName;
            }, "script")
            //allow contents in this script tag
            .allowTextIn("script")
            //keep type attribute in application/json script tag
            .allowAttributes("type").matching(true, new HashSet<>(singletonList("application/json"))).onElements("script")
            .toFactory();
  }
}

[mikesamuel]

@jvbrandis

Escaping the JSON string by escaping the right angular solves the problem, however this is no longer valid JSON.

You can escape '<' in JSON using \u003c.
I would recommend escaping HTML metacharacters so that the JSON is embeddable per https://github.com/owasp/json-sanitizer#output

The sanitizer assumes that the fragment is not in a foreign element context. This doesn't usually matter but if you're using <script> elements then it does matter because an attacker who can inject a <svg> tag earlier can change the way subsequent <script> elements are parsed.
See the difference between

     <script>alert('outside svg: &lt;a&gt;');</script>
<svg><script>alert(' inside svg: &lt;a&gt;');</script></svg>

which means that <script>["<!--" can cause the HTML parser to possibly ignore a subsequent </script> leaving the HTML script element open.

When this happens, the sanitizer completely drops the entire text contents of the script tag.
This happens regardless of whether the text contents are surrounded by a HTML comment block
(see test case scriptTagWithoutCommentBlockContainingHtmlCommentEnd).
There is also no event fired at the HtmlChangeListener when this happens.

This happens because

java-html-sanitizer/src/main/java/org/owasp/html/HtmlLexer.java

Lines 352 to 375 in f07e44b

	// From HTML 5 section 8.1.2.6
	// The text in CDATA and RCDATA elements must not contain any
	// occurrences of the string "</" followed by characters that
	// case-insensitively match the tag name of the element followed
	// by one of U+0009 CHARACTER TABULATION, U+000A LINE FEED (LF),
	// U+000B LINE TABULATION, U+000C FORM FEED (FF), U+0020 SPACE,
	// U+003E GREATER-THAN SIGN (>), or U+002F SOLIDUS (/), unless
	// that string is part of an escaping text span.
	// An escaping text span is a span of text (in CDATA and RCDATA
	// elements) and character entity references (in RCDATA elements)
	// that starts with an escaping text span start that is not itself
	// in an escaping text span, and ends at the next escaping text
	// span end.
	// An escaping text span start is a part of text that consists of
	// the four character sequence "<!--".
	// An escaping text span end is a part of text that consists of
	// the three character sequence "-->".
	// An escaping text span start may share its U+002D HYPHEN-MINUS characters
	// with its corresponding escaping text span end.

which was removed from later versions of the HTML5 spec because it was overly complicated and there was a better fit for what browsers actually did.

That requirement was enforced on rendering at

java-html-sanitizer/src/main/java/org/owasp/html/HtmlStreamRenderer.java

Lines 289 to 304 in f07e44b

	private static int checkHtmlCdataCloseable(
	String localName, StringBuilder sb) {
	int escapingTextSpanStart = -1;
	for (int i = 0, n = sb.length(); i < n; ++i) {
	char ch = sb.charAt(i);
	switch (ch) {
	case '<':
	if (i + 3 < n
	&& '!' == sb.charAt(i + 1)
	&& '-' == sb.charAt(i + 2)
	&& '-' == sb.charAt(i + 3)) {
	if (escapingTextSpanStart == -1) {
	escapingTextSpanStart = i;
	} else {
	return i;
	}

None of that is necessary so I'll put a fix out shortly.

[mikesamuel]

I did a bit of digging. The relevant specs are

• https://www.w3.org/TR/html51/semantics-scripting.html#restrictions-for-contents-of-script-elements and
• https://www.w3.org/TR/html5/scripting-1.html#restrictions-for-contents-of-script-elements which aims to replace the former

Quoting the latter (though they seem to agree on this issue):

4.12.1.3. Restrictions for contents of script elements

The textContent of a script element must match the script production in the following ABNF, the character set for which is Unicode. [ABNF]

script = outer *( comment-open inner comment-close outer )

outer = < any string that doesn’t contain a substring that matches not-in-outer >
not-in-outer = comment-open
inner = < any string that doesn’t contain a substring that matches not-in-inner >
not-in-inner = comment-close / script-open

comment-open = "<!--"
comment-close = "-->"
script-open = "<" s c r i p t tag-end

which establishes that <script> bodies, regardless of type should have balanced, non-nesting <!--...-->s.

I can fix the parser so that it doesn't rely on the deprecated escaping text span language since no security properties depend on the correctness of the parser, but I'm loathe to change the renderer to produce output that violates that must (my emphasis).

[jvbrandis]

Thanks a lot Mike. I was looking at the same spec, so I guessed this was where this was coming from, but great thanks for fixing it, and will look into using the JSON sanitizer also.

[mikesamuel]

@jvbrandis

Unfortunately, I can probably allow

<script type="application/json">{"field":"-->"}</script>

but I can't allow

<script type="application/json">{"field":"<!--"}</script>

per those restrictions so I don't think any fix is going to get at the heart of your issue which, IIUC, is that arbitrary JSON cannot nest in a <script> element.

You can simply replace s/</\u003c/g and s/>/\u003e/g in your JSON though which does embed nicely.

[jvbrandis]

I agree, reading the spec we concluded that a full fix on our part must include escaping.

[jvbrandis]

Mike; one question: Should the sanitizer maybe notify the HtmlChangeListener for such a change?
It is neither a tagDiscarded or an tagAttributeDiscarded, but it does discard content, so I would expect it to notify a listener?

[mikesamuel]

Mike; one question: Should the sanitizer maybe notify the HtmlChangeListener for such a change?
It is neither a tagDiscarded or an tagAttributeDiscarded, but it does discard content, so I would expect it to notify a listener?

Good point. Filed as issue #155.