Bug 70973 - XSSAuditor is silent
: XSSAuditor is silent
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To:
:
: XSSAuditor
:
:
  Show dependency treegraph
 
Reported: 2011-10-26 15:41 PST by
Modified: 2011-11-02 14:05 PST (History)


Attachments
Semi-functional patch to discuss style points. (21.67 KB, patch)
2011-10-27 10:56 PST, Thomas Sepez
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch. (50.34 KB, patch)
2011-10-27 17:45 PST, Thomas Sepez
fishd: review-
Review Patch | Details | Formatted Diff | Diff
Proposed Patch (48.44 KB, patch)
2011-10-28 13:52 PST, Thomas Sepez
no flags Review Patch | Details | Formatted Diff | Diff
Proposed patch + improved boolean naming. (48.50 KB, patch)
2011-10-31 10:16 PST, Thomas Sepez
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (48.53 KB, patch)
2011-11-02 12:53 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-26 15:41:23 PST
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.
------- Comment #1 From 2011-10-27 10:56:06 PST -------
Created an attachment (id=112708) [details]
Semi-functional patch to discuss style points.
------- Comment #2 From 2011-10-27 14:00:25 PST -------
(From update of attachment 112708 [details])
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
------- Comment #3 From 2011-10-27 14:07:14 PST -------
(From update of attachment 112708 [details])
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*
------- Comment #4 From 2011-10-27 14:25:27 PST -------
(From update of attachment 112708 [details])
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.
------- Comment #5 From 2011-10-27 17:45:51 PST -------
Created an attachment (id=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.
------- Comment #6 From 2011-10-27 17:48:25 PST -------
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.
------- Comment #7 From 2011-10-27 17:48:42 PST -------
Please wait for approval from fishd@chromium.org 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
------- Comment #8 From 2011-10-27 17:54:58 PST -------
(From update of attachment 112790 [details])
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.
------- Comment #9 From 2011-10-27 21:18:30 PST -------
(From update of attachment 112790 [details])
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.
------- Comment #10 From 2011-10-27 21:21:18 PST -------
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.  :)
------- Comment #11 From 2011-10-28 09:35:54 PST -------
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?
------- Comment #12 From 2011-10-28 11:05:42 PST -------
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.)
------- Comment #13 From 2011-10-28 11:21:05 PST -------
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.
------- Comment #14 From 2011-10-28 11:26:09 PST -------
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.
------- Comment #15 From 2011-10-28 12:58:20 PST -------
(From update of attachment 112790 [details])
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.
------- Comment #16 From 2011-10-28 12:58:44 PST -------
Addressed comments, new patch coming shortly.
------- Comment #17 From 2011-10-28 13:52:26 PST -------
Created an attachment (id=112905) [details]
Proposed Patch

Next revision.
------- Comment #18 From 2011-10-28 14:23:02 PST -------
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?
------- Comment #19 From 2011-10-30 22:40:24 PST -------
(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.
------- Comment #20 From 2011-10-31 09:49:10 PST -------
(In reply to comment #19)
> (From update of attachment 112905 [details] [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.
------- Comment #21 From 2011-10-31 10:16:09 PST -------
Created an attachment (id=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.
------- Comment #22 From 2011-11-02 11:34:53 PST -------
(From update of attachment 113066 [details])
Chromium WebKit API changes LGTM
------- Comment #23 From 2011-11-02 11:52:26 PST -------
(From update of attachment 113066 [details])
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 ?
------- Comment #24 From 2011-11-02 12:01:55 PST -------
(From update of attachment 113066 [details])
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.
------- Comment #25 From 2011-11-02 12:02:30 PST -------
Adam, was there someone else we wanted from the webkit2 side?  Thanks.
------- Comment #26 From 2011-11-02 12:20:39 PST -------
> >> 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.
------- Comment #27 From 2011-11-02 12:52:34 PST -------
Just for you Tom, I've edited your patch file to rename the variable.  :)
------- Comment #28 From 2011-11-02 12:53:38 PST -------
Created an attachment (id=113353) [details]
Patch for landing
------- Comment #29 From 2011-11-02 13:04:32 PST -------
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?).
------- Comment #30 From 2011-11-02 13:05:32 PST -------
> 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!  :)
------- Comment #31 From 2011-11-02 14:05:27 PST -------
(From update of attachment 113353 [details])
Clearing flags on attachment: 113353

Committed r99096: <http://trac.webkit.org/changeset/99096>
------- Comment #32 From 2011-11-02 14:05:34 PST -------
All reviewed patches have been landed.  Closing bug.