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.

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?

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

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

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

(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.

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?

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.

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.

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.

(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.

MozReview-Commit-ID: 60XDTJJqpFR

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.

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.

(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.

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

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

libav has tagged the new version by bumping the micro version. Let's block all revisions of LibAV prior that one.

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

Block libav < 54.35.1

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).

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.

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 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 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.

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.

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.

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).

(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).

(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.

(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!

(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?

(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.

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.

(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.

(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.

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

(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.

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

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. ------

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.

Rebase, carrying r+.

Rebase, review comments addressed, carrying r+.

Rebase, carrying r+.

Rebase, carrying r+.

Rebase, carrying r+.

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.

[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.

[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 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.

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

Comment on attachment 8797010 [details] [diff] [review] 1263665-beta.patch Sec bug, Beta50+

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.

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 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 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.

(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.

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

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 ?

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

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.

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

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...

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

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

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.)

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.

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.

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.

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

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

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 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

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+.

Updated commit description. Carrying r+.

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

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

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

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

I wish I could use Try to catch these!

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

(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...

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

Rebase, carrying r+.

Rebase, carrying r+.

Rebase, carrying r+.

Rebase, carrying r+.

Rebase and added missing #ifdef, carrying r+.

Rebase, carrying r+.

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

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

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.

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

(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.

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?

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

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

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!

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

(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 #100 turned out to be a problem with my hg repo, fixed and pushed https://hg.mozilla.org/releases/mozilla-beta/rev/c216304c306bda09e8b8766f692885fa378ddb3c

Rebase, carrying r+.

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

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

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 ,

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

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

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 on attachment 8800009 [details] [diff] [review] 1263665-esr45-v5.patch OK, let's take it for security reasons...

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

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.

(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.

(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.

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

(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".

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

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.

(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.

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?

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.

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

Added to Fx50 release notes