Bug 66720 – chromium WebFrameImpl - don't load javascript URLs against chrome internal pages.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Thomas Sepez]

Running JS against a chrome:// page risks sensitive information.  Add a safeguard against malicious bookmarklets and the like to WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp

[Thomas Sepez]

Created attachment 104751 [details]
Patch

[Adam Barth]

Comment on attachment 104751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104751&action=review

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2291
> +    // Protect privileged pages against bookmarklets and other javascript
> +    // manipulations.
> +    if (m_frame->document()->url().protocolIs("chrome")
> +        || m_frame->document()->url().protocolIs("chrome-extension"))
> +        return;

We shouldn't have these string literals in this code.  Isn't there something in SchemeRegistry we could use?

[Thomas Sepez]

Didn't see an appropriate shouldTreatScheme... method in SchemeRegistry.h.  Also didn't see the literal "chrome" anywhere under webkit :( ... would this be added to a SchemeRegistry category outside of webkit by the browser itself?

eg,
$ find . -name \*.h -exec grep '"chrome' {} /dev/null \;
./Source/WebKit/chromium/base/logging.h://     "chromeos" directory.
$ find . -name \*.cpp -exec grep '"chrome' {} /dev/null \;
./Source/WebKit/chromium/src/BackForwardListChromium.cpp:const char backForwardNavigationScheme[] = "chrome-back-forward";
./Source/WebKit/chromium/src/WebFrameImpl.cpp:    if (m_frame->document()->url().protocolIs("chrome")
./Source/WebKit/chromium/src/WebFrameImpl.cpp:        || m_frame->document()->url().protocolIs("chrome-extension"))
./Source/WebKit/chromium/tests/WebFrameTest.cpp:          chromeURL("chrome://"

[Adam Barth]

You can add a new type to SchemeRegistry.h.  Chrome registers a bunch of schemes at with SchemeRegistry when it initializes WebKit.

[Thomas Sepez]

Created attachment 104770 [details]
Patch

[Thomas Sepez]

Ok, but feels kind of wrong since only the chromium code path checks this particular registry.

[Thomas Sepez]

Probably needs to get wrapped in a WebSecurityPolicy method ???

[Adam Barth]

> Ok, but feels kind of wrong since only the chromium code path checks this particular registry.

Yeah, you should probably check this in ScriptController::executeIfJavaScriptURL also.

> Probably needs to get wrapped in a WebSecurityPolicy method ???

Yep!

[Thomas Sepez]

Created attachment 104857 [details]
Patch

[Thomas Sepez]

would ScriptController::executeIfJavaScriptURL affect <a href="javascript:"> cases, or only navigation from the browser itself?   Current patch only applies this test as before.

[Thomas Sepez]

Created attachment 104860 [details]
Patch

[Thomas Sepez]

Patch with two ChangeLogs.  For review.  Thanks.

[Adam Barth]

Comment on attachment 104860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104860&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line will prevent this change from landing.  You should replace it with a line that says you wrote a Chromium WebKit API unit test.

> Source/WebCore/ChangeLog:9
> +

It would be nice to put some more explanation in the ChangeLog as to why we're making this change and whether we think this registry is useful for WebCore's JavaScript URL implementation.

> Source/WebCore/platform/SchemeRegistry.cpp:107
> +static URLSchemesMap& notAllowingJavascriptURLsSchemes()

This name seems slightly tortured, but I can see how it fits the pattern.

[Adam Barth]

> would ScriptController::executeIfJavaScriptURL affect <a href="javascript:"> cases, or only navigation from the browser itself?   Current patch only applies this test as before.

Only the <a href="javascript:"> case.  I think it's fine to handle that case in a separate patch, but we should say that in the ChangeLog entry.

[Adam Barth]

+fishd for WebKit API review.

[Thomas Sepez]

Created attachment 105013 [details]
Patch

[Thomas Sepez]

Reworked comments in change log in latest patch.

[WebKit Review Bot]

Comment on attachment 105013 [details]
Patch

Rejecting attachment 105013 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
at 63 (offset 3 lines).
Hunk #4 succeeded at 98 (offset 3 lines).
Hunk #5 succeeded at 119 (offset 3 lines).
Hunk #6 succeeded at 145 (offset 3 lines).
Hunk #7 succeeded at 191 with fuzz 1 (offset 24 lines).
1 out of 7 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/WebFrameTest.cpp.rej
patching file Source/WebKit/chromium/tests/data/history.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/9485272

[Thomas Sepez]

Created attachment 105033 [details]
Patch

[Thomas Sepez]

Comment on attachment 105033 [details]
Patch

Merge conflict resolved.

[Adam Barth]

Comment on attachment 105033 [details]
Patch

Forwarding fishd's r+.

[WebKit Review Bot]

Comment on attachment 105033 [details]
Patch

Clearing flags on attachment: 105033

Committed r93734: <http://trac.webkit.org/changeset/93734>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.