Fix race in AudioRendererHost around render frame ID validation.

Fix race in AudioRendererHost around render frame ID validation.

A bug was identified where IPC messages controlling audio stream state
were being ignored/dropped: This was due to the fact that, in builds
where DCHECKs were turned on, the additional render frame ID validation
was delaying stream creation to a point after the control IPC messages
were being processed.

This change allows stream creation to proceed, with the render frame ID
validation happening in the background. If the validation fails, it will
error-out and force-close the stream later. The benefit of this change
is that it can also harmlessly be turned on for all builds (i.e., not
just when DCHECKs are turned on), for a more-robust security check on
the AudioHostMsg_CreateStream IPC message.

BUG=633936
Committed: https://crrev.com/2dcff8d1a58297367bc577f4f3554bfffd2c9bd1
Cr-Commit-Position: refs/heads/master@{#417642}

[miu, 2016-09-03 21:09:41]

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

[commit-bot: I haz the power, 2016-09-03 21:09:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2016-09-03 21:14:42]

miu@chromium.org changed reviewers:
+ dalecurtis@chromium.org, olka@chromium.org

[miu, 2016-09-03 21:14:42]

olka and dalecurtis: PTAL. This resolves the race condition noted in the crbug.

Note that we should probably revisit an old design decision exposed by this
problem: Should the render process wait until it has received the
AudioMsg_NotifyStreamCreated before it can send control IPCs for the audio
stream? I would argue "no," since that would add to startup latency. In other
words, IMHO, the render process should optimistically assume the audio stream
gets created. If you all agree, we should follow-up with some clean-up in the
code comments in audio_renderer_host.h/.cc.

[commit-bot: I haz the power, 2016-09-03 21:58:12]

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

[commit-bot: I haz the power, 2016-09-03 21:58:13]

Dry run: This issue passed the CQ dry run.

[o1ka, 2016-09-05 10:20:45]

Description was changed from

==========
Fix race in AudioRendererHost around render frame ID validation.

A bug was identified where IPC messages controlling audio stream state
were being ignored/dropped: This was due to the fact that, in builds
where DCHECKs were turned on, the additional render frame ID validation
was delaying stream creation to a point after the control IPC messages
were being processed.

This change allows stream creation to proceed, with the render frame ID
validation happening in the background. If the validation fails, it will
error-out and force-close the stream later. The benefit of this change
is that it can also harmlessly be turned on for all builds (i.e., not
just when DCHECKs are turned on), for a more-robust security check on
the AudioHostMsg_CreateStream IPC message.

BUG=633936
==========

to

==========
Fix race in AudioRendererHost around render frame ID validation.

A bug was identified where IPC messages controlling audio stream state
were being ignored/dropped: This was due to the fact that, in builds
where DCHECKs were turned on, the additional render frame ID validation
was delaying stream creation to a point after the control IPC messages
were being processed.

This change allows stream creation to proceed, with the render frame ID
validation happening in the background. If the validation fails, it will
error-out and force-close the stream later. The benefit of this change
is that it can also harmlessly be turned on for all builds (i.e., not
just when DCHECKs are turned on), for a more-robust security check on
the AudioHostMsg_CreateStream IPC message.

BUG=633936
==========

[o1ka, 2016-09-05 10:21:14]

cc maxmorin@: FYI

[o1ka, 2016-09-05 11:58:38]

https://codereview.chromium.org/2301353007/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2301353007/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:360:
ReportErrorAndClose(stream_id);
My concern is that (as far as I understand) this may be scheduled before or
after DoCompleteCreation() => we have two sequences for the same situation,
which complicates the system. Is it possible to somehow make a final decision in
DoCompleteCreation() and to not send AudioMsg_NotifyStreamCreated if frame
validation fails (I've tried to come up with the idea how to do that, but got a
bit lost in all the thread hopping - I do not know that code well enough yet)?

[o1ka, 2016-09-05 12:00:51]

On 2016/09/03 21:14:42, miu wrote:
> olka and dalecurtis: PTAL. This resolves the race condition noted in the
crbug.
> 
> Note that we should probably revisit an old design decision exposed by this
> problem: Should the render process wait until it has received the
> AudioMsg_NotifyStreamCreated before it can send control IPCs for the audio
> stream? I would argue "no," since that would add to startup latency. In other
> words, IMHO, the render process should optimistically assume the audio stream
> gets created. If you all agree, we should follow-up with some clean-up in the
> code comments in audio_renderer_host.h/.cc.

I think it's fine to not wait for AudioMsg_NotifyStreamCreated on renderer side,
but it would be nice to make sure AudioMsg_NotifyStreamCreated is not sent if
frame validation fails (see my comment).

[o1ka, 2016-09-05 12:13:33]

I think I'm missing something. Why validating frame id during device
authorization is not enough? Shouldn't device authorizations be updated on frame
deletion (or whatever is the operation which ends its lifetime)?

[DaleCurtis, 2016-09-06 19:24:40]

lgtm

https://codereview.chromium.org/2301353007/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/2301353007/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:360:
ReportErrorAndClose(stream_id);
On 2016/09/05 at 11:58:38, o1ka wrote:
> My concern is that (as far as I understand) this may be scheduled before or
after DoCompleteCreation() => we have two sequences for the same situation,
which complicates the system. Is it possible to somehow make a final decision in
DoCompleteCreation() and to not send AudioMsg_NotifyStreamCreated if frame
validation fails (I've tried to come up with the idea how to do that, but got a
bit lost in all the thread hopping - I do not know that code well enough yet)?

If this isn't okay then none of our error handling code is okay :)

[miu, 2016-09-06 20:08:02]

On 2016/09/05 12:13:33, o1ka wrote:
> I think I'm missing something. Why validating frame id during device
> authorization is not enough? Shouldn't device authorizations be updated on
frame
> deletion (or whatever is the operation which ends its lifetime)?

It's orthogonal. Render frame IDs are not a part of the device auth path. And,
IIUC, it's possible for audio streams to be created for the default device w/o
using device auth at all.

[miu, 2016-09-06 20:09:11]

o1ka: Did Dale's comment (and my response to your other comment) sound good to
you?

[o1ka, 2016-09-07 12:10:47]

On 2016/09/06 20:08:02, miu wrote:
> On 2016/09/05 12:13:33, o1ka wrote:
> > I think I'm missing something. Why validating frame id during device
> > authorization is not enough? Shouldn't device authorizations be updated on
> frame
> > deletion (or whatever is the operation which ends its lifetime)?
> 
> It's orthogonal. Render frame IDs are not a part of the device auth path. And,
> IIUC, it's possible for audio streams to be created for the default device w/o
> using device auth at all.

Check out this call sequence:
1) AudioRendererHost::OnRequestDeviceAuthorization 
2) CheckOutputDeviceAccess
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...)
3) MediaStreamUIProxy::CheckAccess
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...)
4) MediaStreamUIProxy::Core::CheckAccess
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...)
5) MediaStreamUIProxy::Core::GetRenderFrameHostDelegate
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...)
6) RenderFrameHostImpl::FromID
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...)

So, we won't authorize a non-default device if invalid frame id is specified =>
won't allow stream creation later.
Yes, the default device is always authorized, so no permission check/frame id
check is performed for it. But we can do it anyways and not authorize the stream
with invalid frame id, can't we?

[o1ka, 2016-09-07 12:12:20]

> On 2016/09/05 at 11:58:38, o1ka wrote:
> > My concern is that (as far as I understand) this may be scheduled before or
> after DoCompleteCreation() => we have two sequences for the same situation,
> which complicates the system. Is it possible to somehow make a final decision
in
> DoCompleteCreation() and to not send AudioMsg_NotifyStreamCreated if frame
> validation fails (I've tried to come up with the idea how to do that, but got
a
> bit lost in all the thread hopping - I do not know that code well enough yet)?
> 
> If this isn't okay then none of our error handling code is okay :)

None of this sort! :)

[o1ka, 2016-09-07 12:16:38]

On 2016/09/06 20:09:11, miu wrote:
> o1ka: Did Dale's comment (and my response to your other comment) sound good to
> you?

I can't say they sound good, the complexity of this subsystem seems to be at the
border of being manageable. But I'm embracing the reality :) lgtm (= "it should
work")

[miu, 2016-09-08 01:39:21]

On 2016/09/07 12:10:47, o1ka wrote:
> So, we won't authorize a non-default device if invalid frame id is specified
=>
> won't allow stream creation later.

Agreed. So, we can conclude that authorized streams have already had a Render
Frame ID validation check (of sorts) performed.

> Yes, the default device is always authorized, so no permission check/frame id
> check is performed for it. But we can do it anyways and not authorize the
stream
> with invalid frame id, can't we?

So, if I understand you correctly, you are implying that the Render Frame ID
validation check in OnStreamCreated() should be skipped for authorized,
non-default device streams? But, non-auth'ed default device streams should still
be forced to go through the check, correct? Does that sound right?

[o1ka, 2016-09-08 10:25:08]

On 2016/09/08 01:39:21, miu wrote:
> On 2016/09/07 12:10:47, o1ka wrote:
> > So, we won't authorize a non-default device if invalid frame id is specified
> =>
> > won't allow stream creation later.
> 
> Agreed. So, we can conclude that authorized streams have already had a Render
> Frame ID validation check (of sorts) performed.
> 
> > Yes, the default device is always authorized, so no permission check/frame
id
> > check is performed for it. But we can do it anyways and not authorize the
> stream
> > with invalid frame id, can't we?
> 
> So, if I understand you correctly, you are implying that the Render Frame ID
> validation check in OnStreamCreated() should be skipped for authorized,
> non-default device streams? But, non-auth'ed default device streams should
still
> be forced to go through the check, correct? Does that sound right?

No-no-no :) we cannot do frame verification for the default device in one place
and the same thing for non-default devices in another place on another event and
6 floors down :)
My point was that since we need frame id to be valid for a non-default device
authorization, why not to make it mandatory for all authorizations, and to not
authorize a stream with invalid frame id?
Like, validate frame id right after device id validation
(https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...).
If we do not allow audio stream creation, doesn't it basically mean that the
stream is not authorized?

BTW, I'm not sure what are the problems if we allow a stream with invalid frame
id to access the default device? The default device is always authorized
anyways.

All in all, the change as it is now looks acceptable, taking into account that
all that part of the browser side is a big mess.

[miu, 2016-09-08 23:27:14]

On 2016/09/08 10:25:08, o1ka wrote:
> No-no-no :) we cannot do frame verification for the default device in one
place
> and the same thing for non-default devices in another place on another event
and
> 6 floors down :)

Of course! :) I just looked at the consumer end of the code, and the issue is
that Pepper cheats: It goes directly to AudioHostMsg_CreateStream without first
going through the auth process.

> If we do not allow audio stream creation, doesn't it basically mean that the
> stream is not authorized?

Not sure. Actually, I was never part of adding this new concept to ARHost, so
I'm not sure whether "authorized" is purely a user-permission concept or that it
should also mean "all data dependencies are satisfied." AFAIK, the RFIDs are
just a data dependency requirement for the browser process to correctly handle
the streams (for mixing, mute, capture, etc.).

> All in all, the change as it is now looks acceptable, taking into account that
> all that part of the browser side is a big mess.

Agreed. To be fair, it has evolved A LOT over the past 4 years. Believe it or
not, there was a time where I was about to merge ARHost and ARInputHost since
they were nearly identical.

Thanks for the review comments. Feel free to loop me in on any
efforts/discussion around helping to clean things up.

[miu, 2016-09-08 23:27:41]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2016-09-08 23:28:11]

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, 2016-09-08 23:37:12]

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

[commit-bot: I haz the power, 2016-09-08 23:37:13]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[o1ka, 2016-09-09 14:17:59]

> Feel free to loop me in on any
> efforts/discussion around helping to clean things up.

I sure will! We have some plans for that.

[miu, 2016-09-09 18:05:38]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:06:07]

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, 2016-09-09 18:33:23]

Description was changed from

==========
Fix race in AudioRendererHost around render frame ID validation.

A bug was identified where IPC messages controlling audio stream state
were being ignored/dropped: This was due to the fact that, in builds
where DCHECKs were turned on, the additional render frame ID validation
was delaying stream creation to a point after the control IPC messages
were being processed.

This change allows stream creation to proceed, with the render frame ID
validation happening in the background. If the validation fails, it will
error-out and force-close the stream later. The benefit of this change
is that it can also harmlessly be turned on for all builds (i.e., not
just when DCHECKs are turned on), for a more-robust security check on
the AudioHostMsg_CreateStream IPC message.

BUG=633936
==========

to

==========
Fix race in AudioRendererHost around render frame ID validation.

A bug was identified where IPC messages controlling audio stream state
were being ignored/dropped: This was due to the fact that, in builds
where DCHECKs were turned on, the additional render frame ID validation
was delaying stream creation to a point after the control IPC messages
were being processed.

This change allows stream creation to proceed, with the render frame ID
validation happening in the background. If the validation fails, it will
error-out and force-close the stream later. The benefit of this change
is that it can also harmlessly be turned on for all builds (i.e., not
just when DCHECKs are turned on), for a more-robust security check on
the AudioHostMsg_CreateStream IPC message.

BUG=633936
==========

[commit-bot: I haz the power, 2016-09-09 18:33:24]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-09 18:34:41]

Description was changed from

==========
Fix race in AudioRendererHost around render frame ID validation.

A bug was identified where IPC messages controlling audio stream state
were being ignored/dropped: This was due to the fact that, in builds
where DCHECKs were turned on, the additional render frame ID validation
was delaying stream creation to a point after the control IPC messages
were being processed.

This change allows stream creation to proceed, with the render frame ID
validation happening in the background. If the validation fails, it will
error-out and force-close the stream later. The benefit of this change
is that it can also harmlessly be turned on for all builds (i.e., not
just when DCHECKs are turned on), for a more-robust security check on
the AudioHostMsg_CreateStream IPC message.

BUG=633936
==========

to

==========
Fix race in AudioRendererHost around render frame ID validation.

A bug was identified where IPC messages controlling audio stream state
were being ignored/dropped: This was due to the fact that, in builds
where DCHECKs were turned on, the additional render frame ID validation
was delaying stream creation to a point after the control IPC messages
were being processed.

This change allows stream creation to proceed, with the render frame ID
validation happening in the background. If the validation fails, it will
error-out and force-close the stream later. The benefit of this change
is that it can also harmlessly be turned on for all builds (i.e., not
just when DCHECKs are turned on), for a more-robust security check on
the AudioHostMsg_CreateStream IPC message.

BUG=633936

Committed: https://crrev.com/2dcff8d1a58297367bc577f4f3554bfffd2c9bd1
Cr-Commit-Position: refs/heads/master@{#417642}
==========

[commit-bot: I haz the power, 2016-09-09 18:34:43]

Patchset 1 (id:??) landed as
https://crrev.com/2dcff8d1a58297367bc577f4f3554bfffd2c9bd1
Cr-Commit-Position: refs/heads/master@{#417642}