New issue
Advanced search Search tips

Issue 878998 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 696822



Sign in to add a comment

Declarative Net Request: Blocking requests shouldn't require host permissions.

Project Member Reported by karandeepb@chromium.org, Aug 30

Issue description

Declarative Net Request doesn't allow extensions to actually see any network requests. It shouldn't require any host permissions for blocking requests. 
 
Cc: mea...@chromium.org
+meacer@ to make sure he's okay with this.

I think we'll still want some kind of generic permission (even just "block requests" - once we find something more meaningful than "requests") so that extensions can't be permissionless and do this.

I wonder if it would also be useful to still have the extension need to specify hosts that they do this to, mostly for static analysis purposes.  I know we talked about having a separate host entry for declarative net request in the past, but moved away from it to keep inline with webRequest.  Maybe it's worth revisiting?

Strawman:
declarative_net_request: {
  rule_resources: [<resource files>],
  block_hosts: [...],
  modify_hosts: [...]
}

block_hosts would be the set of hosts that we block requests (on? from? to?), and modify_hosts would be the set that are actually affected.

That's pretty ugly, though - what's something better? :)
>> I think we'll still want some kind of generic permission

We already have the declarativeNetRequest permission that extensions should specify as part of the manifest entry. My thinking was:
- That "declarativeNetRequest" should implicitly grant the block permission. This should obviously result in a permission message saying so. 
- To redirect a url, an extension would also need host access to the url and the origin of the request being redirected. The concept of host permissions involves modifying resources, so it seems ok to me.

I would prefer not introducing more host permission concepts. It would involve adding yet more complexity to our permission code, and it also makes it more confusing for developers.
I can see benefits either way.

Specifying the dnr hosts separately makes static analysis easier, and also means that we're more granular with the permissions we end up granting (e.g., if an extension just needs to redirect a resource, we don't also grant it access to tabs.executeScript or cookies).  On the other hand, the reason we often lump these permissions together is that they all fundamentally allow access to the page, and with one, you can usually find workarounds for others (for instance, if you couldn't use tabs.executeScript, you could redirect a js file to another of your choice).

I'm curious about how much more complex you think this will make our permissions code.  It seems like it would just be adding another URLPatternSet that indicates the allowed hosts, and matching against that.  Additionally, we then *wouldn't* need to consult explicit hosts for that, and nothing else would need to consult this.  So it seems like it would be reasonably confined, and would help have defined capabilities granted for each permission.  But, it does come at the cost of having that other set...
As regards the complexity, I had taken a stab earlier at separate host permissions. See the doc at go/dnr-hosts. Basically we would need to incorporate these into effective hosts etc.

Even after ignoring the issues around complexity, I don't think we should pursue separate hosts unless we also do this for web request. I don't see the point of having separate host permissions for redirects using web request and declarative net request apis when the capability is the same. 

Also, I don't really understand how big of a win the static analysis capability is.
Cc: jsc...@chromium.org
Devlin, are you fine with me implementing the proposal in c#2? 

Also, +Justin in case he has thoughts.
Cc: awhalley@chromium.org palmer@chromium.org
cc'ing some more security folks. Is the security team ok with this change?
Discussed offline, and I'm fine with the approach in #2 (a single install-time warning for blocking, with redirects requiring host permissions).
Cc: -palmer@chromium.org
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
I think meacer is the right person for this, as the designated security person for extensions.
To clarify the basics: "Blocking requests" here means cancelling the request altogether, right?

Overall this sounds like a win. I remember a very old discussion about a similar blocking API where there were concerns about extensions breaking the integrity of sites. That was not a declarative API though, which is different.

You might want to check with the Webstore folks about the static analysis capability. I lean towards having it, but they might not be as interested.
>> "Blocking requests" here means cancelling the request altogether, right?
Correct. 

>> You might want to check with the Webstore folks about the static analysis capability.

Will check with them. Thanks.
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49

commit a03bd7e90f6f2c446a039dc23f238ccbfdc8de49
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Wed Oct 10 06:56:15 2018

DNR: Remove host permission requirement for blocking requests; introduce permission message.

This CL changes the permission model for the declarativeNetRequest API:

- Blocking requests no longer requires host permissions.
- Host permission requirement to redirect requests remains the same i.e. the
  extension still needs access to both the request url and initiator.
- Mark the declarativeNetRequest permission as one which cannot be made optional
  and introduce a new permission message for it:
  "Block content on any page you visit".
- Update the documentation and tests to reflect the same.

BUG= 878998 

Change-Id: Ief5f49a88fc18fc55d182f825427d63fa57cfb83
Reviewed-on: https://chromium-review.googlesource.com/c/1269338
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598235}
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/app/generated_resources.grd
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/browser/extensions/api/declarative_net_request/rule_indexing_unittest.cc
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/browser/extensions/api/declarative_net_request/ruleset_manager_unittest.cc
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/browser/extensions/api/declarative_net_request/ruleset_matcher_unittest.cc
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/common/extensions/docs/templates/articles/permission_warnings.html
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/common/extensions/docs/templates/intros/declarativeNetRequest.html
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/common/extensions/docs/templates/intros/permissions.html
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/common/extensions/permissions/chrome_permission_message_rules.cc
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/common/extensions/permissions/permission_set_unittest.cc
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/test/data/extensions/declarative_net_request/cross_site_script.html
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/chrome/test/data/extensions/declarative_net_request/page_with_four_frames.html
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/extensions/browser/api/declarative_net_request/ruleset_manager.cc
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/extensions/browser/api/declarative_net_request/ruleset_manager.h
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/extensions/common/api/declarative_net_request/test_utils.h
[modify] https://crrev.com/a03bd7e90f6f2c446a039dc23f238ccbfdc8de49/extensions/common/permissions/extensions_api_permissions.cc

Status: Fixed (was: Assigned)

Sign in to add a comment