Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Mar 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
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 Back to list
 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
Sign in to add a comment