1263665 - Blacklist or fix vulnerable/unsupported libav versions on Linux

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

I recently reported a security-critical crash bug in H264 code to the libav project. It seems to affect libav9 which is the default on Ubuntu 14.04 LTS (and probably Debian). I tested libav11 and it does not crash, so I assume it got fixed/refactored at some point.

The libav developers said that branch 9 is generally unsupported and they closed the bug as UPSTREAM, indicating that it's up to Ubuntu and other distros to not use this version anymore.

We could reach out to Ubuntu and other distros now, but since there is no fix for that unsupported branch, their only options are

a) fixing the bug themselves
b) upgrading to another branch (which is probably not possible in LTS)

Both options seem unlikely to me (although I'm happy to hear opinions on this and try it out), therefore I recommend we blacklist vulnerable/unsupported versions of these libraries to not expose our users to unnecessary security problems.

Comment 1

[Christian Holler (:decoder)]

I talked to jyavenard on IRC and we concluded that blacklisting would both be an extreme measure to take because it would leave many Ubuntu users without any sort of media support, and also it won't be effective until Ubuntu decides to take that fix into their Firefox.

I think the best strategy here is to try and fix libav first. The developers of libav have signaled that a point release of 9 would be possible but they don't have the time to work on such patches. But maybe the Ubuntu Security Team or we can do that instead. First of all, we should probably contact the Ubuntu Security Team about this and see what they say.

Dan, what do you think and do we have any contacts there?

Comment 2

[Daniel Veditz [:dveditz]]

We seem to be out of Canonical security contacts. What version of libav does RHEL ship?

Comment 3

[Jean-Yves Avenard [:jya]]

I don't believe it ships any libav related packages. Must use 3rd party repository (typically rpmfind or atrpms)

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Gerald - can we use decoder doctor to notify people about the security issue in libav?

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> Gerald - can we use decoder doctor to notify people about the security issue
> in libav?

Anything is possible!

In this particular case, there are a few things to line up:
1. Find which libav is used. From the little I know, it should be easy to know the major version number, but it may be trickier to get a finer-grained version if necessary (e.g., with distro fixes, etc.). Jean-Yves?
2. Catch the version in DecoderDoctorDiagnostics, and notify the front-end. That's my code, I don't foresee any issues.
3. Create UI strings. Easy, unless we need to uplift them.
4. Front-end handling of the notification, to display the infobar. Should be fine.
5. Optionally, create a SUMO page about this issue. I don't know the cost there, but I'm assuming it's possible.

Comment 6

[Jean-Yves Avenard [:jya]]

Looking at the major, minor, macro is good.

macro is >= 100 if FFmpeg and < 100 if libav.

this is true in all version of FFmpeg/LibAV we support.

so the test to find out if there are vulnerability should be something like: major <= 54 && micro < 100 

And how do we tell people not to use the version of LibAV they have installed when we can't tell them what to install?

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

If the exploit is present then we should be able to crash libav. Perhaps the ultimate test is to see if the crashes we're concerned about are reproducible.

Comment 8

[Jean-Yves Avenard [:jya]]

The vulnerability we are talking about can be reproduced and will crash (bug 1257707).

I noticed that LibAV continues attempting to decode because by default LibAV set the recovery mode on. If that were set to false with old version of libav, I believe it would prevent the problem from happening.

It would be very sensitive to minor error in the h264 stream, but I believe that may do the trick.

I'll try that in the coming days.

Comment 9

[Jean-Yves Avenard [:jya]]

Unfortunately, suggestion above didn't help.

patches submitted upstream.

will see how we go.

We could probably get around the issue in FF if we disable multi-threaded decoding. There would be severe performance downsides if doing so... 

Daniel, to answer the question for redhat, I always used the ATrpms repository when I was using CentOS/Fedora and ffmpeg related packages.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> We could probably get around the issue in FF if we disable multi-threaded
> decoding. There would be severe performance downsides if doing so... 

I'm fine with degrading performance, especially if we can limit it only to affected ffmpeg versions.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Disable multi-threaded decoding when using old version of LibAV. r?kentuckyfriedtakahe

MozReview-Commit-ID: 60XDTJJqpFR

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8758558 [details] [diff] [review]
Disable multi-threaded decoding when using old version of LibAV. r?kentuckyfriedtakahe

Actually, this doesn't help.

the use after free is still present. it just stops earlier.

So other than disabling all libavcodec <= 55, I don't know what to do...

the fix is rather significant in size.

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

We need to get this resolved so lets just drop support for older versions of libav now that we can use decoder doctor to notify the user that codecs are not available.

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #13)
> We need to get this resolved so lets just drop support for older versions of
> libav now that we can use decoder doctor to notify the user that codecs are
> not available.

Note that Decoder Doctor is not currently enabled for libav.
To enable it, add "MediaPlatformDecoderNotFound" to "media.decoder-doctor.notifications-allowed" pref.

Comment 15

[Jean-Yves Avenard [:jya]]

Decoder, do you have the LibAv bug number? Can't find it now..

Comment 16

[Christian Holler (:decoder)]

It's listed in the "See also:" of the bug here ;D

But here's the link again: https://bugzilla.libav.org/show_bug.cgi?id=939

Comment 17

[Jean-Yves Avenard [:jya]]

libav has tagged the new version by bumping the micro version.

Let's block all revisions of LibAV prior that one.

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

It's next on my big TODO list!
Thank you for the extra info.

Comment 19

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-1-Bug_1263665___Block_libav___54_35_1___r_jya.patch

Block libav < 54.35.1

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-2-Bug_1263665___Expose_reason_for_libavcodec_linking_failure___r_jya.patch

Expose reason for libavcodec linking failure

FFmpegLibWrapper returns a precise success/failure code.
FFmpegRuntimeLinker uses that to record the most interesting issue and
associated library name (if any).

Comment 21

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-3-Bug_1263665___DecDoc_handling_of_libavcodec_linking_issues___r_jya.patch

Decoder Doctor handling of libavcodec linking issues

If libavcodec is present but cannot be used, Decoder Doctor sends a distinct
notification to better help the user find the issue.

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-4-Bug_1263665___Frontend_notification_of_unsupported_libavcodec___r_gijs.patch

Frontend notification of unsupported libavcodec

Inform the user through a drop-down notification, that the installed
libavcodec is not supported (possibly because it is vulnerable) and should
be updated.

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8793657 [details] [diff] [review]
1263665-2-Bug_1263665___Expose_reason_for_libavcodec_linking_failure___r_jya.patch

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

::: dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp
@@ +87,5 @@
>        break;
>      case 57:
>        version = AV_FUNC_57;
>        break;
>      default:

while at it, we probably want to start adding support for the upcoming 58, so we don't get caught when it's too late

@@ +105,4 @@
>      if (!(func = (decltype(func))PR_FindSymbol(((ver) & AV_FUNC_AVUTIL_MASK) ? mAVUtilLib : mAVCodecLib, #func))) { \
>        FFMPEG_LOG("Couldn't load function " # func);                            \
>        Unlink();                                                                \
> +      return isFFMpeg ? LinkResult::MissingFFMpegFunction : LinkResult::MissingLibAVFunction; \

do we care about FFmpeg vs LibAV here?

the user would have no idea too, as the library is libavcodec / libavformat with both

and the package name on debian is libavcodec.XX.deb

::: dom/media/platforms/ffmpeg/FFmpegRuntimeLinker.h
@@ +16,5 @@
>  {
>  public:
>    static bool Init();
>    static already_AddRefed<PlatformDecoderModule> CreateDecoderModule();
> +  enum LinkStatus {

enum LinkStatus
{

no?

Comment 24

[:Gijs (out for now; he/him)]

Comment on attachment 8793660 [details] [diff] [review]
1263665-4-Bug_1263665___Frontend_notification_of_unsupported_libavcodec___r_gijs.patch

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

::: browser/base/content/browser-media.js
@@ +235,5 @@
>        return gNavigatorBundle.getString("decoder.noPulseAudio.message");
>      }
> +    if (type == "unsupported-libavcodec" &&
> +        AppConstants.platform == "linux") {
> +      return gNavigatorBundle.getString("decoder.unsupportedLibavcodec.message");

Nit: capitalize 'codec'

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +744,5 @@
>  decoder.noCodecsLinux.message = To play video, you may need to install the required video codecs.
>  decoder.noHWAcceleration.message = To improve video quality, you may need to install Microsoft’s Media Feature Pack.
>  decoder.noHWAccelerationVista.message = To improve video quality, you may need to install Microsoft’s Platform Update Supplement for Windows Vista.
>  decoder.noPulseAudio.message = To play audio, you may need to install the required PulseAudio software.
> +decoder.unsupportedLibavcodec.message = libavcodec may be vulnerable or is not supported, and should be updated to play video.

Same nit on the string id.

I would say "Your version of ..."

and probably leave a localization note that "libavcodec" should not be localized.

Comment 25

[:Gijs (out for now; he/him)]

If we want to uplift this we will need to talk with l10n folks, so pulling in :flod and :pike so they're aware early if that's something we want to do.

Comment 26

[:Gijs (out for now; he/him)]

In fact, one other thing that we could potentially change about the frontend side here is make the name of the thing that's outdated a parameter (%S) so that we could reuse the string and message if we ever want the same kind of message for other libraries.

Separately, we should make sure the SUMO link is OK and/or the page we're linking to gets updated.

Comment 27

[Francesco Lodolo [:flod]]

Which version should this be uplifted to? If it's only aurora, I'm fine with breaking string freeze as long as we land it quickly. If this needs to go on beta, I'd recommend a different solution:
* Localizable string in m-c, and let that patch ride the trains with 52.
* Hard-coded string in a different patch for m-a, m-b, m-r if needed

Given how specific the string is ("play video"), not sure we want a variable in there. Unless we're fine with something like "The version in use of %S may be vulnerable or is not supported, and should be updated" (and update the string ID to be more generic).

Comment 28

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Jean-Yves Avenard [:jya] from comment #23)
> Comment on attachment 8793657 [details] [diff] [review]
> 
> while at it, we probably want to start adding support for the upcoming 58,
> so we don't get caught when it's too late

I'll open a new bug for that.

> @@ +108,1 @@
> > +      return isFFMpeg ? LinkResult::MissingFFMpegFunction : LinkResult::MissingLibAVFunction; \
> do we care about FFmpeg vs LibAV here?
> the user would have no idea too, as the library is libavcodec / libavformat with both
> and the package name on debian is libavcodec.XX.deb

The distinction is important to expose here, because the caller FFmpegRuntimeLinker::Init() uses that information to select FFmpeg-related messages in priority to LibAV messages, in the unlikely case where multiple libraries are present but not usable.

Also, I would argue that because 'isFFMpeg' is needed before that anyway, it's very cheap to use it here to choose a more precise code.

> ::: dom/media/platforms/ffmpeg/FFmpegRuntimeLinker.h
> @@ +20,1 @@
> > +  enum LinkStatus {
> enum LinkStatus
> {
> no?

Oops yes. And there's another one I got wrong (enum class LinkResult).

Comment 29

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to :Gijs Kruitbosch from comment #24)
> Comment on attachment 8793660 [details] [diff] [review]
> 
> ::: browser/base/content/browser-media.js
> @@ +239,1 @@
> > +      return gNavigatorBundle.getString("decoder.unsupportedLibavcodec.message");
> 
> Nit: capitalize 'codec'

Note that the library name is officially "libavcodec" (all lowercase, even at the start of a sentence), that's why I only camelCased the 'L'.

I could go around this conundrum with something like "decoder.libavcodecVersionUnsupported.message".
How about that?

> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +744,5 @@
> >  decoder.noCodecsLinux.message = To play video, you may need to install the required video codecs.
> >  decoder.noHWAcceleration.message = To improve video quality, you may need to install Microsoft’s Media Feature Pack.
> >  decoder.noHWAccelerationVista.message = To improve video quality, you may need to install Microsoft’s Platform Update Supplement for Windows Vista.
> >  decoder.noPulseAudio.message = To play audio, you may need to install the required PulseAudio software.
> > +decoder.unsupportedLibavcodec.message = libavcodec may be vulnerable or is not supported, and should be updated to play video.
> 
> Same nit on the string id.

Same response and suggestion ;-)

> I would say "Your version of ..."
> 
> and probably leave a localization note that "libavcodec" should not be
> localized.

Will do, thank you.

Comment 30

[:Gijs (out for now; he/him)]

(In reply to Gerald Squelart [:gerald] from comment #29)
> (In reply to :Gijs Kruitbosch from comment #24)
> > Comment on attachment 8793660 [details] [diff] [review]
> > 
> > ::: browser/base/content/browser-media.js
> > @@ +239,1 @@
> > > +      return gNavigatorBundle.getString("decoder.unsupportedLibavcodec.message");
> > 
> > Nit: capitalize 'codec'
> 
> Note that the library name is officially "libavcodec" (all lowercase, even
> at the start of a sentence), that's why I only camelCased the 'L'.

Oh, right.

> I could go around this conundrum with something like
> "decoder.libavcodecVersionUnsupported.message".
> How about that?

SGTM!

Comment 31

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #27)
> Which version should this be uplifted to? If it's only aurora, I'm fine with
> breaking string freeze as long as we land it quickly. If this needs to go on
> beta, I'd recommend a different solution:

Can you clarify this?

Comment 32

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Francesco Lodolo [:flod] from comment #27)
> Which version should this be uplifted to? If it's only aurora, I'm fine with
> breaking string freeze as long as we land it quickly. If this needs to go on
> beta, I'd recommend a different solution:
> * Localizable string in m-c, and let that patch ride the trains with 52.
> * Hard-coded string in a different patch for m-a, m-b, m-r if needed
> 
> Given how specific the string is ("play video"), not sure we want a variable
> in there. Unless we're fine with something like "The version in use of %S
> may be vulnerable or is not supported, and should be updated" (and update
> the string ID to be more generic).

Al, could you please provide some guidance as to how far we'd want to uplift this? (Or delegate to the appropriate person.)
Especially in view of Francesco's comments regarding the strings that require translation.

Some notes:
- The issue was fixed publicly on 2016-07-26 (but it's not obvious from the patches that a security issue is being fixed)
  https://git.libav.org/?p=libav.git;a=shortlog;h=refs/heads/release/9
- Our patches here don't fix the issue, they only block using the affected libraries.
- The patches depend on bug 1247056, which has not landed yet and which probably won't be uplifted, so porting to aurora and beta would not be trivial (but not too hard either).
- I would prefer not to make the notification parameterized on the library name (as Gijs suggested in comment 26) for now, as it would be quite a bit of work, I'd prefer to do that in another bug later on. If you agree, the 2nd part of Francesco's comment 27 doesn't apply anymore.

Comment 33

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

More notes, after chatting with Anthony:
He thinks we should at least uplift to Aurora, and try for Beta even if strings cannot be translated right away. The idea is that we are blocking vulnerable software, and trying to give some feedback about it to Beta/Linux "power" users.

But in the end we are only providing a technical solution, now it's really between security and translation to decide where to land this to best benefit our users.

Comment 34

[Francesco Lodolo [:flod]]

(In reply to Gerald Squelart [:gerald] from comment #33)
> He thinks we should at least uplift to Aurora, and try for Beta even if
> strings cannot be translated right away.

That's missing the point. Uplifting to string frozen branches (aurora, beta) is going to introduce noise in all tools, both for localizers and l10n-drivers (sign-offs); for beta, most locales won't have any chance to localize these strings because they can only work against aurora. We can get this at the very beginning of the cycle in aurora, not in beta.

As explained, if the plan is to uplift on more than Aurora, of if the decision is going to take a while, I ask to have a patch with the string hard-coded for branches other than m-c: you'll warn users, and won't make a mess with l10n.

Comment 35

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Francesco Lodolo [:flod] from comment #34)
> (In reply to Gerald Squelart [:gerald] from comment #33)
> As explained, if the plan is to uplift on more than Aurora, of if the
> decision is going to take a while, I ask to have a patch with the string
> hard-coded for branches other than m-c: you'll warn users, and won't make a
> mess with l10n.

Sorry, missed the "hard-coded" bit earlier. I can certainly hard-code strings where necessary; definitely for beta, and I'll contact you when/if I start preparing the aurora uplift, to confirm which way you prefer me to go at that time.

Comment 36

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-5-Bug_1263665___Added_test_for_unsupported_libavcodec___r_gijs.patch

Learning from bug 1247056, I've now added a test for the frontend handling of unsupported-libavcodec.

Comment 37

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

(In reply to Gerald Squelart [:gerald] from comment #32)

> Al, could you please provide some guidance as to how far we'd want to uplift
> this? (Or delegate to the appropriate person.)
> Especially in view of Francesco's comments regarding the strings that
> require translation.

I'd like this on Aurora for sure. If we can manage Beta, that would be ideal but I'm willing to not do that if the string issue is going to be problematic.

Comment 38

[Francesco Lodolo [:flod]]

Personally I'd be totally in favor of hard-coding the string and landing that patch on both beta and aurora.

Comment 39

[Jean-Yves Avenard [:jya]]

Here is the message I intend to send to the various debian and ubuntu concerned package maintainers.

---
Hello

I am writing to you as you are listed as one of the libavcodec maintainer on either Debian or Ubuntu distribution.

We discovered a serious security vulnerability in libavcodec 54 and earlier.

We have submitted fixes for libavcodec 54 to the LibAV team which have been accepted. They have also agreed to bump the micro version making the first version with no vulnerability version 54.35.1
https://git.libav.org/?p=libav.git;a=shortlog;h=refs/heads/release/9

As a result, we have blacklisted libavcodec with a version earlier than 54.35.1.
This means that Firefox 50 and later will no longer be able to play some videos on system using libavcodec with the vulnerability.
System using libavcodec from the FFmpeg tree aren’t impacted.

The easiest course of action for whomever is creating the Debian or Ubuntu libav* package is to resync with upstream to grab the fixes…

There will be no binary incompatibilities with existing packages using the fixed libavcodec.

Thank you for updating the packages.
------

Comment 40

[Jean-Yves Avenard [:jya]]

FWIW, while libavcodec 53 doesn't appear impacted in the same fashion, but it does cause asan to crash:
==61154==AddressSanitizer CHECK failed: /Library/Caches/com.apple.xbs/Sources/clang_compiler_rt/clang-800.0.38/src/projects/compiler-rt/lib/asan/asan_allocator.cc:137 "((m->chunk_state)) == ((CHUNK_QUARANTINE))" (0x2, 0x3)
    #0 0x107fee22d in __asan::AsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (libclang_rt.asan_osx_dynamic.dylib+0x5522d)
    #1 0x107ff34da in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (libclang_rt.asan_osx_dynamic.dylib+0x5a4da)
    #2 0x107f9e0f6 in __asan::QuarantineCallback::Recycle(__asan::AsanChunk*) (libclang_rt.asan_osx_dynamic.dylib+0x50f6)
    #3 0x107f9de10 in __sanitizer::Quarantine<__asan::QuarantineCallback, __asan::AsanChunk>::DoRecycle(__sanitizer::QuarantineCache<__asan::QuarantineCallback>*, __asan::QuarantineCallback) (libclang_rt.asan_osx_dynamic.dylib+0x4e10)
    #4 0x107f9dcd8 in __sanitizer::Quarantine<__asan::QuarantineCallback, __asan::AsanChunk>::Recycle(__asan::QuarantineCallback) (libclang_rt.asan_osx_dynamic.dylib+0x4cd8)
    #5 0x107fe3eee in wrap_free (libclang_rt.asan_osx_dynamic.dylib+0x4aeee)
    #6 0x7fffcae1a60e in SLEventCreateFromDataAndSource (SkyLight+0x1ec60e)
    #7 0x7fffcae1c4d6 in CGSDecodeEventRecord (SkyLight+0x1ee4d6)
    #8 0x7fffcacaddd0 in CGSSnarfAndDispatchDatagrams (SkyLight+0x7fdd0)
    #9 0x7fffcac80a3c in SLSGetNextEventRecordInternal (SkyLight+0x52a3c)
    #10 0x7fffcae18dcc in SLEventCreateNextEvent (SkyLight+0x1eadcc)
    #11 0x7fffb8e2cbcc in PullEventsFromWindowServerOnConnection(unsigned int, unsigned char, __CFMachPortBoost*) (HIToolbox+0x3abcc)
    #12 0x7fffb8e2cb38 in MessageHandler(__CFMachPort*, void*, long, void*) (HIToolbox+0x3ab38)
    #13 0x7fffb988b4dc in __CFMachPortPerform (CoreFoundation+0x8d4dc)
    #14 0x7fffb988b3c8 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ (CoreFoundation+0x8d3c8)
    #15 0x7fffb988b340 in __CFRunLoopDoSource1 (CoreFoundation+0x8d340)
    #16 0x7fffb9883434 in __CFRunLoopRun (CoreFoundation+0x85434)
    #17 0x7fffb9882873 in CFRunLoopRunSpecific (CoreFoundation+0x84873)
    #18 0x7fffb7665e9d in _NSEventThread (AppKit+0x193e9d)
    #19 0x7fffceb6aaba in _pthread_body (libsystem_pthread.dylib+0x3aba)
    #20 0x7fffceb6aa06 in _pthread_start (libsystem_pthread.dylib+0x3a06)
    #21 0x7fffceb6a230 in thread_start (libsystem_pthread.dylib+0x3230)

so blacklisting libavcodec 53 appears like the way to go too. Unfortunately, we have no solution to get around this and as such distribution like Ubuntu 12.04 LTS will now longer be able to play some file (mp3, mp4, h264, aac)

libavcodec 53 v0.8 from the FFmpeg tree isn't impacted as we won't load it anyway due to a missing API.
libavcodec 53 v0.9 from the FFmpeg tree is broken here. Though, not a security issue it's a null deref: it's returning an invalid pointer that we do not check as in theory this can't happen (ffmpeg returns success code, yet the frame returned is null).
libavcodec 54 v1.0 from the FFmpeg tree plays properly. no issue.

So in all, the original approach of blocking all LibAV version < 54.35.1 and only LibAV appears to be a good one.

Comment 41

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-1-Bug_1263665___Block_libav___54_35_1___r_jya.patch

Rebase, carrying r+.

Comment 42

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-2-Bug_1263665___Expose_reason_for_libavcodec_linking_failure___r_jya.patch

Rebase, review comments addressed, carrying r+.

Comment 43

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-3-Bug_1263665___DecDoc_handling_of_libavcodec_linking_issues___r_jya.patch

Rebase, carrying r+.

Comment 44

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-4-Bug_1263665___Frontend_notification_of_unsupported_libavcodec___r_gijs.patch

Rebase, carrying r+.

Comment 45

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-5-Bug_1263665___Added_test_for_unsupported_libavcodec___r_gijs.patch

Rebase, carrying r+.

Comment 46

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8796987 [details] [diff] [review]
1263665-v2-1-Bug_1263665___Block_libav___54_35_1___r_jya.patch

Sec-approval for all 'v2' patches, numbered 1 to 5.
(Separate patches for aurora & beta coming separately.)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not at all directly: All we do is block using particular libavcodec versions with a vague user notification saying "may be vulnerable".
The potential attacker would need to go find what patches landed on that first-allowed version, which is not too difficult, but then find the actual issue to exploit, much harder as it's a race thing, and there are a few patches needed to fix the issue, making it more obscure.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
As above, not at all: All we do is block using particular libavcodec versions with a vague user notification saying "may be vulnerable".

Which older supported branches are affected by this flaw?
All, I think (first use of libavcodec seems to be in March 2014).

If not all supported branches, which bug introduced the flaw?
-

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Upcoming patches for Aurora and Beta. Main difference will be hard-coded strings because of translation deadlines.

How likely is this patch to cause regressions; how much testing does it need?
It *will* cause regressions on systems that use the blocked libavcodec, in that some video playback won't work anymore; but at least we'll notify the user about it (And we'll get telemetry about these).
Tested locally on Linux with a range of libavcodec versions, blocked and unblocked.

Comment 47

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-aurora.patch

[Security approval request comment]
See comment 46.

Approval Request Comment
[Feature/regressing bug #]: Video playback on Linux, using old libavcodec library
[User impact if declined]: Potential exploits
[Describe test coverage new/current, TreeHerder]: Tested locally on Linux with a range of libavcodec versions, blocked and unblocked.
[Risks and why]: Low risk, as we're only blocking particular library versions; And the Decoder Doctor notification code has been used before, we're only adding new messages.
[String/UUID change made/needed]: Added hard-coded strings to match the new translatable strings from the nightly patches.

Comment 48

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-beta.patch

[Security approval request comment]
See comment 46.

Approval Request Comment
[Feature/regressing bug #]: Video playback on Linux, using old libavcodec library
[User impact if declined]: Potential exploits
[Describe test coverage new/current, TreeHerder]: Tested locally on Linux with a range of libavcodec versions, blocked and unblocked.
[Risks and why]: Low risk, as we're only blocking particular library versions; And the Decoder Doctor notification code has been used before, we're only adding new messages.
[String/UUID change made/needed]: Added hard-coded strings to match the new translatable strings from the nightly patches.

Comment 49

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

Comment on attachment 8797008 [details] [diff] [review]
1263665-aurora.patch

sec-approval for trunk. Once a trunk patch gets sec-approval, you don't need separate sec-approval for branch variant patches.

Comment 50

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

What about an ESR45 patch?
At least some of the Linux distros only build from ESR, like Redhat.

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8797010 [details] [diff] [review]
1263665-beta.patch

Sec bug, Beta50+

Comment 52

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-esr45.patch

f?k17e:
Anthony, there is no Decoder Doctor infrastructure in ESR45 to show a user notification. This bug just refuse to use libavcodec<54.35.1.
Do you think we should still go ahead with this silent blocking?

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potential exploit through vulnerable libavcodec. More likely because users of old FF are probably on older systems with one of the vulnerable libraries.
User impact if declined: Potential exploit.
Fix Landed on Version: Pending on b50, a51, n52.
Risk to taking this patch (and alternatives if risky): Very little, it is just a small test that prevents uses of a vulnerable library based on its version number.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 53

[Carsten Book [:Tomcat]]

gerald, seems this need dom peer review too for :

remote: WebIDL file dom/webidl/DecoderDoctorNotification.webidl altered in changeset 4c1e9d7b6477 without DOM peer review

Comment 54

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8796989 [details] [diff] [review]
1263665-v2-3-Bug_1263665___DecDoc_handling_of_libavcodec_linking_issues___r_jya.patch

Olli, could you please review the webidl? (It's only a new enum value.)
Thank you.

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8796989 [details] [diff] [review]
1263665-v2-3-Bug_1263665___DecDoc_handling_of_libavcodec_linking_issues___r_jya.patch

>+++ b/dom/media/DecoderDoctorDiagnostics.cpp
I can't really review changes to this file...

> #if defined(MOZ_FFMPEG)
>       if (!formatsRequiringFFMpeg.IsEmpty()) {
>-        DD_INFO("DecoderDoctorDocumentWatcher[%p, doc=%p]::SynthesizeAnalysis() - unplayable formats: %s -> Cannot play media because platform decoder was not found",
>-                this, mDocument, NS_ConvertUTF16toUTF8(formatsRequiringFFMpeg).get());
>-        ReportAnalysis(mDocument, sMediaPlatformDecoderNotFound,
>-                       false, formatsRequiringFFMpeg);
>-        return;
>+        switch (FFmpegRuntimeLinker::LinkStatusCode()) {
>+          case FFmpegRuntimeLinker::LinkStatus_INVALID_FFMPEG_CANDIDATE:
>+          case FFmpegRuntimeLinker::LinkStatus_UNUSABLE_LIBAV57:
>+          case FFmpegRuntimeLinker::LinkStatus_INVALID_LIBAV_CANDIDATE:
>+          case FFmpegRuntimeLinker::LinkStatus_OBSOLETE_FFMPEG:
>+          case FFmpegRuntimeLinker::LinkStatus_OBSOLETE_LIBAV:
>+          case FFmpegRuntimeLinker::LinkStatus_INVALID_CANDIDATE:
>+            DD_INFO("DecoderDoctorDocumentWatcher[%p, doc=%p]::SynthesizeAnalysis() - unplayable formats: %s -> Cannot play media because of unsupported %s (Reason: %s)",
>+                    this, mDocument,
>+                    NS_ConvertUTF16toUTF8(formatsRequiringFFMpeg).get(),
>+                    FFmpegRuntimeLinker::LinkStatusLibraryName(),
>+                    FFmpegRuntimeLinker::LinkStatusString());
>+            ReportAnalysis(mDocument, sUnsupportedLibavcodec,
>+                           false, formatsRequiringFFMpeg);
>+            return;
>+          case FFmpegRuntimeLinker::LinkStatus_INIT:
>+            MOZ_FALLTHROUGH_ASSERT("Unexpected LinkStatus_INIT");
>+          case FFmpegRuntimeLinker::LinkStatus_SUCCEEDED:
>+            MOZ_FALLTHROUGH_ASSERT("Unexpected LinkStatus_SUCCEEDED");
>+          case FFmpegRuntimeLinker::LinkStatus_NOT_FOUND:
>+            DD_INFO("DecoderDoctorDocumentWatcher[%p, doc=%p]::SynthesizeAnalysis() - unplayable formats: %s -> Cannot play media because platform decoder was not found (Reason: %s)",
>+                    this, mDocument,
>+                    NS_ConvertUTF16toUTF8(formatsRequiringFFMpeg).get(),
>+                    FFmpegRuntimeLinker::LinkStatusString());
>+            ReportAnalysis(mDocument, sMediaPlatformDecoderNotFound,
>+                           false, formatsRequiringFFMpeg);
>+            return;
>+        }
...since I'm not familiar with all the FFmpegRuntimeLinker::LinkStatus_* stuff here.


But maybe that got a review already from someone else?

r+ for the rest.

Comment 56

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Olli Pettay [:smaug] from comment #55)
> But maybe that got a review already from someone else?

Yes, everything else had already been reviewed.

> r+ for the rest.

Thank you.

Comment 57

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-3-Bug_1263665___DecDoc_handling_of_libavcodec_linking_issues___r_jya_smaug.patch

Just adding smaug as reviewer, because of the webidl requiring DOM peer review.

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49abb8530a43f10c8e2ddacabde9941445d3f46f
 https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=bebbf04b7fe13ed3e43d729558505d2917691621

al, does the trunk patches 2-5 need also sec-approval or is the one for patch 1 enough ?

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Backed for test failures.

On Aurora, a couple different Taskcluster test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=3742035&repo=mozilla-aurora
https://treeherder.mozilla.org/logviewer.html#?job_id=3741048&repo=mozilla-aurora

On Beta, Taskcluster web-platform-test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=1713680&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-aurora/rev/21158916831f294f155eea12537be9cca8714fbd
https://hg.mozilla.org/releases/mozilla-beta/rev/d9ff0b182f4cda0c7e9bac046bc11774985d393e

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Given more time for results to filter in, the failures affected non-Taskcluster Linux jobs as well. And the failures in comment 59 affected both Aurora and Beta - they weren't specific to one or the other.

Comment 61

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

Carsten, we only need one sec-approval outside of exceptional circumstances.

Comment 62

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

The browser_decoderDoctor.js issue is an easy fix, I forgot to hardcode the message string in the test.

But the others may point at our Linux Try machines using a now-blocked libavcodec, I will investigate...

Comment 63

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

cc:Nick, who will help with checking which version(s) of libavcodec are on the Linux Try images.

Comment 64

[Nick Thomas [:nthomas] (UTC+12)]

Sorry, all the jobs listed in comment #59 ran on taskcluster rather than buildbot.

Comment 65

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Nick forwarded me to the TaskCluster crowd, where :bstack suggested we ask :garndt for help.

Greg: As you can see in this bug, we'd like to block libavcodec.so prior to 54.35.1.
Brian helped me find out that we're using 53.35.0, which explains why H264 tests are failing on Linux.

So now the big question is: Can we upgrade this library, and if yes, how?
(I think it could be done in a separate bug with no direct relation to this sec-bug here, so it just looks like a routine upgrade.)

Comment 66

[Ryan VanderMeulen [:RyanVM]]

To be clear, per comment 60, the failures affected *both* buildbot and Taskcluster. Comment 59 was at a point when not all the job results had filtered through yet, hence the later update.

Comment 67

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I probably misled Nick, pointing only at TaskCluster failures.
Thank you Ryan for the reminder, yes we'd need all relevant platforms to change.

Now :jya tells me that we might be stuck with Ubuntu 12.04 for now, which only has old versions of libav available. :-(
If that's true, would it be possible to manually install a new libav?
Though upgrading the distribution would be nice, as it probably would better reflect our user base, especially when testing media; but that might be too big a job?

So in the worst case, if we cannot upgrade buildbot and/or taskcluster (distribution or just libav), but still want to land this libav-blocking patch, we might have to disable tests, but this would mean losing test coverage for h264/Linux of course.

Comment 68

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

jya just suggested another option, where we won't need to upgrade or disable tests:
Use a new pref that blocks bad libav's by default, and have our tests change that pref to force-enable libav. I'll start working on that option.

Comment 69

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-6-Bug_1263665___media_libavcodec_allow_obsolete_true_bypasses_blocking___r_jya.patch

If "media.libavcodec.allow-obsolete" is created and set to true, the checks
for older libavcodec library versions are disregarded.

Comment 70

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v2-7-Bug_1263665___Set_media_libavcodec_allow_obsolete_true_for_testing___r_jya.patch

Set media.libavcodec.allow-obsolete=true for testing.

Comment 71

[Jean-Yves Avenard [:jya]]

Comment on attachment 8797903 [details] [diff] [review]
1263665-v2-6-Bug_1263665___media_libavcodec_allow_obsolete_true_bypasses_blocking___r_jya.patch

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

r=me with comment addressed

::: dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp
@@ +49,5 @@
>        // support FFmpeg 57 at this stage.
>        Unlink();
>        return LinkResult::CannotUseLibAV57;
> +    } else if (version < (54u << 16 | 35u << 8 | 1u)
> +               && !Preferences::GetBool("media.libavcodec.allow-obsolete",

I can't remember if there's a guarantee that this code is always called from the main thread.

So, just in case, and because it looks nicer anyway, use a MediaPref one

Comment 72

[Jean-Yves Avenard [:jya]]

Comment on attachment 8797904 [details] [diff] [review]
1263665-v2-7-Bug_1263665___Set_media_libavcodec_allow_obsolete_true_for_testing___r_jya.patch

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

r=me with changes

::: testing/profiles/prefs_general.js
@@ +359,5 @@
>  user_pref("plugin.load_flash_only", false);
> +
> +// Don't block old libavcodec libraries when testing, because our test systems
> +// cannot easily be upgraded.
> +user_pref("media.libavcodec.allow-obsolete", true);

please add one in modules/libpref/all/init.js so it's easy to find. I can imagine users being pissed because they no longer get media playback

Comment 73

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v3-6-Bug_1263665___media_libavcodec_allow_obsolete_true_bypasses_blocking___r_jya.patch

Addressed review comments, including the one for part 7, as I think the 'all.js' entry should be in the main patch introducing the new pref and should stay there forever, while part 7 is concentrating on the testing exception which may be reversed in the future.
Carrying r+.

Comment 74

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v3-7-Bug_1263665___Set_media_libavcodec_allow_obsolete_true_for_testing___r_jya.patch

Updated commit description. Carrying r+.

Comment 75

[Carsten Book [:Tomcat]]

landed changes on inbound 

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4af898e0aeb1f9c960444e48d74917c9dcd19e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3b31f14fd42f83829a0d085f79dc2a7fe6614d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad8023a69963f886a381ae0605ed87b18c509f6a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/24313c3c55dadc8f01a264967d9bb2c9ca112500
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cef1bb49cdb7a02d3a1a2136d33a99c459c501e6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5db740625292e7ea4889f699f2138e81e5fc70c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/971e37206edca14a9e746313e3fc2fa0ebfd6278

Comment 76

[Carsten Book [:Tomcat]]

had to back this out again for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=37083884&repo=mozilla-inbound

Comment 77

[Greg Arndt [:garndt]]

gps, looping you in because it might mean the packages that in-tree images use might need to be adjust, as I understand it.

Please let me know if you can't take care of this, or if there is someone else that should be handling it.

Specifically comment 65

Comment 78

[Jean-Yves Avenard [:jya]]

The pref is defined only MOZ_FFMPEG which won't be defined in windows but code is still used to link ffvp9

Comment 79

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I wish I could use Try to catch these!

Comment 80

[Andrew McCreight [:mccr8]]

Once you have sec-approval, I think it is okay to do a try run, FWIW.

Comment 81

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Andrew McCreight [:mccr8] from comment #80)
> Once you have sec-approval, I think it is okay to do a try run, FWIW.

Thank you for the tip.

And I guess the cat's out of the bag, since this has landed twice already! So I guess I'll do a Try run, to avoid further embarrassment...

Comment 82

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-1-Bug_1263665___Block_libav___54_35_1___r_jya.patch

Rebase, carrying r+. Was sec-approved too.

Comment 83

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-2-Bug_1263665___Expose_reason_for_libavcodec_linking_failure___r_jya.patch

Rebase, carrying r+.

Comment 84

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-3-Bug_1263665___DecDoc_handling_of_libavcodec_linking_issues___r_jya.patch

Rebase, carrying r+.

Comment 85

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-4-Bug_1263665___Frontend_notification_of_unsupported_libavcodec___r_gijs.patch

Rebase, carrying r+.

Comment 86

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-5-Bug_1263665___Added_test_for_unsupported_libavcodec___r_gijs.patch

Rebase, carrying r+.

Comment 87

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-6-Bug_1263665___media_libavcodec_allow_obsolete_true_bypasses_blocking___r_jya.patch

Rebase and added missing #ifdef, carrying r+.

Comment 88

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-7-Bug_1263665___Set_media_libavcodec_allow_obsolete_true_for_testing___r_jya.patch

Rebase, carrying r+.

Comment 89

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-aurora-v4.patch

Re-ported from trunk v4 patches, carrying r+. Aurora-approval given in comment 49.

Comment 90

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-beta-v4.patch

Re-ported from trunk v4 patches, carrying r+. Beta-approval given in comment 51.

Comment 91

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-esr45-v4.patch

Added pref (so that tests can run on our systems with an old lib, and giving power users an escape hatch), carrying r+.

f?k17e:
Anthony, there is no Decoder Doctor infrastructure in ESR45 to show a user notification. This bug just silently refuses to use libavcodec<54.35.1.
Do you think we should still go ahead with this silent blocking?

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potential exploit through vulnerable libavcodec. More likely because users of old FF are probably on older systems with one of the vulnerable libraries.
User impact if declined: Potential exploit.
Fix Landed on Version: Pending on b50, a51, n52.
Risk to taking this patch (and alternatives if risky): Very little, it is just a small test that prevents uses of a vulnerable library based on its version number.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 92

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Gerald - I don't think we have a choice for ESR45. Can we print an error to the console?

Comment 93

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-esr45-v5.patch

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #92)
> Gerald - I don't think we have a choice for ESR45. Can we print an error to
> the console?
This can certainly be arranged, but because I don't have access to the document in that code, it will have to go to the Browser Console (i.e., not the Web Console for that page).

Here is the updated patch, carrying r+ (only added trivial one-line console log.)

Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potential exploit through vulnerable libavcodec. More likely because users of old FF are probably on older systems with one of the vulnerable libraries.
User impact if declined: Potential exploit.
Fix Landed on Version: Pending on b50, a51, n52.
Risk to taking this patch (and alternatives if risky): Very little, it is just a small test that prevents uses of a vulnerable library based on its version number.
String or UUID changes made by this patch: One hard-coded string, nothing to translate.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 94

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Not sure if this one dropped from the radar? (Or I'm just impatient!?)
Adding checkin-needed to get attention.

Do I need to officially re-request aurora&beta uplifts?

Comment 95

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-aurora-v5.patch

Rebased, carrying r+. Aurora-approval given in comment 49.

Comment 96

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-beta-v5.patch

Rebased, carrying r+. Beta-approval given in comment 51.

Comment 97

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

CC:sheriffs as advised on IRC.

In case of conflict, it would most likely be in testing/profiles/prefs_general.js.
Note that my patches should only add one pref with two comment lines at the end of the file:
> +
> +// Don't block old libavcodec libraries when testing, because our test systems
> +// cannot easily be upgraded.
> +user_pref("media.libavcodec.allow-obsolete", true);

Or if in modules/libpref/init/all.js, I'm only adding ",MediaUnsupportedLibavcodec" at the end of the "media.decoder-doctor.notifications-allowed" pref.

Or ping me!

Comment 98

[Carsten Book [:Tomcat]]

Hi Gerald, for trunk (m-i) this patches need rebasing i guess because it does not apply like:

Hunk #4 succeeded at 138 with fuzz 2 (offset 8 lines).
1 out of 4 hunks FAILED -- saving rejects to file dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1263665-2.patch

Comment 99

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a2e2e689b1e709da59b07708d61b4aeb10457438

Comment 100

[Carsten Book [:Tomcat]]

(In reply to Gerald Squelart [:gerald] from comment #97)
> CC:sheriffs as advised on IRC.
> 
> In case of conflict, it would most likely be in
> testing/profiles/prefs_general.js.
> Note that my patches should only add one pref with two comment lines at the
> end of the file:
> > +
> > +// Don't block old libavcodec libraries when testing, because our test systems
> > +// cannot easily be upgraded.
> > +user_pref("media.libavcodec.allow-obsolete", true);
> 
> Or if in modules/libpref/init/all.js, I'm only adding
> ",MediaUnsupportedLibavcodec" at the end of the
> "media.decoder-doctor.notifications-allowed" pref.
> 
> Or ping me!

yeah beta has conflicts but there are 2 prefs ?

<<<<<<< dest
=======

// Don't block old libavcodec libraries when testing, because our test systems
// cannot easily be upgraded.
user_pref("media.libavcodec.allow-obsolete", true);
user_pref("browser.usedOnWindows10.introURL", "");
>>>>>>> source

Comment 101

[Carsten Book [:Tomcat]]

comment #100 turned out to be a problem with my hg repo, fixed and pushed

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

Comment 102

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1263665-v4-2-Bug_1263665___Expose_reason_for_libavcodec_linking_failure___r_jya.patch rebase

Rebase, carrying r+.

Comment 103

[Carsten Book [:Tomcat]]

thanks gerald, landed on inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f44800ed010b51014f0ff43174904121a0565bc7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b504cca08c71f703c8218ff1c57820f733135cf8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0fe4e6cae3f640d0ce4659f2d88476c36ddd6ba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e57ef1945741ed45618e3629a4c077d18e226d64
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/836424e598f349d2e522ba341c5d14ce7fe792cf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b43d41a664f7f82dde2512054f1ada4f0c44487
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/679bbfdf2c808f5e3017057edf54c07cb188f938

Comment 104

[Carsten Book [:Tomcat]]

Attachment: talos.patch

need to add the new pref to talos too to avoid perma g4 failure

Comment 105

[Carsten Book [:Tomcat]]

Attachment: talos patch

Comment 106

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8801092 [details] [diff] [review]
talos.patch

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

::: testing/talos/talos/config.py
@@ +174,5 @@
>          'devtools.chrome.enabled': False,
>          'devtools.debugger.remote-enabled': False,
>          'devtools.theme': "light",
>          'devtools.timeline.enabled': False,
>          'identity.fxaccounts.migrateToDevEdition': False

we need a trailing , on the previous line

@@ +175,5 @@
>          'devtools.debugger.remote-enabled': False,
>          'devtools.theme': "light",
>          'devtools.timeline.enabled': False,
>          'identity.fxaccounts.migrateToDevEdition': False
> +	'media.libavcodec.allow-obsolete', True

this needs to be have a : not a ,

Comment 107

[Carsten Book [:Tomcat]]

thanks joel, landed in this in https://hg.mozilla.org/integration/mozilla-inbound/rev/9797bbeb275ab3ac5c21333783ccdcf7c8cfde43

Comment 108

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5fca1c8ef0b8198e902a3c1f640a49cd4d76905b
https://hg.mozilla.org/releases/mozilla-beta/rev/4654bfb375e4b83bf3f09a6afa797b4bb5b9fa3f

Comment 109

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f44800ed010b
https://hg.mozilla.org/mozilla-central/rev/b504cca08c71
https://hg.mozilla.org/mozilla-central/rev/c0fe4e6cae3f
https://hg.mozilla.org/mozilla-central/rev/e57ef1945741
https://hg.mozilla.org/mozilla-central/rev/836424e598f3
https://hg.mozilla.org/mozilla-central/rev/5b43d41a664f
https://hg.mozilla.org/mozilla-central/rev/679bbfdf2c80
https://hg.mozilla.org/mozilla-central/rev/9797bbeb275a

Comment 110

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8800009 [details] [diff] [review]
1263665-esr45-v5.patch

OK, let's take it for security reasons...

Comment 111

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/a79cafee93f40a0007547320e018fadd7c709e5c

Comment 112

[Gervase Markham [:gerv]]

The error message this patch gives is effectively non-actionable. Most users won't know how to "upgrade their libav-codec". Even if they know what one is, if your distro hasn't released an update (which you would get automatically anyway in most cases), they would need to figure out how to install a non-distro version.

I'd say that simply turning off video with no sensible recourse is a significant breakage of the web and, particularly if the security issue isn't public yet, shouldn't be done. We should continue to support old versions until new ones are released. Are we working with the distro maintainers to make that happen?

Gerv

Comment 113

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Anthony, what do you think about Gerv's comment 112?

On my side, I could easily add something to SUMO (and link there from the notification bar), that vaguely tells what the issue is, what to do (update libav), and if that's not possible with some distros we could mention the pref, but this is re-introducing sec risks.

Comment 114

[Jean-Yves Avenard [:jya]]

(In reply to Gervase Markham [:gerv] from comment #112)

> versions until new ones are released. Are we working with the distro
> maintainers to make that happen?

We have contacted the distro distributing those broken versions of Libav. We have even given them the fix to use and worked with Libav so that they include the fix in their tree.

If they don't support their LTS release when they have known for months of the issue, what more can we do?

In the mean time, the easiest course of action is to upgrade your ubuntu release to 16.04. Also a LTS, though I'm not sure LTS means much these days.

Comment 115

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Gervase Markham [:gerv] from comment #112)
> Are we working with the distro maintainers to make that happen?

Ubuntu have known about this for a month. Debian and Redhat do not ship libav/ffmpeg. A version with the fix has been available for over a month.

We are not responsible for security maintenance on old Linux distributions.

It seems reasonable to expect that someone capable of adding a backports PPA is also capable of updating libavcodec or filing a bug with their distribution. If you wish to write a Sumo article then please file a legal bug ahead of doing so.

Comment 116

[Gervase Markham [:gerv]]

Can you point me at the route(s) you've used to contact Ubuntu and/or the person concerned so I can follow up?

I agree we are not responsible for security maintenance of Linux distributions, but it also seems disproportionate to disable video for all Linux users while the bug remains undisclosed, particularly as we at Mozilla discovered it!

Gerv

Comment 117

[:Gijs (out for now; he/him)]

(In reply to Gervase Markham [:gerv] from comment #116)
> Can you point me at the route(s) you've used to contact Ubuntu and/or the
> person concerned so I can follow up?
> 
> I agree we are not responsible for security maintenance of Linux
> distributions, but it also seems disproportionate to disable video for all
> Linux users

It is only disabled for users on old (ie affected) versions of libav, which is hardly the same as "all Linux users".

Comment 118

[Gervase Markham [:gerv]]

True, although Ubuntu 14.04 is hardly an uncommon version either.

Gerv

Comment 119

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Will affect many Linux users; we need a note for 45.5.0esr and for 50. 
[Affects Firefox for Android]: no
[Suggested wording]:  Block unsupported video libraries for Linux users
[Links (documentation, blog post, etc)]:   I like the idea of a SUMO article that advises people to update libav. 

Dan or Gerald, can you suggest better wording for the release note? Thanks.

Comment 120

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #119)
> [Suggested wording]:  Block unsupported video libraries for Linux users
> 
> Dan or Gerald, can you suggest better wording for the release note? Thanks.

Maybe consider saying "unsupported and potentially vulnerable", as it would feel less arbitrary?
(Disclaimer: I have zero experience with release notes)

> [Links (documentation, blog post, etc)]:   I like the idea of a SUMO article
> that advises people to update libav. 

As Anthony said in comment 115, we would need legal advice about this, as we (Mozilla) should not be seen as recommending ffmpeg or libav.

Comment 121

[Daniel Veditz [:dveditz]]

It's just going to raise questions "which libraries?", "which versions are unsupported?", and possibly "My distro supports it! What do you mean unsupported?"

Can we say "Blocked versions of libavcodec older than 54.35.1" or similar?

Comment 122

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding Elvin so he can review the issue. I like Dan's suggested wording in comment 121, simply stating the version/number that we're blocking.

Comment 123

[Liz Henry (:lizzard) (relman/hg->git project)]

Note, it looks like the underlying bug is public now, https://bugzilla.libav.org/show_bug.cgi?id=939

Comment 124

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx50 release notes