[Permissions Policy + CSP] (1) Ban opaque origins from OriginWithPossibleWildcard [chromium/src : main]
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland.
Ari Chivukula would like Ian Clelland to review this change.
[Permissions Policy + CSP] (1) Ban opaque origins from OriginWithPossibleWildcard
In order to migrate to using the underlying CSP wildcard matching logic
(even if we don't expand the wildcard options available) we need to
prevent opaque origins from ending up in the allowlist as CSP matching
does not support them. This isn't a big issue as the only origin that
can be opaque is `self`, so we can just split that out in the policy
itself as an optional datatype.
This touches a lot of code, but is heavily tested and the new DCHECKS in
OriginWithPossibleWildcards should ensure self was properly moved.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
Bug: 1418009
Change-Id: Ib35511b741f3168d9f28618fcb3eb8b8f3eb8f2a
---
M chrome/browser/permissions/permission_context_base_permissions_policy_unittest.cc
M chrome/browser/web_applications/web_app_database.cc
M chrome/browser/web_applications/web_app_database_unittest.cc
M chrome/browser/web_applications/web_app_unittest.cc
M components/autofill/content/browser/form_forest_unittest.cc
M components/browsing_topics/browsing_topics_page_load_data_tracker_unittest.cc
M components/permissions/permission_manager_unittest.cc
M components/permissions/permission_uma_util_unittest.cc
M content/browser/attribution_reporting/attribution_host_unittest.cc
M content/browser/browsing_topics/browsing_topics_url_loader_service_unittest.cc
M content/browser/direct_sockets/direct_sockets_test_utils.cc
M content/browser/geolocation/geolocation_service_impl_unittest.cc
M content/browser/interest_group/ad_auction_service_impl_unittest.cc
M content/browser/media/media_devices_permission_checker_unittest.cc
M content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc
M content/browser/webauth/webauth_request_security_checker_unittest.cc
M content/shell/browser/shell_content_browser_client.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy.cc
M third_party/blink/common/permissions_policy/permissions_policy_declaration.cc
M third_party/blink/common/permissions_policy/permissions_policy_declaration_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/common/permissions_policy/permissions_policy_unittest.cc
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/public/common/permissions_policy/permissions_policy.h
M third_party/blink/public/common/permissions_policy/permissions_policy_declaration.h
M third_party/blink/public/mojom/permissions_policy/permissions_policy.mojom
M third_party/blink/renderer/core/html/client_hints_util.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_parser.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
32 files changed, 569 insertions(+), 343 deletions(-)
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland.
Patch set 1:Auto-Submit +1
Ari Chivukula (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Ian Clelland.
Ari Chivukula would like Chromium IPC Reviews to review this change.
[Permissions Policy + CSP] (1) Ban opaque origins from OriginWithPossibleWildcard
In order to migrate to using the underlying CSP wildcard matching logic
(even if we don't expand the wildcard options available) we need to
prevent opaque origins from ending up in the allowlist as CSP matching
does not support them. This isn't a big issue as the only origin that
can be opaque is `self`, so we can just split that out in the policy
itself as an optional datatype.
This touches a lot of code, but is heavily tested and the new DCHECKS in
OriginWithPossibleWildcards should ensure self was properly moved.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
Bug: 1418009
Change-Id: Ib35511b741f3168d9f28618fcb3eb8b8f3eb8f2a
---
M chrome/browser/permissions/permission_context_base_permissions_policy_unittest.cc
M chrome/browser/web_applications/web_app.cc
M chrome/browser/web_applications/web_app_database.cc
M chrome/browser/web_applications/web_app_database_unittest.cc
M chrome/browser/web_applications/web_app_install_utils.cc
M chrome/browser/web_applications/web_app_unittest.cc
M components/autofill/content/browser/form_forest_unittest.cc
M components/browsing_topics/browsing_topics_page_load_data_tracker_unittest.cc
M components/permissions/permission_manager_unittest.cc
M components/permissions/permission_uma_util_unittest.cc
M content/browser/attribution_reporting/attribution_host_unittest.cc
M content/browser/browsing_topics/browsing_topics_url_loader_service_unittest.cc
M content/browser/direct_sockets/direct_sockets_test_utils.cc
M content/browser/geolocation/geolocation_service_impl_unittest.cc
M content/browser/interest_group/ad_auction_service_impl_unittest.cc
M content/browser/media/media_devices_permission_checker_unittest.cc
M content/browser/renderer_host/media/media_stream_ui_proxy_unittest.cc
M content/browser/serial/serial_unittest.cc
M content/browser/site_per_process_browsertest.cc
M content/browser/smart_card/smart_card_browsertest.cc
M content/browser/webauth/webauth_request_security_checker_unittest.cc
M content/shell/browser/shell_content_browser_client.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy.cc
M third_party/blink/common/permissions_policy/permissions_policy_declaration.cc
M third_party/blink/common/permissions_policy/permissions_policy_declaration_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/common/permissions_policy/permissions_policy_unittest.cc
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/public/common/permissions_policy/permissions_policy.h
M third_party/blink/public/common/permissions_policy/permissions_policy_declaration.h
M third_party/blink/public/mojom/permissions_policy/permissions_policy.mojom
M third_party/blink/renderer/core/html/client_hints_util.cc
M third_party/blink/renderer/core/permissions_policy/dom_feature_policy.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_parser.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
38 files changed, 667 insertions(+), 383 deletions(-)
Ari Chivukula (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Ian Clelland.
Patch set 6:Auto-Submit +1
gwsq (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Ian Clelland, Robert Sesek.
gwsq would like Robert Sesek to review this change authored by Ari Chivukula.
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Ian Clelland, Robert Sesek.
Ari Chivukula has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Ian Clelland, Robert Sesek.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: rse...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): rse...@chromium.org
Reviewer source(s):
rse...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Robert Sesek (Gerrit)
Attention is currently required from: Ari Chivukula, Ian Clelland.
Patch set 6:Code-Review +1
1 comment:
Patchset:
IPC LGTM
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ian Clelland (Gerrit)
Attention is currently required from: Ari Chivukula.
4 comments:
File chrome/browser/web_applications/web_app.cc:
Patch Set #6, Line 953:
allowlist_json.Append(decl.matches_self->Serialize());We probably shouldn't be serializing opaque origins (although perhaps we were previously...) should there be a guard or a DCHECK on that here?
File chrome/browser/web_applications/web_app_database.cc:
Patch Set #6, Line 722:
if (decl.matches_self) {Also, since this appears to be identical to the code in web_app.cc, then there's an opportunity to factor out the serialization code for origin lists, so that other people don't also need to work out the details themselves.
(At least a TODO here if it's out of scope for this CL)
File third_party/blink/common/permissions_policy/permissions_policy.cc:
Patch Set #6, Line 24:
const PermissionsPolicyFeatureList& feature_list) {Thanks for that cleanup!
File third_party/blink/public/common/permissions_policy/permissions_policy_declaration.h:
Patch Set #6, Line 58:
bool matches_opaque_src{false};Ideally, we could use this same technique to store the actual opaque origin for this case as well; have you looked at all into whether that's possible?
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland.
Patch set 7:Auto-Submit +1Commit-Queue +1
3 comments:
File chrome/browser/web_applications/web_app.cc:
Patch Set #6, Line 953:
allowlist_json.Append(decl.matches_self->Serialize());
We probably shouldn't be serializing opaque origins (although perhaps we were previously... […]
I don't want to change behavior but I can add a TODO
File chrome/browser/web_applications/web_app_database.cc:
Patch Set #6, Line 722:
if (decl.matches_self) {
Also, since this appears to be identical to the code in web_app. […]
Done
File third_party/blink/public/common/permissions_policy/permissions_policy_declaration.h:
Patch Set #6, Line 58:
bool matches_opaque_src{false};
Ideally, we could use this same technique to store the actual opaque origin for this case as well; h […]
I can followup after but that seems like a pretty big behavior change. The 'src' path could have just been adding the opaque origin to the list before but it was specifically using this boolean instead to allow all opaque origins.
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ian Clelland (Gerrit)
Attention is currently required from: Ari Chivukula.
6 comments:
Patchset:
This looks good, aside from a little bit of test coverage.
One point that I keep coming back to is the naming of the attribute -- "matches_*" is a boolean predicate everywhere else it is used, and here it's an optional origin that we check, too. Would a name like `self_origin` or `included_self_origin` be clearer?
File chrome/browser/web_applications/web_app_database.cc:
Patch Set #11, Line 1388:
maybe_origin_with_possible_wildcardsI very slightly prefer using .has_value() and .value() here, rather than implicit boolean conversion and dereferencing, but also okay to leave as-is.
File content/browser/site_per_process_browsertest.cc:
if (matches_self) {
declaration.matches_self = url::Origin::Create(*matches_self);ditto here on .has_value and .value
File third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc:
Patch Set #11, Line 53:
return absl::nullopt;Is this path tested in unit tests?
File third_party/blink/common/permissions_policy/permissions_policy_declaration_unittest.cc:
Patch Set #11, Line 52:
// Self match.Can we test the case where `matches_self` is `kOpaqueOrigin` as well?
File third_party/blink/common/permissions_policy/permissions_policy_unittest.cc:
This file has approximately infinity tests already, but I think there are a couple that are missing now:
- A test where the top frame at Origin A has a header policy that includes Origin A (but critically *not* 'self')
- A test where the top frame is sandboxed, so has an opaque origin, and includes 'self in its header policy.
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Ian Clelland.
Patch set 12:Auto-Submit +1
5 comments:
File chrome/browser/web_applications/web_app_database.cc:
Patch Set #11, Line 1388:
maybe_origin_with_possible_wildcards
I very slightly prefer using .has_value() and . […]
Done
File content/browser/site_per_process_browsertest.cc:
if (matches_self) {
declaration.matches_self = url::Origin::Create(*matches_self);
ditto here on .has_value and . […]
Done
File third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc:
Patch Set #11, Line 53:
return absl::nullopt;Is this path tested in unit tests?
Yes, the empty string `origin` values in the `Parse` test
File third_party/blink/common/permissions_policy/permissions_policy_declaration_unittest.cc:
Patch Set #11, Line 52:
// Self match.Can we test the case where `matches_self` is `kOpaqueOrigin` as well?
Done
File third_party/blink/common/permissions_policy/permissions_policy_unittest.cc:
This file has approximately infinity tests already, but I think there are a couple that are missing […]
Done
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ian Clelland (Gerrit)
Attention is currently required from: Ari Chivukula.
Patch set 12:Code-Review +1
1 comment:
File third_party/blink/common/permissions_policy/permissions_policy_unittest.cc:
Done
That test works; I was thinking of one where the top was also sandboxed (as in a CSP header sandbox) but I think this covers the same logic.
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Daniel Cheng.
Ari Chivukula would like Daniel Cheng to review this change.
M content/browser/usb/web_usb_service_impl_unittest.cc
M content/browser/webauth/webauth_request_security_checker_unittest.cc
M content/shell/browser/shell_content_browser_client.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards.cc
M third_party/blink/common/permissions_policy/origin_with_possible_wildcards_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy.cc
M third_party/blink/common/permissions_policy/permissions_policy_declaration.cc
M third_party/blink/common/permissions_policy/permissions_policy_declaration_unittest.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.cc
M third_party/blink/common/permissions_policy/permissions_policy_mojom_traits.h
M third_party/blink/common/permissions_policy/permissions_policy_unittest.cc
M third_party/blink/public/common/permissions_policy/origin_with_possible_wildcards.h
M third_party/blink/public/common/permissions_policy/permissions_policy.h
M third_party/blink/public/common/permissions_policy/permissions_policy_declaration.h
M third_party/blink/public/mojom/permissions_policy/permissions_policy.mojom
M third_party/blink/renderer/core/html/client_hints_util.cc
M third_party/blink/renderer/core/permissions_policy/dom_feature_policy.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_parser.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_test.cc
39 files changed, 741 insertions(+), 390 deletions(-)
Daniel Cheng (Gerrit)
Attention is currently required from: Ari Chivukula.
5 comments:
Patchset:
The general approach seems fine to me, but can we think of a different name for matches_self? it's a bit confusing to have matches_self (which is an absl::optional<Origin>) and then several other matches_x arguments (which are bools). Some ideas...
- self_if_specified?
- self_if_matches?
- ???
File third_party/blink/common/permissions_policy/permissions_policy.cc:
Patch Set #12, Line 52:
matches_self_ = self;Nit: std::move()
Patch Set #12, Line 77:
return matches_self_;This could return by const ref (i.e. `const absl::optional<url::Origin>&`) and avoid a copy.
File third_party/blink/common/permissions_policy/permissions_policy_declaration.cc:
Patch Set #12, Line 30:
matches_self(matches_self),Nit: std::move()
File third_party/blink/renderer/core/permissions_policy/dom_feature_policy.cc:
Patch Set #12, Line 124:
Vector<String> result;Not introduced by this CL but since Vector's resize strategy is 1.25x [1], it's often a good idea to pre-reserve().
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Daniel Cheng.
Patch set 13:Auto-Submit +1Commit-Queue +1
5 comments:
Patchset:
The general approach seems fine to me, but can we think of a different name for matches_self? it's a […]
I think the name is reasonable, it doesn't seem overly clever to have matches_self be unset if no and set to the origin self if so.
Also, I'll be looking at removing matches_opaque_src if possible in the near future so there will only be two with the matches_ format.
File third_party/blink/common/permissions_policy/permissions_policy.cc:
Patch Set #12, Line 52:
matches_self_ = self;Nit: std::move()
Done
Patch Set #12, Line 77:
return matches_self_;This could return by const ref (i.e. `const absl::optional<url::Origin>&`) and avoid a copy.
Done
File third_party/blink/common/permissions_policy/permissions_policy_declaration.cc:
Patch Set #12, Line 30:
matches_self(matches_self),Nit: std::move()
Done
File third_party/blink/renderer/core/permissions_policy/dom_feature_policy.cc:
Patch Set #12, Line 124:
Vector<String> result;
Not introduced by this CL but since Vector's resize strategy is 1. […]
Done
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Ari Chivukula.
1 comment:
Patchset:
I think the name is reasonable, it doesn't seem overly clever to have matches_self be unset if no an […]
I think the point is that it's a bit confusing that we have two variables named matches_x and matches_y; one of them is definitely a bool, so it's kind of obvious how it works; the other *looks* like a bool but definitely is not a bool. Thus I'd prefer a different name.
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Daniel Cheng.
Patch set 14:Auto-Submit +1
1 comment:
Patchset:
I think the point is that it's a bit confusing that we have two variables named matches_x and matche […]
Done
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Ari Chivukula.
Patch set 14:Owners-Override +1Code-Review +1Commit-Queue +2
1 comment:
Patchset:
LGTM
To view, visit change 4317285. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Permissions Policy + CSP] (1) Ban opaque origins from OriginWithPossibleWildcard
In order to migrate to using the underlying CSP wildcard matching logic
(even if we don't expand the wildcard options available) we need to
prevent opaque origins from ending up in the allowlist as CSP matching
does not support them. This isn't a big issue as the only origin that
can be opaque is `self`, so we can just split that out in the policy
itself as an optional datatype.
This touches a lot of code, but is heavily tested and the new DCHECKS in
OriginWithPossibleWildcards should ensure self was properly moved.
This CL is part of a series:
(1) Ban opaque origins from OriginWithPossibleWildcard
(2) Switch to using underlying CSP data type
(3) Use CSP parsing and comparison paths
Bug: 1418009
Change-Id: Ib35511b741f3168d9f28618fcb3eb8b8f3eb8f2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4317285
Owners-Override: Daniel Cheng <dch...@chromium.org>
Auto-Submit: Ari Chivukula <ari...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Ian Clelland <icle...@chromium.org>
Commit-Queue: Daniel Cheng <dch...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1120662}
39 files changed, 749 insertions(+), 391 deletions(-)