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

Issue 117418 link

Starred by 1 user

Issue metadata

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

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Security: Don't grant WebUI bindings to a process shared with normal views

Project Member Reported by creis@chromium.org, Mar 8 2012

Issue description

 Issue 117230  showed a case that RenderViewHost::AllowBindings is granting WebUI bindings to a newly created RenderViewHost that happens to share a process with normal web views.  This escalates their privileges in an unsafe way.

We should avoid granting privileges if the process has already been shared with normal web views.
 
Labels: Mstone-18 SecImpacts-Stable SecImpacts-Beta SecSeverity-High

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

Cc: jam@chromium.org
I've started a CL for this one here:
https://chromiumcodereview.appspot.com/9660001/

Unfortunately, it's looking non-trivial to determine if the process is shared or not, at least in the tests.  (It behaves ok in actual builds and blocks the exploit.)  I'm trying to use RenderProcessHost's ListenersIterator, but it's quite unhappy about acquiring the lock on whatever thread the unit tests are on.  For example, in WebUITest.WebUIToStandard:

[18561:18561:0308/154911:3822783498939:FATAL:lock_impl_posix.cc(45)] Check failed: rv == 0 (22 vs. 0)
Backtrace:
        base::debug::StackTrace::StackTrace() [0x7ffff370baca]
        logging::LogMessage::~LogMessage() [0x7ffff373c69c]
        base::internal::LockImpl::Lock() [0x7ffff378afce]
        base::Lock::Acquire() [0x7ffff36e42b4]
        base::AutoLock::AutoLock() [0x7ffff36e4316]
        base::ThreadCheckerImpl::EnsureThreadIdAssigned() [0x7ffff37a854e]
        base::ThreadCheckerImpl::CalledOnValidThread() [0x7ffff37a84bd]
        base::NonThreadSafeImpl::CalledOnValidThread() [0x7ffff379b804]
        _ZN5IDMapIN3IPC7Channel8ListenerEL23IDMapOwnershipSemantics0EE8IteratorIKS2_E7AdvanceEv [0x7ffff5f96fd7]
        content::RenderViewHostImpl::AllowBindings() [0x7ffff5f8e7d7]

John, do you have ideas about a safer way to do this?  We were discussing the need for an IsSharedByMultipleViews() API in the context of Nasko's CL as well.

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

Status: Fixed
The test issue I was seeing in comment 2 was just because MockRenderProcessHost wasn't handling listeners_ properly.  Fix landed.
Labels: -Restrict-View-SecurityTeam -Mstone-18 -SecSeverity-High Restrict-View-SecurityNotify Mstone-19 SecSeverity-Low
Status: FixUnreleased
@creis: presumably, we'd be insane to merge this to M18? I've speculatively marked as Mstone-19

I've marked this secseverity=low because this is a hardening measure. Let me know if that seems wrong.

Thanks so much for your continued attention on these hardening measures.

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

@scarybeasts: Actually, merging this to M18 might not be such a bad idea.  In terms of the hardening measures being considered, this one seems like it has the highest impact-- it prevents the key moment of granting bindings to a process that might have an attack page in it.  Even if an attack can get around the other measures we're working on, it can't do much without these bindings.

I also don't see much risk with this CL, since I can't think of any safe ways to load a WebUI page into an already used but unprivileged process.  At worst, the WebUI page wouldn't function, and if we heard reports of that, we'd have a lead on another potential attack.

Thoughts?

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

Labels: -Mstone-19 Mstone-18 Merge-Approved
Discussed offline, and we agreed this is low risk and high benefit.  It's worth merging to M18.

I'll see about adding a unit test to prevent regression as well.
Labels: -Merge-Approved Merge-Merged
M18: r126271

Project Member

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

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

------------------------------------------------------------------------
r126929 | creis@chromium.org | Thu Mar 15 10:06:00 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/render_view_host_unittest.cc?r1=126929&r2=126928&pathrev=126929

Add unit test for AllowBindings check.

BUG= 117418 
TEST=none


Review URL: http://codereview.chromium.org/9701038
------------------------------------------------------------------------
Labels: -SecSeverity-Low SecSeverity-Medium
This fix erects a stronger wall between renderers handling web content and renderers handling webui content. Accordingly, the Sergey Pwnium exploit is blocked by this defensive measure.
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 20 2012

Labels: merge-merged-963
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=127803

------------------------------------------------------------------------
r127803 | cevans@chromium.org | Tue Mar 20 14:56:38 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/963/src/content/browser/renderer_host/mock_render_process_host.cc?r1=127803&r2=127802&pathrev=127803
 M http://src.chromium.org/viewvc/chrome/branches/963/src/content/browser/renderer_host/render_view_host.cc?r1=127803&r2=127802&pathrev=127803
 M http://src.chromium.org/viewvc/chrome/branches/963/src/content/renderer/render_view_impl.cc?r1=127803&r2=127802&pathrev=127803

Apply fix for 117418 to M17.
Note that I don't see tab_contents_unittest on M17 so that file is omitted.

BUG= 117418 
Review URL: https://chromiumcodereview.appspot.com/9773003
------------------------------------------------------------------------
Labels: -Mstone-18 -SecSeverity-Medium Mstone-17 SecSeverity-Low
Labels: CVE-2011-3054
Labels: pwnium

Comment 17 by cdn@chromium.org, May 15 2012

Status: Fixed
Marking old security bugs Fixed..
Labels: -Restrict-View-SecurityNotify
Project Member

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

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 20 by bugdroid1@chromium.org, Mar 10 2013

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

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

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

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

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

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

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

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

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

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

Labels: -security_impact-beta
Project Member

Comment 26 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 27 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
Project Member

Comment 30 by sheriffbot@chromium.org, Jul 29

Labels: -Pri-1 Pri-2

Sign in to add a comment