Bug 70973 - XSSAuditor is silent
Summary: XSSAuditor is silent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Thomas Sepez
URL:
Keywords: XSSAuditor
Depends on:
Blocks:
 
Reported: 2011-10-26 15:41 PDT by Thomas Sepez
Modified: 2011-11-02 14:05 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Sepez 2011-10-26 15:41:23 PDT
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 Thomas Sepez 2011-10-27 10:56:06 PDT
Created attachment 112708 [details]
Semi-functional patch to discuss style points.
Comment 2 Adam Barth 2011-10-27 14:00:25 PDT
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
Comment 3 Alexey Proskuryakov 2011-10-27 14:07:14 PDT
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*
Comment 4 Thomas Sepez 2011-10-27 14:25:27 PDT
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.
Comment 5 Thomas Sepez 2011-10-27 17:45:51 PDT
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.
Comment 6 WebKit Review Bot 2011-10-27 17:48:25 PDT
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 WebKit Review Bot 2011-10-27 17:48:42 PDT
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 Adam Barth 2011-10-27 17:54:58 PDT
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.
Comment 9 Darin Fisher (:fishd, Google) 2011-10-27 21:18:30 PDT
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.
Comment 10 Adam Barth 2011-10-27 21:21:18 PDT
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 Thomas Sepez 2011-10-28 09:35:54 PDT
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 Adam Barth 2011-10-28 11:05:42 PDT
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 Thomas Sepez 2011-10-28 11:21:05 PDT
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 Adam Barth 2011-10-28 11:26:09 PDT
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 Thomas Sepez 2011-10-28 12:58:20 PDT
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.
Comment 16 Thomas Sepez 2011-10-28 12:58:44 PDT
Addressed comments, new patch coming shortly.
Comment 17 Thomas Sepez 2011-10-28 13:52:26 PDT
Created attachment 112905 [details]
Proposed Patch

Next revision.
Comment 18 Adam Barth 2011-10-28 14:23:02 PDT
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 Darin Fisher (:fishd, Google) 2011-10-30 22:40:24 PDT
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.
Comment 20 Thomas Sepez 2011-10-31 09:49:10 PDT
(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.
Comment 21 Thomas Sepez 2011-10-31 10:16:09 PDT
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.
Comment 22 Darin Fisher (:fishd, Google) 2011-11-02 11:34:53 PDT
Comment on attachment 113066 [details]
Proposed patch + improved boolean naming.

Chromium WebKit API changes LGTM
Comment 23 Adam Barth 2011-11-02 11:52:26 PDT
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 ?
Comment 24 Thomas Sepez 2011-11-02 12:01:55 PDT
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.
Comment 25 Thomas Sepez 2011-11-02 12:02:30 PDT
Adam, was there someone else we wanted from the webkit2 side?  Thanks.
Comment 26 Adam Barth 2011-11-02 12:20:39 PDT
> >> 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 Adam Barth 2011-11-02 12:52:34 PDT
Just for you Tom, I've edited your patch file to rename the variable.  :)
Comment 28 Adam Barth 2011-11-02 12:53:38 PDT
Created attachment 113353 [details]
Patch for landing
Comment 29 Thomas Sepez 2011-11-02 13:04:32 PDT
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 Adam Barth 2011-11-02 13:05:32 PDT
> 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 WebKit Review Bot 2011-11-02 14:05:27 PDT
Comment on attachment 113353 [details]
Patch for landing

Clearing flags on attachment: 113353

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