media: Move the callsite of CdmHostFiles::InitVerification()

media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't work well on Windows. Move the call up so it's
called out of the sandbox on Windows. On other platforms this is still
called within the sandbox.

Also rename VerifyFiles() to InitVerification() to better reflect
what the call does.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.

Review-Url: https://codereview.chromium.org/2849163003
Cr-Commit-Position: refs/heads/master@{#468564}
Committed: https://chromium.googlesource.com/chromium/src/+/4ae3646b574b83fec0213bab924259cf9d4b130f

[xhwang, 2017-05-01 17:55:28]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 17:56:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 19:02:32]

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

[commit-bot: I haz the power, 2017-05-01 19:02:33]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-05-01 19:56:44]

xhwang@chromium.org changed reviewers:
+ hmchen@chromium.org

[xhwang, 2017-05-01 19:56:45]

hmchen: PTAL. After your approval, I'll add owners.

[Haoming Chen, 2017-05-01 20:28:09]

lgtm

[xhwang, 2017-05-01 20:32:51]

xhwang@chromium.org changed reviewers:
+ wfh@chromium.org, yzshen@chromium.org

[xhwang, 2017-05-01 20:32:52]

wfh: PTAL from security's perspective.
yzshen: PTAL from ppapi's perspective.
kerrnel: FYI

[Will Harris, 2017-05-01 20:59:00]

https://codereview.chromium.org/2849163003/diff/1/content/ppapi_plugin/ppapi_...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2849163003/diff/1/content/ppapi_plugin/ppapi_...
content/ppapi_plugin/ppapi_thread.cc:398: if
(!cdm_host_files->VerifyFiles(library.get(), path)) {
what exactly does VerifyFiles do?

We agreed in the thread to just open the files and store the file paths, does it
do anything more than this?

[yzshen1, 2017-05-01 21:10:07]

Ppapi LGTM (please wait for LG from security experts)

[xhwang, 2017-05-01 22:49:58]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 22:51:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-05-01 23:05:13]

Description was changed from

==========
media: Move the callsite of CdmHostFiles::VerifyFiles()

Previously CdmHostFiles::VerifyFiles() was called within the sandbox
which doesn't always work. Move the call up so it's called out of the
sandbox.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.
==========

to

==========
media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't always work. Move the call up so it's called
out of the sandbox on Windows and Mac.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.
==========

[xhwang, 2017-05-01 23:05:45]

Will: PTAL again!

https://codereview.chromium.org/2849163003/diff/1/content/ppapi_plugin/ppapi_...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2849163003/diff/1/content/ppapi_plugin/ppapi_...
content/ppapi_plugin/ppapi_thread.cc:398: if
(!cdm_host_files->VerifyFiles(library.get(), path)) {
On 2017/05/01 20:59:00, Will Harris wrote:
> what exactly does VerifyFiles do?
> 
> We agreed in the thread to just open the files and store the file paths, does
it
> do anything more than this?

As discussed offline, added links and updated function name.

[xhwang, 2017-05-01 23:06:24]

Description was changed from

==========
media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't always work. Move the call up so it's called
out of the sandbox on Windows and Mac.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.
==========

to

==========
media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't always work. Move the call up so it's called
out of the sandbox on Windows and Mac.

Also rename VerifyFiles() to InitVerification() to better reflect
what the call does.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.
==========

[Will Harris, 2017-05-01 23:22:49]

https://codereview.chromium.org/2849163003/diff/20001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2849163003/diff/20001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:395: // unsandboxed. On Linux, this is
called sandboxed.
where is the linux call now?

Also, does this have to be unsandboxed on Mac, I thought the issue of
translating file handles to files only existed in Windows-land?

[xhwang, 2017-05-01 23:57:01]

Will: PTAL again! Thanks!

https://codereview.chromium.org/2849163003/diff/20001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2849163003/diff/20001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:395: // unsandboxed. On Linux, this is
called sandboxed.
On 2017/05/01 23:22:49, Will Harris wrote:
> where is the linux call now?

CDM host verification is disabled on Linux now but this file is generic so it
supports verification on all platforms.
 
> Also, does this have to be unsandboxed on Mac, I thought the issue of
> translating file handles to files only existed in Windows-land?

I only need this change for Windows. Updated this CL to only affect Windows.

https://codereview.chromium.org/2849163003/diff/30004/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2849163003/diff/30004/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:498: #endif  //
BUILDFLAG(ENABLE_CDM_HOST_VERIFICATION) && !defined(OS_WIN)
Move this to be above "initialize_module" to be consistent with Windows. Now we
are always calling InitVerification() before InitializeModule().

[Will Harris, 2017-05-02 00:44:00]

lgtm but please change CL description to state the initialization code is only
called unsandboxed on Windows.

As discussed, I expect some progress to be made on changing this to something
else e.g. using the Windows binary verification routines, please keep me posted
on these designs.

[xhwang, 2017-05-02 04:10:32]

Description was changed from

==========
media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't always work. Move the call up so it's called
out of the sandbox on Windows and Mac.

Also rename VerifyFiles() to InitVerification() to better reflect
what the call does.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.
==========

to

==========
media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't work well on Windows. Move the call up so it's
called out of the sandbox on Windows. On other platforms this is still
called within the sandbox.

Also rename VerifyFiles() to InitVerification() to better reflect
what the call does.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.
==========

[xhwang, 2017-05-02 04:11:01]

On 2017/05/02 00:44:00, Will Harris wrote:
> lgtm but please change CL description to state the initialization code is only
> called unsandboxed on Windows.

Done.

> As discussed, I expect some progress to be made on changing this to something
> else e.g. using the Windows binary verification routines, please keep me
posted
> on these designs.

Sure. Thanks!

[xhwang, 2017-05-02 04:11:13]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-05-02 04:11:13]

The patchset sent to the CQ was uploaded after l-g-t-m from hmchen@chromium.org,
yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2849163003/#ps30004
(title: "windows-only")

[commit-bot: I haz the power, 2017-05-02 04:11:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 05:13:00]

CQ is committing da patch.

Bot data: {"patchset_id": 30004, "attempt_start_ts": 1493698273054470,
"parent_rev": "b08fe054f49f402b9e513ecc8283152f80eefb09", "commit_rev":
"4ae3646b574b83fec0213bab924259cf9d4b130f"}

[commit-bot: I haz the power, 2017-05-02 05:13:43]

Description was changed from

==========
media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't work well on Windows. Move the call up so it's
called out of the sandbox on Windows. On other platforms this is still
called within the sandbox.

Also rename VerifyFiles() to InitVerification() to better reflect
what the call does.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.
==========

to

==========
media: Move the callsite of CdmHostFiles::InitVerification()

Previously CdmHostFiles::InitVerification() was called within the
sandbox which doesn't work well on Windows. Move the call up so it's
called out of the sandbox on Windows. On other platforms this is still
called within the sandbox.

Also rename VerifyFiles() to InitVerification() to better reflect
what the call does.

See http://shortn/_lAkREmcQYG and http://shortn/_o4p8OPylLu on how
init verification is implemented in the CDM.

BUG=709614
TEST=Manually tested on Windows; Existing browser_tests still works.

Review-Url: https://codereview.chromium.org/2849163003
Cr-Commit-Position: refs/heads/master@{#468564}
Committed:
https://chromium.googlesource.com/chromium/src/+/4ae3646b574b83fec0213bab9242...
==========

[commit-bot: I haz the power, 2017-05-02 05:13:44]

Committed patchset #3 (id:30004) as
https://chromium.googlesource.com/chromium/src/+/4ae3646b574b83fec0213bab9242...