[Bug 2004580] Re: Possible arbitrary file leak

[David Zuelke]

Hi Paulo Flabiano Smorigo (pfsmorigo),

alright so, I looked at it for jammy, focal and bionic.


First, jammy (I guess kinetic too?) has missing commits. The contents of CVE-44267.patch should be undone by CVE-44268.patch; this line remains in the source but has been removed upstream in the improved/refactored fix for the vulnerability:

+                    (LocaleCompare(key,"profile") == 0) ||

And at least one code formatting commit (e.g. the last one in the list
below) is also not there, might be useful for the future, as it would
allow patches for the same piece of code to apply more cleanly, should
there be the need for another backport.

If you take the jammy sources and apply all the existing patches except
the last two CVE-2022-*, you can then simply apply all the commits from
upstream in sequence:

(original fix)
https://github.com/ImageMagick/ImageMagick6/commit/be3b2a02cbb9c9affa7b0afa0665ed4b4bb0f47b
https://github.com/ImageMagick/ImageMagick6/commit/3c5188b41902a909e163492fb0c19e49efefcefe
(improved or refactored fix)
https://github.com/ImageMagick/ImageMagick6/commit/222845f6a0848c1e1c567bb1618617e786523bb2
https://github.com/ImageMagick/ImageMagick6/commit/87d719c194cc9356cdcf5df578bbea25582a290c
https://github.com/ImageMagick/ImageMagick6/commit/23bf43133d5fc525afafdc47398cd92b3b68797d
https://github.com/ImageMagick/ImageMagick6/commit/d77c01e560e973177feed4915ffd7dd1a45fd763
https://github.com/ImageMagick/ImageMagick6/commit/48b46bc91301b7206bfd4126a459984bd6abe3d4

They apply cleanly:

curl -s
https://github.com/ImageMagick/ImageMagick6/commit/be3b2a02cbb9c9affa7b0afa0665ed4b4bb0f47b.patch
https://github.com/ImageMagick/ImageMagick6/commit/3c5188b41902a909e163492fb0c19e49efefcefe.patch
https://github.com/ImageMagick/ImageMagick6/commit/222845f6a0848c1e1c567bb1618617e786523bb2.patch
https://github.com/ImageMagick/ImageMagick6/commit/87d719c194cc9356cdcf5df578bbea25582a290c.patch
https://github.com/ImageMagick/ImageMagick6/commit/23bf43133d5fc525afafdc47398cd92b3b68797d.patch
https://github.com/ImageMagick/ImageMagick6/commit/d77c01e560e973177feed4915ffd7dd1a45fd763.patch
https://github.com/ImageMagick/ImageMagick6/commit/48b46bc91301b7206bfd4126a459984bd6abe3d4.patch
| patch

I'd either put them into two patch files (first two commits in one, the
rest in the others), or all into one. Right now, CVE-44267.patch
contains just one of the two commits necessary for the original
mitigation, so applying only that results in half-fixed and possibly
broken code until CVE-44268.patch is also applied.


Second, focal, there is now an anomaly in the code that makes it behave
differently. The bionic and jammy versions, when fed an exploiting PNG,
will just ignore the exploit.

Focal however, while no longer vulnerable in that it exfiltrates the
file to the headers, now returns the "profile" field with the value that
the attacker fed in:

# convert exfiltrate-etc-hosts.png -resize 100x100 PNG:- | identify -verbose PNG:-
…
    png:text: 3 tEXt/zTXt/iTXt chunks were found
    png:tIME: 2023-02-28T22:00:26Z
    png:tRNS: chunk was found
    profile: /etc/hosts
    signature: e7e2dcff542de95352682dc186432e98f0188084896773f1973276b0577d5305
…

On the bionic and jammy versions, only 2 tEXt/zTXt/iTXt chunks are
reported as found, and the "profile" field is not there.

Now this is obviously not a vulnerability, as it simply "echos back" the
value the attacker fed in, but it points to an incorrect patch.

The reason is that the additions to wand/mogrify.c were taken over from
the jammy patches, and there are now extra lines in focal that add logic
that should not be there.

Compare focal's and jammy's CVE-2022-44268.patch files and look at the
removal in magic/property.c for both.

In this block:

if (profile != (StringInfo *) NULL)

the jammy version has one more line (and thus curly braces around the
block):

profile=DestroyStringInfo(profile);

But this extra line (and the curly braces) are added both in focal and
jammy.

I guess that is where this behavior of "forwarding" the "profile" value
into the output comes from.

I am guessing that focal patch to wand/mogrify.c should instead just
have this code for that if:

+                    if (profile != (StringInfo *) NULL)
+                      status=SetImageProfile(image,image_info->magick,profile);

But please test it ;) I can email you a PoC .png file for testing if you
need.


Third, bionic... the patch there is very different, as the codebase is
obviously older. Might be easier to just leave as is. The change in
there mitigates the vulnerability, so it's fine.

-- 
You received this bug notification because you are a member of Ubuntu
Foundations Bugs, which is subscribed to imagemagick in Ubuntu.
https://bugs.launchpad.net/bugs/2004580

Title:
  Possible arbitrary file leak

Status in imagemagick package in Ubuntu:
  Fix Released

Bug description:
  More details can be found here:

  https://www.metabaseq.com/imagemagick-zero-days/

  Affected versions:

      Injection via "-authenticate"
      - ImageMagick 6: 6.9.8-1 up to 6.9.11-40
      Explotation via MSL:
      -ImageMagick 6: 6.9.11-35 up to 6.9.11-40

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/imagemagick/+bug/2004580/+subscriptions