0

Clean up integrity checking code.

We check a resource's integrity disposition in a few places, with
somewhat inconsistent formatting (checking "!passed" vs "failed",
for instance). To make it simpler to add some functionality here,
this patch makes a few small changes to align these callsites,
and centralize the decision about whether or not to force
integrity checks for cached/prefetched resources.

No change in behavior is intended; the checks should have the same
outcomes as in the status quo.

Bug: 375343417
Change-Id: Iea10d3f730cc60359790070fcb06ce62acb97f48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5965057
Reviewed-by: Yoav Weiss (@Shopify) <yoavweiss@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1374688}
This commit is contained in:
Mike West
2024-10-28 17:03:06 +00:00
committed by Chromium LUCI CQ
parent bc79c91d8f
commit 2f7e8821db
8 changed files with 36 additions and 36 deletions

@ -183,25 +183,24 @@ void RemoteFontFaceSource::NotifyFinished(Resource* resource) {
auto* font = To<FontResource>(resource); auto* font = To<FontResource>(resource);
histograms_.RecordRemoteFont(font); histograms_.RecordRemoteFont(font);
// Refer to the comments in classic_pending_script.cc for the reason why // Refer to the comments in `Resource::ForceIntegrityChecks()`:
// SRI checks should be done here in ResourceClient instead of // SRI checks should be done here in ResourceClient instead of
// ResourceFetcher. SRI failure should behave as network error // ResourceFetcher. SRI failure should behave as network error
// (ErrorOccurred()). PreloadCache even caches network errors. // (ErrorOccurred()). PreloadCache even caches network errors.
// Font fetch itself doesn't support SRI but font preload does. // Font fetch itself doesn't support SRI but font preload does.
// So, if the resource was preloaded we need to check // So, if the resource was preloaded we need to check
// SRI failure and simulate network error if it happens. // SRI failure and simulate network error if it happens.
bool force_integrity_checks = resource->ForceIntegrityChecks();
if (resource->IsLinkPreload()) { if (force_integrity_checks) {
SubresourceIntegrityHelper::DoReport(*execution_context, SubresourceIntegrityHelper::DoReport(*execution_context,
resource->IntegrityReportInfo()); resource->IntegrityReportInfo());
} }
DCHECK(!custom_font_data_);
// font->GetCustomFontData() returns nullptr if network error happened // font->GetCustomFontData() returns nullptr if network error happened
// (ErrorOccurred() is true). To simulate network error we don't update // (ErrorOccurred() is true). To simulate network error we don't update
// custom_font_data_ to keep the nullptr value in case of SRI failures. // custom_font_data_ to keep the nullptr value in case of SRI failures.
if (!resource->IsLinkPreload() || resource->IntegrityDisposition() != DCHECK(!custom_font_data_);
ResourceIntegrityDisposition::kFailed) { if (resource->PassedIntegrityChecks() || !force_integrity_checks) {
custom_font_data_ = font->GetCustomFontData(); custom_font_data_ = font->GetCustomFontData();
} }
url_ = resource->Url().GetString(); url_ = resource->Url().GetString();

@ -70,19 +70,14 @@ void LinkStyle::NotifyFinished(Resource* resource) {
} }
auto* cached_style_sheet = To<CSSStyleSheetResource>(resource); auto* cached_style_sheet = To<CSSStyleSheetResource>(resource);
// See the comment in pending_script.cc about why this check is necessary
// here, instead of in the resource fetcher. https://crbug.com/500701.
if ((!cached_style_sheet->ErrorOccurred() && if ((!cached_style_sheet->ErrorOccurred() &&
!owner_->FastGetAttribute(html_names::kIntegrityAttr).empty() && !owner_->FastGetAttribute(html_names::kIntegrityAttr).empty() &&
!cached_style_sheet->IntegrityMetadata().empty()) || !cached_style_sheet->IntegrityMetadata().empty()) ||
resource->IsLinkPreload()) { resource->ForceIntegrityChecks()) {
ResourceIntegrityDisposition disposition =
cached_style_sheet->IntegrityDisposition();
SubresourceIntegrityHelper::DoReport( SubresourceIntegrityHelper::DoReport(
*GetExecutionContext(), cached_style_sheet->IntegrityReportInfo()); *GetExecutionContext(), cached_style_sheet->IntegrityReportInfo());
if (disposition == ResourceIntegrityDisposition::kFailed) { if (!cached_style_sheet->PassedIntegrityChecks()) {
loading_ = false; loading_ = false;
RemovePendingSheet(); RemovePendingSheet();
NotifyLoadedSheetAndAllCriticalSubresources( NotifyLoadedSheetAndAllCriticalSubresources(

@ -84,10 +84,8 @@ LinkLoader::LinkLoader(LinkLoaderClient* client) : client_(client) {
} }
void LinkLoader::NotifyFinished(Resource* resource) { void LinkLoader::NotifyFinished(Resource* resource) {
if (resource->ErrorOccurred() || if (resource->ErrorOccurred() || (resource->ForceIntegrityChecks() &&
(resource->IsLinkPreload() && !resource->PassedIntegrityChecks())) {
resource->IntegrityDisposition() ==
ResourceIntegrityDisposition::kFailed)) {
client_->LinkLoadingErrored(); client_->LinkLoadingErrored();
} else { } else {
client_->LinkLoaded(); client_->LinkLoaded();

@ -46,8 +46,7 @@ bool ModuleScriptFetcher::WasModuleLoadSuccessful(
// <spec step="9">... response's type is "error" ...</spec> // <spec step="9">... response's type is "error" ...</spec>
if (!resource || resource->ErrorOccurred() || if (!resource || resource->ErrorOccurred() ||
resource->IntegrityDisposition() != !resource->PassedIntegrityChecks()) {
ResourceIntegrityDisposition::kPassed) {
return false; return false;
} }

@ -433,17 +433,10 @@ void ClassicPendingScript::NotifyFinished(Resource* resource) {
SubresourceIntegrityHelper::DoReport(*execution_context, SubresourceIntegrityHelper::DoReport(*execution_context,
resource->IntegrityReportInfo()); resource->IntegrityReportInfo());
// It is possible to get back a script resource with integrity metadata
// for a request with an empty integrity attribute. In that case, the
// integrity check should be skipped, as the integrity may not have been
// "meant" for this specific request. If the resource is being served from
// the preload cache however, we know any associated integrity metadata and
// checks were destined for this request, so we cannot skip the integrity
// check.
bool integrity_failure = false; bool integrity_failure = false;
if (!options_.GetIntegrityMetadata().empty() || resource->IsLinkPreload()) { if (!options_.GetIntegrityMetadata().empty() ||
integrity_failure = resource->IntegrityDisposition() != resource->ForceIntegrityChecks()) {
ResourceIntegrityDisposition::kPassed; integrity_failure = !resource->PassedIntegrityChecks();
} }
if (intervened_) { if (intervened_) {

@ -45,7 +45,8 @@ class PLATFORM_EXPORT IntegrityMetadata {
enum class ResourceIntegrityDisposition : uint8_t { enum class ResourceIntegrityDisposition : uint8_t {
kNotChecked = 0, kNotChecked = 0,
kFailed, kNetworkError,
kFailedIntegrityMetadata,
kPassed kPassed
}; };

@ -193,14 +193,15 @@ void Resource::SetLoader(ResourceLoader* loader) {
void Resource::CheckResourceIntegrity() { void Resource::CheckResourceIntegrity() {
// Skip the check and reuse the previous check result, especially on // Skip the check and reuse the previous check result, especially on
// successful revalidation. // successful revalidation.
if (IntegrityDisposition() != ResourceIntegrityDisposition::kNotChecked) if (integrity_disposition_ != ResourceIntegrityDisposition::kNotChecked) {
return; return;
}
// Loading error occurred? Then result is uncheckable. // Loading error occurred? Then result is uncheckable.
integrity_report_info_.Clear(); integrity_report_info_.Clear();
if (ErrorOccurred()) { if (ErrorOccurred()) {
CHECK(!Data()); CHECK(!Data());
integrity_disposition_ = ResourceIntegrityDisposition::kFailed; integrity_disposition_ = ResourceIntegrityDisposition::kNetworkError;
return; return;
} }
@ -214,10 +215,11 @@ void Resource::CheckResourceIntegrity() {
IntegrityMetadata(), Data(), Url(), *this, integrity_report_info_)) { IntegrityMetadata(), Data(), Url(), *this, integrity_report_info_)) {
integrity_disposition_ = ResourceIntegrityDisposition::kPassed; integrity_disposition_ = ResourceIntegrityDisposition::kPassed;
} else { } else {
integrity_disposition_ = ResourceIntegrityDisposition::kFailed; integrity_disposition_ =
ResourceIntegrityDisposition::kFailedIntegrityMetadata;
} }
DCHECK_NE(IntegrityDisposition(), ResourceIntegrityDisposition::kNotChecked); DCHECK_NE(integrity_disposition_, ResourceIntegrityDisposition::kNotChecked);
} }
void Resource::NotifyFinished() { void Resource::NotifyFinished() {

@ -338,9 +338,22 @@ class PLATFORM_EXPORT Resource : public GarbageCollected<Resource>,
const IntegrityMetadataSet& IntegrityMetadata() const { const IntegrityMetadataSet& IntegrityMetadata() const {
return options_.integrity_metadata; return options_.integrity_metadata;
} }
ResourceIntegrityDisposition IntegrityDisposition() const { bool PassedIntegrityChecks() const {
return integrity_disposition_; return integrity_disposition_ == ResourceIntegrityDisposition::kPassed;
} }
// Caching makes it possible for a resource that was requested with integrity
// metadata to be reused for a request that didn't itself require integrity
// checks. In that case, the integrity check can be skipped, as the integrity
// may not have been "meant" for this specific request. If the resource is
// being served from the preload cache however, we know any associated
// integrity metadata and checks were destined for this request, so we cannot
// skip the integrity check.
//
// TODO(375343417): This will also be the case for server-initiated integrity
// checks like `Identity-Digest`.
bool ForceIntegrityChecks() const { return IsLinkPreload(); }
const SubresourceIntegrity::ReportInfo& IntegrityReportInfo() const { const SubresourceIntegrity::ReportInfo& IntegrityReportInfo() const {
return integrity_report_info_; return integrity_report_info_;
} }