1662507 - Racy access to WebrtcMediaDataEncoder::mEncoder

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

WebrtcMediaDataEncoder::mEncoder is nulled out by a webrtc.org thread here, inside a lambda capture:

https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/media/webrtc/signaling/src/media-conduit/WebrtcMediaDataEncoderCodec.cpp#175

But we use this RefPtr on other threads (mTaskQueue) without any synchronization protection in many other places.

I see a nullptr crash due to this problem here, but we could easily end up with a UAF instead. We should not be modifying this RefPtr on any thread other than mTaskQueue:

https://crash-stats.mozilla.org/report/index/29cac5a2-001d-4e47-8d9a-707b90200828

This might be related to bug 1662036 in some way, but I do not know if it is the cause.

This may also be related to bug 1597848, bug 1609240, and bug 1621617.

Comment 1

[Maire Reavy [:mreavy]]

Hey Jan-Ivar, Can you find an owner for this? It's a sec-high. If I can help in any way, let me know. Thanks!

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

John, can you have a look at this one as well? It's in code you and Dan have touched recently (and Alastor before that).

Comment 3

[Maire Reavy [:mreavy]]

Hi John, (Like I said in the other sec bug) -- If you need additional help from anyone with this or any other bug, please let me know. We're actively trying to drive down all our sec-crit and sec-high bugs. Thanks!

Comment 4

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1662507 - don't use mEncoder outside webrtc.org thread. r?jya

Comment 5

[Maire Reavy [:mreavy]]

Hi John, When you do think you'll be able to have a new patch ready for review? It'd be great if we could get a fix landed before the company break. Thank you!

Comment 6

[John Lin [:jhlin][:jolin]]

(In reply to Maire Reavy [:mreavy] from comment #5)

Hi John, When you do think you'll be able to have a new patch ready for review? It'd be great if we could get a fix landed before the company break. Thank you!

It should be ready by the end of the week or sooner. Sorry for taking so long.

Comment 7

[John Lin [:jhlin][:jolin]]

Comment on attachment 9187720 [details]
Bug 1662507 - don't use mEncoder outside webrtc.org thread. r?jya

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's very hard. No known path in existing codebase invokes the Encode() method between InitEncoder() and Shutdown(), where the race condition could occur.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: none
• If not all supported branches, which bug introduced the flaw?: Bug 1599799
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It would be hard to create for there have been several other changes to the code in the same file. Backporting will require all of them.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely as there is no existing code that actually read the null-ed pointer.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I'm confused - the approval form says "No known path" and "no existing code" but the first comment links to a crash report? (As well as speculating about several crashes.) And it says "No older branches" but also "It would be hard to create" and the regressing bug (which is not set in the bugzilla field) is a year old?

Comment 9

[John Lin [:jhlin][:jolin]]

(In reply to Tom Ritter [:tjr] (ni? for response to sec-[advisories/bounties/ratings/cves]) from comment #8)

I'm confused - the approval form says "No known path" and "no existing code" but the first comment links to a crash report? (As well as speculating about several crashes.) And it says "No older branches" but also "It would be hard to create" and the regressing bug (which is not set in the bugzilla field) is a year old?

Thanks a lot for the review and I am very, very sorry for causing the confusion.

'No known path' in the 1st answer means that because the patch no longer calls Encode() [1] in another thread, there will be no known client of WebrtcMediaDataEncoderCodec that calls Shutdown() and InitEncode() in the webrtc thread can preempt and causes the situation in the crash report from comment 1. The 'no existing code' in the final item basically means the same thing.

And I made mistakes in the older branches and backporting answers: I failed to follow the changes to WebrtcMediaDataEncoderCodec.cpp prior to bug 1664900 and use incorrect date to compare with ESR branch date. I also forgot that beta and release are also counted as 'supported'. Let me try again:

• Which older supported branches are affected by this flaw?: beta, release, and ESR
• If not all supported branches, which bug introduced the flaw?: Bug 1599799
• Do you have backports for the affected branches?: Working on patches for release and beta
• If not, how different, hard to create, and risky will they be?: Patches for beta and release are not too different/hard/risky since there are limited changes. It would be harder/riskier for ESR because the file has been moved and more changes (bug 1664900 and bug 1653335) are involved.

Please let me know if there is further problem. Thanks!

[1] https://searchfox.org/mozilla-central/source/dom/media/webrtc/libwebrtcglue/WebrtcMediaDataEncoderCodec.cpp#255

Comment 10

[John Lin [:jhlin][:jolin]]

Looks like the patch can be applied to beta and release without modification:

beta -- https://treeherder.mozilla.org/jobs?repo=try&revision=582c0e73d88e76b3d87b1b72d541cecd0321c5d1&selectedTaskRun=Q18Du5YBRSSIKT9jZOpC2g.0

release -- https://treeherder.mozilla.org/jobs?repo=try&revision=7b573c479bb896857ffd53e63f8ef2c449dd124a&selectedTaskRun=JbL4D2RrQkiHk6NupFcExg.0

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9187720 [details]
Bug 1662507 - don't use mEncoder outside webrtc.org thread. r?jya

Approved to land, please attach an esr78 patch when able for uplift.

Comment 12

[John Lin [:jhlin][:jolin]]

Attachment: Patch for ESR78

Try server result -- https://treeherder.mozilla.org/jobs?repo=try&revision=4fe11d4ec26f008c087c08f75e9b59b6597373fd

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Hi John, is this ready to land?

Comment 14

[John Lin [:jhlin][:jolin]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

Hi John, is this ready to land?

Thanks a lot for the remainder! It is ready. I'm just holding it until the week before the next release.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/5404a5e79e253fbf84aeab8945de7e668fa14acf
https://hg.mozilla.org/mozilla-central/rev/5404a5e79e25

Comment 16

[John Lin [:jhlin][:jolin]]

Comment on attachment 9187720 [details]
Bug 1662507 - don't use mEncoder outside webrtc.org thread. r?jya

Beta/Release Uplift Approval Request

• User impact if declined: Possible tab process crashes when changing video resolution in WebRTC video chat.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch reduces task dispatching between threads and prevents race conditions.
• String changes made/needed:

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 9187720 [details]
Bug 1662507 - don't use mEncoder outside webrtc.org thread. r?jya

approved for 85.0b7

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd89174422ea

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9194640 [details] [diff] [review]
Patch for ESR78

Approved for 78.7esr also.

Comment 20

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/acc2e43bf248

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.