Add a test to ensure that obfuscated javascript: URLs don't allow XSS

• Component: DOM
• Status: ASSIGNED ---

[Pam Greene (IRC:pamg)]

Test that XSS using a javascript: URL is not allowed even when "javascript:" is obfuscated in various ways.

[Pam Greene (IRC:pamg)]

Created attachment 28508 [details]
New test + result

[Alexey Proskuryakov]

Comment on attachment 28508 [details]
New test + result

I don't think that this test needs to use js-test resources at all - it already prints FAIL on mistake, which is sufficient for both automated tests and manual inspection. Comparing "PASS" to "PASS" just to print "PASS" seems less than useful.

+        Also add appendTestDialogue() to js-test-pre.js for use by this and
+        other tests that need to wait until completion.

A typo, it's appendTestEpilogue. But I do not think that this is the right approach - the whole point of checking successfullyParsed is that it is performed from a separate script, so a failure to execute the test script doesn't affect it. But within the test script, it just won't be checked in failure case (which is when we want it to be checked).

This test doesn't do what it attempts to do, for two reasons. First, due to bug 8961, document.write("FAIL") doesn't write "FAIL" into the target, it only clears it. Second, the test performs frame navigation, which happens from a timer - so checking frame content synchronously doesn't work. Indeed, if it worked, then the last test (which uses a plain javascript: URL) would have failed.

It's confusing that the ChangeLog talks about XSS, but the frame being scripted is not from a different domain (especially after the first programmatic navigation). So, I'm not sure what the potential bug is - is it about bypassing packet filters, or bypassing engine security checks?

[Alexey Proskuryakov]

This looks rather similar to tests in http/tests/security/javaScriptURL, e.g. javascriptURL-execution-context-frame-location-htmldom.html

[Pam Greene (IRC:pamg)]

Created attachment 28558 [details]
Add cases to javascriptURL-execution-context-frame-location-htmldom.html instead

> A typo, it's appendTestEpilogue. But I do not think that this is the right
> approach - the whole point of checking successfullyParsed is that it is
> performed from a separate script, so a failure to execute the test script
> doesn't affect it.

Good point. I was trying for some consistency when appending these pieces manually, but if nothing else, the js-test-post.js file should duplicate the epilogue rather than calling the other file. And perhaps not even that. In any case, it no longer matters here.

> It's confusing that the ChangeLog talks about XSS, but the frame being scripted
> is not from a different domain (especially after the first programmatic
> navigation). So, I'm not sure what the potential bug is - is it about bypassing
> packet filters, or bypassing engine security checks?

The history I have available for this one is unfortunately very sparse, but it looks like it was added in response to the same problems that led to http/tests/security/javascriptURL/javascriptURL-execution-context-frame-location-htmldom.html and its associated fixes.  As you note, the tests are strikingly similar in a few ways.

So, I've simply added a few more test cases to that one.

[Pam Greene (IRC:pamg)]

To preempt questions: since location.href and location.replace() don't raise exceptions, the expected results don't change when these new cases are added.

[Alexey Proskuryakov]

Comment on attachment 28558 [details]
Add cases to javascriptURL-execution-context-frame-location-htmldom.html instead

Looking at the existing test more closely, I think that it doesn't work either: since frame navigation happens asynchronously, it is a no-op. The relevant bits of code are navigateIfAllowed() in JSLocationCustom.cpp and FrameLoader::scheduleLocationChange().

It's a pre-existing problem, but fixing it seems to be in scope for this bug. There should be enough time for navigation to take place asynchronously after each assignment, which is admittedly a bit tricky.

[Adam Barth]

Yeah, we need a setTimeout after each location set.  If you want to go faster, you can try using postMessage instead of setTimeout to trigger an async event.