Skip to content

Commit

Permalink
Recognize that <style> is not really workable inside <select>
Browse files Browse the repository at this point in the history
Rather than mucking with `<style>` tag content in all cases, this is a more
tailored fix to the recent vulnerability that just closes `<style>` elements
when we realize they're in a dodgy parsing context.
  • Loading branch information
mikesamuel committed Oct 18, 2021
1 parent e2b29e8 commit 14f84fd
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 39 deletions.
21 changes: 0 additions & 21 deletions src/main/java/org/owasp/html/HtmlStreamRenderer.java
Original file line number Diff line number Diff line change
Expand Up @@ -254,25 +254,8 @@ private final void writeCloseTag(String uncanonElementName)
Encoding.stripBannedCodeunits(cdataContent);
int problemIndex = checkHtmlCdataCloseable(lastTagOpened, cdataContent);
if (problemIndex == -1) {
String prefix = "";
String suffix = "";
Set<String> bannedSubstrings = Collections.emptySet();
if ("style".equals(elementName)) {
prefix = "/*<![CDATA[<!--*/\n";
suffix = "\n/*-->]]>*/";
bannedSubstrings = BANNED_IN_STYLE_ELEMENTS;
}

for (String bannedSubstring : bannedSubstrings) {
if (cdataContent.indexOf(bannedSubstring) >= 0) {
cdataContent.setLength(0);
}
}

if (cdataContent.length() != 0) {
output.append(prefix);
output.append(cdataContent);
output.append(suffix);
}
} else {
error(
Expand Down Expand Up @@ -457,8 +440,4 @@ public void close() throws IOException {
private static boolean isTagEnd(char ch) {
return ch < 63 && 0 != (TAG_ENDS & (1L << ch));
}

private static Set<String> BANNED_IN_STYLE_ELEMENTS = ImmutableSet.of(
"<![CDATA[", "]]>", "<!--", "-->"
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,15 @@ && canContain(elIndex, toResume, nOpen)) {
private boolean canContain(
int child, int container, int containerIndexOnStack) {
Preconditions.checkArgument(containerIndexOnStack >= 0);
if (child == HtmlElementTables.TEXT_NODE && hasSpecialTextMode(container)) {
// If there's a select element on the stack, then we need to be extra careful.
int selectElementIndex = METADATA.indexForName("select");
for (int i = containerIndexOnStack; --i >= 0;) {
if (selectElementIndex == openElements.get(i)) {
return false;
}
}
}
int anc = container;
int ancIndexOnStack = containerIndexOnStack;
while (true) {
Expand Down Expand Up @@ -363,6 +372,17 @@ private static boolean isHeaderElementName(String canonElementName) {
&& canonElementName.charAt(1) <= '9';
}

private static boolean hasSpecialTextMode(int elementIndex) {
String name = METADATA.canonNameForIndex(elementIndex);
switch (HtmlTextEscapingMode.getModeForTag(name)) {
case PCDATA: case VOID:
return false;
case CDATA: case CDATA_SOMETIMES: case RCDATA: case PLAIN_TEXT:
return true;
}
throw new IllegalArgumentException(name);
}

private static final byte ALL_SCOPES;
private static final byte[] SCOPES_BY_ELEMENT;
private static final byte[] SCOPE_FOR_END_TAG;
Expand Down
39 changes: 24 additions & 15 deletions src/test/java/org/owasp/html/SanitizersTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -439,48 +439,57 @@ public static final void testStyleTagsInAllTheWrongPlaces() {
String input = ""
+ "<select><option><style><script>alert(1)</script></style></option></select>"
+ "<svg><style>.r { color: red }</style></svg>"
+ "<style>.b { color: blue }</style>"
+ "<style>#a { content: \"<!--\" }</style>"
+ "<style>#a { content: \"<![CDATA[\" }</style>"
+ "<style>#a { content: \"-->\" }</style>"
+ "<style>#a { content: \"]]>\" }</style>";
+ "<style>.b { color: blue }</style>";
PolicyFactory pf = new HtmlPolicyBuilder()
.allowElements("option", "select", "style", "svg")
.allowTextIn("style")
.toFactory();
assertEquals(
""
+ "<select><option>"
+ "<style>/*<![CDATA[<!--*/\n<script>alert(1)</script>\n/*-->]]>*/</style>"
+ "<style></style>&lt;script&gt;alert(1)&lt;/script&gt;"
+ "</option></select>"
+ "<svg>"
+ "<style>/*<![CDATA[<!--*/\n.r { color: red }\n/*-->]]>*/</style>"
+ "<style>.r { color: red }</style>"
+ "</svg>"
+ "<style>/*<![CDATA[<!--*/\n.b { color: blue }\n/*-->]]>*/</style>"
+ "<style></style>"
+ "<style></style>"
+ "<style></style>"
+ "<style></style>",
+ "<style>.b { color: blue }</style>",
pf.sanitize(input)
);
}

@Test
public static final void testSelectIsOdd() {
// Special text modes interact badly with select and option
String input = "<select><option><xmp><script>alert(1)</script></xmp></option></select>";
PolicyFactory pf = new HtmlPolicyBuilder()
.allowElements("option", "select", "xmp")
.allowTextIn("xmp")
.allowTextIn("xmp", "option")
.toFactory();
assertEquals(
""
+ "<select><option>"
+ "<pre>&lt;script&gt;alert(1)&lt;/script&gt;</pre>"
+ "<select><option><pre></pre>"
+ "&lt;script&gt;alert(1)&lt;/script&gt;"
+ "</option></select>",
pf.sanitize(input)
);
}

@Test
public static final void testOptionAllowsText() {
String input = "<select><option><pre>code goes here</pre></option></select>";
PolicyFactory pf = new HtmlPolicyBuilder()
.allowElements("option", "select", "pre")
.allowTextIn("pre", "option")
.toFactory();
assertEquals(
""
+ "<select><option>"
+ "<pre>code goes here</pre>"
+ "</option></select>",
pf.sanitize(input)
);
}

@Test
public static final void testStyleGlobally() {
PolicyFactory policyBuilder = new HtmlPolicyBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ public final void testTextContent() {
+ "<p>Hello, <textarea>World!</textarea></p>"
+ "<h1>Hello"
// Text allowed in special style tag.
+ "<style type=\"text/css\">/*<![CDATA[<!--*/\n"
+ "\n.World {\n color: blue\n}\n"
+ "\n/*-->]]>*/</style></h1>"
+ "<style type=\"text/css\">\n"
+ ".World {\n color: blue\n}\n"
+ "</style></h1>"
// Whitespace allowed inside <ul> but non-whitespace text nodes are
// moved inside <li>.
+ "<ul><li>Hello,</li><li>World!</li></ul>",
Expand Down

0 comments on commit 14f84fd

Please sign in to comment.