Implement a reflective XSS filter

• Component: DOM
• Status: RESOLVED FIXED

[Adam Barth]

We should implement a filter for reflected XSS.  The goal of the filter is to prevent an attacker from exploiting some common kinds of reflected XSS vulnerabilities in web sites.

A student I'm working with at Berkeley is working on one that improves on the IE8 filter in a number of ways.  I'll update this bug with more information as it becomes available.

What's the best way to measure the performance impact of the filter?  We can obviously run SunSpider, but that's probably measuring the wrong thing.  Do we have something like page cycler that we can try the patch on?

[Daniel Bates]

Created attachment 31107 [details]
Prototype of XSS Auditor

Initial prototype of XSS Auditor. The patch does not include the build files as I was unclear how to modify the command-line build files. It does build using when the files are added to the WebCore Xcode project file and built.

[Daniel Bates]

Created attachment 31108 [details]
Prototype of XSS Auditor

Copied and pasted header twice. Fixed in this attachment.

[Daniel Bates]

Created attachment 31109 [details]
SunSpider comparison

The XSS Auditor does not appear to affect SunSpider performance. From = disabled XSS Auditor, To = enabled XSS Auditor.

[Sam Weinig]

What Computer/OS were those numbers taken with.  The overall number seems quite high.

[Daniel Bates]

(In reply to comment #4)
> What Computer/OS were those numbers taken with.  The overall number seems quite
> high.
> 

2.6 Ghz MacBook Pro (MacBookPro4,1) running Mac OS 10.5.6.

[Sam Weinig]

Are you running the computer under any kind of non-normal settings.  Rosetta?  Inside a VM?  These numbers are dramatically slower than they should be.  It might be a good idea to test these numbers on another machine.

[Sam Weinig]

Another idea!  Are you testing on a debug build?

[Adam Barth]

Created attachment 31126 [details]
Cleaned up patch (disabled by default)

Here's a version cleaned up to (hopefully!) match WebKit style.  The XSSAuditor is disabled by default.

Basically, I'd to land this patch (and maybe some follow ups) so we can experiment with this feature behind a command line flag in Chromium and measure things like false positives via the Dev channel.

Once we've gathered enough data, we can revisit the issue of whether to turn this on by default.

[Adam Barth]

Created attachment 31127 [details]
Patch (disabled by default)

Made some changes suggested by Eric.

[Sam Weinig]

Is it possible to make tests for this?

[Adam Barth]

> Is it possible to make tests for this?

I'm not sure we have a way to test something that's disabled by default.  We'd need Bug 20534 to be fixed first.  Dan Bates has put together a manual test suite:

http://webblaze.org/dbates/

We could also hack around the issue by letting a site opt-in to the filter using an experimental header, like X-Enable-WebKit-XSS-Auditor: true.

[Sam Weinig]

The best way to test something that is disabled by default is to add method to LayoutTestController that turns it on via WebPreferences.  I believe we do this for some tests of private browsing.

[Adam Barth]

Comment on attachment 31127 [details]
Patch (disabled by default)

Ok.  I'll give that a try and upload a new patch.

[Adam Barth]

Created attachment 31154 [details]
Patch (disabled by default, with a test)

I added a simple test.  We can add more tests as we iterate on this feature.

[Sam Weinig]

Comment on attachment 31154 [details]
Patch (disabled by default, with a test)

> @@ -132,6 +133,9 @@ public:
>      NPObject* createScriptObjectForPluginElement(HTMLPlugInElement*);
>      NPObject* windowScriptNPObject();
>  #endif
> +    
> +    // Returns the XSSAuditor associated with this ScriptController.
> +    XSSAuditor* activeXSSAuditor() { return m_XSSAuditor.get(); }

Why activeXSSAuditor?  Will there be more than one per scriptController in the future?

>  
>  private:
>      void initScriptIfNeeded()
> @@ -164,6 +168,9 @@ private:
>  #if PLATFORM(MAC)
>      RetainPtr<WebScriptObject> m_windowScriptObject;
>  #endif
> +    
> +    // The XSSAuditor associated with this ScriptController.
> +    OwnPtr<XSSAuditor> m_XSSAuditor;

Does the auditor have to be a pointer here? 

>  
>      class Tokenizer {
>      public:
> @@ -58,11 +59,19 @@ namespace WebCore {
>          virtual void executeScriptsWaitingForStylesheets() {}
>  
>          virtual bool isHTMLTokenizer() const { return false; }
> +        
> +        // Returns the XSSAuditor associated with this tokenizer.
> +        XSSAuditor* activeXSSAuditor() const { return m_XSSAuditor; }
> +        
> +        // Associates the specified XSSAuditor with this tokenizer.
> +        // @param auditor the XSSAuditor to associate with this tokenizer.

We don't use @param syntax.  In fact, neither of these comments are really necessary as the code is self explanatory.

> +        void setXSSAuditor(XSSAuditor* auditor) { m_XSSAuditor = auditor; }
>  
>      protected:
>          Tokenizer(bool viewSourceMode = false) 
>              : m_parserStopped(false)
>              , m_inViewSourceMode(viewSourceMode)
> +            , m_XSSAuditor(0)
>          {
>          }
>  
> +namespace WebCore {
> +  
> +// This method also appears in file ResourceResponseBase.cpp.
> +static bool isControlCharacter(UChar c)
> +{
> +   return c < ' ' || c == 127;
> +}
> +  
> +XSSAuditor::XSSAuditor(Frame* frame)
> +    : m_isEnabled(false)
> +    , m_frame(frame)
> +{
> +    if (Settings* settings = frame->settings())
> +        m_isEnabled = settings->xssAuditorEnabled();
> +}
> +
> +XSSAuditor::~XSSAuditor()
> +{
> +}
> +  
> +bool XSSAuditor::canEvaluate(const ScriptSourceCode& sourceCode) const
> +{
> +    if (!m_isEnabled)
> +        return true;
> +    
> +    return canEvaluate(String(sourceCode.jsSourceCode().data(), sourceCode.jsSourceCode().length()));
> +}
> +  
> +bool XSSAuditor::canEvaluate(const String& sourceCode) const
> +{
> +    if (!m_isEnabled)
> +        return true;
> +    
> +    if (findInRequest(sourceCode)) {
> +        String consoleMessage = "Refused to execute a JavaScript script. Source code of script found within request.\n";

The message should be a static.

> +        m_frame->domWindow()->console()->addMessage(JSMessageSource, ErrorMessageLevel, consoleMessage, 1, String());
> +        return false;
> +    }
> +    return true;
> +}
> +
> +bool XSSAuditor::canCreateInlineEventListener(const String&, const String& code) const
> +{
> +    if (!m_isEnabled)
> +        return true;
> +    
> +    return canEvaluate(code);
> +}
> +
> +bool XSSAuditor::canLoadExternalScriptFromSrc(const String& url) const
> +{
> +    if (!m_isEnabled)
> +        return true;
> +  
> +    if (findInRequest(url)) {
> +        String consoleMessage = "Refused to execute a JavaScript script. Source code of script found within request.\n";

The message should be a static.

> +        m_frame->domWindow()->console()->addMessage(JSMessageSource, ErrorMessageLevel, consoleMessage, 1, String());
> +        return false;
> +    }
> +    return true;
> +}
> +
> +bool XSSAuditor::canLoadObject(const String& url) const
> +{
> +    if (!m_isEnabled)
> +        return true;
> +
> +    if (findInRequest(url)) {
> +        String consoleMessage = "Refused to execute a JavaScript script. Source code of script found within request";

The message should be a static.

> +        m_frame->domWindow()->console()->addMessage(OtherMessageSource, ErrorMessageLevel, consoleMessage, 1, String());    
> +        return false;
> +    }
> +    return true;
> +}
> +
> +// static

This comment is not necessary.

> +String XSSAuditor::decodeURL(const String& str, const TextEncoding& encoding, bool allowNullCharacters)
> +{
> +    String result;
> +    String url = str;
> +    
> +    url.replace('+', ' ');
> +    result = decodeURLEscapeSequences(url, encoding);
> +    return (allowNullCharacters)? result : result.removeCharacters(&isControlCharacter);

Missing space after (allowNullCharacters).  The parens are not needed as well.


> +    // The XSSAuditor class is used to prevent type 1 cross-site scripting
> +    // vulnerabilites (also known as reflected vulnerabilities).
> +    //
> +    // More specifically, the XSSAuditor class decides whether the execution of
> +    // a script is to be allowed or denied based on the content of any
> +    // user-submitted data, including:
> +    //
> +    // * the query string of the URL.
> +    // * the HTTP-POST data.
> +    //
> +    // If the source code of a script resembles any user-submitted data then it
> +    // is denied execution.
> +    //
> +    // When you instantiate the XSSAuditor you must specify the {@link Frame}
> +    // of the page that you wish to audit.
> +    //
> +    // Bindings
> +    //
> +    // An XSSAuditor is instantiated within the contructor of a
> +    // ScriptController object and passed the Frame the script originated. The
> +    // ScriptController calls back to the XSSAuditor to determine whether a
> +    // JavaScript script is safe to execute before executing it. The following
> +    // methods call into XSSAuditor:
> +    //
> +    // * ScriptController::evaluate - used to evaluate JavaScript scripts.
> +    // * ScriptController::createInlineEventListener - used to create JavaScript event handlers.
> +    // * HTMLTokenizer#scriptHandler - used to load external JavaScript scripts.

The use of # is inconsistent with the rest of the comment.

> +    // 
> +    class XSSAuditor {
> +    public:
> +        // Creates a new XSSAuditor object that is associated with the specified frame.
> +        // @param frame the frame to associate with this auditor.

We don't use @param syntax.  In fact, this comment is not necessary.


Overall this looks great, but I want to defer r+ing this until we have some performance analysis done on it, even it only in the disabled state.  Sunspider and some form of plt testing would be sufficient I think.  Though I would also like to see some targeted performance tests of the cases where the auditor will be used.

[Adam Barth]

Created attachment 31269 [details]
updated patch

(In reply to comment #15)
> Why activeXSSAuditor?  Will there be more than one per scriptController in the
> future?

Fixed.

> > +    // The XSSAuditor associated with this ScriptController.
> > +    OwnPtr<XSSAuditor> m_XSSAuditor;
> 
> Does the auditor have to be a pointer here? 

I make this a pointer because I didn't want to include XSSAuditor.h here because ScriptController.h is included by the entire world (including outside of WebCore).

> We don't use @param syntax.  In fact, neither of these comments are really
> necessary as the code is self explanatory.

Fixed.

> The message should be a static.

Fixed x3.

> > +// static
> 
> This comment is not necessary.

Fixed.

> Missing space after (allowNullCharacters).  The parens are not needed as well.

Fixed.

> The use of # is inconsistent with the rest of the comment.

Fixed.

> We don't use @param syntax.  In fact, this comment is not necessary.

Fixed.

> Overall this looks great, but I want to defer r+ing this until we have some
> performance analysis done on it, even it only in the disabled state.

You mean you'd like to measure the performance impact when its enabled, right?

> Sunspider and some form of plt testing would be sufficient I think.

Sunspider I can do.  How do I do plt testing?

Thanks for reviewing the patch.  :)

[Sam Weinig]

> 
> > Overall this looks great, but I want to defer r+ing this until we have some
> > performance analysis done on it, even it only in the disabled state.
> 
> You mean you'd like to measure the performance impact when its enabled, right?

Ideally we would measure both, but I was actually just suggesting that we measure it while it is disabled to begin with, since that is how it is going to be landed.

> 
> > Sunspider and some form of plt testing would be sufficient I think.
> 
> Sunspider I can do.  How do I do plt testing?

PLT testing unfortunately requires internal resources, so I will probably have to do it.  I will try and get that done on Monday.

[Adam Barth]

(In reply to comment #17)
> Ideally we would measure both, but I was actually just suggesting that we
> measure it while it is disabled to begin with, since that is how it is going to
> be landed.

Ok.  I'm building a release version now.  I'll try to do the sunspider measurements today.

> > Sunspider I can do.  How do I do plt testing?
> 
> PLT testing unfortunately requires internal resources, so I will probably have
> to do it.  I will try and get that done on Monday.

Thanks.  :)

[Adam Barth]

Created attachment 31270 [details]
updated patch

Oops.  Forgot DEFINE_STATIC_LOCAL.

[Adam Barth]

Created attachment 31274 [details]
Sunspider (before patch)

[Adam Barth]

Created attachment 31275 [details]
Sunspider (after patch, XSSAuditor disabled)

[Adam Barth]

Created attachment 31276 [details]
Sunspider (after patch, XSSAuditor enabled)

[Adam Barth]

Looks like there is no statistically significant impact on Sunsider from the XSSAuditor (either enabled or disabled)

[Sam Weinig]

Comment on attachment 31270 [details]
updated patch

Perf seemed fine.  r=me!  Hot!

[Adam Barth]

Will land.  Might be tricky without try bots.  Blind editing makefiles for the win.

[Adam Barth]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/http/tests/security/xssAuditor
Adding         LayoutTests/http/tests/security/xssAuditor/resources
Adding         LayoutTests/http/tests/security/xssAuditor/resources/echo-intertag.php
Adding         LayoutTests/http/tests/security/xssAuditor/script-tag-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/script-tag.html
Sending        WebCore/ChangeLog
Sending        WebCore/GNUmakefile.am
Sending        WebCore/WebCore.base.exp
Sending        WebCore/WebCore.pro
Sending        WebCore/WebCore.vcproj/WebCore.vcproj
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/bindings/js/ScriptController.cpp
Sending        WebCore/bindings/js/ScriptController.h
Sending        WebCore/bindings/js/ScriptEventListener.cpp
Sending        WebCore/dom/Tokenizer.h
Sending        WebCore/html/HTMLTokenizer.cpp
Sending        WebCore/loader/FrameLoader.cpp
Sending        WebCore/page/Settings.cpp
Sending        WebCore/page/Settings.h
Adding         WebCore/page/XSSAuditor.cpp
Adding         WebCore/page/XSSAuditor.h
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebPreferenceKeysPrivate.h
Sending        WebKit/mac/WebView/WebPreferences.mm
Sending        WebKit/mac/WebView/WebPreferencesPrivate.h
Sending        WebKit/mac/WebView/WebView.mm
Sending        WebKit/win/ChangeLog
Sending        WebKit/win/Interfaces/IWebPreferencesPrivate.idl
Sending        WebKit/win/WebPreferenceKeysPrivate.h
Sending        WebKit/win/WebPreferences.cpp
Sending        WebKit/win/WebPreferences.h
Sending        WebKit/win/WebView.cpp
Sending        WebKitTools/ChangeLog
Sending        WebKitTools/DumpRenderTree/LayoutTestController.cpp
Sending        WebKitTools/DumpRenderTree/LayoutTestController.h
Sending        WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp
Sending        WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
Sending        WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
Sending        WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp
Sending        WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp
Sending        WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp
Transmitting file data ........................................
Committed revision 44846.