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

Issue 834794 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Do not treat requests for missing files as evidence of Extension corruption?

Project Member Reported by elawrence@chromium.org, Apr 19 2018

Issue description

Chrome Version: 65, 66.0.3359.117
OS: CrOS

What steps will reproduce the problem?
(1) Install https://chrome.google.com/webstore/detail/windows-defender-browser/bkbeeeffjjeopflfhgeknacdieedcoml/reviews?hl=en-US

OBSERVE: Extension not loaded. chrome://extensions shows "This extension may have been corrupted"

EXPECT: Extension loads.

(2) Click "Repair" then "Repair Extension"

OBSERVE: Extension unloads and chrome://extensions shows "This extension may have been corrupted"

 
This doesn't repro in Windows or Mac.

Theory: Perhaps this happens only on CrOS (and maybe Linux) due to case-sensitivity in the filesystem.

Background.html has a <script src="configs/Config.js"></script> reference, but the file in the crx is named "configs/config.js" (lowercase "c").
Reportedly, the extension works without issue in Chromium M68, so either this isn't case-insensitivity, or there's some other change that makes it work.
When I unzip the .CRX file and use the "Load Unpacked" command, Chrome shows the extension is properly loaded.

Consistent with the theory in #1, however, there are two errors in background.html, noting that ERR_FILE_NOT_FOUND for both Config.js and IconManager.js. (In the package, the file is named iconManager.js).
TIL:

chrome-extension URLs are indeed case-sensitive only on Linux and CrOS. 

The extension is functionally-broken but not marked "corrupt" on Linux because Linux does not perform Content Verification, while CrOS does.

Issue 29941 concerns the fact that lack of checks for case-sensitivity mean that you can easily write an extension that works correctly on Mac/Win and fails on CrOS and Linux, which is what happened here.

We've previously relaxed our content verification somewhat (e.g.  Issue 792538 ) such that certain URLs can be requested even if they're not in the package, but that doesn't cover this case. And the extension would still be broken anyway due to the lack of JS. 

I'll reach out to Microsoft about fixing this.
Cc: rdevlin....@chromium.org
Labels: -Pri-1 Pri-2
Summary: Do not treat requests for missing files as evidence of Extension corruption? (was: Windows Defender Chrome Extension always "Corrupt" on CrOS)
Microsoft is working to fix the issue in their extension.

For Chrome, we can continue to track the problem of case-insensitivity handling in Issue 29941.

For this issue, we might consider whether we should not deem an extension that requests a non-existent file as 'corrupt'. We already ignore invalid URL requests in some cases (e.g.  Issue 792538 ) but those accommodations don't cover the scenario encountered in this bug. I think we could safely ignore requests for URLs not in the manifest if the file referenced does not exist. (We should still treat as corrupt attempted loads URLs that are not in the manifest when the file *does* exist on disk to avoid "file planting" attacks).

An extension with this case-sensitivity problem will probably still be broken (due to its failed request for scripts or other resources), but it is much easier for devs or users to debug the FILE_NOT_FOUND error than the current "Extension corrupt" error.

WDYT?
Cc: proberge@chromium.org lazyboy@chromium.org
> chrome-extension URLs are indeed case-sensitive only on Linux and CrOS.
Filesystems are indeed case-sensitive; chrome-extension:// urls are glorified file:// urls.

This is being disabled because we request a file for which we don't have hashes.  We can't just let that fly, because of a risk of if binaries dropped a malicious file on disk that wasn't originally present in the package.  But I wonder if we could avoid a content verification failure if the file isn't in the package *and* not on disk?  proberge@ + lazyboy@, any reason to not do that?

(Note: the extension would still be broken; we'd return a 404 for the resource.  But it at least wouldn't be "corrupted".)
comment collision.  TL;DR: What elawrence@ said. :)

> For Chrome, we can continue to track the problem of case-insensitivity handling in Issue 29941.

I'm not sure this is really helpful in this scenario.  That issue is about the files specified in the manifest (where we *could* validate), but the issue here is with a <script> tag including it.  I don't think there's a reasonably sane way we could detect that at e.g. packaging time, and I don't think checking the manifest will help, so I'm not sure issue 29941 is applicable.

We could potentially throw runtime warnings, which might be helpful, but wouldn't be a panacea.  It also depends on us properly detecting the case mismatch.  I assume we can do this, but haven't actually checked our base/file* code closely enough.
Owner: proberge@chromium.org
Status: Assigned (was: Untriaged)
Triage!  Assigning to proberge@, optimistically hoping he'll be up for the challenge. :) proberge@, feel free to assign back to me if you don't have the bandwidth for this.
I'm not convinced this is solely a case-sensitivity issue.

VerifiedContents::HasTreeHashRoot uses base::ToLowerASCII to see if we have hashes for a file.
ComputedHashes::Reader::GetHashes uses base::FilePath::CompareEqualIgnoreCase to get the hashes of the file on disk.

In theory, requesting "configs/Configs.js" on CrOS should not result in content verification failure: the requested file is considered present in verified_contents.json and in computed_hashes.json; content verification should succeed. 
We have the original extension if it would be possible for you to look at it under the debugger. 

What we know today is that the "Corrupt" UI appeared on CrOS only. The extension loaded and worked properly on Mac and Windows and loaded but showed ERR_FILE_NOT_FOUND on Linux.
> But I wonder if we could avoid a content verification failure if the file isn't in the package *and* not on disk?  proberge@ + lazyboy@, any reason to not do that?

We already avoid content verification failures for files which aren't in the package and not on disk (see |file_missing_from_verified_contents|).

In this case, it seems like the case-sensitivity issue is causing the content verifier to think that the file is in the package - or that the file is on disk, but not both. From a brief look at the code (comment 9), it's not clear how this happens. 

In comment #2, you mentioned not being able to repro on Chromium 68. Was that on ChromeOS or Linux?
It didn't repro on Chromium 68 on Linux, which, it turns out, is almost certainly because Linux doesn't have Content Verification enabled.
Re #11: In the place where we set |file_missing_from_verified_contents_ = true| at [1], is the base::PathExists call case-insensitive? Should it be? 

If we bail out there, I don't think we get to the reader.GetHashes() call which seems willing to hash files whose path is in the wrong case [2].

[1] https://cs.chromium.org/chromium/src/extensions/browser/content_hash_reader.cc?l=56&rcl=8dab8cfefbdc92721e825a466b1b249ef9842443 

[2] https://cs.chromium.org/chromium/src/extensions/browser/computed_hashes.cc?l=108&rcl=d2493d2f2b2d5ea0fe2408ead141d8cc5c3e28f9
There's another condition before we set |file_missing_from_verified_contents_ = true| at [1] - we call VerifiedContents::HasTreeHashRoot. 

HasTreeHashRoot calls ToLowerASCII on the given path before looking it up in the data extracted from verified_contents.json [2]

HasTreeHashRoot("configs/Config.js") should return true if the extension has a file at "configs/config.js" - we consider that the file is in verified_contents.json and should never set file_missing_from_verified_contents_.

[1] https://cs.chromium.org/chromium/src/extensions/browser/content_hash_reader.cc?l=47&rcl=8dab8cfefbdc92721e825a466b1b249ef9842443

[2] https://cs.chromium.org/chromium/src/extensions/browser/verified_contents.cc?l=171&rcl=b372f1d8d371f231b4f8e3dce511f8a5917b854d
Could you guys maybe do a scan of the extensions in the gallery and contact developers when the extension is trying to load a file that actually has different upper / lowercase spelling?

Developers only testing their extension on Windows are likely to not realize that their extension won't work on other platforms. 

Sign in to add a comment