Create child window in GPU process for DirectComposition

Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU process will create a new child window to present into. This will be necessary when using DirectComposition because DirectComposition can only render into windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child window to the browser window, so a new IPC is added to allow that.

BUG=524838, 545203
Committed: https://crrev.com/278b2ccf77a0e32fba6322278b2b4e1afd893da1
Cr-Commit-Position: refs/heads/master@{#368450}

[jbauman, 2015-12-29 00:17:18]

Description was changed from

==========
Add Child window for win32
==========

to

==========
Create child window in GPU process for direct composition.

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.
==========

[jbauman, 2015-12-29 00:17:45]

Description was changed from

==========
Create child window in GPU process for direct composition.

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.
==========

to

==========
Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.
==========

[jbauman, 2015-12-29 00:19:19]

jbauman@chromium.org changed reviewers:
+ wfh@chromium.org

[jbauman, 2015-12-29 00:19:19]

wfh@: I've added some security checks to the IPC to ensure this is safe.
Hopefully those are enough.

[jbauman, 2016-01-04 23:23:06]

Description was changed from

==========
Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.
==========

to

==========
Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.

BUG=524838,545203
==========

[jbauman, 2016-01-06 17:58:11]

wfh: ping

[Will Harris, 2016-01-06 18:23:32]

wfh@chromium.org changed reviewers:
+ jschuh@chromium.org

[Will Harris, 2016-01-06 18:23:32]

On 2016/01/06 17:58:11, jbauman wrote:
> wfh: ping

I worry here that creating a disabled window and then making the parent be the
GLsurface in the browser process would allow the gpu process to send windows
messages to the browser, as disable child windows will forward messages to their
parent and you create this window WS_DISABLED. I don't know enough about crazy
Windows GDI code to know what kind of vulnerabilities this could expose. Perhaps
+jschuh can take a look, as he's a GDI expert.

Otherwise your checks look reasonable, they only allow a valid gpu child of the
browser to successfully call the IPC, and they check both window values...

https://codereview.chromium.org/1538803004/diff/40001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1538803004/diff/40001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:657: return;
could consider calling bad_message::ReceivedBadMessage here, this shouldn't
happen except if someone is trying to fuzz or break ipc.

https://codereview.chromium.org/1538803004/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1538803004/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:477:
IPC_MESSAGE_CONTROL2(GpuHostMsg_AcceleratedSurfaceCreatedChildWindow,
#if defined(OS_WIN)

[jschuh, 2016-01-07 01:41:11]

Yes, this is scary looking but it was my recommendation and is reasonable in
context. Reparenting will allow messages to go to the more privileged window,
but UIPI should restrict the message types to the same set that it allows across
normal integrity-level boundaries. I.e. the desktop window already has the same
exposure anyway, so this shouldn't make anything less safe unless we poke new
holes (e.g. via ChangeWindowMessageFilter) or start trusting the data in the
messages from the low-integrity process.

tl;dr - lgtm for security if you make the bad_message::ReceivedBadMessage
change.

https://codereview.chromium.org/1538803004/diff/40001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1538803004/diff/40001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:657: return;
On 2016/01/06 18:23:32, Will Harris wrote:
> could consider calling bad_message::ReceivedBadMessage here, this shouldn't
> happen except if someone is trying to fuzz or break ipc.

Yep. This is bad so kill the sender.

[jbauman, 2016-01-07 03:42:06]

jbauman@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-01-07 03:46:56]

lgtm given the security team's review.

I wonder whether this might be useful in other situations that the ANGLE team's
encountered.

What happens if the GPU process crashes and is restarted?

[jbauman, 2016-01-07 03:52:40]

On 2016/01/07 03:46:56, Ken Russell wrote:
> lgtm given the security team's review.
> 
> I wonder whether this might be useful in other situations that the ANGLE
team's
> encountered.
> 
> What happens if the GPU process crashes and is restarted?

The child window is destroyed when the GPU process is, so either the new GPU
process creates a new child window, or (if no new GPU process) the software
compositor draws into the toplevel window.

[Will Harris, 2016-01-07 15:02:54]

On 2016/01/07 03:52:40, jbauman wrote:
> On 2016/01/07 03:46:56, Ken Russell wrote:
> > lgtm given the security team's review.
> > 
> > I wonder whether this might be useful in other situations that the ANGLE
> team's
> > encountered.
> > 
> > What happens if the GPU process crashes and is restarted?
> 
> The child window is destroyed when the GPU process is, so either the new GPU
> process creates a new child window, or (if no new GPU process) the software
> compositor draws into the toplevel window.

lgtm on IPC messages.

[Geoff Lang, 2016-01-07 15:16:37]

Thanks, this will be useful for OpenGL on Windows.

[jbauman, 2016-01-07 22:39:56]

jbauman@chromium.org changed reviewers:
+ avi@chromium.org

[jbauman, 2016-01-07 22:39:56]

avi@: content/ OWNERS review.

[Avi (use Gerrit), 2016-01-08 16:33:02]

lgtm

[jbauman, 2016-01-08 21:28:38]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-01-08 21:28:38]

The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/1538803004/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-01-08 21:29:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538803004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538803004/60001

[commit-bot: I haz the power, 2016-01-08 22:49:00]

Description was changed from

==========
Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.

BUG=524838,545203
==========

to

==========
Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.

BUG=524838,545203
==========

[commit-bot: I haz the power, 2016-01-08 22:49:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-08 22:50:05]

Description was changed from

==========
Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.

BUG=524838,545203
==========

to

==========
Create child window in GPU process for DirectComposition

When the new --use-direct-composition command-line flag is specified the GPU
process will create a new child window to present into. This will be necessary
when using DirectComposition because DirectComposition can only render into
windows created by the same process.

The sandbox prevents the GPU process from directly doing SetParent of its child
window to the browser window, so a new IPC is added to allow that.

BUG=524838,545203

Committed: https://crrev.com/278b2ccf77a0e32fba6322278b2b4e1afd893da1
Cr-Commit-Position: refs/heads/master@{#368450}
==========

[commit-bot: I haz the power, 2016-01-08 22:50:06]

Patchset 4 (id:??) landed as
https://crrev.com/278b2ccf77a0e32fba6322278b2b4e1afd893da1
Cr-Commit-Position: refs/heads/master@{#368450}