1003006 - Camera rotation query in B2G must run on Mainthread

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

The OnHardwareChangeNotification callback from B2G can't read the ScreenConfiguration from a random thread; it has to be MainThread - however this is only tested on a debug build.

See also bug 1003003

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: temporarily disable rotation check

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I am willing to take this bug.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Read B2G camera rotation on MainThread

Working fix

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Stealing bug since I already did a fix

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8414298 [details] [diff] [review]
Read B2G camera rotation on MainThread

Review of attachment 8414298 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +801,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MonitorAutoLock enter(mMonitor);
> +
> +  mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, mCameraAngle);

Is this the line that is throwing the off-Main Thread assertion? If so, I'll need to fix that--this should be gettable from anywhere.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

This turns out to involve a security problem so hiding bug for now.  Filing a meta-bug for the issue.

Comment 7

[Jason Smith [:jsmith]]

Paul - Is this critical enough that you'd like to see this on 1.4? Trying to figure out if this needs to be nomed for 1.4 or not.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

jsmith: this will make gUM assert/crash on all Debug builds of 1.4.  Also note that this patch includes security fixes to the lack of nsRefPtr<>'s on the B2G camera WrapRunnable calls, which in theory would allow the calling object (MediaEngineWebRTCVideoSource) to go away (free, UAF) before the runnable runs.  (very unlikely, perhaps impossible.  Hard to be 100% sure.)

In addition, accessing this from the wrong thread may lead to unexpected results due to races and things like TSAN.

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Randell Jesup [:jesup] from comment #6)
> This turns out to involve a security problem so hiding bug for now.  Filing
> a meta-bug for the issue.

Can you CC me on this bug? I don't seem to have access.

But yes, this sound like it should be blocking from Randell's description.

Comment 11

[Preeti Raghunath(:Preeti)]

Maire,

Can you please help with next steps here?

Comment 12

[Maire Reavy [:mreavy]]

We need sec review and then we can land this. Randell can you ask for sec review (including an analysis of the security issues)?  Thanks.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8414298 [details] [diff] [review]
Read B2G camera rotation on MainThread

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily.  You'd have to exploit the object getting released due to thread-switching-timing/queue-lengths, and then get it re-allocated to something useful.  I won't say impossible, but not easy.  For the main part of the patch, it's not clear reading it from the wrong thread is exploitable at all.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Not really, though someone with the right bent of mind may infer from switching a bunch of WrapRunnables to nsRefPtr<> implies a recounting issue to look at.  Also, they may infer that other uses of 'this' may be vulnerable (see bug 1003283, though I think we've decided they're all safe currently).

Which older supported branches are affected by this flaw? B2G code introduced this flaw, and the WrapRunnable stuff only exists on B2G.

If not all supported branches, which bug introduced the flaw? MikeH's camera refactor bug 909542 (1.4S1) I believe, and rotation in bug 970183

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?  Unlikely to cause regressions.  The nsRefPtr<> stuff, not at all.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8414298 [details] [diff] [review]
Read B2G camera rotation on MainThread

sec-approval+

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6a5186429c

Comment 16

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I guess this just missed getting the merge pasted in here:
https://hg.mozilla.org/mozilla-central/rev/dc6a5186429c

This still needs to get backports put up for approval on B2G branches, it looks like.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

sec-high = auto-approval for B2G backports :)

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/a9df4486994c