Changeset 145348 in webkit

Timestamp:

Mar 11, 2013 2:48:14 AM (11 years ago)

Author:

mkwst@chromium.org

Message:

XSSAuditor doesn't need a copy of the original document's body.
​https://bugs.webkit.org/show_bug.cgi?id=111946

Reviewed by Darin Adler.

The XSSAuditor currently copies the original HTTP body of the document
that's being audited in order to include it into a violation report if
reflected XSS is detected. We don't actually need to do this, as we
have access to the original request information from inside the
XSSAuditorDelegate where the report is generated.
XSSAuditorDelegate::didBlockScript ASSERTs that it's running on the
main thread, so it should be safe to reach through the document's
loader to get that information directly, rather than passing it from
thread to thread via XSSInfo object properties.

• html/parser/XSSAuditor.h:
• html/parser/XSSAuditor.cpp:

(WebCore::XSSAuditor::init):
(WebCore::XSSAuditor::filterToken):
(WebCore::XSSAuditor::isSafeToSendToAnotherThread):

• html/parser/XSSAuditorDelegate.h:

(WebCore::XSSInfo::create):
(WebCore::XSSInfo::XSSInfo):

• html/parser/XSSAuditorDelegate.cpp:

(WebCore::XSSInfo::isSafeToSendToAnotherThread):

Drop the XSSInfo and XSSAuditor properties that held an
isolatedCopy of the the original HTTP body. Depending on the
document's size, this could be a significant savings.

(WebCore::XSSAuditorDelegate::didBlockScript):

Reach into the document's loader's original request in order to
grab the body as a String, and feed that into the violation report
object.

As a drive-by, this patch creates a FrameLoader* temporary
variable to minimize repetition in this area of the code. We use
the loader a few times, but should only have to grab it once.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/parser/XSSAuditor.cpp (modified) (3 diffs)
• html/parser/XSSAuditor.h (modified) (1 diff)
• html/parser/XSSAuditorDelegate.cpp (modified) (4 diffs)
• html/parser/XSSAuditorDelegate.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-03-11  Mike West  <mkwst@chromium.org>
+
+        XSSAuditor doesn't need a copy of the original document's body.
+        https://bugs.webkit.org/show_bug.cgi?id=111946
+
+        Reviewed by Darin Adler.
+
+        The XSSAuditor currently copies the original HTTP body of the document
+        that's being audited in order to include it into a violation report if
+        reflected XSS is detected. We don't actually need to do this, as we
+        have access to the original request information from inside the
+        XSSAuditorDelegate where the report is generated.
+        XSSAuditorDelegate::didBlockScript ASSERTs that it's running on the
+        main thread, so it should be safe to reach through the document's
+        loader to get that information directly, rather than passing it from
+        thread to thread via XSSInfo object properties.
+
+        * html/parser/XSSAuditor.h:
+        * html/parser/XSSAuditor.cpp:
+        (WebCore::XSSAuditor::init):
+        (WebCore::XSSAuditor::filterToken):
+        (WebCore::XSSAuditor::isSafeToSendToAnotherThread):
+        * html/parser/XSSAuditorDelegate.h:
+        (WebCore::XSSInfo::create):
+        (WebCore::XSSInfo::XSSInfo):
+        * html/parser/XSSAuditorDelegate.cpp:
+        (WebCore::XSSInfo::isSafeToSendToAnotherThread):
+            Drop the XSSInfo and XSSAuditor properties that held an
+            isolatedCopy of the the original HTTP body. Depending on the
+            document's size, this could be a significant savings.
+        (WebCore::XSSAuditorDelegate::didBlockScript):
+            Reach into the document's loader's original request in order to
+            grab the body as a String, and feed that into the violation report
+            object.
+
+            As a drive-by, this patch creates a FrameLoader* temporary
+            variable to minimize repetition in this area of the code. We use
+            the loader a few times, but should only have to grab it once.
+
 2013-03-11  Silvia Pfeiffer  <silviapf@chromium.org>
 

trunk/Source/WebCore/html/parser/XSSAuditor.cpp

@@ -312 +312 @@
         return;
     }
-
-    if (!m_reportURL.isEmpty())
-        m_originalHTTPBody = httpBodyAsString;
 }
 
@@ -335 +332 @@
     if (didBlockScript) {
         bool didBlockEntirePage = (m_xssProtection == ContentSecurityPolicy::BlockReflectedXSS);
-        OwnPtr<XSSInfo> xssInfo = XSSInfo::create(m_reportURL, m_originalHTTPBody, didBlockEntirePage);
-        if (!m_reportURL.isEmpty()) {
-            m_reportURL = KURL();
-            m_originalHTTPBody = String();
-        }
+        OwnPtr<XSSInfo> xssInfo = XSSInfo::create(m_reportURL, didBlockEntirePage);
+        m_reportURL = KURL();
         return xssInfo.release();
     }
@@ -728 +722 @@
 {
     return m_documentURL.isSafeToSendToAnotherThread()
-        && m_originalHTTPBody.isSafeToSendToAnotherThread()
         && m_decodedURL.isSafeToSendToAnotherThread()
         && m_decodedHTTPBody.isSafeToSendToAnotherThread()

trunk/Source/WebCore/html/parser/XSSAuditor.h

@@ -106 +106 @@
     ContentSecurityPolicy::ReflectedXSSDisposition m_xssProtection;
 
-    String m_originalHTTPBody;
     String m_decodedURL;
     String m_decodedHTTPBody;

trunk/Source/WebCore/html/parser/XSSAuditorDelegate.cpp

@@ -30 +30 @@
 #include "DOMWindow.h"
 #include "Document.h"
+#include "DocumentLoader.h"
 #include "FormData.h"
 #include "Frame.h"
@@ -43 +44 @@
 bool XSSInfo::isSafeToSendToAnotherThread() const
 {
-    return m_reportURL.isSafeToSendToAnotherThread()
-        && m_originalHTTPBody.isSafeToSendToAnotherThread();
+    return m_reportURL.isSafeToSendToAnotherThread();
 }
 
@@ -63 +63 @@
     m_document->addConsoleMessage(SecurityMessageSource, ErrorMessageLevel, consoleMessage);
 
+    FrameLoader* frameLoader = m_document->frame()->loader();
+
     if (xssInfo.m_didBlockEntirePage)
-        m_document->frame()->loader()->stopAllLoaders();
+        frameLoader->stopAllLoaders();
 
     if (!m_didNotifyClient) {
-        m_document->frame()->loader()->client()->didDetectXSS(m_document->url(), xssInfo.m_didBlockEntirePage);
+        frameLoader->client()->didDetectXSS(m_document->url(), xssInfo.m_didBlockEntirePage);
         m_didNotifyClient = true;
     }
@@ -74 +76 @@
         RefPtr<InspectorObject> reportDetails = InspectorObject::create();
         reportDetails->setString("request-url", m_document->url().string());
-        reportDetails->setString("request-body", xssInfo.m_originalHTTPBody);
+
+        String httpBody;
+        if (frameLoader->documentLoader()) {
+            if (FormData* formData = frameLoader->documentLoader()->originalRequest().httpBody())
+                httpBody = formData->flattenToString();
+        }
+        reportDetails->setString("request-body", httpBody);
 
         RefPtr<InspectorObject> reportObject = InspectorObject::create();

trunk/Source/WebCore/html/parser/XSSAuditorDelegate.h

@@ -40 +40 @@
 class XSSInfo {
 public:
-    static PassOwnPtr<XSSInfo> create(const KURL& reportURL, const String& originalHTTPBody, bool didBlockEntirePage)
+    static PassOwnPtr<XSSInfo> create(const KURL& reportURL, bool didBlockEntirePage)
     {
-        return adoptPtr(new XSSInfo(reportURL, originalHTTPBody, didBlockEntirePage));
+        return adoptPtr(new XSSInfo(reportURL, didBlockEntirePage));
     }
 
@@ -48 +48 @@
 
     KURL m_reportURL;
-    String m_originalHTTPBody;
     bool m_didBlockEntirePage;
     TextPosition m_textPosition;
 
 private:
-    XSSInfo(const KURL& reportURL, const String& originalHTTPBody, bool didBlockEntirePage)
+    XSSInfo(const KURL& reportURL, bool didBlockEntirePage)
         : m_reportURL(reportURL)
-        , m_originalHTTPBody(originalHTTPBody)
         , m_didBlockEntirePage(didBlockEntirePage)
     { }