Improve extension content verification logic when the extension requests a resource at folder urls |
||||||||||||
Issue descriptionIn 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? ""
,
Jan 30 2018
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.
,
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
,
Feb 8 2018
,
Feb 21 2018
Issue 814315 has been merged into this issue.
,
Feb 21 2018
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.
,
Feb 21 2018
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
,
Feb 21 2018
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.
,
Feb 21 2018
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
,
Feb 21 2018
Thanks for the additional info! This shouldn't be a risky merge. Updating to P2.
,
Feb 21 2018
+awhalley@ for M65 merge review per comments #6 and #9, this is related to security.
,
Feb 21 2018
Thanks for the info all. I'm OK waiting for 66 to pick this up.
,
Feb 21 2018
,
Feb 22 2018
+Reporter of 814315.
,
Feb 22 2018
,
Apr 17 2018
,
May 18 2018
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 |
||||||||||||
Comment 1 by wmaslow...@opera.com
, Dec 12 2017FWIW 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;