Validate hash_sha256 checksum on .crx update.

Validate hash_sha256 checksum on .crx update.

BUG=338837
Committed: https://crrev.com/553af32002b282cf9bdd823e475f5646e1ab7182
Cr-Commit-Position: refs/heads/master@{#314547}

[Ivan Podogov, 2014-12-30 13:19:02]

ginkage@chromium.org changed reviewers:
+ dpolukhin@chromium.org

[Ivan Podogov, 2014-12-30 13:25:08]

Added "--enable-crx-hash-check" command line option and
"Extensions.SandboxUnpackHashCheck" UMA histogram (boolean, the hash check
result, updates always when the "hash_sha256" is present in the manifest, but
only fails installation if the command line option was specified) and three unit
tests (hash sum not checked, is incorrect, is correct).

[Dmitry Polukhin, 2015-01-15 12:45:13]

lgtm

https://codereview.chromium.org/829583002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_external_updater.h (right):

https://codereview.chromium.org/829583002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.h:48: ~ExternalUpdate();
nit, if they are empty, we shouldn't need them at all.

https://codereview.chromium.org/829583002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/sandboxed_unpacker.cc (right):

https://codereview.chromium.org/829583002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/sandboxed_unpacker.cc:613: uint8
output3[crypto::kSHA256Length];
Why so strange name 'output3'?

https://codereview.chromium.org/829583002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/sandboxed_unpacker.cc:619: if (check_crx_hash_) {
I would also print error message to log.

[ginkage, 2015-01-15 15:20:14]

ginkage@google.com changed reviewers:
+ ginkage@google.com

[ginkage, 2015-01-15 15:20:15]

https://codereview.chromium.org/829583002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_external_updater.h (right):

https://codereview.chromium.org/829583002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.h:48: ~ExternalUpdate();
On 2015/01/15 12:45:13, Dmitry Polukhin wrote:
> nit, if they are empty, we shouldn't need them at all.

I've tried. :)

../../chrome/browser/chromeos/app_mode/kiosk_external_updater.h:46:3: error:
[chromium-style] Complex class/struct needs an explicit out-of-line constructor.
  struct ExternalUpdate {
  ^
../../chrome/browser/chromeos/app_mode/kiosk_external_updater.h:46:3: error:
[chromium-style] Complex class/struct needs an explicit out-of-line destructor.
2 errors generated.

[ginkage, 2015-01-15 19:58:05]

ginkage@google.com changed reviewers:
+ asargent@chromium.org

[ginkage, 2015-01-15 19:58:06]

asargent@, please owner review.

[asargent_no_longer_on_chrome, 2015-01-16 19:13:06]

I'm a little uncomfortable with how many places you needed to modify to add
knowledge of the crx file hashes. (I think this is partly a sign that
independent of your CL, we need some refactoring to isolate various concerns). I
need to think about it a bit more to come up with more concrete suggestions, but
off the top of my head I hope we can avoid having to add knowledge of the
concept of file hashes in the following files:

sandboxed_unpacker.{h,cc}
unpacker.{h,cc}
extension_service.{h,cc}
crx_installer.{h,cc}

That might mean encapsulating the information about a downloaded crx file into
one class or something.

[ginkage, 2015-01-16 21:18:09]

On 2015/01/16 19:13:06, Antony Sargent wrote:
> I'm a little uncomfortable with how many places you needed to modify to add
> knowledge of the crx file hashes. (I think this is partly a sign that
> independent of your CL, we need some refactoring to isolate various concerns).
I
> need to think about it a bit more to come up with more concrete suggestions,
but
> off the top of my head I hope we can avoid having to add knowledge of the
> concept of file hashes in the following files:
> 
> sandboxed_unpacker.{h,cc}
> unpacker.{h,cc}
> extension_service.{h,cc}
> crx_installer.{h,cc}
> 
> That might mean encapsulating the information about a downloaded crx file into
> one class or something.

In fact, we cannot avoid concept of hashes in sandboxed_unpacker.*, as that's
where the hash is being calculated (we wanted to calculate hash sum at the same
time as we check the signature, to avoid any additional file reads).
Other than that, I like an idea of a dedicated class (or, rather, a struct),
with a name like CRXFile. Things it would contain are (naming to be discussed):

{
  const std::string id_;
  const base::FilePath extension_path_;
  const std::string package_hash_;
  // We could add a bool filed should_check_hash_ as well, which depends on the
command line parameter and hash string emptiness
}

And that would actually reduce number of parameters in a number of methods,
instead of increasing it.

[asargent_no_longer_on_chrome, 2015-01-17 00:10:52]

On 2015/01/16 21:18:09, ginkage wrote:
> On 2015/01/16 19:13:06, Antony Sargent wrote:
> > I'm a little uncomfortable with how many places you needed to modify to add
> > knowledge of the crx file hashes. (I think this is partly a sign that
> > independent of your CL, we need some refactoring to isolate various
concerns).
> I
> > need to think about it a bit more to come up with more concrete suggestions,
> but
> > off the top of my head I hope we can avoid having to add knowledge of the
> > concept of file hashes in the following files:
> > 
> > sandboxed_unpacker.{h,cc}
> > unpacker.{h,cc}
> > extension_service.{h,cc}
> > crx_installer.{h,cc}
> > 
> > That might mean encapsulating the information about a downloaded crx file
into
> > one class or something.
> 
> In fact, we cannot avoid concept of hashes in sandboxed_unpacker.*, as that's
> where the hash is being calculated (we wanted to calculate hash sum at the
same
> time as we check the signature, to avoid any additional file reads).
> Other than that, I like an idea of a dedicated class (or, rather, a struct),
> with a name like CRXFile. Things it would contain are (naming to be
discussed):
> 
> {
>   const std::string id_;
>   const base::FilePath extension_path_;
>   const std::string package_hash_;
>   // We could add a bool filed should_check_hash_ as well, which depends on
the
> command line parameter and hash string emptiness
> }
> 
> And that would actually reduce number of parameters in a number of methods,
> instead of increasing it.

As discussed in person, I agree that using a struct/class to encapsulate the
state about a downloaded .crx file would be helpful as you point out above.

Also I think it's worth investigating using a specialized URLFetcherFileWriter 
inside extension_downloader.cc to compute the hash as we're downloading the .crx
file, since having that available before handing off for unpacking might be
useful.

[Dmitry Polukhin, 2015-01-17 10:02:15]

On 2015/01/17 00:10:52, Antony Sargent wrote:
> Also I think it's worth investigating using a specialized URLFetcherFileWriter

> inside extension_downloader.cc to compute the hash as we're downloading the
.crx
> file, since having that available before handing off for unpacking might be
> useful.

Special URLFetcherFileWriter won't help for verifying CRX that come from cache.
Therefore IMHO we need check during install anyway. Make check in separate pass
is not efficient because we already read/copy this file several times + checking
signature so adding more more pass will make thing even worse.  I would
consolidate all reading/checking/unpacking in single pass.

[ginkage, 2015-01-27 15:40:56]

So, here's a new draft.
I don't really like the structure name (there is already a CrxFile structure in
components/crx_file/), any suggestions?
Other than that, the only files that are now aware of the hash existence are:
chrome/browser/extensions/sandboxed_unpacker{.h,.cc,_unittest.cc}
chrome/browser/extensions/startup_helper.cc
extensions/browser/updater/extension_downloader.cc
which seems kinda neat.

[asargent_no_longer_on_chrome, 2015-01-28 21:55:06]

This is looking better - I have a couple of suggestions here and there.

https://codereview.chromium.org/829583002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/sandboxed_unpacker.cc (right):

https://codereview.chromium.org/829583002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/sandboxed_unpacker.cc:440: static size_t
fread_sha256(void* ptr,
nit: instead of fread_sha256, a better name might be something like ReadAndHash
or something like that (style guide encourages title case names for functions
that do significant work, and also technically crypto::SecureHash abstracts
which hash function is being used, and this function doesn't need to know that
detail.

https://codereview.chromium.org/829583002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/sandboxed_unpacker.cc:631: }
nit: this ValidateSignature function is getting pretty long - it would probably
be better to encapsulate this code block into a helper function.

https://codereview.chromium.org/829583002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/startup_helper.cc (right):

https://codereview.chromium.org/829583002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/startup_helper.cc:227: std::string hash =
cmd_line.GetSwitchValueASCII(switches::kValidateCrxHash);
How important is it to provide this hash validation option? In general we're
trying to reduce the number of command line flags in chrome, and be pretty
conservative about when to add them. 

Anyone interested in checking the hash of a .crx file can easily use commonly
available command line tools in linux/OSX/cygwin to compute them (eg
md5sum/sha256sum/etc. from gnu coreutils, or the openssl command line tool).

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
File extensions/browser/crx_file.cc (right):

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
extensions/browser/crx_file.cc:16: }
consider adding DCHECK(!path.empty()) in body of each of these constructors

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
File extensions/browser/crx_file.h (right):

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
extensions/browser/crx_file.h:15: struct CRXFile {
Here are some suggestions for alternative names to avoid the name clash with
CRXFile in components/crx_file/:

CRXFileInfo
CRXInstallData
CRXMetadata
CRXToInstall

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
extensions/browser/crx_file.h:23: bool operator==(const CRXFile& that) const;
Where do we need an equality operator? Perhaps I missed it, but do we end up
either directly comparing them anywhere, or putting them in an STL container
somewhere or something?

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
extensions/browser/crx_file.h:27: std::string hash;
I think it would be useful to make 2 changes:

Rename |extension_id| and |hash| to |expected_id| and |expected_hash|, to make
it clear that these are things we need to verify. 

Add comments explicitly stating that if they are empty, we simply don't have an
expected value.

[Ivan Podogov, 2015-01-29 09:26:18]

New CL ready.

https://codereview.chromium.org/829583002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/startup_helper.cc (right):

https://codereview.chromium.org/829583002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/startup_helper.cc:227: std::string hash =
cmd_line.GetSwitchValueASCII(switches::kValidateCrxHash);
On 2015/01/28 21:55:06, Antony Sargent wrote:
> How important is it to provide this hash validation option? In general we're
> trying to reduce the number of command line flags in chrome, and be pretty
> conservative about when to add them. 
> 
> Anyone interested in checking the hash of a .crx file can easily use commonly
> available command line tools in linux/OSX/cygwin to compute them (eg
> md5sum/sha256sum/etc. from gnu coreutils, or the openssl command line tool). 

We may as well skip this, of course. I don't clearly remember why I added this,
probably just to keep things neatly organized and uniform.

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
File extensions/browser/crx_file.h (right):

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
extensions/browser/crx_file.h:15: struct CRXFile {
On 2015/01/28 21:55:06, Antony Sargent wrote:
> Here are some suggestions for alternative names to avoid the name clash with
> CRXFile in components/crx_file/:
> 
> CRXFileInfo
> CRXInstallData
> CRXMetadata
> CRXToInstall

CRXFileInfo it is, then.

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
extensions/browser/crx_file.h:23: bool operator==(const CRXFile& that) const;
On 2015/01/28 21:55:06, Antony Sargent wrote:
> Where do we need an equality operator? Perhaps I missed it, but do we end up
> either directly comparing them anywhere, or putting them in an STL container
> somewhere or something?
> 

That's for unit tests, at
chrome/browser/extensions/updater/extension_updater_unittest.cc : 1224

EXPECT_CALL(delegate, OnExtensionDownloadFinished(
      CRXFileInfo(id, extension_file_path, hash), _, _,
      version.GetString(), _, requests));

would check the arguments for equality.

https://codereview.chromium.org/829583002/diff/180001/extensions/browser/crx_...
extensions/browser/crx_file.h:27: std::string hash;
On 2015/01/28 21:55:06, Antony Sargent wrote:
> I think it would be useful to make 2 changes:
> 
> Rename |extension_id| and |hash| to |expected_id| and |expected_hash|, to make
> it clear that these are things we need to verify. 
> 
> Add comments explicitly stating that if they are empty, we simply don't have
an
> expected value.
> 

I would like to avoid that: right now, in
chrome/browser/extensions/updater/extension_updater.{h,cc} I could subclass
FetchedCRXFile from the new struct, keeping the original names and passing that
subclass instance itself to UpdateExtension. This basically just keeps thing a
little bit neater.

[asargent_no_longer_on_chrome, 2015-02-03 00:42:55]

Ok, just a few more small issues. Sorry to be so nit picky!

https://codereview.chromium.org/829583002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater.cc (right):

https://codereview.chromium.org/829583002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.cc:195: : CRXFileInfo(file),
As FYI, if we were to continue inheritance instead of composition (I don't think
we should), relying on the constructor-provided default constructor here would
be outlawed by the style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova...

"Due to the risk of slicing, avoid providing an assignment operator or public
copy/move constructor for a class that's intended to be derived from (and avoid
deriving from a class with such members). If your base class needs to be
copyable, provide a public virtual Clone() method, and a protected copy
constructor that derived classes can use to implement it. "

https://codereview.chromium.org/829583002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater.h (right):

https://codereview.chromium.org/829583002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.h:140: struct FetchedCRXFile
: public CRXFileInfo {
I think we'd be better off using composition (have a FetchedCRXFile member
variable) instead of inheritance here.

See style guide entry which encourages this:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance

https://codereview.chromium.org/829583002/diff/220001/extensions/browser/sand...
File extensions/browser/sandboxed_unpacker.h (right):

https://codereview.chromium.org/829583002/diff/220001/extensions/browser/sand...
extensions/browser/sandboxed_unpacker.h:15: #include "crypto/secure_hash.h"
nit: I think you can just add a forward declare below and move the #include to
the .cc file. (Similar to how we do for base::DictionaryValue,
base::SequencedTaskRunner, etc.)

https://codereview.chromium.org/829583002/diff/220001/extensions/common/updat...
File extensions/common/update_manifest.cc (right):

https://codereview.chromium.org/829583002/diff/220001/extensions/common/updat...
extensions/common/update_manifest.cc:190: // package_hash is optional. It is
only required for blacklist. It is a
nit: remove the "It is only required for blacklist." part of this comment, since
it is now outdated. 

(We used to fetch the extension blacklist through the extension autoupdate
system, but we now use the Safe Browsing infrastructure code instead)

BTW, I assume you've verified that the Omaha server output actually already
includes this hash_sha256 attribute?

[Ivan Podogov, 2015-02-03 07:43:55]

New iteration. :)

https://codereview.chromium.org/829583002/diff/220001/extensions/common/updat...
File extensions/common/update_manifest.cc (right):

https://codereview.chromium.org/829583002/diff/220001/extensions/common/updat...
extensions/common/update_manifest.cc:190: // package_hash is optional. It is
only required for blacklist. It is a
On 2015/02/03 00:42:55, Antony Sargent wrote:
> BTW, I assume you've verified that the Omaha server output actually already
> includes this hash_sha256 attribute?

Yes, although it's not always correct (as I've already told you when discussed
in person), that's why command-line option and histogram were added.

[Ivan Podogov, 2015-02-03 09:53:01]

ginkage@chromium.org changed reviewers:
+ isherman@chromium.org
- ginkage@google.com

[Ivan Podogov, 2015-02-03 09:53:42]

isherman@, please owner-review histograms.

[asargent_no_longer_on_chrome, 2015-02-03 19:26:33]

lgtm w/ a few more small things

https://codereview.chromium.org/829583002/diff/260001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater.cc (right):

https://codereview.chromium.org/829583002/diff/260001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.cc:201: : info(),
file_ownership_passed(true) {
do you need to explicitly call the default constructor here?

https://codereview.chromium.org/829583002/diff/260001/extensions/browser/crx_...
File extensions/browser/crx_file_info.h (right):

https://codereview.chromium.org/829583002/diff/260001/extensions/browser/crx_...
extensions/browser/crx_file_info.h:29: std::string hash;
I still think it would be better to rename


extension_id -> expected_extension_id
hash -> expected_hash

to avoid the possibility that consumers of this class in other unrelated code
will get confused and rely on the id/hash properties as representing "correct"
information that has already been verified with the contents of the file at
|path| and can be relied on for some security sensitive operation.

https://codereview.chromium.org/829583002/diff/260001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/829583002/diff/260001/extensions/extensions.g...
extensions/extensions.gyp:669: 'browser/crx_file_info.h',
Please also add these files to extensions/browser/BUILD.gn

(we don't use the gn build system by default yet, but we're trying to keep the
config files for it up to date)

[Ilya Sherman, 2015-02-03 21:40:29]

LGTM % comments.

https://codereview.chromium.org/829583002/diff/260001/extensions/browser/sand...
File extensions/browser/sandboxed_unpacker.cc (right):

https://codereview.chromium.org/829583002/diff/260001/extensions/browser/sand...
extensions/browser/sandboxed_unpacker.cc:67: #define
CRX_HASH_CHECK_HISTOGRAM(name, success) \
Why do you need a wrapper macro?

https://codereview.chromium.org/829583002/diff/260001/extensions/browser/sand...
extensions/browser/sandboxed_unpacker.cc:449:
CRX_HASH_CHECK_HISTOGRAM("Extensions.SandboxUnpackHashCheck", true);
I'd recommend having a single code path per histogram name, so that there's less
chance of typos.  Concretely, I'd recommend factoring out a helper function for
this logging.

[Ilya Sherman, 2015-02-03 21:41:18]

https://codereview.chromium.org/829583002/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/829583002/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8949: +<histogram
name="Extensions.SandboxUnpackHashCheck">
Actually, please also associate an enum with this histogram.  I'm thinking the
values would be something like "Valid hash sum" and "Invalid hash sum".

[Ivan Podogov, 2015-02-04 07:49:52]

New patchsets have been uploaded after l-g-t-m from
asargent@chromium.org,isherman@chromium.org

[Ivan Podogov, 2015-02-04 07:53:04]

The CQ bit was checked by ginkage@chromium.org

[commit-bot: I haz the power, 2015-02-04 07:54:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829583002/280001

[commit-bot: I haz the power, 2015-02-04 08:47:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 08:47:51]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Ivan Podogov, 2015-02-04 09:07:51]

The CQ bit was checked by ginkage@chromium.org

[commit-bot: I haz the power, 2015-02-04 09:08:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829583002/300001

[commit-bot: I haz the power, 2015-02-04 11:27:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 11:27:46]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ivan Podogov, 2015-02-04 11:34:44]

The CQ bit was checked by ginkage@chromium.org

[commit-bot: I haz the power, 2015-02-04 11:35:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829583002/300001

[commit-bot: I haz the power, 2015-02-04 12:39:45]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-02-04 12:40:54]

Patchset 16 (id:??) landed as
https://crrev.com/553af32002b282cf9bdd823e475f5646e1ab7182
Cr-Commit-Position: refs/heads/master@{#314547}