Bug 70973 – XSSAuditor is silent

• Component: Page Loading
• Status: RESOLVED FIXED

[Thomas Sepez]

Apart from a console log message, there's no good way for a browser to indicate that the XSSAuditor has fired -- it typically just navigates to about:blank.
Add a method in FrameLoaderClient to provide this information to the outside world.

[Thomas Sepez]

Created attachment 112708 [details]
Semi-functional patch to discuss style points.

[Adam Barth]

Comment on attachment 112708 [details]
Semi-functional patch to discuss style points.

View in context: https://bugs.webkit.org/attachment.cgi?id=112708&action=review

> Source/WebCore/loader/FrameLoader.cpp:940
> +    String message = "Refused to execute a JavaScript script. Source code of script found within request.\n";

DEFINE_STATIC_LOCAL

[Alexey Proskuryakov]

Comment on attachment 112708 [details]
Semi-functional patch to discuss style points.

View in context: https://bugs.webkit.org/attachment.cgi?id=112708&action=review

>> Source/WebCore/loader/FrameLoader.cpp:940
>> +    String message = "Refused to execute a JavaScript script. Source code of script found within request.\n";
> 
> DEFINE_STATIC_LOCAL

const char*

[Thomas Sepez]

Comment on attachment 112708 [details]
Semi-functional patch to discuss style points.

View in context: https://bugs.webkit.org/attachment.cgi?id=112708&action=review

>>> Source/WebCore/loader/FrameLoader.cpp:940
>>> +    String message = "Refused to execute a JavaScript script. Source code of script found within request.\n";
>> 
>> DEFINE_STATIC_LOCAL
> 
> const char*

Yes, const char* is much better until the time when we start cobbling together strings.  Thanks.

[Thomas Sepez]

Created attachment 112790 [details]
Proposed patch.

This patch add the didDetectXSS method to the FrameLoader and FrameLoaderClient, and adds plumbing to all the places I could find that it needs to flow into.  The Webkit2 code is mostly cut-n-paste from existing routines.

[WebKit Review Bot]

Attachment 112790 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebKit/chromium/public/WebFrameClient.h:289:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Adam Barth]

Comment on attachment 112790 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=112790&action=review

We'll need fishd for the WebKit API review.

This patch shows us just how crazy this project has become.

> Source/WebCore/loader/FrameLoader.cpp:948
> +    SecurityOrigin* origin = m_frame->document()->securityOrigin();
> +    m_client->didDetectXSS(origin, m_frame->document()->url(), errorPageURL, blockEnabled);

This client callback can do arbitrary things, which could cause origin to be deleted.  I'd just grab it from m_frame->document()->securityOrigin() twice.

> Source/WebCore/loader/FrameLoaderClient.h:211
> +        virtual void didDetectXSS(SecurityOrigin*, const KURL&, KURL&, bool) = 0;

It's probably better to use names for these KURL parameters.  It's hard to tell what they are from this declaration.

> Source/WebKit2/UIProcess/API/C/WKPage.h:67
> +typedef void (*WKPageDidDetectXSSForFrameCallback)(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void *clientInfo);

We might not want to expose this in the WebKit2 C API right away.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:957
> +    webPage->send(Messages::WebPageProxy::DidDetectXSSForFrame(m_frame->frameID(), InjectedBundleUserMessageEncoder(userData.get())));

Looks like you're dropping the parameters on the floor.  I'd recommend just having this function do nothing and adding a FIXME.

> Source/WebKit/chromium/public/WebFrameClient.h:292
> +    virtual void didDetectXSS(WebFrame*, const WebSecurityOrigin&, const WebURL&, WebURL&, bool) { }

We probably want parameter names here too.

> LayoutTests/http/tests/security/xssAuditor/script-tag-with-callbacks.html:8
> +  layoutTestController.dumpAsText();
> +  layoutTestController.dumpFrameLoadCallbacks();
> +  layoutTestController.setXSSAuditorEnabled(true);

Four-space indent.

[Darin Fisher (:fishd, Google)]

Comment on attachment 112790 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=112790&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:287
> +    virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&, const WebURL&) { }

nit: i found that naming the parameter here was a bit helpful.

>> Source/WebKit/chromium/public/WebFrameClient.h:292
>> +    virtual void didDetectXSS(WebFrame*, const WebSecurityOrigin&, const WebURL&, WebURL&, bool) { }
> 
> We probably want parameter names here too.

Ditto.  It is not obvious how to tell apart the two URL parameters, and the bool parameter also needs a name.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1198
> +void FrameLoaderClientImpl::didDetectXSS(SecurityOrigin* origin, const KURL& insecureURL, KURL& errorPageURL, bool blockEnabled)

hmm... it is a bit interesting that this would create another error-page loading path.  are you sure this is the best design?

elsewhere we handle this kind of thing by generating a ResourceError that when later passed up to didFailLoad triggers
an error page corresponding to the error code.  maybe we should do something like that here as well?

also, no where else in the WebKit API have we assumed that embedders display errors as error pages.

[Adam Barth]

The error page isn't essential to what we're trying to accomplish in this patch.  I think Tom just didn't want to have to edit all these files again.  :)

[Thomas Sepez]

Thanks Darin.  I'm willing to go down the route of introducing a new error class, possibly in a second patch to try and keep the size of this one down.  But there are two cases to consider here: one where the (neutered) page continues to load and the client would perhaps like to show an inforbar or equivalent, and a second where the load immediately terminates in error.  I'd like to keep the proposed callback for the first case, and actually have it called in both cases -- my ulterior motive being that the Chromium client can then generate histograms about how often this is happening in the world.

So ... I think I'm going to pull the second URL parameter from the callback, and leave the current navigation to about:blank as-is in this patch.  Would that be reasonable?

Adam, question:  It dawns on me on the way into work that  when we immediately terminante, we get once invocation of the callback.  But when continuing, we'd get one callback for each of the many spots on the page where the xss is repeated.  So ... is there a good place inside the auditor / frameloader / frameloader client to track that the callback has already been fired -- any of these live the correct length of time to do this easily?

[Adam Barth]

Yeah, we should only send the callback once per document.

Maybe the best thing to do is to not make any changes to FrameLoader and keep all this code in XSSAuditor.cpp.  It can call methods on FrameLoaderClient just fine.  Then it can keep a counter/bool and dispatch the message only once.  (I think it's good to log to the console each time though.)

[Thomas Sepez]

Sure, putting that state into the XSSAuditor is easy.  It feels wrong that anything outside of frameloader knows that there's such a thing as a frameloaderclient, but it sounds like that is violated all over the place.

[Adam Barth]

The trade-off is otherwise FrameLoader needs to have fingers in many more pies.  FrameLoader used to encapsulate FrameLoaderClient, bit it doesn't anymore.  We probably should rename FrameLoaderClient to something more appropriate.

[Thomas Sepez]

Comment on attachment 112790 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=112790&action=review

>> Source/WebCore/loader/FrameLoader.cpp:948
>> +    m_client->didDetectXSS(origin, m_frame->document()->url(), errorPageURL, blockEnabled);
> 
> This client callback can do arbitrary things, which could cause origin to be deleted.  I'd just grab it from m_frame->document()->securityOrigin() twice.

File removed from patch

>> Source/WebCore/loader/FrameLoaderClient.h:211
>> +        virtual void didDetectXSS(SecurityOrigin*, const KURL&, KURL&, bool) = 0;
> 
> It's probably better to use names for these KURL parameters.  It's hard to tell what they are from this declaration.

done.

>> Source/WebKit2/UIProcess/API/C/WKPage.h:67
>> +typedef void (*WKPageDidDetectXSSForFrameCallback)(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void *clientInfo);
> 
> We might not want to expose this in the WebKit2 C API right away.

Not sure what to do here.  My intention was to put enough into WK2 so that a test could run if needed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:957
>> +    webPage->send(Messages::WebPageProxy::DidDetectXSSForFrame(m_frame->frameID(), InjectedBundleUserMessageEncoder(userData.get())));
> 
> Looks like you're dropping the parameters on the floor.  I'd recommend just having this function do nothing and adding a FIXME.

Same as above.  Only interested in getting a the method to run and do printf's for test.

>> Source/WebKit/chromium/public/WebFrameClient.h:287
>> +    virtual void didRunInsecureContent(WebFrame*, const WebSecurityOrigin&, const WebURL&) { }
> 
> nit: i found that naming the parameter here was a bit helpful.

Ok.

>> Source/WebKit/chromium/public/WebFrameClient.h:289
>> +    // A reflected XSS was encountered in the page and suppressed.  Return true
> 
> Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]

Comment truncated.

>>> Source/WebKit/chromium/public/WebFrameClient.h:292
>>> +    virtual void didDetectXSS(WebFrame*, const WebSecurityOrigin&, const WebURL&, WebURL&, bool) { }
>> 
>> We probably want parameter names here too.
> 
> Ditto.  It is not obvious how to tell apart the two URL parameters, and the bool parameter also needs a name.

Now one URL parameter, bool named.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1198
>> +void FrameLoaderClientImpl::didDetectXSS(SecurityOrigin* origin, const KURL& insecureURL, KURL& errorPageURL, bool blockEnabled)
> 
> hmm... it is a bit interesting that this would create another error-page loading path.  are you sure this is the best design?
> 
> elsewhere we handle this kind of thing by generating a ResourceError that when later passed up to didFailLoad triggers
> an error page corresponding to the error code.  maybe we should do something like that here as well?
> 
> also, no where else in the WebKit API have we assumed that embedders display errors as error pages.

Rewriten.  No errorPageURL any more

>> LayoutTests/http/tests/security/xssAuditor/script-tag-with-callbacks.html:8
>> +  layoutTestController.setXSSAuditorEnabled(true);
> 
> Four-space indent.

Fixed.

[Thomas Sepez]

Addressed comments, new patch coming shortly.

[Thomas Sepez]

Created attachment 112905 [details]
Proposed Patch

Next revision.

[Adam Barth]

Looks fine to me.

We'll need fishd to look at the Chromium API parts.

@Sam: Would you be willing to look at the WebKit2 parts?

[Darin Fisher (:fishd, Google)]

Comment on attachment 112905 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112905&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:290
> +    virtual void didDetectXSS(WebFrame*, const WebURL&, bool blockEnabled) { }

what does "blockEnabled" mean?  i should be able to figure it out from just reading the comment here or based on method signature / parameter naming.

[Thomas Sepez]

(In reply to comment #19)
> (From update of attachment 112905 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112905&action=review
> 
> > Source/WebKit/chromium/public/WebFrameClient.h:290
> > +    virtual void didDetectXSS(WebFrame*, const WebURL&, bool blockEnabled) { }
> 
> what does "blockEnabled" mean?  i should be able to figure it out from just reading the comment here or based on method signature / parameter naming.

This comes from the mode setting from the x-xss-enabled header, e.g.
X-XSS-OPTIONS: 1; mode=block
which causes the entire page to be blocked rather than just having the injected scripts removed from the page.

[Thomas Sepez]

Created attachment 113066 [details]
Proposed patch + improved boolean naming.

Note to self:
$ sed -i .keep -e 's/blockEnabled/blockEntirePage/g' `svn status | awk '/^M/{print $2}'`
is your friend.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113066 [details]
Proposed patch + improved boolean naming.

Chromium WebKit API changes LGTM

[Adam Barth]

Comment on attachment 113066 [details]
Proposed patch + improved boolean naming.

View in context: https://bugs.webkit.org/attachment.cgi?id=113066&action=review

The WebKit2 stuff is kind of mysterious to me, but looks plausible.

> Source/WebCore/html/parser/XSSAuditor.cpp:292
> +        if (blockEntirePage)
> +             m_parser->document()->frame()->loader()->stopAllLoaders();

It's kind of odd that we do half of this before notifying the client and half afterwards.  Maybe we should do it all before now that we're not getting a URL from the client?

> Source/WebCore/loader/FrameLoaderClient.h:211
> +        virtual void didDetectXSS(const KURL&, bool blockEntirePage) = 0;

didBlockEntirePage ?

[Thomas Sepez]

Comment on attachment 113066 [details]
Proposed patch + improved boolean naming.

View in context: https://bugs.webkit.org/attachment.cgi?id=113066&action=review

>> Source/WebCore/html/parser/XSSAuditor.cpp:292
>> +             m_parser->document()->frame()->loader()->stopAllLoaders();
> 
> It's kind of odd that we do half of this before notifying the client and half afterwards.  Maybe we should do it all before now that we're not getting a URL from the client?

My thinking was that we want to stop the loaders ASAP since we know the load can't work, no matter how long the client spends in its callback.  Was also thinking that firing the callback after scheduling a navigation seemed wrong too, just a feeling that some state may go away without actually tracking down the code path to see what happens ... sorry.

>> Source/WebCore/loader/FrameLoaderClient.h:211
>> +        virtual void didDetectXSS(const KURL&, bool blockEntirePage) = 0;
> 
> didBlockEntirePage ?

Damned verb tense.  Howzabout I do this if some other issue forces generation of a new patch?  Or do you insist?  Either way.

[Thomas Sepez]

Adam, was there someone else we wanted from the webkit2 side?  Thanks.

[Adam Barth]

> >> Source/WebCore/loader/FrameLoaderClient.h:211
> >> +        virtual void didDetectXSS(const KURL&, bool blockEntirePage) = 0;
> > 
> > didBlockEntirePage ?
> 
> Damned verb tense.  Howzabout I do this if some other issue forces generation of a new patch?  Or do you insist?  Either way.

Ok.

> Adam, was there someone else we wanted from the webkit2 side?  Thanks.

Sam has been CCed on this bug for a while.  I think it's fine to land as-is.

[Adam Barth]

Just for you Tom, I've edited your patch file to rename the variable.  :)

[Adam Barth]

Created attachment 113353 [details]
Patch for landing

[Thomas Sepez]

If you want to be super picky, the bool in XSSAuditor.cpp should be blockEntirePage since it's taking action upon it, and the others should be didBlockEntirePage (blockedEntirePage?).

[Adam Barth]

> If you want to be super picky, the bool in XSSAuditor.cpp should be blockEntirePage since it's taking action upon it, and the others should be didBlockEntirePage (blockedEntirePage?).

Oh noes!  :)

[WebKit Review Bot]

Comment on attachment 113353 [details]
Patch for landing

Clearing flags on attachment: 113353

Committed r99096: <http://trac.webkit.org/changeset/99096>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.