New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 113496 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocking:
issue 117592

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Links in settings page (like learn more, google dashboard) are opened in the webui renderer process

Project Member Reported by atwilson@chromium.org, Feb 9 2012

Issue description

What steps will reproduce the problem?
1. Bring up settings in chrome, go to the sync section.
2. Click on the google dashboard link. This opens the dashboard in a separate window.
3. Switch back to the settings page, try to click on stuff.

What is the expected output? What do you see instead?
Expected: that I should be able to interact with the settings page.
Actual: I can't. Looks like the google dashboard usurps the renderer process. I'm not sure what it's doing - maybe a synchronous XHR? I see the CPU go to 100% on that page for several seconds, so maybe it's just busy-waiting on something.

jochen: What's the google dashboard doing to block the renderer? Is this avoidable? This is really bad for other pages that get plopped in the same renderer process.
jhawkins: I believe it's dangerous for us to open random web pages (even "trusted" origins like www.google.com) inside the webui process because that renderer has special permissions. Can we open these pages in a separate process (maybe using the window.open().opener=null trick)?

Please use labels and text to provide additional information.

 
jochen: CPU issue logged as: b/5991519
it's a normal gwt page. is this new?
Tyler, can you find the bug that this is a dupe of?  It was a V8 issue, right?

Comment 4 by creis@chromium.org, Feb 9 2012

Cc: jhawkins@chromium.org nasko@chromium.org
Labels: -Pri-2 Pri-1
Owner: creis@chromium.org
Status: Assigned
Wait, why are we loading web pages in the same process as chrome://settings?  That's a serious bug.  We should recognize that it's a change from WebUI to a normal page and force a process swap.  I'll investigate.
Summary: Links in settings page (like learn more, google dashboard) are opened in the webui renderer process
 I think I am only seeing this slowdown on the dashboard page in a debug version of chrome. I bet it's just that GWT is generating a relatively heavy layout, and the debug version of webkit takes a long time to lay it out. I'll close out that part of the bug - sorry about the red herring.

Comment 6 by creis@chromium.org, Feb 9 2012

Labels: OS-All Internals-Core
Wow, we're carefully enforcing a process swap when we go to/from extensions or hosted apps but not WebUI pages.  Apparently we didn't usually hit this case because things like the NTP use a specific chrome.send call to navigate, rather than just a link or a window.open.

Glad you caught this.  I'll try to find the right place to enforce the process swap.
re comment 3, I think you're thinking of http://code.google.com/p/chromium/issues/detail?id=113320 which is a different, but related, issue.

Comment 8 by creis@chromium.org, Mar 8 2012

Labels: Restrict-View-SecurityTeam
Turns out this is indeed security relevant.  Having a WebUI check in RenderViewImpl::decidePolicyForNavigation would have been useful as an additional line of defense against  issue 117230 .  I'll get right on it.
Labels: -Type-Bug Type-Security SecSeverity-High Mstone-18

Comment 10 by creis@chromium.org, Mar 10 2012

Cc: est...@chromium.org
Blocking: 117592

Comment 12 by creis@chromium.org, Mar 10 2012

Cc: mpcomplete@chromium.org jam@chromium.org
We actually had logic for telling the browser to handle WebUI URLs and any navigations within a WebUI renderer, but it excluded two important cases: form submissions and links targeted to new windows.  We should not be excluding either of those cases, even though we don't yet support POSTs across renderer processes ( issue 101395 ).

CL up for review: https://chromiumcodereview.appspot.com/9663045/

Comment 13 by creis@chromium.org, Mar 12 2012

Status: Started
I should note that the CL is growing to be a larger (but better) patch, using HasWebUIScheme in a more consistent way across browser and renderer processes.  This larger CL might prevent us from merging it to M18, so I wanted to check if merging is a requirement.  If so, we might want to land a smaller CL first and then complete the rest.

Here's my concern: in the current state of affairs, following a link from a WebUI page to a new window (as in the original repro steps) causes a normal web page to load in a WebUI-enabled process.  If the user happens to follow links in this process and ends up at an attacker's page, then the attacker can exploit the process's privileges.  That seems bad for M18 to me.

Should I try to split this into a smaller CL (to close this hole and merge to M18) followed by a larger one (to refactor the HasWebUIScheme logic)?  Or should we punt this to M19 and just do one big CL?

(John, don't worry-- if I do the smaller CL, I'll leave the kChromeUIScheme in chrome rather than content, and then I'll fix the rest in the larger CL.)
Would 117418 have blocked Sergey's attack on its own? If so, I don't think we need to merge any more pieces back to M18.

Comment 15 by cdn@chromium.org, Mar 13 2012

Labels: SecImpacts-Stable SecImpacts-Beta
Project Member

Comment 16 by bugdroid1@chromium.org, Mar 15 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=126949

------------------------------------------------------------------------
r126949 | creis@chromium.org | Thu Mar 15 11:42:04 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/audio_renderer_impl_unittest.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_process.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/test_content_client.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/mock_render_process.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/shell_content_client.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/shell_content_client.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/mock_render_process.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_browsertest.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/test_content_client.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/chrome_content_renderer_client.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/mock_render_thread.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/chrome_content_client.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_impl.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_process_impl.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/common/content_client.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/tab_contents/tab_contents_unittest.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_process_impl.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/chrome_web_ui_controller_factory.h?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/render_view_test.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/tab_contents/render_view_host_manager_unittest.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/chrome_content_client.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_impl.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/webrtc_audio_device_test.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/site_instance_impl_unittest.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_debugger_api.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/mock_render_thread.cc?r1=126949&r2=126948&pathrev=126949
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/web_ui_controller_factory.h?r1=126949&r2=126948&pathrev=126949

Allow browser to handle all WebUI navigations.

BUG= 113496 
TEST="Google Dashboard" link in Sync settings loads in new process.


Review URL: http://codereview.chromium.org/9663045
------------------------------------------------------------------------

Comment 17 by creis@chromium.org, Mar 15 2012

Labels: -Mstone-18 Mstone-19
Status: Fixed
Comment 14: the fix for 117418 would have been enough to stop the exploit, so we can leave this as M19.
Labels: -SecSeverity-High SecSeverity-Low
Thanks, Charlie.
Very much agree on M19.

I've put the severity down to "Low" as it seems like the other fix is more significant. I'll probably bump that severity up to "Medium" unless anyone had any other suggestions for relative severities.
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Status: FixUnreleased
Labels: CVE-2011-3084
Labels: -Restrict-View-SecurityNotify
Status: Fixed
Project Member

Comment 22 by bugdroid1@chromium.org, Oct 13 2012

Blocking: -chromium:117592 chromium:117592
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 23 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-UI -Internals-Core -SecSeverity-Low -Mstone-19 -SecImpacts-Stable -SecImpacts-Beta Security-Severity-Low Security-Impact-Stable Security-Impact-Beta Cr-UI M-19 Cr-Internals-Core Type-Bug-Security
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-Low Security_Severity-Low
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 27 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 28 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 29 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 30 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Labels: CVE_description-submitted

Sign in to add a comment