[MIR] libheif

Bug #1827442 reported by Steve Langasek
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
libde265 (Ubuntu)
Won't Fix
Undecided
Unassigned
libheif (Ubuntu)
Won't Fix
Undecided
Unassigned
x265 (Ubuntu)
Won't Fix
Undecided
Unassigned

Bug Description

[Availability]
Available on all architectures in universe from bionic forward.

[Rationale]
This is a new build-dependency added to imagemagick in Debian unstable. It implements support for decoding ISO/IEC 23008-12:2017 HEIF files, which are not otherwise supported by any libraries in Ubuntu main.

[Security]
One vulnerability was reported this year against libheif 1.4.0 (http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-11471). Debian currently has libheif 1.3.2. According to the upstream issue at https://github.com/strukturag/libheif/issues/123 the vulnerability was first introduced in an unreleased, git-only version of libheif (post-1.4.0), and found and fixed by the upstream community prior to finding its way into a tagged release. It is not clear to me that the vulnerability in question applies to 1.3.2.

This is a media file parser, so is security-sensitive because it will be processing complex untrusted input.

[Quality assurance]
Packaging is lintian-clean using modern dh(1) patterns and shows no problematic bug history in Debian or Ubuntu.

Package runs make check at build time (debhelper), but has no build-time tests or autopkgtests available.

[Dependencies]
Also depends on x265 and libde265 which are in universe.

[Maintenance]
Package would be maintained by Ubuntu Foundations Team.

Tags: eoan

CVE References

Steve Langasek (vorlon)
description: updated
Changed in libheif (Ubuntu):
status: Incomplete → New
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

I've had a look, certainly seems fine packaging-wise; but should be looked at by the Security Team.

Changed in libheif (Ubuntu):
assignee: nobody → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Steve Langasek (vorlon) wrote :

I just noticed via https://people.canonical.com/~ubuntu-archive/component-mismatches-proposed.svg that libde265 wants to pull in:
 - qtbase-opensource-src (qt4)
  - qtchooser
  - double-conversion
  - qtsvg-opensource-src
  - qttranslations-opensource-src
 - libsd1.2
 - *ffmpeg*
  - zeromq3
   - norm
   - libpgm
  - libdc1394-22
  - libbs2b
  - x264
  - lilv
   - sratom
   - serd
   - sord
  - zvbi
  - libva
   - intel-vaapi-driver
   - intel-media-driver
    - intel-gmmlib
   - libset-scalar-perl
  - codec2
  - chromaprint
  - twitter-bootstrap3
  - libgsm
  - crystalhd
  - libopenmpt
  - game-music-emu
  - libvidstab
  - rubberband
  - aom
  - xvidcore
  - openjpeg2
  - libbluray
   - libaacs
    - libbdplus
  - openal-soft
   - sndio
  - shine
  - flite
  - libmysofa
  - libass

Sooo that's a huge pile of work and it's not justifiable in terms of reducing delta on the imagemagick package. I'm going to reupload imagemagick to drop the libheif build-dependency. Security feedback on the suitability of libheif is still welcome but is a low priority.

Revision history for this message
Seth Arnold (seth-arnold) wrote :

Wow, that's a pretty significant chain of deps. Thanks for pruning this one.

I had started in on libheif but not much beyond inspecting a few Coverity results. (More in the spirit of learning Coverity than inspecting libheif, I must be honest.)

I passed along the Coverity results to upstream on https://github.com/strukturag/libheif/issues/128 -- hopefully it'll be useful to them.

Thanks

Revision history for this message
Joachim Bauch (fancycode) wrote :

libde265-0 actually pulls in a lot less dependencies:
https://packages.ubuntu.com/bionic/libde265-0

The dependencies on Qt, SDL, swscale are being pulled in by the examples:
https://packages.ubuntu.com/bionic/libde265-examples

libheif only depends on libde265-0 and thus doesn't pull in the whole list of dependencies mentioned in #2

Revision history for this message
Steve Langasek (vorlon) wrote :

Joachim, thanks for bringing this to my attention. I've gone ahead and added libde265-examples to the exclusion list for main, and readded the libheif build-dep to imagemagick in order to revisit this MIR.

Seth, can this go back in the queue for security team review?

Revision history for this message
Seth Arnold (seth-arnold) wrote :

Thanks Joachim, Steve, I've removed the comment in trello saying this is on hold.

Thanks

Revision history for this message
Amr Ibrahim (amribrahim1987) wrote :

Just a question, I thought build-dependencies need not be in main any more to build packages in main. It was announced some time ago on ubuntu-devel. Am I wrong?

Revision history for this message
Steve Langasek (vorlon) wrote :

It's correct that packages do not have to be in main merely for being build dependencies. But most libraries that are build dependencies are linked against during build and become runtime dependencies.

Revision history for this message
Balint Reczey (rbalint) wrote :

This MIR keeps imagemagick out of the release pocket in the devel release.

Changed in imagemagick (Ubuntu):
status: New → Invalid
tags: added: update-excuse
Revision history for this message
Seth Arnold (seth-arnold) wrote :

Apparently this is useful for photos from iPhones.

Revision history for this message
Joachim Bauch (fancycode) wrote :
Balint Reczey (rbalint)
Changed in imagemagick (Ubuntu):
status: Invalid → Fix Released
Balint Reczey (rbalint)
Changed in imagemagick (Ubuntu):
status: Fix Released → Won't Fix
Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in libde265 (Ubuntu):
status: New → Confirmed
Changed in libheif (Ubuntu):
status: New → Confirmed
Changed in x265 (Ubuntu):
status: New → Confirmed
Balint Reczey (rbalint)
no longer affects: imagemagick (Ubuntu)
Balint Reczey (rbalint)
tags: removed: update-excuse
Revision history for this message
Seth Arnold (seth-arnold) wrote :

Here's the Coverity results on libheif. The first third of the file is all about example code. It might be nice to get those issues fixed, both to help bring the overall number of findings closer to zero, but more importantly to improve the quality of potential copy-and-paste coding.

I haven't looked at the results in any real depth.

Thanks

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I forgot I already passed along the coverity results to the libheif team:
https://github.com/strukturag/libheif/issues/128

Their response is very encouraging.

Thanks

Revision history for this message
Seth Arnold (seth-arnold) wrote :

Is the libheif package still lintian clean? Using disco's lintian I get the following:

Output of lintian:
 W: libheif source: debhelper-compat-file-is-missing
 W: libheif source: package-uses-deprecated-debhelper-compat-version 1
 E: libheif source: package-uses-debhelper-but-lacks-build-depends
 W: libheif source: newer-standards-version 4.4.1 (current is 4.1.4)

This is libheif 1.6.1-1.

Thanks

Revision history for this message
Seth Arnold (seth-arnold) wrote :

Has anyone looked to see if x265 and libde265 are also required to be MIR'd for this package to be useful in its target use? There's no install time dependencies declared in the Debian packaging but it's hard for me to imagine why these libraries would be required at build in order to enable some of the functionality, unless they also influence the runtime behaviour of the library.

Thanks

Revision history for this message
Steve Langasek (vorlon) wrote :

Seth, I'm not sure how you reached the conclusion that there are "no install time dependencies declared in the Debian packaging". libheif1-1 in focal depends on both libx265-179 and libde265-0.

Revision history for this message
Seth Arnold (seth-arnold) wrote :

Thanks Steve, I'm not sure why I thought the dependencies on libx265-179 and libde265-0 were overlooked. Now that I take another look it all looks pretty normal.

Thanks

Revision history for this message
Sebastian Ramacher (s-ramacher) wrote :

> Is the libheif package still lintian clean?

Looks like that lintian is too old.

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I've returned to this package, and I'm seeing enough // TODO markers that I'm starting to be concerned that this library implements sufficient functionality to be useful for us. The code that's there mostly looks good, but there's enough markers for "handle errors" and "find out .." that I'm wondering if enough of this is useful for the things we want it for.

Has it been used and found useful?

Thanks

Revision history for this message
Steve Langasek (vorlon) wrote : Re: [Bug 1827442] Re: [MIR] libheif

On Wed, Feb 26, 2020 at 04:02:33AM -0000, Seth Arnold wrote:
> Has it been used and found useful?

Not by me. This was pulled into Ubuntu automatically via Debian.

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I reviewed libheif 1.6.1-1 as checked into focal. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

libheif is an image codec library necessary for decoding photos from some
newer phones.

- CVE History:
  - CVE-2019-11471, our database says still unfixed in 18.04 LTS.
- Build-Depends: debhelper-compat, libde265-dev, libgdk-pixbuf2.0-dev,
  libjpeg-dev, libpng-dev, libx265-dev, pkg-config
- no pre/post inst/rm scripts
- no init scripts
- no systemd units
- no dbus services
- no setuid binaries
- binaries in PATH
  - heif-thumbnailer in heif-thumbnailer
  - heif-convert, heif-enc, heif-info in libheif-examples
- no sudo fragments
- no udev rules
- There are some very thin unit tests. No autopkgtests. THere's some
  support for fuzzers but I don't see it used.
- no cron jobs
- relatively clean build logs

- no processes spawned
- significant memory management and C-style manipulation of data. Most
  calls looked like there were checks in place, but this level of C-style
  memory manipulation probably has errors.
- No file IO in library, only in examples
- Very little human logging; looked fine
- No environment variable usage
- No use of privileged functions
- No cryptography
- No temp files
- No networking
- No webkit
- No polkit

- cppcheck false positive
- an earlier look found some coverity issues, which the team addressed

Here's a list of the small handful of things I noticed:

In convert_libde265_image_to_heif_image() what constrains stride to
reasonable values? I get lost reading libde265 code to find the stride.

setjmp() used for error handling in example code; this kind of error
handling is very difficult to use correctly over time.

Y4MEncoder::Encode() doesn't appear to guard against integer overflow in
fwrite() calls

Box_iloc::write_mdat_after_iloc() 4gig outputs unhandled

I'm not sure the consequences of any of these issues.

Code quality looked goodh, especially for a codec library; the examples didn't
look as good, but this is common.

Security team ACK for promoting the libheif library packages libheif1 and
heif-gdk-pixbuf to main. I'd like to keep the examples in heif-thumbnailer
and libheif-examples in universe.

Thanks

Changed in libheif (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Revision history for this message
Joachim Bauch (fancycode) wrote :

Anything we could do to help move this forward?

Revision history for this message
Dan Streetman (ddstreet) wrote :

It's unclear to me if this ever was approved by the MIR team, possibly comment 1 was intended to be a MIR team review, but it's not clear if that was the intention and certainly doesn't have the normal MIR review details.

Technically after security team review, this should have been changed to status 'In Progress' to wait for seeding updates, but I think this got 'lost' in the process since it was in state 'CONFIRMED' but the MIR team has only been looking at 'NEW' state unclaimed bugs.

@paelzer I updated the 'All open unclaimed MIR bugs' link on the MIR wiki page to show both NEW and CONFIRMED bugs, and I think we should be sure to include 'CONFIRMED' state bugs in our weekly review, not just 'NEW' state bugs.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> @paelzer I updated the 'All open unclaimed MIR bugs' link on the MIR
> wiki page to show both NEW and CONFIRMED bugs, and I think we should be
> sure to include 'CONFIRMED' state bugs in our weekly review, not just
> 'NEW' state bugs.

Yeah I've got a notification about it.
I agree and thank you Dan!
I have shortened the link (same function, but omitting the defaults)
and updated also the link that we use in the weekly meeting (also on
that page, but in a different place).
Finally I have updated the state-machine info on the page to reflect
that we now also consider "Confirmed" tasks in our incoming queue.

We'll have to re-review all these cases that pop up by that, they are
all a bit fallen through the cracks by this.
Once discussed we will update each of one either assigning a
(re-)reviewer or whatever else seems to be the appropriate next step.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Per comment #1 + #24 libheif is ready - updating the state.
But the dependencies are not yet looked at in depth at all.

We will need to look for it, but first we need to make sure (likely but not state explicitly yet) that someone wants to look after it.
I assume that foundations will own them, but would like to ask for a MIR-request template for x265 and libde265 in the description please.
That will also inlcude the QA-checks that the requester is supposed to do as part of [1].
Setting them to incomplete to reflect that.
Please set them back to NEW once this is ready.

Then (probably next week meeting if it is ready by the time) we assigned them for review (and if generally ok they most likely will have to go to security review as well).

[1]: https://wiki.ubuntu.com/MainInclusionProcess?action=show&redirect=MIRTeam#Filing_a_MIR_bug

Changed in libheif (Ubuntu):
status: Confirmed → In Progress
Changed in x265 (Ubuntu):
status: Confirmed → Incomplete
Changed in libde265 (Ubuntu):
status: Confirmed → Incomplete
Revision history for this message
Steve Langasek (vorlon) wrote :

imagemagick is no longer in main, so this is wontfix.

Changed in libde265 (Ubuntu):
status: Incomplete → Won't Fix
Changed in libheif (Ubuntu):
status: In Progress → Won't Fix
Changed in x265 (Ubuntu):
status: Incomplete → Won't Fix
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.