Commit e5661098 authored by Tom Sepez's avatar Tom Sepez Committed by Commit Bot

XSSAuditor must match raw page content for animate tags.

Otherwise it is vulnerable to an entity bypass. The fix to bug
384077 solved the otherwise difficult problem of finding a match
endpoint by using decoded data to find an actual semicolon, which
solved one problem but resulted in this issue.

To do these accurately, we'd need an HTMLSourceTracker than can
track the position of each decoded byte within a value back to the
raw content, rather than just the start and end position. Doing this
would be expensive.

So just stop at any semicolon in the attribute. I'd worried that
this would not give enough signal, but we are already checking for
an animate tag containing a javascript: URL, which is likely only
ever used by the bad guys.

Tweak the one existing testcase to cover the variation involving
entities. Two other existing testcases show that we don't re-introduce
384077.

I'd deferred fixing this hoping for animate to be deprecated, but
those plans are on hold.

Bug: 517547
Change-Id: Ib63dbdc691f11b143dc6e2fdd4b4232cfdf9629d
Reviewed-on: https://chromium-review.googlesource.com/599045Reviewed-by: default avatarMike West <mkwst@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491771}
parent 866e9910
CONSOLE ERROR: line 4: The XSS Auditor refused to execute a script in 'http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=%3Csvg%3E%3Ca%3E%3Ccircle%20r=100%20/%3E%3Canimate%20attributeName=href%20values=%3Bjavascript%3Aalert(1)%20begin=0s%20end=0.1s%20fill=freeze%20/%3E%3C/a%3E%3C/svg%3E&notifyDone=1&dumpElementBySelector=animate' because its source code was found within the request. The server sent an 'X-XSS-Protection' header requesting this behavior. CONSOLE ERROR: line 4: The XSS Auditor refused to execute a script in 'http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=%3Csvg%3E%3Ca%3E%3Ccircle%20r=100%20/%3E%3Canimate%20attributeName=href%20values=%3Bj%26%23x61%3Bvascript%3Aalert(1)%20begin=0s%20end=0.1s%20fill=freeze%20/%3E%3C/a%3E%3C/svg%3E&notifyDone=1&dumpElementBySelector=animate' because its source code was found within the request. The server sent an 'X-XSS-Protection' header requesting this behavior.
This test passes if the element displayed in the frame below has a 'values' attribute containing only 'javascript:void(0)'. This test passes if the element displayed in the frame below has a 'values' attribute containing only 'javascript:void(0)'.
......
...@@ -12,6 +12,6 @@ ...@@ -12,6 +12,6 @@
</head> </head>
<body> <body>
<p>This test passes if the element displayed in the frame below has a 'values' attribute containing only 'javascript:void(0)'.</p> <p>This test passes if the element displayed in the frame below has a 'values' attribute containing only 'javascript:void(0)'.</p>
<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<svg><a><circle%20r=100%20/><animate%20attributeName=href%20values=%3Bjavascript%3Aalert(1)%20begin=0s%20end=0.1s%20fill=freeze%20/></a></svg>&notifyDone=1&dumpElementBySelector=animate"></iframe> <iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<svg><a><circle%20r=100%20/><animate%20attributeName=href%20values=%3Bj%26%23x61%3Bvascript%3Aalert(1)%20begin=0s%20end=0.1s%20fill=freeze%20/></a></svg>&notifyDone=1&dumpElementBySelector=animate"></iframe>
</body> </body>
</html> </html>
...@@ -307,21 +307,30 @@ static void TruncateForScriptLikeAttribute(String& decoded_snippet) { ...@@ -307,21 +307,30 @@ static void TruncateForScriptLikeAttribute(String& decoded_snippet) {
} }
} }
static void TruncateForSemicolonSeparatedScriptLikeAttribute(
String& decoded_snippet) {
// Same as script-like attributes, but semicolons can introduce page data.
TruncateForScriptLikeAttribute(decoded_snippet);
size_t position = decoded_snippet.Find(";");
if (position != kNotFound)
decoded_snippet.Truncate(position);
}
static bool IsSemicolonSeparatedAttribute( static bool IsSemicolonSeparatedAttribute(
const HTMLToken::Attribute& attribute) { const HTMLToken::Attribute& attribute) {
return ThreadSafeMatch(attribute.NameAsVector(), SVGNames::valuesAttr); return ThreadSafeMatch(attribute.NameAsVector(), SVGNames::valuesAttr);
} }
static String SemicolonSeparatedValueContainingJavaScriptURL( static bool IsSemicolonSeparatedValueContainingJavaScriptURL(
const String& value) { const String& value) {
Vector<String> value_list; Vector<String> value_list;
value.Split(';', value_list); value.Split(';', value_list);
for (size_t i = 0; i < value_list.size(); ++i) { for (size_t i = 0; i < value_list.size(); ++i) {
String stripped = StripLeadingAndTrailingHTMLSpaces(value_list[i]); String stripped = StripLeadingAndTrailingHTMLSpaces(value_list[i]);
if (ProtocolIsJavaScript(stripped)) if (ProtocolIsJavaScript(stripped))
return stripped; return true;
} }
return g_empty_string; return false;
} }
XSSAuditor::XSSAuditor() XSSAuditor::XSSAuditor()
...@@ -711,15 +720,14 @@ bool XSSAuditor::EraseDangerousAttributesIfInjected( ...@@ -711,15 +720,14 @@ bool XSSAuditor::EraseDangerousAttributesIfInjected(
Canonicalize(SnippetFromAttribute(request, attribute), Canonicalize(SnippetFromAttribute(request, attribute),
kScriptLikeAttributeTruncation)); kScriptLikeAttributeTruncation));
} else if (IsSemicolonSeparatedAttribute(attribute)) { } else if (IsSemicolonSeparatedAttribute(attribute)) {
String sub_value = if (IsSemicolonSeparatedValueContainingJavaScriptURL(attribute.Value())) {
SemicolonSeparatedValueContainingJavaScriptURL(attribute.Value());
if (!sub_value.IsEmpty()) {
value_contains_java_script_url = true; value_contains_java_script_url = true;
erase_attribute = erase_attribute =
IsContainedInRequest(Canonicalize( IsContainedInRequest(Canonicalize(
NameFromAttribute(request, attribute), kNoTruncation)) && NameFromAttribute(request, attribute), kNoTruncation)) &&
IsContainedInRequest( IsContainedInRequest(
Canonicalize(sub_value, kScriptLikeAttributeTruncation)); Canonicalize(SnippetFromAttribute(request, attribute),
kSemicolonSeparatedScriptLikeAttributeTruncation));
} }
} else if (ProtocolIsJavaScript( } else if (ProtocolIsJavaScript(
StripLeadingAndTrailingHTMLSpaces(attribute.Value()))) { StripLeadingAndTrailingHTMLSpaces(attribute.Value()))) {
...@@ -825,6 +833,8 @@ String XSSAuditor::Canonicalize(String snippet, TruncationKind treatment) { ...@@ -825,6 +833,8 @@ String XSSAuditor::Canonicalize(String snippet, TruncationKind treatment) {
TruncateForSrcLikeAttribute(decoded_snippet); TruncateForSrcLikeAttribute(decoded_snippet);
else if (treatment == kScriptLikeAttributeTruncation) else if (treatment == kScriptLikeAttributeTruncation)
TruncateForScriptLikeAttribute(decoded_snippet); TruncateForScriptLikeAttribute(decoded_snippet);
else if (treatment == kSemicolonSeparatedScriptLikeAttributeTruncation)
TruncateForSemicolonSeparatedScriptLikeAttribute(decoded_snippet);
} }
return decoded_snippet.RemoveCharacters(&IsNonCanonicalCharacter); return decoded_snippet.RemoveCharacters(&IsNonCanonicalCharacter);
......
...@@ -86,7 +86,8 @@ class XSSAuditor { ...@@ -86,7 +86,8 @@ class XSSAuditor {
kNoTruncation, kNoTruncation,
kNormalAttributeTruncation, kNormalAttributeTruncation,
kSrcLikeAttributeTruncation, kSrcLikeAttributeTruncation,
kScriptLikeAttributeTruncation kScriptLikeAttributeTruncation,
kSemicolonSeparatedScriptLikeAttributeTruncation,
}; };
enum HrefRestriction { kProhibitSameOriginHref, kAllowSameOriginHref }; enum HrefRestriction { kProhibitSameOriginHref, kAllowSameOriginHref };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment