Bug 24533 - Add a test to ensure that obfuscated javascript: URLs don't allow XSS
Summary: Add a test to ensure that obfuscated javascript: URLs don't allow XSS
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pam Greene (IRC:pamg)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-11 17:53 PDT by Pam Greene (IRC:pamg)
Modified: 2010-08-16 00:11 PDT (History)
2 users (show)

See Also:


Attachments
New test + result (4.49 KB, patch)
2009-03-11 18:00 PDT, Pam Greene (IRC:pamg)
ap: review-
Details | Formatted Diff | Diff
Add cases to javascriptURL-execution-context-frame-location-htmldom.html instead (2.45 KB, patch)
2009-03-12 14:47 PDT, Pam Greene (IRC:pamg)
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pam Greene (IRC:pamg) 2009-03-11 17:53:49 PDT
Test that XSS using a javascript: URL is not allowed even when "javascript:" is obfuscated in various ways.
Comment 1 Pam Greene (IRC:pamg) 2009-03-11 18:00:31 PDT
Created attachment 28508 [details]
New test + result
Comment 2 Alexey Proskuryakov 2009-03-12 00:38:20 PDT
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?
Comment 3 Alexey Proskuryakov 2009-03-12 04:36:13 PDT
This looks rather similar to tests in http/tests/security/javaScriptURL, e.g. javascriptURL-execution-context-frame-location-htmldom.html
Comment 4 Pam Greene (IRC:pamg) 2009-03-12 14:47:17 PDT
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.
Comment 5 Pam Greene (IRC:pamg) 2009-03-12 14:50:24 PDT
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.
Comment 6 Alexey Proskuryakov 2009-03-13 00:52:30 PDT
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.
Comment 7 Adam Barth 2010-08-16 00:11:37 PDT
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.