Bug 29523 – [XSSAuditor] JavaScript URLs that are URL-encoded twice can by bypass the XSSAuditor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

The method FrameLoader::executeIfJavaScriptURL decodes the URL escape sequences in a JavaScript URL before it is eventually passed to the XSSAuditor. Because the XSSAuditor also decodes the URL escape sequences as part of its canonicalization, the double decoding of a JavaScript URL would not match the canonicalization of the input parameters.

[Daniel Bates]

Created attachment 39824 [details]
Patch with test cases

[Adam Barth]

Comment on attachment 39824 [details]
Patch with test cases

+ m_frame->script()->isEnabled() && !m_frame->script()->isPaused()

Why did we add these conditions that weren't there before?

Can we remove any of the other instances of canEvaluateJavaScriptURL?

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 39824 [details])
> + m_frame->script()->isEnabled() && !m_frame->script()->isPaused()
> 
> Why did we add these conditions that weren't there before?

This is an optimization.

I added these so that we can avoid calling the XSSAuditor when scripts aren't enabled or paused. Notice, these cases are checked in FrameLoader::executeScript and at present (i.e. without this patch) the XSSAuditor is only called after these cases are checked.

Because we now call the XSSAuditor in FrameLoader::executeIfJavaScriptURL, in particular before calling executeScript, we can save some processing time/function call, by only calling the XSSAuditor when scripts are enabled and not paused.

> Can we remove any of the other instances of canEvaluateJavaScriptURL?

Yes, I can clean up ScriptSourceController

[Daniel Bates]

I meant, cleanup ScriptController.

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 39824 [details] [details])
> > + m_frame->script()->isEnabled() && !m_frame->script()->isPaused()
> > 
> > Why did we add these conditions that weren't there before?
> 
> This is an optimization.
> 
> I added these so that we can avoid calling the XSSAuditor when scripts aren't
> enabled or paused. Notice, these cases are checked in
> FrameLoader::executeScript and at present (i.e. without this patch) the
> XSSAuditor is only called after these cases are checked.
> 
> Because we now call the XSSAuditor in FrameLoader::executeIfJavaScriptURL, in
> particular before calling executeScript, we can save some processing
> time/function call, by only calling the XSSAuditor when scripts are enabled and
> not paused.
> 
> > Can we remove any of the other instances of canEvaluateJavaScriptURL?
> 
> Yes, I can clean up ScriptSourceController

[Daniel Bates]

Created attachment 39826 [details]
Patch with test cases

Cleaned up ScriptController.

[Adam Barth]

> This is an optimization.

I think this optimization might be slightly premature.  Those cases aren't very common, and the added complexity probably isn't worth it.

Other than that, the patch looks good.  Can you post a version without these checks?

[Daniel Bates]

Created attachment 39828 [details]
Patch with test cases

On Adam's remarks, removed checks m_frame->script()->isEnabled(),  m_frame->script()->isPaused() from patch

[Adam Barth]

Comment on attachment 39828 [details]
Patch with test cases

Awesome.  Thanks Dan.

[Daniel Bates]

Comment on attachment 39828 [details]
Patch with test cases

Rejecting patch 39828 from commit-queue.

This patch will require manual commit. Failed to run "['svn', 'commit', '-m', '2009-09-22  Daniel Bates  <[e-mail]>\n\n        Reviewed by Adam Barth.\n\n        https://bugs.webkit.org/show_bug.cgi?id=29523\n        \n        Fixes an issue where a JavaScript URL that was URL-encoded twice can bypass the\n        XSSAuditor.\n        \n        The method FrameLoader::executeIfJavaScriptURL decodes the URL escape \n        sequences in a JavaScript URL before it is eventually passed to the XSSAuditor.\n        Because the XSSAuditor also decodes the URL escape sequences as part of its\n        canonicalization, the double decoding of a JavaScript URL would\n        not match the canonicalization of the input parameters.\n\n        Tests: http/tests/security/xssAuditor/iframe-javascript-url-url-encoded.html\n               http/tests/security/xssAuditor/javascript-link-url-encoded.html\n\n        * bindings/js/ScriptController.cpp:\n        (WebCore::ScriptController::evaluate): Moved call to \n        XSSAuditor::canEvaluateJavaScriptURL into FrameLoader::executeIfJavaScriptURL.\n        * bindings/v8/ScriptController.cpp:\n        (WebCore::ScriptController::evaluate): Ditto.\n        * loader/FrameLoader.cpp:\n        (WebCore::FrameLoader::executeIfJavaScriptURL): Modified to call \n        XSSAuditor::canEvaluateJavaScriptURL on the JavaScript URL before it is\n        decoded.\n2009-09-22  Daniel Bates  <[e-mail]>\n\n        Reviewed by Adam Barth.\n\n        https://bugs.webkit.org/show_bug.cgi?id=29523\n        \n        Tests that JavaScript URLs that were URL-encoded twice do not bypass the XSSAuditor.\n\n        * http/tests/security/xssAuditor/iframe-javascript-url-url-encoded-expected.txt: Added.\n        * http/tests/security/xssAuditor/iframe-javascript-url-url-encoded.html: Added.\n        * http/tests/security/xssAuditor/javascript-link-url-encoded-expected.txt: Added.\n        * http/tests/security/xssAuditor/javascript-link-url-encoded.html: Added.\n']"  exit_code: 1  cwd: None

[Daniel Bates]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/http/tests/security/xssAuditor/iframe-javascript-url-url-encoded-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/iframe-javascript-url-url-encoded.html
Adding         LayoutTests/http/tests/security/xssAuditor/javascript-link-url-encoded-expected.txt
Adding         LayoutTests/http/tests/security/xssAuditor/javascript-link-url-encoded.html
Sending        WebCore/ChangeLog
Sending        WebCore/bindings/js/ScriptController.cpp
Sending        WebCore/bindings/v8/ScriptController.cpp
Sending        WebCore/loader/FrameLoader.cpp
Transmitting file data .........
Committed revision 48680.

[Daniel Bates]

Comment on attachment 39828 [details]
Patch with test cases

Clearing review and commit flags