Re: ImageMagick - marking issue as not affecting wheezy?
On 2016-10-27 22:28:17, Roberto C. Sánchez wrote:
> [ Unknown signature status ]
> Hello,
>
> I decided (perhaps because I don't know any better) to take over
> ImageMagick after Ben released his lock on it.
For the record, I did the same yesterday, except I forgot to lock the
package... :/
I have work mainly on identifying exactly what CVEs Ben had fixed. Turns
out that he did a lot of them, but forgot to mark CVE-2014-9839 as done
in the changelog. I noted in dla-needed.txt which patches were done
already.
I started working on CVE-2014-9850 as well. I was puzzled by the fix
provided, and have similar questions than you. I was able to find that
patch in Debian's history of the sid package:
https://anonscm.debian.org/cgit/collab-maint/imagemagick.git/commit/?h=debian-patches/6.8.9.9-4-for-upstream&id=2257d1eadd02d89d225fce21013a1219d221dc7d
This is the resulting code after patching:
https://anonscm.debian.org/cgit/collab-maint/imagemagick.git/tree/magick/resource.c?h=debian-patches/6.8.9.9-4-for-upstream&id=2257d1eadd02d89d225fce21013a1219d221dc7d#n1187
First problem is that the ThrottleResource case isn't present in wheezy,
so I am tempted to just part that as not-affected.
However, looking more closely at the patch, you'll notice that the patch
checks resource_info.thread_limit in the ThrottleResource case. That
makes no sense at all: that variable is probably not initialized at all
at that point. It looks to me a crash was replaced by a crash.
Roberto, since you took on that package, I'll let you make the final
decision, but in any case I think the package maintainer (and secteam?)
should be brought in the loop if my analysis is correct: CVE-2014-9850
may not be fixed in jessie/sid after all.
Below, I have taken some time to review your own work as well. :)
[...]
> The feedback I am seeking here is:
>
> 1. Is my interperation of the applicability of the patch correct?
I have reviewed it and it seems like you are correct. In fact, I wonder
if the original patch makes any sense at all either. The only division
by zero I could see in there is:
if (rows_per_strip < 1)
rows_per_strip=1;
if ((image->rows/rows_per_strip) >= (1UL << 15))
Notice how the first check is not part of the patch.
I really wonder how the crash could have happened in the first place and
how that patch could have possibly fixed it.
Maybe I overlooked something?
> 2. Is what I am proposing the correct way to resolve the issue so that
> it no longer appears as vulnerable in the security tracker?
Yes.
--
If quantum mechanics hasn't profoundly shocked you, you haven't
understood it yet.
- Niels Bohr
Reply to: