[Webkit-unassigned] [Bug 26199] Implement a reflective XSS filter

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26199





------- Comment #15 from sam at webkit.org  2009-06-14 12:29 PDT -------
(From update of attachment 31154)
> @@ -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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.