[Backport] Security bug 937131

Partial cherry-pick (skipping tests) of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/2363169:
Change Feature-Policy header semantics

This change implements the algorithmic changes for a recent change
to the Feature/Permissions policy spec:
w3c/webappsec-permissions-policy#378

With this change, the Feature-Policy or Permissions-Policy headers by
themselves cannot be used to delegate powerful features to cross-origin
iframes; the allow attribute must be used as well. To allow this to
still be ergonomic, the default value for the header for powerful
features is effectively '*', so that delegation is allowed by the header
implicitly. The header can now be used effectively to completely block
access to a feature, as any origins not present in the header allowlist
cannot be granted the feature through the allow attribute.

This also removes some code which previously only existed to track the
cases where this change would affect the output of an IsFeatureEnabled
call.

Several tests will have been modified or rewritten prior to landing
this change; this CL depends on the following (though they are all
independent, so they are not chained together):
 - https://crrev.com/c/2424633
 - https://crrev.com/c/2424634
 - https://crrev.com/c/2424635
 - https://crrev.com/c/2424654
 - https://crrev.com/c/2424655
 - https://crrev.com/c/2424657
 - https://crrev.com/c/2425003
 - https://crrev.com/c/2425004
(See Patchset 8 for a version with the changes from all of those CLs
included.)

This CL, while large, can best be understood as the union of the
following changes:

 - Algorithm changes, including the removal of previous "what-if" code
   and metrics:
    feature_policy.cc
    feature_policy.h
    execution_context.cc

 - Unit tests to cover those changes:
    feature_policy_unittest.cc
    render_frame_host_feature_policy_unittest.cc

 - Update WPT test expectations to account for the change in behaviour
   when only the header is used:
    3p/b/web_tests/external/wpt/feature-policy/feature-policy-*
    3p/b/web_tests/external/wpt/permissions-policy/permissions-policy-*

 - Update Blink web tests for fullscreen and payment request to validate
    that both are now working correctly with the new header semantics:
    3p/b/web_tests/http/tests/feature-policy/fullscreen*
    3p/b/web_tests/http/tests/feature-policy/payment*

 - Update Blink web tests for the iframe policy JS interface because of
   new test expectations when features are allowed/disallowed by header:
    3p/b/renderer/core/feature_policy/policy_test.cc
    3p/b/web_tests/http/tests/feature-policy/policy_iframes.php

Bug: 1095641, 937131
Change-Id: Iecbb0950c27a4565998ee5192590d6691a03b4a3
Reviewed-by: Yoav Weiss <[email protected]>
Reviewed-by: Charlie Hu <[email protected]>
Reviewed-by: Ken Buchanan <[email protected]>
Commit-Queue: Ian Clelland <[email protected]>
Cr-Commit-Position: refs/heads/master@{#826408}
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: clelland

chromium/third_party/blink/common/feature_policy/feature_policy.cc

@@ -117,7 +117,6 @@ std::unique_ptr<FeaturePolicy> FeaturePolicy::CreateWithOpenerPolicy(
   std::unique_ptr<FeaturePolicy> new_policy = base::WrapUnique(
       new FeaturePolicy(origin, GetFeaturePolicyFeatureList()));
   new_policy->inherited_policies_ = inherited_policies;
-  new_policy->proposed_inherited_policies_ = inherited_policies;
   return new_policy;
 }
 
@@ -129,12 +128,6 @@ bool FeaturePolicy::IsFeatureEnabled(
 bool FeaturePolicy::IsFeatureEnabledForOrigin(
     mojom::FeaturePolicyFeature feature,
     const url::Origin& origin) const {
-  return GetFeatureValueForOrigin(feature, origin);
-}
-
-bool FeaturePolicy::GetFeatureValueForOrigin(
-    mojom::FeaturePolicyFeature feature,
-    const url::Origin& origin) const {
   DCHECK(base::Contains(feature_list_, feature));
   DCHECK(base::Contains(inherited_policies_, feature));
 
@@ -149,27 +142,22 @@ bool FeaturePolicy::GetFeatureValueForOrigin(
   if (default_policy == FeaturePolicyFeatureDefault::EnableForSelf &&
       !origin_.IsSameOriginWith(origin))
     return false;
+
   return inherited_value;
 }
 
-// Temporary code to support metrics: (https://crbug.com/937131)
-// This method implements a proposed algorithm change to feature policy in which
-// the default allowlist for a feature if not specified in the header, is always
-// '*', but where the header allowlist *must* allow the nested frame origin in
-// order to delegate use of the feature to that frame.
-bool FeaturePolicy::GetProposedFeatureValueForOrigin(
+bool FeaturePolicy::GetFeatureValueForOrigin(
     mojom::FeaturePolicyFeature feature,
     const url::Origin& origin) const {
   DCHECK(base::Contains(feature_list_, feature));
-  DCHECK(base::Contains(proposed_inherited_policies_, feature));
+  DCHECK(base::Contains(inherited_policies_, feature));
 
-  auto inherited_value = proposed_inherited_policies_.at(feature);
+  auto inherited_value = inherited_policies_.at(feature);
   auto allowlist = allowlists_.find(feature);
   if (allowlist != allowlists_.end()) {
     return inherited_value && allowlist->second->Contains(origin);
   }
 
-  // If no allowlist is specified, return default feature value.
   return inherited_value;
 }
 
@@ -236,53 +224,16 @@ std::unique_ptr<FeaturePolicy> FeaturePolicy::CreateFromParentPolicy(
       base::WrapUnique(new FeaturePolicy(origin, features));
   for (const auto& feature : features) {
     new_policy->inherited_policies_[feature.first] =
-        new_policy->GetInheritedValueForFeature(parent_policy, feature,
-                                                container_policy);
-    new_policy->proposed_inherited_policies_[feature.first] =
-        new_policy->GetProposedInheritedValueForFeature(parent_policy, feature,
-                                                        container_policy);
+        new_policy->InheritedValueForFeature(parent_policy, feature,
+                                             container_policy);
   }
   return new_policy;
 }
 
-// Implements Feature Policy 9.8: Define an inherited policy for feature in
-// document and 9.9: Define an inherited policy for feature in container at
-// origin.
-bool FeaturePolicy::GetInheritedValueForFeature(
-    const FeaturePolicy* parent_policy,
-    std::pair<mojom::FeaturePolicyFeature, FeaturePolicyFeatureDefault> feature,
-    const ParsedFeaturePolicy& container_policy) const {
-  // 9.8 3: Otherwise [If context is not a nested browsing context,] return
-  // "Enabled".
-  if (!parent_policy)
-    return true;
-
-  for (const auto& decl : container_policy) {
-    if (decl.feature == feature.first) {
-      // 9.9 3.1: If the allowlist for feature in container policy does not
-      // match origin, return "Disabled".
-      if (!AllowlistFromDeclaration(decl, feature_list_)->Contains(origin_))
-        return false;
-
-      // 9.9 3.2: If feature is enabled in parent for parent’s origin, return
-      // "Enabled".
-      if (parent_policy->GetFeatureValueForOrigin(feature.first,
-                                                  parent_policy->origin_))
-        return true;
-    }
-  }
-
-  // 9.9 4: If feature is enabled in parent for parent’s origin, return
-  // "Enabled".
-  // 9.9 5: Otherwise return "Disabled".
-  return parent_policy->GetFeatureValueForOrigin(feature.first, origin_);
-}
-
-// Temporary code to support metrics (https://crbug.com/937131)
 // Implements Permissions Policy 9.7: Define an inherited policy for feature in
 // browsing context and 9.8: Define an inherited policy for feature in container
 // at origin.
-bool FeaturePolicy::GetProposedInheritedValueForFeature(
+bool FeaturePolicy::InheritedValueForFeature(
     const FeaturePolicy* parent_policy,
     std::pair<mojom::FeaturePolicyFeature, FeaturePolicyFeatureDefault> feature,
     const ParsedFeaturePolicy& container_policy) const {
@@ -293,14 +244,14 @@ bool FeaturePolicy::GetProposedInheritedValueForFeature(
 
   // 9.8 2: If policy’s inherited policy for feature is "Disabled", return
   // "Disabled".
-  if (!parent_policy->GetProposedFeatureValueForOrigin(feature.first,
-                                                       parent_policy->origin_))
+  if (!parent_policy->GetFeatureValueForOrigin(feature.first,
+                                               parent_policy->origin_))
     return false;
 
   // 9.8 3: If feature is present in policy’s declared policy, and the allowlist
   // for feature in policy’s declared policy does not match origin, then return
   // "Disabled".
-  if (!parent_policy->GetProposedFeatureValueForOrigin(feature.first, origin_))
+  if (!parent_policy->GetFeatureValueForOrigin(feature.first, origin_))
     return false;
 
   for (const auto& decl : container_policy) {

chromium/third_party/blink/public/common/feature_policy/feature_policy.h

@@ -180,13 +180,6 @@ class BLINK_COMMON_EXPORT FeaturePolicy {
   bool IsFeatureEnabledForOrigin(mojom::FeaturePolicyFeature feature,
                                  const url::Origin& origin) const;
 
-  // Returns the value of the given feature on the given origin.
-  bool GetFeatureValueForOrigin(mojom::FeaturePolicyFeature feature,
-                                const url::Origin& origin) const;
-
-  bool GetProposedFeatureValueForOrigin(mojom::FeaturePolicyFeature feature,
-                                        const url::Origin& origin) const;
-
   // Returns the allowlist of a given feature by this policy.
   const Allowlist GetAllowlistForFeature(
       mojom::FeaturePolicyFeature feature) const;
@@ -218,17 +211,15 @@ class BLINK_COMMON_EXPORT FeaturePolicy {
       const url::Origin& origin,
       const FeaturePolicyFeatureList& features);
 
-  bool GetInheritedValueForFeature(
+  bool InheritedValueForFeature(
       const FeaturePolicy* parent_policy,
       std::pair<mojom::FeaturePolicyFeature, FeaturePolicyFeatureDefault>
           feature,
       const ParsedFeaturePolicy& container_policy) const;
 
-  bool GetProposedInheritedValueForFeature(
-      const FeaturePolicy* parent_policy,
-      std::pair<mojom::FeaturePolicyFeature, FeaturePolicyFeatureDefault>
-          feature,
-      const ParsedFeaturePolicy& container_policy) const;
+  // Returns the value of the given feature on the given origin.
+  bool GetFeatureValueForOrigin(mojom::FeaturePolicyFeature feature,
+                                const url::Origin& origin) const;
 
   // The origin of the document with which this policy is associated.
   url::Origin origin_;
@@ -241,11 +232,6 @@ class BLINK_COMMON_EXPORT FeaturePolicy {
   // parent frame.
   FeaturePolicyFeatureState inherited_policies_;
 
-  // Temporary member to support metrics. These are the values which would be
-  // stored in |inherited_policies_| under the proposal in
-  // https://crbug.com/937131.
-  FeaturePolicyFeatureState proposed_inherited_policies_;
-
   const FeaturePolicyFeatureList& feature_list_;
 
   DISALLOW_COPY_AND_ASSIGN(FeaturePolicy);

chromium/third_party/blink/renderer/core/execution_context/execution_context.cc

@@ -412,20 +412,6 @@ bool ExecutionContext::FeatureEnabled(OriginTrialFeature feature) const {
   return origin_trial_context_->IsFeatureEnabled(feature);
 }
 
-void ExecutionContext::FeaturePolicyPotentialBehaviourChangeObserved(
-    mojom::blink::FeaturePolicyFeature feature) const {
-  size_t feature_index = static_cast<size_t>(feature);
-  if (feature_policy_behaviour_change_counted_.size() == 0) {
-    feature_policy_behaviour_change_counted_.resize(
-        static_cast<size_t>(mojom::blink::FeaturePolicyFeature::kMaxValue) + 1);
-  } else if (feature_policy_behaviour_change_counted_[feature_index]) {
-    return;
-  }
-  feature_policy_behaviour_change_counted_[feature_index] = true;
-  UMA_HISTOGRAM_ENUMERATION(
-      "Blink.UseCounter.FeaturePolicy.ProposalWouldChangeBehaviour", feature);
-}
-
 bool ExecutionContext::IsFeatureEnabled(
     mojom::blink::FeaturePolicyFeature feature,
     ReportOptions report_on_failure,
@@ -440,20 +426,6 @@ bool ExecutionContext::IsFeatureEnabled(
   bool should_report;
   bool enabled = security_context_.IsFeatureEnabled(feature, &should_report);
 
-  if (enabled) {
-    // Report if the proposed header semantics change would have affected the
-    // outcome. (https://crbug.com/937131)
-    const FeaturePolicy* policy = security_context_.GetFeaturePolicy();
-    url::Origin origin = GetSecurityOrigin()->ToUrlOrigin();
-    if (!policy->GetProposedFeatureValueForOrigin(feature, origin)) {
-      // Count that there was a change in this page load.
-      const_cast<ExecutionContext*>(this)->CountUse(
-          WebFeature::kFeaturePolicyProposalWouldChangeBehaviour);
-      // Record the specific feature whose behaviour was changed.
-      FeaturePolicyPotentialBehaviourChangeObserved(feature);
-    }
-  }
-
   if (should_report && report_on_failure == ReportOptions::kReportOnFailure) {
     mojom::blink::PolicyDisposition disposition =
         enabled ? mojom::blink::PolicyDisposition::kReport

chromium/third_party/blink/renderer/core/execution_context/execution_context.h

@@ -422,11 +422,6 @@ class CORE_EXPORT ExecutionContext : public Supplementable<ExecutionContext>,
   virtual void AddConsoleMessageImpl(ConsoleMessage*,
                                      bool discard_duplicates) = 0;
 
-  // Temporary method to record when the result of calling IsFeatureEnabled
-  // would change under the proposal in https://crbug.com/937131.
-  void FeaturePolicyPotentialBehaviourChangeObserved(
-      mojom::blink::FeaturePolicyFeature feature) const;
-
   v8::Isolate* const isolate_;
 
   SecurityContext security_context_;
@@ -470,11 +465,6 @@ class CORE_EXPORT ExecutionContext : public Supplementable<ExecutionContext>,
   network::mojom::blink::IPAddressSpace address_space_;
 
   Member<OriginTrialContext> origin_trial_context_;
-
-  // Tracks which feature policy features have been logged in this execution
-  // context as to the FeaturePolicyProposalWouldChangeBehaviour
-  // histogram, in order not to overcount.
-  mutable Vector<bool> feature_policy_behaviour_change_counted_;
 };
 
 }  // namespace blink