[gin] Fingerprint the V8 snapshot files on Windows and verify before loading the snapshot.

[gin] Fingerprint the V8 snapshot files on Windows and verify before loading the snapshot.

Adds code which generates a SHA256 fingerprint of the external V8 snapshot
blob files during the build process, and embeds these values into the
binary.  During startup the SHA256 hash of the snapshot blobs are compared
against these fingerprints and Chrome will fail to load if they differ.

BUG=421063, 439661
Committed: https://crrev.com/da69cd0f7c2f92640b7d00e59f66eb3b9fabba2e
Cr-Commit-Position: refs/heads/master@{#310755}

[rmcilroy, 2015-01-07 13:56:32]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2015-01-07 13:56:38]

Patchset #1 (id:20001) has been deleted

[rmcilroy, 2015-01-07 14:06:43]

Patchset #1 (id:40001) has been deleted

[rmcilroy, 2015-01-07 14:14:12]

rmcilroy@chromium.org changed reviewers:
+ davidben@chromium.org, jochen@chromium.org

[rmcilroy, 2015-01-07 14:14:12]

jochen: please review changes in gin.
davidben: please review addition of crypto to gin DEPS file.

gab/grt: would this cover the issues you mentioned in crbug.com/421063#c29 ?

[jochen (gone - plz use gerrit), 2015-01-07 15:17:38]

jochen@chromium.org changed reviewers:
+ dslomov@chromium.org

[jochen (gone - plz use gerrit), 2015-01-07 15:17:38]

can you also hack this into the gin/BUILD.gn file?

maybe we should restrict this to windows, as computing the fingerprint will be
expensive?

[davidben, 2015-01-07 15:43:20]

On 2015/01/07 14:14:12, rmcilroy wrote:
> davidben: please review addition of crypto to gin DEPS file.

gin/DEPS lgtm

[grt (UTC plus 2), 2015-01-07 19:27:18]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2015-01-07 19:27:19]

approach looks okay to me. can you make it work for the GN build, too?

https://codereview.chromium.org/832393003/diff/60001/gin/fingerprint/fingerpr...
File gin/fingerprint/fingerprint_v8_snapshot.py (right):

https://codereview.chromium.org/832393003/diff/60001/gin/fingerprint/fingerpr...
gin/fingerprint/fingerprint_v8_snapshot.py:18: _HEADER = """// Copyright (c)
2015 The Chromium Authors. All rights reserved.
nit: no "(c)" in new copyright notices
(http://www.chromium.org/developers/coding-style#TOC-File-headers)

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:79: scoped_ptr<crypto::SecureHash> hash(
can you use crypto::SHA256HashString(base::StringPiece(snapshot_file->data(),
snapshot_file->length()), output, sizeof(output)); in place of lines 79-83?

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:84: for (size_t i = 0; i < crypto::kSHA256Length; i++) {
?
  return !memcmp(fingerprint, output, sizeof(output));

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:107: // Declared in gen/gin/v8_snapshot_fingerprint.cc
Declared -> Defined (these are declarations here)

[rmcilroy, 2015-01-08 15:05:35]

Added Gin support and made this Windows only, PTAL, thanks.

https://codereview.chromium.org/832393003/diff/60001/gin/fingerprint/fingerpr...
File gin/fingerprint/fingerprint_v8_snapshot.py (right):

https://codereview.chromium.org/832393003/diff/60001/gin/fingerprint/fingerpr...
gin/fingerprint/fingerprint_v8_snapshot.py:18: _HEADER = """// Copyright (c)
2015 The Chromium Authors. All rights reserved.
On 2015/01/07 19:27:18, grt wrote:
> nit: no "(c)" in new copyright notices
> (http://www.chromium.org/developers/coding-style#TOC-File-headers)

Done.

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:79: scoped_ptr<crypto::SecureHash> hash(
On 2015/01/07 19:27:19, grt wrote:
> can you use crypto::SHA256HashString(base::StringPiece(snapshot_file->data(),
> snapshot_file->length()), output, sizeof(output)); in place of lines 79-83?

Done.

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:84: for (size_t i = 0; i < crypto::kSHA256Length; i++) {
On 2015/01/07 19:27:19, grt wrote:
> ?
>   return !memcmp(fingerprint, output, sizeof(output)); 

Done.

https://codereview.chromium.org/832393003/diff/60001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:107: // Declared in gen/gin/v8_snapshot_fingerprint.cc
On 2015/01/07 19:27:19, grt wrote:
> Declared -> Defined (these are declarations here)

Done.

[jochen (gone - plz use gerrit), 2015-01-08 15:11:53]

https://codereview.chromium.org/832393003/diff/80001/gin/BUILD.gn
File gin/BUILD.gn (right):

https://codereview.chromium.org/832393003/diff/80001/gin/BUILD.gn#newcode73
gin/BUILD.gn:73: if (v8_use_external_startup_data) {
shouldn't that be v8_use_external_startup_data && windows?

https://codereview.chromium.org/832393003/diff/80001/gin/gin.gyp
File gin/gin.gyp (right):

https://codereview.chromium.org/832393003/diff/80001/gin/gin.gyp#newcode82
gin/gin.gyp:82: ['v8_use_external_startup_data==1', {
same here?

https://codereview.chromium.org/832393003/diff/80001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/80001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:76: bool VerifyV8SnapshotFile(base::MemoryMappedFile*
snapshot_file,
#if defined(OS_WIN)

https://codereview.chromium.org/832393003/diff/80001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:139: bool IsolateHolder::LoadAndVerifyV8Snapshot() {
#if OS_WIN

[rmcilroy, 2015-01-08 16:29:21]

https://codereview.chromium.org/832393003/diff/80001/gin/BUILD.gn
File gin/BUILD.gn (right):

https://codereview.chromium.org/832393003/diff/80001/gin/BUILD.gn#newcode73
gin/BUILD.gn:73: if (v8_use_external_startup_data) {
On 2015/01/08 15:11:53, jochen (slow) wrote:
> shouldn't that be v8_use_external_startup_data && windows?

Done.

https://codereview.chromium.org/832393003/diff/80001/gin/gin.gyp
File gin/gin.gyp (right):

https://codereview.chromium.org/832393003/diff/80001/gin/gin.gyp#newcode82
gin/gin.gyp:82: ['v8_use_external_startup_data==1', {
On 2015/01/08 15:11:53, jochen (slow) wrote:
> same here?

Done.

https://codereview.chromium.org/832393003/diff/80001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/80001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:76: bool VerifyV8SnapshotFile(base::MemoryMappedFile*
snapshot_file,
On 2015/01/08 15:11:53, jochen (slow) wrote:
> #if defined(OS_WIN)

Done.

https://codereview.chromium.org/832393003/diff/80001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:139: bool IsolateHolder::LoadAndVerifyV8Snapshot() {
On 2015/01/08 15:11:53, jochen (slow) wrote:
> #if OS_WIN

Done (and also ifdeffed out LoadV8Snapshot on Windows as discussed offline)

[jochen (gone - plz use gerrit), 2015-01-08 16:30:43]

lgtm, thx

[grt (UTC plus 2), 2015-01-08 17:46:35]

https://codereview.chromium.org/832393003/diff/120001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/832393003/diff/120001/content/app/content_mai...
content/app/content_main_runner.cc:741:
CHECK(gin::IsolateHolder::LoadAndVerifyV8Snapshot());
why should consumers need to know which platforms verify and which don't? it
seems cleaner to hide the verify-or-dont-verify smarts entirely within
gin::IsolateHolder.

https://codereview.chromium.org/832393003/diff/120001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/120001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:76: #if defined(OS_WIN)
i can imagine other platforms eventually wanting the verified path as well (why
not on OSX?). how about switching from OS_WIN to something like
V8_VERIFY_EXTERNAL_STARTUP_DATA that is currently only set for OS_WIN?

https://codereview.chromium.org/832393003/diff/120001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:104: // Declared in gen/gin/v8_snapshot_fingerprint.cc
ping: Declared -> Defined

[rmcilroy, 2015-01-08 18:54:15]

Comments addressed, thanks.

https://codereview.chromium.org/832393003/diff/120001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/832393003/diff/120001/content/app/content_mai...
content/app/content_main_runner.cc:741:
CHECK(gin::IsolateHolder::LoadAndVerifyV8Snapshot());
On 2015/01/08 17:46:35, grt wrote:
> why should consumers need to know which platforms verify and which don't? it
> seems cleaner to hide the verify-or-dont-verify smarts entirely within
> gin::IsolateHolder.

Sounds reasonable, done.

https://codereview.chromium.org/832393003/diff/120001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/120001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:76: #if defined(OS_WIN)
On 2015/01/08 17:46:35, grt wrote:
> i can imagine other platforms eventually wanting the verified path as well
(why
> not on OSX?). how about switching from OS_WIN to something like
> V8_VERIFY_EXTERNAL_STARTUP_DATA that is currently only set for OS_WIN?

Done.

https://codereview.chromium.org/832393003/diff/120001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:104: // Declared in gen/gin/v8_snapshot_fingerprint.cc
On 2015/01/08 17:46:35, grt wrote:
> ping: Declared -> Defined

Oops, sorry missed this. Done.

[grt (UTC plus 2), 2015-01-08 19:19:02]

lgtm with a comment nit

https://codereview.chromium.org/832393003/diff/140001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/140001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:26: #endif  // defined(OS_MACOSX)
i believe that
#endif  // FOO
is much more prevalent in the codebase than
#endif  // defined(FOO)

unless everything in gin/ does it the way you have it here, please stick with
the more popular way. thanks.

[rmcilroy, 2015-01-09 10:27:18]

Thanks for the reviews.

https://codereview.chromium.org/832393003/diff/140001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/832393003/diff/140001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:26: #endif  // defined(OS_MACOSX)
On 2015/01/08 19:19:02, grt wrote:
> i believe that
> #endif  // FOO
> is much more prevalent in the codebase than
> #endif  // defined(FOO)
> 
> unless everything in gin/ does it the way you have it here, please stick with
> the more popular way. thanks.

It looks like it's done both ways in gin, but changed this code to be consistent
with "#endif  // FOO".

[rmcilroy, 2015-01-09 10:27:36]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-01-09 10:27:56]

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

[commit-bot: I haz the power, 2015-01-09 11:31:49]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-01-09 11:32:42]

Patchset 6 (id:??) landed as
https://crrev.com/da69cd0f7c2f92640b7d00e59f66eb3b9fabba2e
Cr-Commit-Position: refs/heads/master@{#310755}