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 0 users
Status: Fixed
Owner:
Closed: Dec 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: memory address disclosure through JavaScript in WebUI's cookies page
Project Member Reported by nasko@chromium.org, Jun 25 2012 Back to list
VULNERABILITY DETAILS
The cookies UI contains two pieces - C++ objects to access the cookie jar and JavaScript that renders it in WebUI. The issue is that tree nodes are identified using their memory address, converted to string, and passed to the JavaScript running in the WebUI renderer. The relevant functions are:

GetChildNodeList - http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/cookies_tree_model_util.cc&exact_package=chromium&q=GetChildNodeList&ct=rc&cd=2&sq=&l=264
GetTreeNodeFromPath - http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/cookies_tree_model_util.cc&exact_package=chromium&q=GetTreeNodeFromPath&ct=rc&cd=1&sq=&l=276


VERSION
Chrome Version: All versions for at least the last 1.25 years (http://codereview.chromium.org/6611030)
Operating System: Any OS

REPRODUCTION CASE
Open the cookies UI, launch the developer tools, set a breakpoint on CookiesView.loadChildren. Observe the contents of args[1] is an array of objects. The id property of each object is the hex version of the address of the object in the browser process.

Modifying the values in JS to be invalid doesn't crash the browser process, as the TreeNode's GetIndexOf method is doing a lookup of the pointer into a vector of child nodes. It is still a disclosure of heap addresses in the browser process to the renderer process.
 
Comment 1 by tsepez@chromium.org, Jun 25 2012
Disclosing addresses to JS, even if only to a privileged WebUI page seems bad, given what we've recently seen folks do with WebUI in pwnium. Looking at the code, having a function called "PointerToHexString()" under /webui seems like it should be a red flag. This code shows up in certificate_manger_handler2.cc as well.  
Comment 2 by palmer@chromium.org, Jun 25 2012
Labels: -Pri-0 -Area-Undefined Pri-2 Area-Internals OS-All SecSeverity-Medium SecImpacts-Stable SecImpacts-Beta Mstone-21
Status: Available
FYI, the CookiesView.loadChildren function is in settings-frame, options_bundle.js.
Comment 3 by tsepez@chromium.org, Jun 25 2012
Cc: est...@chromium.org
Comment 4 by palmer@chromium.org, Jun 25 2012
Another way to see the problem live, thanks to Nasko, is to break at line 8483:

        chrome.send('loadCookie', [this.pathId]);

and print this.data.id.
Comment 5 by palmer@chromium.org, Jun 25 2012
Cc: xiy...@chromium.org
Adding xiyuan, author of GetChildNodeList and GetTreeNodeFromPath. xiyuan, can you use some other distinct ID number to identify the cookies? We don't want to leak specific heap addresses to JavaScript, because it can help attackers learn details they need to exploit vulnerabilities. Thank you!
Comment 6 by xiy...@chromium.org, Jun 25 2012
Cc: -xiy...@chromium.org
Owner: xiy...@chromium.org
Comment 7 by tsepez@chromium.org, Jun 25 2012
If I'm reading this right, then this is quite bad.

CookiesViewHandler::RegisterMessages makes a binding between the WebUI JS environment and the CookiesViewHandler::Remove() method, so given compromised WebUI JS, we can pass any value to it. CookiesViewHandler::Remove() takes its argument, decodes its hex value, casts it to a pointer, and invokes CookiesTreeModel::DeleteCookieNode with it as its argument.  CookiesTreeModel::DeleteCookieNode() invokes methods off this pointer, and deletes it (or some value derived from it).  I'd expect all sorts of mischief to be possible at this point.

In short, it looks like you're passing an arbitrary unchecked pointer across an IPC.  This can't happen.  It will need to change to be a key used to lookup the object in some safe manner.

Comment 8 by tsepez@chromium.org, Jun 25 2012
Labels: -SecSeverity-Medium SecSeverity-High
Comment 9 by tsepez@chromium.org, Jun 25 2012
Cc: cevans@chromium.org jsc...@chromium.org
Comment 10 by nasko@chromium.org, Jun 25 2012
The pointer does get "checked" though. If you look at GetTreeNodeFromPath, it uses parent->GetIndexOf(child), which internally does a lookup into a vector<TreeNode*>. If the value of the pointer is to random location, it will not be found in the vector and the GetTreeNodeFromPath method will return NULL. The CookiesViewHandler::Remove() won't call DeketeCookieNode if the node is NULL.

I do agree, though, that the identifier should not be the raw memory location, but some other unique identifier that doesn't leak memory layout information.
Labels: -SecSeverity-High SecSeverity-Medium
Ah. Thanks.
Status: Started
Re #C7:  the usage of this same technique in certificate_manager_handler2.cc looks to be "unchecked".   Should we track that issue here or spin off a new bug? 
I'll clean that up too.
Labels: -SecSeverity-Medium SecSeverity-High
Per #c13, severity high and consider merging down the road.
Project Member Comment 17 by bugdroid1@chromium.org, Jul 2 2012
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: FixUnreleased
Cc: nasko@chromium.org
Is this really SecSeverity-High?
Do you need access to a renderer process tagged with the WebUI permission in order to exploit this?
Project Member Comment 20 by bugdroid1@chromium.org, Jul 10 2012
Labels: merge-merged-1180
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=145934

------------------------------------------------------------------------
r145934 | cevans@chromium.org | Tue Jul 10 12:53:29 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/webui/options2/cookies_view_handler2.h?r1=145934&r2=145933&pathrev=145934
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/webui/options2/certificate_manager_handler2.cc?r1=145934&r2=145933&pathrev=145934
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/webui/cookies_tree_model_util.h?r1=145934&r2=145933&pathrev=145934
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/webui/cookies_tree_model_util.cc?r1=145934&r2=145933&pathrev=145934
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/webui/options2/cookies_view_handler2.cc?r1=145934&r2=145933&pathrev=145934
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/ui/webui/options2/certificate_manager_handler2.h?r1=145934&r2=145933&pathrev=145934

Merge 145197 - clean-up: Use an id instead of memory pointer string in webui.

Clean up cookie and cert webui to use id instead of memory pointer hex
string.

BUG= 134519 
TEST=Verify cookie and cert webui is not using memory pointer hex value as js object id.


Review URL: https://chromiumcodereview.appspot.com/10701029

TBR=xiyuan@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10703127
------------------------------------------------------------------------
Labels: -Merge-Approved Merge-Merged
I merged it because it's a trivial merge, but we do want to tag the severity correctly as always :)
Project Member Comment 22 by bugdroid1@chromium.org, Jul 16 2012
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=146920

------------------------------------------------------------------------
r146920 | mattm@chromium.org | Mon Jul 16 16:08:09 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/options2/certificate_manager_browsertest.js?r1=146920&r2=146919&pathrev=146920
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/options2/certificate_tree.js?r1=146920&r2=146919&pathrev=146920
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/options2/certificate_manager.js?r1=146920&r2=146919&pathrev=146920

Fix certificate manager buttons never becoming clickable.

Add some simple tests.

BUG= 134519 , 136864 
TEST=open cert manager, select a cert, try to view/delete/etc

Review URL: https://chromiumcodereview.appspot.com/10700195
------------------------------------------------------------------------
Project Member Comment 23 by bugdroid1@chromium.org, Jul 24 2012
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=148230

------------------------------------------------------------------------
r148230 | gspencer@chromium.org | 2012-07-24T22:34:56.579212Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/resources/options2/certificate_manager.js?r1=148230&r2=148229&pathrev=148230
   M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/browser/resources/options2/certificate_tree.js?r1=148230&r2=148229&pathrev=148230

Merge 146920 - Fix certificate manager buttons never becoming clickable.

MERGE NOTE: This patch was merged manually, not using drover because
the original patch included a change to the matching browser test that
didn't exist when M21 was branched.  The patch is otherwise unchanged.

Add some simple tests.

TBR=mattm@chromium.org
BUG= 134519 , 136864 
TEST=open cert manager, select a cert, try to view/delete/etc
Review URL: https://chromiumcodereview.appspot.com/10808114
------------------------------------------------------------------------
Labels: -SecSeverity-High SecSeverity-Low CVE-2012-2854
Project Member Comment 25 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.
Status: Fixed
Cc: jiayl@chromium.org
Project Member Comment 28 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Type-Security -Area-Internals -SecSeverity-Low -SecImpacts-Stable -SecImpacts-Beta -Mstone-21 Security-Severity-Low Security-Impact-Stable Security-Impact-Beta M-21 Cr-Internals Type-Bug-Security
Project Member Comment 29 by bugdroid1@chromium.org, Mar 13 2013
Labels: Restrict-View-EditIssue
Project Member Comment 30 by bugdroid1@chromium.org, Mar 14 2013
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Project Member Comment 32 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-Low Security_Severity-Low
Project Member Comment 33 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 34 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member Comment 35 by sheriffbot@chromium.org, Jun 14 2016
Labels: -security_impact-beta
Project Member Comment 36 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 37 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