Skip to content

Commit

Permalink
Render style tag content more strictly.
Browse files Browse the repository at this point in the history
This addresses a vulnerability where policies that allow `<style>`
elements with text in `<option>` elements are vulnerable to XSS as
disclosed in

https://docs.google.com/document/d/11SoX296sMS0XoQiQbpxc5pNxSdbJKDJkm5BDv0zrX50/edit?usp=sharing

This changes behavior for rendering of `<style>` element text so may
change behavior.

Specifically, `<style>` element text that includes the strings `-->`
or `]]>` will no longer sanitize.
  • Loading branch information
mikesamuel committed Oct 18, 2021
1 parent ad287c3 commit be33ec6
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 4 deletions.
1 change: 1 addition & 0 deletions docs/credits.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* rfamilya
* robinhouston
* sneha patil
* tomanthony
* vytah
* willikins_bear
* yangbongsoo
Expand Down
5 changes: 5 additions & 0 deletions docs/cve202142575.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# CVE-2021-42575

Policies that allow `<style>` tags inside `<option>` elements are vulnerable to RCE

See https://docs.google.com/document/d/11SoX296sMS0XoQiQbpxc5pNxSdbJKDJkm5BDv0zrX50/edit?usp=sharing for mitigations.
3 changes: 2 additions & 1 deletion docs/vulnerabilities.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Known & public vulnerabilities in this project
# Known & public vulnerabilities in this project

* [CVE-2021-42575](cve202142575.md) - 18 Oct. 2021 - Recommend upgrade to latest version.
* [CVE-2011-4457](cve20114457.md) - 17 Nov. 2011 - Recommend upgrade to r88 or later.
29 changes: 28 additions & 1 deletion src/main/java/org/owasp/html/HtmlStreamRenderer.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,15 @@
package org.owasp.html;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;

import java.io.Closeable;
import java.io.Flushable;
import java.io.IOException;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import javax.annotation.WillCloseWhenClosed;
import javax.annotation.concurrent.NotThreadSafe;

Expand Down Expand Up @@ -250,7 +254,26 @@ private final void writeCloseTag(String uncanonElementName)
Encoding.stripBannedCodeunits(cdataContent);
int problemIndex = checkHtmlCdataCloseable(lastTagOpened, cdataContent);
if (problemIndex == -1) {
output.append(cdataContent);
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(
"Invalid CDATA text content",
Expand Down Expand Up @@ -434,4 +457,8 @@ 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[", "]]>", "<!--", "-->"
);
}
49 changes: 48 additions & 1 deletion src/test/java/org/owasp/html/SanitizersTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,53 @@ public static final void testStyleTagInTable() {
pf.sanitize(input));
}

@Test
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>";
PolicyFactory pf = new HtmlPolicyBuilder()
.allowElements("option", "select", "style", "svg")
.allowTextIn("style")
.toFactory();
assertEquals(
""
+ "<select><option>"
+ "<style>/*<![CDATA[<!--*/\n<script>alert(1)</script>\n/*-->]]>*/</style>"
+ "</option></select>"
+ "<svg>"
+ "<style>/*<![CDATA[<!--*/\n.r { color: red }\n/*-->]]>*/</style>"
+ "</svg>"
+ "<style>/*<![CDATA[<!--*/\n.b { color: blue }\n/*-->]]>*/</style>"
+ "<style></style>"
+ "<style></style>"
+ "<style></style>"
+ "<style></style>",
pf.sanitize(input)
);
}

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

@Test
public static final void testStyleGlobally() {
PolicyFactory policyBuilder = new HtmlPolicyBuilder()
Expand All @@ -449,7 +496,7 @@ static int fac(int n) {
int ifac = 1;
for (int i = 1; i <= n; ++i) {
int ifacp = ifac * i;
if (ifacp < ifac) { throw new IllegalArgumentException("undeflow"); }
if (ifacp < ifac) { throw new IllegalArgumentException("underflow"); }
ifac = ifacp;
}
return ifac;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ public final void testTextContent() {
+ "<p>Hello, <textarea>World!</textarea></p>"
+ "<h1>Hello"
// Text allowed in special style tag.
+ "<style type=\"text/css\">\n.World {\n color: blue\n}\n</style></h1>"
+ "<style type=\"text/css\">/*<![CDATA[<!--*/\n"
+ "\n.World {\n color: blue\n}\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 be33ec6

Please sign in to comment.