Changeset 60964 in webkit

Timestamp:

Jun 10, 2010 10:39:59 AM (14 years ago)

Author:

abarth@webkit.org

Message:

2010-06-10 Adam Barth <​abarth@webkit.org>

Reviewed by Eric Seidel.

Use allowRequestIfNoIllegalURICharacters instead of context for XSSAuditor::canLoadExternalScriptFromSrc
​https://bugs.webkit.org/show_bug.cgi?id=40404

We originally added the context parameter to
canLoadExternalScriptFromSrc to work around some false positives caused
by folks checking external script URLs on the server. Our thought was
that we could tell these were not real XSS attacks because the
surrounding context wouldn't match in the URL and the document.

Implementing this feature in the HTML5 parser is hard because it
pierces a layer of abstraction (the token abstraction of the input
stream). We could hack this into the new parser, but instead I think
it's better to switch to using the allowRequestIfNoIllegalURICharacters
heuristic.

We designed the allowRequestIfNoIllegalURICharacters after the context
heuristic to deal with other cases where the server was validating
input before echoing it. However, we never tried applying it to
canLoadExternalScriptFromSrc.

It's possible that this will cause false positives and will need to be
reverted, which is why I've left in some of the infrustructure for
computing context. We don't have a good way to know if that will
happen except to try. We do know, however, that this heuristic will
work for the original false positives we saw.

• html/HTML5Tokenizer.cpp: (WebCore::HTML5Tokenizer::shouldLoadExternalScriptFromSrc):
• html/HTMLTokenizer.cpp: (WebCore::HTMLTokenizer::parseTag):
• page/XSSAuditor.cpp: (WebCore::XSSAuditor::canLoadExternalScriptFromSrc):
• page/XSSAuditor.h:

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/HTML5Tokenizer.cpp (modified) (1 diff)
• html/HTMLTokenizer.cpp (modified) (2 diffs)
• page/XSSAuditor.cpp (modified) (2 diffs)
• page/XSSAuditor.h (modified) (1 diff)

trunk/WebCore/ChangeLog

@@ +1 @@
+2010-06-10  Adam Barth  <abarth@webkit.org>
+
+        Reviewed by Eric Seidel.
+
+        Use allowRequestIfNoIllegalURICharacters instead of context for XSSAuditor::canLoadExternalScriptFromSrc
+        https://bugs.webkit.org/show_bug.cgi?id=40404
+
+        We originally added the context parameter to
+        canLoadExternalScriptFromSrc to work around some false positives caused
+        by folks checking external script URLs on the server.  Our thought was
+        that we could tell these were not real XSS attacks because the
+        surrounding context wouldn't match in the URL and the document.
+
+        Implementing this feature in the HTML5 parser is hard because it
+        pierces a layer of abstraction (the token abstraction of the input
+        stream).  We could hack this into the new parser, but instead I think
+        it's better to switch to using the allowRequestIfNoIllegalURICharacters
+        heuristic.
+
+        We designed the allowRequestIfNoIllegalURICharacters after the context
+        heuristic to deal with other cases where the server was validating
+        input before echoing it.  However, we never tried applying it to
+        canLoadExternalScriptFromSrc.
+
+        It's possible that this will cause false positives and will need to be
+        reverted, which is why I've left in some of the infrustructure for
+        computing context.  We don't have a good way to know if that will
+        happen except to try.  We do know, however, that this heuristic will
+        work for the original false positives we saw.
+
+        * html/HTML5Tokenizer.cpp:
+        (WebCore::HTML5Tokenizer::shouldLoadExternalScriptFromSrc):
+        * html/HTMLTokenizer.cpp:
+        (WebCore::HTMLTokenizer::parseTag):
+        * page/XSSAuditor.cpp:
+        (WebCore::XSSAuditor::canLoadExternalScriptFromSrc):
+        * page/XSSAuditor.h:
+
 2010-06-10  Kwang Yul Seo  <skyul@company100.net>
 

trunk/WebCore/html/HTML5Tokenizer.cpp

@@ -226 +226 @@
     if (!m_XSSAuditor)
         return true;
-    // FIXME: We have no easy way to provide the XSSAuditor with the original
-    // un-processed attribute source, so for now we pass nullAtom.
-    return m_XSSAuditor->canLoadExternalScriptFromSrc(nullAtom, srcValue);
+    return m_XSSAuditor->canLoadExternalScriptFromSrc(srcValue);
 }
 

trunk/WebCore/html/HTMLTokenizer.cpp

@@ -1396 +1396 @@
                         if (m_currentToken.beginTag && m_currentToken.tagName == scriptTag && !inViewSourceMode() && !m_parser->skipMode() && m_attrName == srcAttr) {
                             String context(m_rawAttributeBeforeValue.data(), m_rawAttributeBeforeValue.size());
-                            if (m_XSSAuditor && !m_XSSAuditor->canLoadExternalScriptFromSrc(context, attributeValue))
+                            if (m_XSSAuditor && !m_XSSAuditor->canLoadExternalScriptFromSrc(attributeValue))
                                 attributeValue = blankURL().string();
                         }
@@ -1433 +1433 @@
                         if (m_currentToken.beginTag && m_currentToken.tagName == scriptTag && !inViewSourceMode() && !m_parser->skipMode() && m_attrName == srcAttr) {
                             String context(m_rawAttributeBeforeValue.data(), m_rawAttributeBeforeValue.size());
-                            if (m_XSSAuditor && !m_XSSAuditor->canLoadExternalScriptFromSrc(context, attributeValue))
+                            if (m_XSSAuditor && !m_XSSAuditor->canLoadExternalScriptFromSrc(attributeValue))
                                 attributeValue = blankURL().string();
                         }

trunk/WebCore/page/XSSAuditor.cpp

@@ -171 +171 @@
 }
 
-bool XSSAuditor::canLoadExternalScriptFromSrc(const String& context, const String& url) const
+bool XSSAuditor::canLoadExternalScriptFromSrc(const String& url) const
 {
     if (!isEnabled())
@@ -180 +180 @@
 
     FindTask task;
-    task.context = context;
     task.string = url;
+    task.allowRequestIfNoIllegalURICharacters = true;
 
     if (findInRequest(task)) {

trunk/WebCore/page/XSSAuditor.h

@@ -91 +91 @@
         // Determines whether the external script should be loaded based on the
         // content of any user-submitted data.
-        bool canLoadExternalScriptFromSrc(const String& context, const String& url) const;
+        bool canLoadExternalScriptFromSrc(const String& url) const;
 
         // Determines whether object should be loaded based on the content of