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
