[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: ImageMagick - marking issue as not affecting wheezy?

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.

> 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/
> This is the resulting code after patching:
> https://anonscm.debian.org/cgit/collab-maint/imagemagick.git/tree/magick/resource.c?h=debian-patches/
> 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.



Roberto C. Sánchez

Attachment: signature.asc
Description: Digital signature

Reply to: