Extensions: incorrect privilege increase determination for swapping an api permission for a less powerful one |
|
Issue descriptionWhen an extension updates, we try to determine if it was a permissions increase over the old permissions the extension had. Just looking at the permissions is not enough, since some permissions might be warning-less, and others might be consumed by existing granted permissions. We only want to warn the user if there's something new for them to approve. The current implementation has a bug in the following case: - Extension version 1 has history permission - Extension version 2 has topSites permission topSites is less powerful than history, and so should not be considered a permissions increase. However, it has a different permission warning (a less scary one), and the current implementation only checks that. Ironically, had this been: - Extension version 1 has history permission - Extension version 2 has topSites AND history permission the code correctly detects that this is not an escalation. So by removing a permission, the extension is considered to be more privileged. We should fix that. :) +treib@ FYI, since he's worked on extension permissions quite a bit in the past.
,
May 11 2018
@1: Ah, definitely a similar issue (though only really referenced in the last sentence of that bug - most of it is devoted to the cross over between host/api/manifest). I *think* we can do better here than we currently do. I've sent a CL with a new approach, which is basically "check if adding the new permissions results in new warnings" rather than "check if the new permissions result in different warnings". Since we don't remove granted permissions on extension installation, this seems more conceptually correct. I *think* this approach would also work with the permission sets as a whole (rather than separately comparing host and manifest/api permissions), but I'd like to tackle that separately.
,
May 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f04a75acce94425f92dc9d517f92abfabccf1665 commit f04a75acce94425f92dc9d517f92abfabccf1665 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed May 16 18:34:21 2018 [Extensions] Properly classify privilege increase for swapping permissions Correct a bug where an extension would be improperly disabled for a privilege increase when it went from a more powerful permission to a less powerful one, if the permission messages were different. Previously, privilege escalation code would look check if there were any warning strings in the new set of permissions that were not in the old set. This failed, because a new warning (not contained in the previous set) might be less powerful. Instead, compare whether *adding* the new permissions to the already-granted permissions results in any new (or different) warnings. Consider it a permissions increase only if there is a difference. Conceptually, this is more accurate, since when an extension is updated, new permissions are added to the list of granted permissions (and any already-granted permissions are not revoked). Update the IsPrivilegeIncrease() arguments to make this slightly more clear by replacing "old_permissions" and "new_permissions" with "granted_permissions" and "requested_permissions", respectively. Add a regression test for the same. TBR=slan@chromium.org Bug: 841938 Change-Id: I41542c6813c1f4a2f0318609bd75b838eef00cb5 Reviewed-on: https://chromium-review.googlesource.com/1054607 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Stephen Lanham <slan@chromium.org> Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#559201} [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/chrome/common/extensions/permissions/chrome_permission_message_provider.cc [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/chrome/common/extensions/permissions/chrome_permission_message_provider.h [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/chrome/common/extensions/permissions/permission_set_unittest.cc [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/chromecast/common/cast_extensions_client.cc [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/extensions/common/permissions/permission_message_provider.h [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/extensions/shell/common/shell_extensions_client.cc [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/extensions/test/test_permission_message_provider.cc [modify] https://crrev.com/f04a75acce94425f92dc9d517f92abfabccf1665/extensions/test/test_permission_message_provider.h |
|
►
Sign in to add a comment |
|
Comment 1 by treib@chromium.org
, May 11 2018