New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 792538 link

Starred by 5 users

Issue metadata

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


Show other hotlists

Hotlists containing this issue:
Hotlist-3


Sign in to add a comment

Improve extension content verification logic when the extension requests a resource at folder urls

Project Member Reported by proberge@chromium.org, Dec 6 2017

Issue description

In https://bugs.chromium.org/p/chromium/issues/detail?id=791929 we provided a quick fix for extensions which were erroneously marked as corrupted by the content verifier.

A better fix would involve:

1. 
""
As @veranika pointed out, a better long term solution would likely involve ignoring subfolders as well. 

An extension shouldn't corrupt itself just for making a request to one of its existing folders. 
""

2. 
""
ContentVerification actually does a lot of work (e.g., even just checking if the path exists can be expensive).  Can we move this relative_path_ check significantly higher in the execution flow to save a bunch of unnecessary cost?
""
 
FWIW I quick fixed this recently in Opera by ignoring requests to directories ie.:

   if (!verified_contents.HasTreeHashRoot(relative_path_)) {
     // Making a request to a non-existent resource should not result in
     // content verification failure.
-    if (!base::PathExists(extension_root_.Append(relative_path_)))
+    if (!base::PathExists(extension_root_.Append(relative_path_)) ||
+        base::DirectoryExists(extension_root_.Append(relative_path_)))
       file_missing_from_verified_contents_ = true;
 
     return false;

After further investigation:

The second suggestion (moving the relative_path_ check higher in the execution flow) might not be feasible. base::PathExists is an IO blocking call, which would not be allowed in ExtensionProtocolHandler::MaybeCreateJob. The check is fine in ContentHashReader::Init() since it's launched by 
base::PostTaskWithTraitsAndReplyWithResult(
      FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
      base::Bind(&ContentHashReader::Init, hash_reader_),
      base::Bind(&ContentVerifyJob::OnHashesReady, this));

The suggested fix by @wmaslowski seems reasonable. I sent out https://chromium-review.googlesource.com/c/chromium/src/+/894162 which contains the base::DirectoryExists logic. 
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 1 2018

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

commit bfc5ba78ca27a45a099bb1fac477822e49dfa266
Author: proberge <proberge@chromium.org>
Date: Thu Feb 01 22:11:19 2018

Extension content verification: improve corruption detection logic

In https://bugs.chromium.org/p/chromium/issues/detail?id=791929 we
provided a quick fix for extensions which were erroneously marked as
corrupted by the content verifier. The fix involved checking for empty
paths. However, extensions could also make requests to one of their
folders and trigger the content verifier.

Bug:  792538 
Change-Id: Ie5c62e6e54481bcd4670e8b19cc5608268014494
Reviewed-on: https://chromium-review.googlesource.com/894162
Commit-Queue: proberge <proberge@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533826}
[modify] https://crrev.com/bfc5ba78ca27a45a099bb1fac477822e49dfa266/extensions/browser/content_hash_reader.cc
[modify] https://crrev.com/bfc5ba78ca27a45a099bb1fac477822e49dfa266/extensions/browser/content_verifier.cc
[modify] https://crrev.com/bfc5ba78ca27a45a099bb1fac477822e49dfa266/extensions/browser/content_verify_job_unittest.cc

Status: Fixed (was: Assigned)
 Issue 814315  has been merged into this issue.
Labels: Merge-Request-65
Would it be appropriate to merge this to M65? As noted in 814315, this issue would allow a web-based attacker to brick a visiting user's extensions if they have any web-accessible paths configured.
Project Member

Comment 7 by sheriffbot@chromium.org, Feb 21 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
I don't have access to 814315; please update the priority of this bug if there's signs of active abuse that would be fixed by merging this to M65.

Otherwise, it's unlikely that a P3 bug is going to be merged.
I've added you to the CC on  Issue 814315 . From a security triage[1] point-of-view, this is probably Severity-Low (P2), although we don't have examples that map perfectly to this scenario. If this would be a risky merge, I think we can skip it.

[1] https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md
Labels: -Pri-3 Pri-2
Thanks for the additional info!

This shouldn't be a risky merge. Updating to P2. 
Cc: awhalley@chromium.org
+awhalley@ for M65 merge review per comments #6 and #9, this is related to security.
Labels: -Type-Bug Security_Impact-Stable Security_Severity-Low Type-Bug-Security
Thanks for the info all. I'm OK waiting for 66 to pick this up.
Labels: -Merge-Review-65 Merge-Rejected-65 M-66
Cc: avi...@arpeely.com
+Reporter of 814315.
Project Member

Comment 15 by sheriffbot@chromium.org, Feb 22 2018

Labels: Restrict-View-SecurityNotify
Labels: Release-0-M66
Project Member

Comment 17 by sheriffbot@chromium.org, May 18 2018

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment