Bug 83281 - [EFL] Add setting API to enable/disable XSSAuditor
Summary: [EFL] Add setting API to enable/disable XSSAuditor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sudarsana Nagineni (babu)
URL:
Keywords:
: 83163 (view as bug list)
Depends on: 83030 84052
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-05 07:50 PDT by Sudarsana Nagineni (babu)
Modified: 2012-04-17 12:57 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.57 KB, patch)
2012-04-05 08:32 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (14.67 KB, patch)
2012-04-10 06:23 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (14.46 KB, patch)
2012-04-11 10:12 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (14.44 KB, patch)
2012-04-11 11:39 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (14.35 KB, patch)
2012-04-17 09:45 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2012-04-05 07:50:26 PDT
Add API to enable/disable XSSAuditor.
Comment 1 Sudarsana Nagineni (babu) 2012-04-05 08:32:10 PDT
Created attachment 135831 [details]
Patch

setting API to enable/disable XSSAuditor
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-04-05 12:16:52 PDT
To summarize what we discussed on #webkit-efl: please implement the FrameLoaderClientEfl::didDetectXSS side by dispatching a signal and explaining this in the documentation, otherwise this feature is only half-implemented.
Comment 3 Gyuyoung Kim 2012-04-05 22:13:34 PDT
Comment on attachment 135831 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:8
> +

Nowadays, some patches have added public APIs for LTC. In my opinion, we should review whether this new APIs are needed surely. It looks this APIs is able to be landed.
If you write functionality of this API in ChangeLog as well as in API description, it is a little better.
Comment 4 Sudarsana Nagineni (babu) 2012-04-06 03:53:35 PDT
(In reply to comment #3)
> Nowadays, some patches have added public APIs for LTC. In my opinion, we should review whether this new APIs are needed surely. It looks this APIs is able to be landed.
> If you write functionality of this API in ChangeLog as well as in API description, it is a little better.

Thanks for your review. I will update the changelog entry. 

As we discussed on #webkit-efl, the main purpose of adding these methods to enable/disable WebKit's XSSAuditor to protect from reflective cross-site scripting attacks.
Comment 5 Sudarsana Nagineni (babu) 2012-04-06 03:57:03 PDT
(In reply to comment #2)
> To summarize what we discussed on #webkit-efl: please implement the FrameLoaderClientEfl::didDetectXSS side by dispatching a signal and explaining this in the documentation, otherwise this feature is only half-implemented.

Should I add this signal dispatching in the same patch or separate bug? Looked around the failing test due to this missing implementation and noticed that the test is also dependent on dumpFrameLoadCallbacks (bug 81891). So, fixing didDetectXSS doesn't fully fix the test.

I think better to create a separate bug for signal dispatching, catching it in DRT and unskipping the test and make the bug dependent on these two bugs.
test: http/tests/security/xssAuditor/script-tag-with-callbacks.html
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-04-06 04:30:29 PDT
To me, it looks like a single feature being implemented, so I'd rather see it all in a single patch here (it also makes it easier to dig through the commit history later, and to roll back the patch in case it is needed).
Comment 7 Sudarsana Nagineni (babu) 2012-04-06 04:36:44 PDT
(In reply to comment #6)
> To me, it looks like a single feature being implemented, so I'd rather see it all in a single patch here (it also makes it easier to dig through the commit history later, and to roll back the patch in case it is needed).

Ok, I will update the patch.
Comment 8 Sudarsana Nagineni (babu) 2012-04-10 06:23:26 PDT
Created attachment 136444 [details]
Patch

Added all changes in a single patch here.
1. Added setting API to enable/disable XSSAuditor
2. Emits signal 'xss,detected' from FrameLoaderClientEfl::didDetectXSS 
3. Catch signal in DRT and print the expected output 'didDetectXSS' for script-tag-with-callbacks.html test
4. Added missing implementation setXSSAuditorEnabled to EFL's LayoutTestController and unskiped tests in http/tests/security/xssAuditor
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-04-10 22:13:43 PDT
Comment on attachment 136444 [details]
Patch

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

Looks almost there to me. My main grip here is related to what additional information is emitted with the frame and view signals: right now I don't know the URL that caused the issue or whether the whole page was blocked, and in the case of the view signal I only know that "something XSS-related happened", as I don't even know which frame it is related to.

> Source/WebKit/efl/ewk/ewk_view.h:2386
> + * The XSSAuditor (cross-site scripting protection) feature provides protection 
> + * from reflected XSS attacks on vulnerable web sites. This feature is enabled 
> + * by default.

It'd be good to expand this by explaining how XSS attacks are presented. There's no need for a detailed explanation, but from only looking at this paragraph I can't tell if requests are blocked, if the whole page is not loaded etc.
Comment 10 Sudarsana Nagineni (babu) 2012-04-11 10:12:22 PDT
Created attachment 136686 [details]
Patch

As discussed in #webkit-efl, providing additional information with the frame signal when XSSAutior notifies that XSS is encountered in the page.

This patch is dependent on 83030, so not requesting r/cq flags now.
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-04-11 10:29:29 PDT
Comment on attachment 136686 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:1787
> +void ewk_frame_xss_detected(Evas_Object* ewkFrame, const Ewk_Frame_Xss_Notification *xssInfo)

Style nit: inconsistent positioning of the asterisks.

> Source/WebKit/efl/ewk/ewk_view.h:2388
> + * from reflected XSS attacks on vulnerable web sites. It notifies FrameLoaderClient 
> + * with didDetectXSS when XSS is encountered in the page and provides additional 
> + * information on whether the entire page was blocked or only injected scripts were 
> + * removed. This feature is enabled by default.

Thanks for rephrasing this. Mentioning FLC and the method name is not necessary; when writing these documentation bits, try to think from the user's point of view: they are using webkit-efl in their program, but probably have no idea of how it works and have never heard of FrameLoaderClient and friends.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:108
> +    evas_object_smart_callback_add(mainFrame, "xss,detected", onDidDetectXSS, 0);

Don't you need to listen for this in all frames?
Comment 12 Sudarsana Nagineni (babu) 2012-04-11 10:42:08 PDT
(In reply to comment #11)
> > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:108
> > +    evas_object_smart_callback_add(mainFrame, "xss,detected", onDidDetectXSS, 0);
> 
> Don't you need to listen for this in all frames?
Already listing for all frames in onFrameCreated.
evas_object_smart_callback_add(frame, "xss,detected", onDidDetectXSS, 0);
Comment 13 Sudarsana Nagineni (babu) 2012-04-11 11:39:54 PDT
Created attachment 136713 [details]
Patch

Fixed review comments
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-04-11 11:43:41 PDT
Comment on attachment 136713 [details]
Patch

Informal r+ from my side.
Comment 15 Sudarsana Nagineni (babu) 2012-04-17 09:45:25 PDT
Created attachment 137550 [details]
Patch

rebased after r114386
Comment 16 Sudarsana Nagineni (babu) 2012-04-17 11:47:22 PDT
*** Bug 83163 has been marked as a duplicate of this bug. ***
Comment 17 WebKit Review Bot 2012-04-17 12:57:03 PDT
Comment on attachment 137550 [details]
Patch

Clearing flags on attachment: 137550

Committed r114419: <http://trac.webkit.org/changeset/114419>
Comment 18 WebKit Review Bot 2012-04-17 12:57:09 PDT
All reviewed patches have been landed.  Closing bug.