Closed Bug 1188590 Opened 4 years ago Closed 4 years ago

jsjni_GetGlobalClassRef goes reentrant on main

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox42 + fixed
firefox-esr38 --- wontfix
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- unaffected
Blocking Flags:

People

(Reporter: bwc, Assigned: bwc)

References

Details

(Keywords: sec-critical, Whiteboard: [adv-main40+][post-critsmash-triage])

Attachments

(2 files)

This has only been observed on android 4.0. (And only on mozilla-beta, since the android 4.0 tests are disabled on aurora and central) The patch from bug 1182289 detected this problem.
Keywords: sec-critical
Summary: Unknown reentrancy on main somewhere below CreateOrUpdateMediaPipeline → jsjni_GetGlobalClassRef goes reentrant on main
This might work:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb6def2e8aaf
Assignee: nobody → docfaraday
Ok, fixed the compile bug, looks good (there's a known perma-orange on android, but that's about it).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b584fd6ab3f
Attachment #8640498 - Attachment filename: . → remove_gratuitous_dispatch.patch
Attachment #8640498 - Flags: review?(snorp)
Attachment #8640498 - Flags: review?(snorp) → review+
Comment on attachment 8640503 [details] [diff] [review]
(Beta only) Reenable mochitests for bug 1013809

Approval Request Comment

Just re-enabling some tests that were disabled to allow bug 1182289 to land, now that we've found the cause of the mystery reentrancy.
Attachment #8640503 - Flags: review?(rjesup)
Attachment #8640503 - Flags: approval-mozilla-beta?
Comment on attachment 8640498 [details] [diff] [review]
Don't sync dispatch if we're already on main

Approval Request Comment for aurora, beta, and esr38
[Feature/regressing bug #]:

   Bug 839836

[User impact if declined]:

   Content will probably be able to cause UAFs in webrtc code (and probably other code too) on android.

[Describe test coverage new/current, TreeHerder]:

   The patches from bug 1182289 uncovered this bug, because there are assertions that are tripped when main goes reentrant if CreateOrUpdateMediaPipeline is on the stack. These asserts do not cover all of the API in PeerConnection, nor do they cover other code that cannot tolerate main going reentrant.


[Risks and why]: 

   This patch alters timing (GetGlobalClassRef will finish earlier than stuff in main's event queue), so it is possible it could uncover other problems. However, I'm not seeing evidence of this on try.

[String/UUID change made/needed]:

   None.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   Since our mochitests for bug 1013809 hit this every time on android 4.0, it would seem pretty easy to hit this reliably. There are probably other ways to cause this bug too. It is hard to say how easy it would be to actually exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   The patch for re-enabling the tests for bug 1013809 on beta would provide a pretty good hint at how to trigger this bug, but the fix itself does not paint this kind of bullseye. We may wish to open a separate bug for the test patch, so the two are harder to link together.

Which older supported branches are affected by this flaw?

   All.

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

   I'm pretty sure the patch will apply cleanly. If not, it will be a trivial backport.

How likely is this patch to cause regressions; how much testing does it need?

   This patch alters timing (GetGlobalClassRef will finish earlier than stuff in main's event queue), so it is possible it could uncover other problems. However, I'm not seeing evidence of this on try.
Attachment #8640498 - Flags: sec-approval?
Attachment #8640498 - Flags: approval-mozilla-esr38?
Attachment #8640498 - Flags: approval-mozilla-beta?
Attachment #8640498 - Flags: approval-mozilla-aurora?
Comment on attachment 8640498 [details] [diff] [review]
Don't sync dispatch if we're already on main

Android isn't supported on esr38
Attachment #8640498 - Flags: approval-mozilla-esr38?
Comment on attachment 8640498 [details] [diff] [review]
Don't sync dispatch if we're already on main

Approvals given!
Attachment #8640498 - Flags: sec-approval?
Attachment #8640498 - Flags: sec-approval+
Attachment #8640498 - Flags: approval-mozilla-beta?
Attachment #8640498 - Flags: approval-mozilla-beta+
Attachment #8640498 - Flags: approval-mozilla-aurora?
Attachment #8640498 - Flags: approval-mozilla-aurora+
Comment on attachment 8640503 [details] [diff] [review]
(Beta only) Reenable mochitests for bug 1013809

Test-only changes can land w/o approval.
Attachment #8640503 - Flags: approval-mozilla-beta?
Attachment #8640503 - Flags: review?(rjesup) → review+
backlog: --- → webRTC+
Rank: 5
Priority: -- → P1
Whiteboard: [adv-main40+]
https://hg.mozilla.org/mozilla-central/rev/67ee052e79c9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
See bug 1189058. The issue there was detected on the media thread.

This bug will fix it for the main thread (and is a good idea) but it doesn't fix the underlying cause.
Whiteboard: [adv-main40+] → [adv-main40+][post-critsmash-triage]
I'm very worried about the fact that this was only caught by chance on an uplift to beta on a platform that's soon to be decommissioned.

Looking at the test manifest, we disable all WebRTC tests on Android 2.3/4.3 - http://hg.mozilla.org/mozilla-central/file/e9389ca320ff/dom/media/tests/mochitest/mochitest.ini#l4. Looks like this goes back to when 2.3/4.3 were enabled to begin with.

Maire, any idea what needs to be done to get these running again on 41+? We obviously got lucky this time.
Flags: needinfo?(mreavy)
I've opened a new bug and made it a P1: Bug 1189784. I gave Catlee a heads up that we'd likely need someone from releng to assist, and kmoir has already pinged me to ask how she can help.
Flags: needinfo?(mreavy)
Blocks: 1181694
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.