Beginning of support for extension content verification

Beginning of support for extension content verification

The basic idea is that the webstore will provide signed expected hashes
of file content that can be checked during runtime in the browser to
detect corruption due to disk errors or malware.

This CL has a lot of the high-level pieces, with several of the details
left out for subsequent CLs to make this one more easily digestible.

The design is that there is a ContentVerifier for each BrowserContext
which can be used anywhere we read from files inside an extension. It
vends out ContentVerifyJob's, which need to be informed of the bytes
read from each file, and will know how to check those against a set of
expected block hashes. If the job detects contents that don't match what
was expected, it will notify the verifier.


BUG=369895
R=rvargas@chromium.org, yoz@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269108

[asargent_no_longer_on_chrome, 2014-05-05 20:25:54]

yoz: please review

(It may look big at first glance but a lot of it is just plumbing things around,
with many details to be filled in with subsequent CLs)

Suggested order to look at first:

extensions/browser/content_verifier.{h,cc}
extensions/browser/content_verify_job.{h,cc}
extensions/browser/extension_system.{h,cc}
chrome/browser/extensions/extensions_system_impl.{h,cc}

Then the rest in whatever order. 

Note: I considered making ContentVerifier it's own KeyedService, but wasn't sure
whether that made sense given the usage of it in ExtensionsSystemImpl. I'd be
curious for your opinion.

[Yoyo Zhou, 2014-05-07 02:25:59]

overall design LGTM, some nits

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/content_verifier_browsertest.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/content_verifier_browsertest.cc:124:
ContentVerifyJob::SetDelegateForTests(NULL);
This doesn't seem to belong here. TearDownOnMainThread perhaps.

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_system_impl.cc:141: static bool
VerifyExtensionContentFilter(const Extension* extension) {
I'd find this clearer with a name like ShouldVerifyExtensionContent.

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_system_impl.cc:264: void
ExtensionSystemImpl::Shared::ContentVerifyFailed(
Should ExtensionService be the observer instead?

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/content_verifier.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/content_verifier.cc:119: return std::max(experiment_value,
cmdline_value);
This probably deserves some explanation.

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/content_verify_job.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/content_verify_job.cc:37: return
DispatchFailureCallback(reason);
Why are we returning from a void?

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/content_verify_job.h (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/content_verify_job.h:25: // from only one thread.
consider using a ThreadChecker

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/extension_system.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/extension_system.cc:27: return NULL;
Most of the other members of this class don't provide a default NULL
implementation. Why do it here?

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/common...
File extensions/common/switches.h (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/common...
extensions/common/switches.h:24: extern const char
kExtensionContentVerification[];
nit: this one looks like it should come first alphabetically

[asargent_no_longer_on_chrome, 2014-05-07 06:56:42]

responded to comments

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/content_verifier_browsertest.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/content_verifier_browsertest.cc:124:
ContentVerifyJob::SetDelegateForTests(NULL);
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> This doesn't seem to belong here. TearDownOnMainThread perhaps.

Done.

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_system_impl.cc:141: static bool
VerifyExtensionContentFilter(const Extension* extension) {
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> I'd find this clearer with a name like ShouldVerifyExtensionContent.

Good suggestion, done.

https://chromiumcodereview.appspot.com/266963003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_system_impl.cc:264: void
ExtensionSystemImpl::Shared::ContentVerifyFailed(
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> Should ExtensionService be the observer instead?

Yeah, I think I like that better. Done.

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/content_verifier.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/content_verifier.cc:119: return std::max(experiment_value,
cmdline_value);
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> This probably deserves some explanation.

Added a comment here and above the Mode enum in the .h file.

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/content_verify_job.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/content_verify_job.cc:37: return
DispatchFailureCallback(reason);
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> Why are we returning from a void?

Just a shorthand for:

if (reason != NONE) {
  DispatchFailureCallback(reason);
  return;
}

Since you found it surprising, clearly this idiom isn't that common, so I just
changed it to the longhand verison.

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/content_verify_job.h (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/content_verify_job.h:25: // from only one thread.
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> consider using a ThreadChecker

Good idea. Done.

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
File extensions/browser/extension_system.cc (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/browse...
extensions/browser/extension_system.cc:27: return NULL;
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> Most of the other members of this class don't provide a default NULL
> implementation. Why do it here?

Huh, I guess I was sort of thinking along the lines of this documenting the fact
that content verification is an optional feature and the rest of the
ExtensionSystem doesn't depend on having a non-null ContentVerifier, so other
kinds of ExtensionSystem's don't have to provide one if they don't want to. 

But you make a good point about this going against the prevailing style, so I've
changed it to be pure virtual and added the NULL-returning version in the other
subclasses of ExtensionSystem.

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/common...
File extensions/common/switches.h (right):

https://chromiumcodereview.appspot.com/266963003/diff/40001/extensions/common...
extensions/common/switches.h:24: extern const char
kExtensionContentVerification[];
On 2014/05/07 02:25:59, Yoyo Zhou wrote:
> nit: this one looks like it should come first alphabetically

Ugh, in this case emacs' "M-x sort-lines" (what I used) differs from the unix
command line's sort command. 

I guess this means I have to switch to vi?

...

ok I just learned vi so I could try it there and using ":'<,'>sort" (who comes
up with these commands?) agrees with emacs.

[asargent_no_longer_on_chrome, 2014-05-07 06:58:34]

rvargas: please review net/url_request/url_request_file_job.h (you can see where
this is used in extensions/browser/extension_protocols.cc)

Thanks!

[rvargas (doing something else), 2014-05-07 18:29:55]

https://codereview.chromium.org/266963003/diff/60001/extensions/browser/exten...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/266963003/diff/60001/extensions/browser/exten...
extensions/browser/extension_protocols.cc:220: if (verify_job_.get())
nit: get() is not needed for boolean conditions. (and elsewhere)

https://codereview.chromium.org/266963003/diff/60001/extensions/browser/exten...
extensions/browser/extension_protocols.cc:246: if (remaining_bytes() == result)
I think it's a little confusing that this relies on the exact place that
remaining_bytes_ is updated in relation to this callback (aka, after the
callback).

Maybe we should move the value update to the start of DidRead so that derived
classes see a consistent value of remaining_bytes(). I mean, the state should
reflect that a read completed.

[asargent_no_longer_on_chrome, 2014-05-07 20:21:51]

rvargas: PTAL

https://codereview.chromium.org/266963003/diff/60001/extensions/browser/exten...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/266963003/diff/60001/extensions/browser/exten...
extensions/browser/extension_protocols.cc:220: if (verify_job_.get())
On 2014/05/07 18:29:55, rvargas wrote:
> nit: get() is not needed for boolean conditions. (and elsewhere)

Done.

https://codereview.chromium.org/266963003/diff/60001/extensions/browser/exten...
extensions/browser/extension_protocols.cc:246: if (remaining_bytes() == result)
On 2014/05/07 18:29:55, rvargas wrote:
> I think it's a little confusing that this relies on the exact place that
> remaining_bytes_ is updated in relation to this callback (aka, after the
> callback).
> 
> Maybe we should move the value update to the start of DidRead so that derived
> classes see a consistent value of remaining_bytes(). I mean, the state should
> reflect that a read completed.

Yes, good point. I've updated URLRequestFileJob::DidRead in
url_request_file_job.cc to update remaining_bytes_ before calling
OnReadComplete, and switched this to checking for remaining_bytes() == 0 here.

[Yoyo Zhou, 2014-05-07 20:32:26]

extensions changes LGTM

[rvargas (doing something else), 2014-05-08 17:50:07]

lgtm

https://codereview.chromium.org/266963003/diff/80001/extensions/browser/exten...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/266963003/diff/80001/extensions/browser/exten...
extensions/browser/extension_protocols.cc:245: if (remaining_bytes() == 0)
Very small nit: maybe !remaining_bytes() reads better.

[asargent_no_longer_on_chrome, 2014-05-08 18:02:07]

all comments addressed, thanks for reviews

https://codereview.chromium.org/266963003/diff/80001/extensions/browser/exten...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/266963003/diff/80001/extensions/browser/exten...
extensions/browser/extension_protocols.cc:245: if (remaining_bytes() == 0)
On 2014/05/08 17:50:07, rvargas wrote:
> Very small nit: maybe !remaining_bytes() reads better.

Done.

[asargent_no_longer_on_chrome, 2014-05-08 23:54:35]

Committed patchset #6 manually as r269108 (presubmit successful).