New issue
Advanced search Search tips

Issue 703888 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 703892

Blocking:
issue 699420



Sign in to add a comment

File content corruption isn't detected in for 0 byte corruption, unreadable file, or missing file

Project Member Reported by lazyboy@chromium.org, Mar 21 2017

Issue description

There are 3 cases of file corruption/deletion in extensions I've found that is not detected by our code (primarily by URLRequestExtensionJob):

1) If a file is 0 bytes, URLRequestExtensionJob never sees OnReadComplete.

2) If a file is missing, we should be overriding URLRequestFileJob::OnOpenComplete and detect that.

3) If a file doesn't have read permissions (can happen during corruption), #2 applies.

The result is that since URLRequestExtensionJob::OnReadComplete never fires, ContentVerifyJob::DoneReading() doesn't get called and content verification never kicks in.

Notes about #1: a 0 byte file doesn't necessarily mean that it's due to corruption. An extension can legitimately have a 0 byte file in it.
 
Blockedon: 703892
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 30 2017

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

commit d6dbb26e31818eed0097654ecb89aaddc2c64f8d
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Mar 30 00:57:30 2017

Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

Also make ContentHashReader not skip legitimate deleted files: the
files that are listed in verified_contents.json. It will still skip
non-existent resources, e.g. xhrs requesting non-existent file (see
 crbug.com/404802  for example).

BUG= 703888 
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.

Review-Url: https://codereview.chromium.org/2771953003
Cr-Commit-Position: refs/heads/master@{#460607}

[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/chrome/browser/extensions/content_verifier_browsertest.cc
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/chrome/browser/extensions/extension_protocols_unittest.cc
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/BUILD.gn
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/content_hash_reader.cc
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/content_hash_reader.h
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/content_verify_job.cc
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/content_verify_job.h
[add] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/content_verify_job_unittest.cc
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/extension_protocols.cc
[modify] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/browser/scoped_ignore_content_verifier_for_test.h
[add] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/test/data/content_hash_fetcher/with_verified_contents/README.txt
[add] https://crrev.com/d6dbb26e31818eed0097654ecb89aaddc2c64f8d/extensions/test/data/content_hash_fetcher/with_verified_contents/source_all.zip

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 30 2017

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

commit e83ab9c6c27deb3e96dee4a33a6518c206724d16
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Mar 30 03:18:26 2017

Make content verification see 0-byte files.

Currently ContentVerifyJob never sees DoneReading() for (legitimate
or not) 0-byte files. This means 0-byte files (legitimate or corrupted)
never goes through content verification process.
Override URLRequestJob::DoneReading() in URLRequestExtensionJob to
detect the end of 0-byte files so that ContentVerifyJob knows about
these 0-byte files.

Add a test to ensure that URLRequestExtensionJob will see 0-byte files.
ContentVerifyJobUnittest.DeletedAndMissingFiles already makes sure
that 0-byte read will fail content verification.

BUG= 703888 , 703892 
Test=With content verification turned on (e.g. with
--extension-content-verification=enforce_strict flag), install an
extension from webstore and modify a JS content inside that extension
to 0 byte file. Content verification should fail for that extension.
If the extension is part of system policy, the extension will be
reinsalled.

Review-Url: https://codereview.chromium.org/2779083005
Cr-Commit-Position: refs/heads/master@{#460638}

[modify] https://crrev.com/e83ab9c6c27deb3e96dee4a33a6518c206724d16/chrome/browser/extensions/extension_protocols_unittest.cc
[modify] https://crrev.com/e83ab9c6c27deb3e96dee4a33a6518c206724d16/extensions/browser/extension_protocols.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 30 2017

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

commit 6665648f820bad8de48fc0699ba315696b6dd127
Author: lazyboy <lazyboy@chromium.org>
Date: Thu Mar 30 07:07:53 2017

Add a ContentVerifyJob test for sanity checking legitimate 0 byte file.

There is no restriction that an extension can upload zero byte resource.
Add a test to test that behavior to make sure content verificaton works
as expected.

Take the opportunity to refactor repeated code into RunContentVerifyJob
function.

BUG= 703888 
Test=No visible change.

Review-Url: https://codereview.chromium.org/2786803002
Cr-Commit-Position: refs/heads/master@{#460685}

[modify] https://crrev.com/6665648f820bad8de48fc0699ba315696b6dd127/extensions/browser/content_verify_job_unittest.cc
[add] https://crrev.com/6665648f820bad8de48fc0699ba315696b6dd127/extensions/test/data/content_hash_fetcher/zero_byte_file/README.txt
[add] https://crrev.com/6665648f820bad8de48fc0699ba315696b6dd127/extensions/test/data/content_hash_fetcher/zero_byte_file/source.zip

Status: Fixed (was: Assigned)

Sign in to add a comment