Re: [xml] Redhat security update for libxml2

[Mike Hommey <mh glandium org>]

On Wed, Nov 19, 2008 at 10:31:05AM +0100, Mike Hommey wrote:
> On Wed, Nov 19, 2008 at 09:28:30AM +0100, Daniel Veillard <veillard redhat com> wrote:
> > On Tue, Nov 18, 2008 at 08:28:49PM +0100, Mike Hommey wrote:
> > > On Tue, Nov 18, 2008 at 07:16:50PM +0000, Graham Bennett wrote:
> > > > Hi all,
> > > >
> > > > I've been notified of a Redhat security update for libxml2:
> > > > https://rhn.redhat.com/errata/RHSA-2008-0988.html, and was hoping to
> > > > update my own builds with a version that doesn't suffer from these
> > > > vulnerabilities (I build from the standard source distribution, not the
> > > > Redhat source).  
> > > >
> > > > It wasn't immediately obvious from the release notes and recent mailing
> > > > list traffic if these have been fixed in a released version of the
> > > > libxml distribution yet.  If they haven't, is a new released planned to
> > > > address them?
> >
> >   Yeah sorry about that. Basically it was embargoed until monday, it's
> > not that easy to trigger the bugs, I didn't generate a new release for
> > this I will probably do one within a week or so including those and I
> > hope a solution for the PHP SAX problem.
> >
> > > Speaking of which, the patch for the SAX2Characters issue seems strange
> > > to me. While it is okay on 32-bits architectures, it doesn't make much
> > > sense on 64-bits architectures, where the addition of 2 ints can hardly
> > > be greater than SIZE_T_MAX.
> > > FWIW, as SIZE_T_MAX was not defined on glibc, the patch I applied on
> > > debian replaces SIZE_T_MAX with UINT_MAX.
> >
> >   Actually in SVN there is a define of SIZE_T_MAX as (size_t) -1 which
> > solves the pxprotability problem.
>
> ctxt->nodelen, ctxt->nodemem, and len are all ints.
> On 64 bits arches, ctxt->nodelen won't ever be greater than
> SIZE_T_MAX - len, because 2^31 won't ever be greater than 2^64 - 2^31.
> Likewise, ctxt->nodemem + len won't ever be greater than SIZE_T_MAX / 2
> because 2^32 won't ever be greater than 2^63.
>
> gcc might even remove the if clause, actually...
>
> So, without this overflow prevention code doing something, we're left
> with a ctxt->nodemem + len that still can overflow in the following if
> clause.
>
> Now, writing that, I realize my own code, with UINT_MAX, might not prevent
> much either, because I don't cast to unsigned int...

And actually, it's even worse than that, because the following size *= 2
overflows before ctxt->nodemem + len does. And changing size type to
size_t only defers the overflow to later, when it is assigned to
ctxt->nodemem a few lines after.

The testcase actually fails in xmlRealloc right after the size *= 2
overflow before hitting the original overflow prevention code. It
obviously also does so without the patch.

I wasn't able to trigger the overflow prevention code on x86 nor x86-64
with the patches from svn nor with some more changes as suggested
earlier. My guess is this will only trigger for some particular values
of MINLEN in xmlIO.c, or with custom IO functions.

Mike