Skip to content

Commit

Permalink
[Backport] Security bug 937131
Browse files Browse the repository at this point in the history 
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 <yoavweiss@chromium.org>
Reviewed-by: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826408}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
  • Loading branch information
@clelland @mibrunin
clelland authored and mibrunin committed Feb 16, 2021
1 parent 75d13f3 commit 5e43ff0
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 115 deletions.
69 changes: 10 additions & 59 deletions chromium/third_party/blink/common/feature_policy/feature_policy.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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));

Expand All @@ -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;
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down
22 changes: 4 additions & 18 deletions chromium/third_party/blink/public/common/feature_policy/feature_policy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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_;
Expand All @@ -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);
Expand Down
28 changes: 0 additions & 28 deletions chromium/third_party/blink/renderer/core/execution_context/execution_context.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
10 changes: 0 additions & 10 deletions chromium/third_party/blink/renderer/core/execution_context/execution_context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 5e43ff0

Please sign in to comment.