1756598 - AddressSanitizer: heap-use-after-free [@ __atomic_add] through [@ mozilla::gl::GLContextEGL::CreateEGLPBufferOffscreenContextImpl] and FFmpegVideoDecoder with WRITE of size 4

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 99.0a1-20220208070047-https://hg.mozilla.org/mozilla-central/rev/a25fb4010591aab59ca028aa3a3be3bc7a8f3886.

For detailed crash information, see attachment.

This was already reported on Feburary 8, not sure if this might already be fixed.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Andrew McCreight [:mccr8]]

Kelsey, maybe you could take a look? Thanks. There's both WebGL and FFmpegVideoDecoder stuff in the stack so I'm not sure exactly who might be best to look at it.

Comment 3

[Kelsey Gilbert [:jgilbert]]

This is a race here: https://searchfox.org/mozilla-central/source/widget/gtk/DMABufSurface.cpp#63

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Christian, is that a kind of static analysis or is that from actual Firefox run? It may be possible to hit the race here but we use only one decode thread with VA-API.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

btw. multi-thread va-api decode may be enabled for Bug 1750388.

Comment 6

[Christian Holler (:decoder)]

(In reply to Martin Stránský [:stransky] (ni? me) from comment #4)

Christian, is that a kind of static analysis or is that from actual Firefox run? It may be possible to hit the race here but we use only one decode thread with VA-API.

This is from an actual run, like a crash report. But we do not have information about steps to reproduce, prefs, or who submitted it.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

VA-API is disabled by default so we're no affected.

Comment 8

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:stransky, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #8)

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:stransky, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

It's disabled by default and you need to try pretty hard to enable it so i think it's ok.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Okay, I think I understand what's going on. Looks like two media playback happens on the same time so we use two surface pools but they share one GL context where we create the GL texture.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1756598 [Linux] Make GL context creation thread safe r?sotaro

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 9273239 [details]
Bug 1756598 [Linux] Make GL context creation thread safe r?sotaro

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's disabled by default, user need to flip pref and disable RDD sandbox.
  Then we can create a page where lot of video clips are played and that can be exploited - but it affects RDD process only.
  So I don't think it's actually exploitable.
• 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?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely.
• Is Android affected?: No

Comment 13

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

Comment on attachment 9273239 [details]
Bug 1756598 [Linux] Make GL context creation thread safe r?sotaro

If the pref need to be modified from default, we don't need sec-approval and can land it.

Comment 14

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

I'm also going to downgrade it to moderate

Comment 15

[Christian Holler (:decoder)]

(In reply to Tom Ritter [:tjr] (ni for sec-approvals and such) from comment #14)

I'm also going to downgrade it to moderate

Is the downgrade here because it only affects RDD or because it requires a pref to be flipped?

Comment 16

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

pref flip - if you go and disable the RDD sandbox I think it's reasonable to say you've knowingly put your Firefox in a configuration where it's less secure and we shouldn't consider bugs in (only) that configuration as High.

Comment 17

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

[Linux] Make GL context creation thread safe r=sotaro,jgilbert
https://hg.mozilla.org/integration/autoland/rev/bef9104f01c528a0b0fac514ebcf35b505924224
https://hg.mozilla.org/mozilla-central/rev/bef9104f01c5