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 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Security



Sign in to add a comment
link

Issue 168442: Security: Non-privileged extensions can monitor browsing activity via chrome.tabs.onUpdated events

Reported by mvrable@google.com, Jan 5 2013 Project Member

Issue description

VULNERABILITY DETAILS
The chrome.tabs.onUpdated event is accessible to Chrome extensions even without the "tabs" permission, and leaks the URLs the user navigates to in the changeInfo.url argument to the event callback.

Much (all?) of the tabs API is accessible to unprivileged extensions, beyond the whitelisted set listed in the documentation (create, update, remove, and onRemoved).  Some functions (e.g., query) run without the tabs permission but return sanitized data, but at least the onUpdated event lacks sanitization and leaks the URL.

VERSION
Chrome Version: tested on both "24.0.1312.45 beta" and SVN 174805
Operating System: Ubuntu 12.04 LTS

REPRODUCTION CASE
An extension which includes the code (running in the background page):
  chrome.tabs.onUpdated.addListener(
      function(tabId, changeInfo, tab) {
        logVisitedURLs(changeInfo.url);
      });
will be notified of URLs visited by the user.

NOTES
It's not clear to me what the intended behavior is.  While the documentation states that only a small set of functions are available without the tabs behavior, the code (for example, the query function and the ExtensionTabUtil::CreateTabValue method in chrome/browser/extensions/extension_tab_util.cc make it look like an attempt was made to broaden the API available to unprivileged extension.  However, extensions::BrowserEventRouter::TabUpdated() is missing any similar type of permission check, and so passes the URL even when the tabs permission is not present.  The onUpdated event should have permissions checks added (and other parts of the API should be audited), or the tabs API exported to extensions should be restricted.  The documentation may need updating too.
 

Comment 1 by jsc...@chromium.org, Jan 5 2013

Cc: mea...@chromium.org mpcomplete@chromium.org
Labels: -Pri-0 -Area-Undefined Area-Internals Feature-Extensions
Would one of the CC's mind handling triage here?

Comment 2 by mea...@chromium.org, Jan 7 2013

Labels: SecSeverity-Low
Status: Available
Looks similar to 54006: Accessing history/browsing activity without requiring any permission warnings. I'll find out how many extensions use chrome.tabs.onUpdated before adding any permission checks.

Comment 3 by mpcomplete@chromium.org, Jan 7 2013

Cc: kalman@chromium.org
Is this a regression? We've slowly added certain tabs events/functions to the whitelist of things that don't need the tabs permission. I'm not sure whether onUpdated was added to this list purposely or not.

At the very least, we should be sanitizing the data sent to onUpdated.

Comment 4 by kalman@chromium.org, Jan 7 2013

I would need to check this more deeply, but I believe that what mvrable says in the "notes" section is exactly right. We need to update the documentation to say that all tabs functions are available, and we need to add a permission check for the event.

Comment 5 by mea...@chromium.org, Jan 7 2013

The number of extensions using chrome.tabs.onUpdated without having a tabs permission is small. Adding the permissions check will not have a significant impact.

https://docs.google.com/a/google.com/spreadsheet/ccc?key=0Ap9jGhxN6UpadE9tRzJyVDhfbF8xV0R5VHd2d2NoSHc

Comment 6 by jsc...@chromium.org, Jan 8 2013

Do we grant access to browsing history without any permissions? Because that's effectively what this allows, and it seems like we shouldn't be doing that without a permission.

Comment 7 by kalman@chromium.org, Jan 8 2013

Well it depends what you define as "browsing history". Without tabs, this would let extensions know when tabs are updated... but not actually any sensitive data about them (it would, however, let them do things like close tabs that are opened).

However, we already allow this sort of stuff without the tabs permission, it seems relatively harmless.

Comment 8 by mvrable@google.com, Jan 8 2013

kalman: Are you referring to the current behavior or the intended behavior?  The current behavior is, without the tabs permission, extensions can learn when tabs are updated *and* learn the URLs that are visited.  I'd consider the URL to be "sensitive data".

I think the intended behavior ought to be that extensions can at most learn when tabs are updated without learning the URL.  Right now the third argument to the OnUpdated callback (tab) has this sensitive data scrubbed out when the extension lacks the tabs permission, but the second argument (changeInfo) is not scrubbed.

Comment 9 by kalman@chromium.org, Jan 8 2013

Yes I mean intended behaviour.

Comment 10 by mvrable@google.com, Jan 8 2013

Perhaps some change along those in the attached patch: if the extension does not have the tabs permission, then clear the url property out of the changeInfo dictionary.  This is about the minimal change possible (with regards to behavior) that still provides the privacy properties we want.

Compiles and runs for me, seeming to do the right thing, but I haven't given it much testing.
tabs-events.patch
2.7 KB View Download

Comment 11 by jsc...@chromium.org, Jan 8 2013

Was anyone on security or privacy consulted on this behavior change, because I'm very uncomfortable with it? I think it's arguably reasonable that an extension could know a tab is updated without requiring additional permission. However, an extension should require some permission to know the URL of tabs, since that information effectively allows the extension to reconstruct the user's history from the time it's installed. By analogy, if I frame a page on the Web I can set the src and get onload events for navigations of that frame. However, I can't read back that frame's src attribute to get later navigations or redirects after I set the src value.

Comment 12 by kalman@chromium.org, Jan 8 2013

"However, an extension should require some permission to know the URL of tabs" - this is the bug being discussed here, I think? Am I misunderstanding?

I'm just saying the behaviour *should* be that the URL and other sensitive data should be stripped out. The fact that the URL isn't is a bug. However, the fact that the event is exposed is a feature.

Comment 13 by mvrable@google.com, Jan 8 2013

Updated patch is attached: this one adds a generic ExtensionTabUtil::ScrubTabValue method, though only onUpdate needs it for now.

If this (or something like it) seems reasonable, I can submit it for review--though is there a special process I need to go through since this is a security bug (restricted visibility)?
tabs-events-2.patch
4.8 KB View Download

Comment 14 by kalman@chromium.org, Jan 8 2013

Uploading for review SGTM, let's discuss code in the review.

Comment 15 by mvrable@google.com, Jan 8 2013

I meant to add as well: this patch assumes that all we want to do is not make URLs visible unless the tabs permission is present.  That seems like a good first step.  However, it seems like there should either be a discussion of what exactly un-privileged extensions should be able to do within the tabs API, or if that discussion has already happened, the documentation should be updated to reflect the decision.

Comment 16 by meacer@google.com, Jan 8 2013

Also, onCreated is similar and the documentation says 'url' property is only available if the extension has 'tabs' or 'webNavigation'. onUpdated should also make 'url' available if the extension has 'webNavigation' too, right?

Comment 17 by jsc...@chromium.org, Jan 8 2013

@kalman - My bad. I misread your previous comment as meaning the opposite of what you intended.

Comment 18 by kalman@chromium.org, Jan 8 2013

@jschuh - no problem.

@mvrable - put me as reviewer. We should do whatever the query functions do.

Comment 19 by mvrable@google.com, Jan 8 2013

Code is uploaded for review as https://codereview.chromium.org/11824004/.

Comment 20 by bugdroid1@chromium.org, Jan 11 2013

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

------------------------------------------------------------------------
r176406 | mvrable@chromium.org | 2013-01-11T19:19:26.485345Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/browser_event_router.h?r1=176406&r2=176405&pathrev=176406
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json?r1=176406&r2=176405&pathrev=176406
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/tabs/no_permissions?r1=176406&r2=176405&pathrev=176406
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/messaging/message_service.cc?r1=176406&r2=176405&pathrev=176406
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tabs_apitest.cc?r1=176406&r2=176405&pathrev=176406
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/menu_manager.cc?r1=176406&r2=176405&pathrev=176406
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util.cc?r1=176406&r2=176405&pathrev=176406
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util_android.cc?r1=176406&r2=176405&pathrev=176406
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util.h?r1=176406&r2=176405&pathrev=176406
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/tabs/no_permissions/test.js?r1=176406&r2=176405&pathrev=176406
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/browser_event_router.cc?r1=176406&r2=176405&pathrev=176406

Do not pass URLs in onUpdated events to extensions unless they have the
"tabs" permission.

BUG= 168442 


Review URL: https://chromiumcodereview.appspot.com/11824004
------------------------------------------------------------------------

Comment 21 by infe...@chromium.org, Jan 11 2013

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved SecImpacts-Stable SecImpacts-Beta Mstone-24
Status: Fixed

Comment 22 by bugdroid1@chromium.org, Jan 11 2013

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

------------------------------------------------------------------------
r176426 | nick@chromium.org | 2013-01-11T20:21:27.974660Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/browser_event_router.h?r1=176426&r2=176425&pathrev=176426
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/tabs/no_permissions?r1=176426&r2=176425&pathrev=176426
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/messaging/message_service.cc?r1=176426&r2=176425&pathrev=176426
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tabs_apitest.cc?r1=176426&r2=176425&pathrev=176426
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/menu_manager.cc?r1=176426&r2=176425&pathrev=176426
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util.cc?r1=176426&r2=176425&pathrev=176426
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util_android.cc?r1=176426&r2=176425&pathrev=176426
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util.h?r1=176426&r2=176425&pathrev=176426
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/browser_event_router.cc?r1=176426&r2=176425&pathrev=176426

Revert 176406
> Do not pass URLs in onUpdated events to extensions unless they have the
> "tabs" permission.
> 
> BUG= 168442 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/11824004

TBR=mvrable@chromium.org
Review URL: https://codereview.chromium.org/11866012
------------------------------------------------------------------------

Comment 23 by kalman@chromium.org, Jan 11 2013

Cc: nick@chromium.org
Status: Started
@nick it's helpful to put a link to the bots that failed so we can fix the problem more quickly.

Anyhow, here it is: http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%2010.7%20Tests%20%28dbg%29%281%29&number=5947 only seems to be mac bots, and for whatever reason it wasn't picked up in the try jobs.

Comment 24 by mvrable@google.com, Jan 11 2013

Nick sent a private e-mail with a link to the failing test, and I am taking a look.  It's in the newly-written test.

My guess is some type of race in the tests; the onUpdated events received in the extension are not arriving in the same order as expected by the test (this is in a test that navigates the page twice):
    Actual: [{"status":"loading"},{"status":"loading"},{"status":"complete"},{"status":"complete"}]
    Expected: [{"status":"loading"},{"status":"complete"},{"status":"loading"},{"status":"complete"}]
If it's a non-deterministic race that could explain why it didn't show up in the try jobs.

I will restructure the test so that we don't care about the exact contents, but only check that sensitive fields (URLs or others) are not present.  I'll send an updated patch once soon.

Comment 25 by mpcomplete@chromium.org, Jan 11 2013

If this is navigating a single tab, it doesn't make sense that "complete" is arriving twice in a row. Once a navigation completes, it shouldn't complete again without a navigation. Something's wrong with either the test or the tabs.onUpdated event.

Comment 26 by mvrable@google.com, Jan 11 2013

There were two calls to tabs.create previously.  I've improved the test now though (made it less sensitive to the exact data received by the onUpdated event callback, and removed the extra tabs.create call).  Hopefully this will fix the problem.

Comment 27 by bugdroid1@chromium.org, Jan 14 2013

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

------------------------------------------------------------------------
r176546 | mvrable@chromium.org | 2013-01-12T16:54:44.006079Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util.h?r1=176546&r2=176545&pathrev=176546
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/tabs/no_permissions/test.js?r1=176546&r2=176545&pathrev=176546
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/browser_event_router.cc?r1=176546&r2=176545&pathrev=176546
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/browser_event_router.h?r1=176546&r2=176545&pathrev=176546
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json?r1=176546&r2=176545&pathrev=176546
   A http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/api_test/tabs/no_permissions?r1=176546&r2=176545&pathrev=176546
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/messaging/message_service.cc?r1=176546&r2=176545&pathrev=176546
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tabs_apitest.cc?r1=176546&r2=176545&pathrev=176546
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/menu_manager.cc?r1=176546&r2=176545&pathrev=176546
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util.cc?r1=176546&r2=176545&pathrev=176546
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tab_util_android.cc?r1=176546&r2=176545&pathrev=176546

Do not pass URLs in onUpdated events to extensions unless they have the
"tabs" permission.

BUG= 168442 


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176406

Review URL: https://chromiumcodereview.appspot.com/11824004
------------------------------------------------------------------------

Comment 28 by infe...@chromium.org, Jan 14 2013

Status: Fixed

Comment 29 by mvrable@google.com, Jan 14 2013

I haven't reported/fixed a security bug before, I was curious: Is there any additional tracking needed on the bug and the fix?  When might it be deployed, and to which branch(es)?

Also, though this bug was closed there is at least one part that hasn't gotten addressed (I think): updating the documentation for the tabs API to reflect the current intended behavior.  Should I open a separate bug for that?

Comment 30 by kalman@chromium.org, Jan 14 2013

To answer first question: I don't know, jschuh@ do you think it needs to be merged in?

To answer second question: no need to file a separate bug for it.

Comment 31 by infe...@chromium.org, Jan 14 2013

We will handle the merges to the required branches when the merge window opens (we check with RM). Thanks for the fix.

Comment 32 by scarybea...@gmail.com, Jan 17 2013

Labels: -Merge-Approved -Mstone-24 Release-0 Mstone-26
Seems like a large change for a lower severity issue. How about just letting the fix roll into M26?

Comment 33 by kalman@chromium.org, Jan 17 2013

fine with me

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

Project Member
Labels: -Type-Security -Area-Internals -Feature-Extensions -SecSeverity-Low -SecImpacts-Stable -SecImpacts-Beta -Mstone-26 Cr-Platform-Extensions Security-Impact-Stable Security-Impact-Beta Security-Severity-Low Cr-Internals M-26 Type-Bug-Security

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

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

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

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

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

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

Comment 38 by scarybea...@gmail.com, Mar 23 2013

Labels: CVE-2013-0925

Comment 39 by jsc...@chromium.org, Nov 18 2013

Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

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

Project Member
Labels: -security_impact-beta

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

Project Member
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

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

Project Member
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

Comment 43 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Comment 44 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Comment 45 by sheriffbot@chromium.org, Jul 28 2018

Project Member
Labels: Pri-2

Sign in to add a comment