On Fri, Oct 28, 2016 at 09:28:37AM -0400, Antoine Beaupré wrote: > 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. > Thanks. > 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. > OK. I will definitely have a closer look at this. > 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. > Thanks very much for the feedback. I am always glad to get an additional review on my work. Regards, -Roberto -- Roberto C. Sánchez http://people.connexer.com/~roberto http://www.connexer.com
Attachment:
signature.asc
Description: Digital signature