Bug 29523

Summary: [XSSAuditor] JavaScript URLs that are URL-encoded twice can by bypass the XSSAuditor
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, mario.heiderich, sam
Priority: P2 Keywords: XSSAuditor
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://eaea.sirdarckcat.net/xss.php?html_xss=<iframe+src="javascript:'1%25251';alert(document.domain)">
Bug Depends on:    
Bug Blocks: 29278    
Attachments:
Description Flags
Patch with test cases
abarth: review-
Patch with test cases
none
Patch with test cases none

Description Daniel Bates 2009-09-19 14:50:47 PDT
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.
Comment 1 Daniel Bates 2009-09-19 14:52:53 PDT
Created attachment 39824 [details]
Patch with test cases
Comment 2 Adam Barth 2009-09-19 15:41:45 PDT
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?
Comment 3 Daniel Bates 2009-09-19 16:15:07 PDT
(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
Comment 4 Daniel Bates 2009-09-19 16:15:50 PDT
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
Comment 5 Daniel Bates 2009-09-19 16:17:22 PDT
Created attachment 39826 [details]
Patch with test cases

Cleaned up ScriptController.
Comment 6 Adam Barth 2009-09-19 17:46:17 PDT
> 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?
Comment 7 Daniel Bates 2009-09-19 17:55:38 PDT
Created attachment 39828 [details]
Patch with test cases

On Adam's remarks, removed checks m_frame->script()->isEnabled(),  m_frame->script()->isPaused() from patch
Comment 8 Adam Barth 2009-09-19 17:57:40 PDT
Comment on attachment 39828 [details]
Patch with test cases

Awesome.  Thanks Dan.
Comment 9 Daniel Bates 2009-09-22 20:14:30 PDT
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  <dbates@webkit.org>\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  <dbates@webkit.org>\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
Comment 10 Daniel Bates 2009-09-23 11:21:17 PDT
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.
Comment 11 Daniel Bates 2009-09-23 11:22:07 PDT
Comment on attachment 39828 [details]
Patch with test cases

Clearing review and commit flags