Re: [Security] Qt QXmlSimpleReader

[Solar Designer <solar () openwall com>]

Hi Thiago,

Thank you for your helpful response.

On Mon, Jan 09, 2017 at 09:24:51AM -0800, Thiago Macieira wrote:
> On s?bado, 24 de dezembro de 2016 16:18:33 PST Solar Designer wrote:
> > To what extent has Qt's QXmlSimpleReader class been reviewed for
> > vulnerabilities?  I found only Florian Weimer's CVE-2013-4549
> > "XML entity expansion denial of service", which Red Hat somehow chose
> > not to fix (no intent to parse untrusted XML?) even though they got
> > upstream to fix it.
>
> It has not been at all reviewed. That class is deprecated and we have zero 
> resources paying attention to it.
[...]
> We don't have anyone who knows the source code anymore, so we simply 
> can't tell you how much it may or may not cache.
>
> The only recommended class for reading XML is QXmlStreamReader. Using any of 
> the classes from the QtXml library should only happen with trusted sources.

Oh.  Is this stated somewhere in the documentation for QXmlSimpleReader,
along with the suggestion to use QXmlStreamReader instead?  Perhaps it
should be.

Right now, I see this:

http://doc.qt.io/qt-5/qxmlsimplereader.html#details

"The QXmlSimpleReader class provides an implementation of a simple XML
parser.

This XML reader is suitable for a wide range of applications.  It is
able to parse well-formed XML and can report the namespaces of elements
to a content handler; however, it does not parse any external entities."

http://doc.qt.io/qt-5/qxmlstreamreader.html#details

"The QXmlStreamReader class provides a fast parser for reading
well-formed XML via a simple streaming API.

QXmlStreamReader is a faster and more convenient replacement for Qt's
own SAX parser (see QXmlSimpleReader).  In some cases it might also be a
faster and more convenient alternative for use in applications that
would otherwise use a DOM tree"

This doesn't give the impression that either one is deprecated.  Also,
both say they're only for "well-formed" XML, suggesting they might be
unsuitable for use on untrusted input, but not explicitly stating so.
In fact, QXmlStreamReader has this "well-formed" requirement in the
one-sentence summary seen at top of page, whereas for QXmlSimpleReader
this requirement is only included in the details (second paragraph).

FYI, the stack overflow from recursive calls between parseElement() and
parseContent() got assigned CVE-2016-10040 here:

http://www.openwall.com/lists/oss-security/2016/12/24/2

> > Then there's value.resize(), which also accepts a signed int (so the
> > above code's use of signed int may have been justified, after all):
> >
> > http://doc.qt.io/qt-4.8/qstring.html#resize
> >
> > "If size is greater than the current size, the string is extended to
> > make it size characters long with the extra characters added to the end.
> > The new characters are uninitialized.
> >
> > If size is less than the current size, characters are removed from the end."
> >
> > No clear explanation on what will happen on a negative size, and besides
> > it might also be possible to exceed 4 GB and get to positive values again.
>
> Negative sizes are the same as zero. You can't exceed 4 GB with a signed int 
> in QString.

I see this is now documented:

http://doc.qt.io/qt-4.8/qstring.html#resize

"If size is negative, it is equivalent to passing zero."

Either I overlooked this detail before, or you improved the
documentation since.

If I understand correctly, this means that in this piece I quoted before:

 8187 inline static void updateValue(QString &value, const QChar *array, int &arrayPos, int &valueLen)
 8188 {
 8189     value.resize(valueLen + arrayPos);
 8190     memcpy(value.data() + valueLen, array, arrayPos * sizeof(QChar));
 8191     valueLen += arrayPos;
 8192     arrayPos = 0;
 8193 }

it will be an up to 256 bytes write with the memcpy() at about 2 GiB
beyond the allocation, which value.resize() would reduce to 0 bytes.
(And if this somehow succeeds, then the second time it may be a similar
write at about 2 GiB below that allocation.)

Is this right?  Is it really that bad?

> > Is there anything at higher layers, yet applicable to all published Qt's
> > APIs, consistenly limiting XML inputs to below 2 GB?  If so, this may be
> > OK (but a comment would be nice).  If not, we have a problem.
>
> No, there's no such limitation, but many classes will impose 2 GB limits due 
> to array sizes. The only problem is that getting close to that limit will 
> already run into code we don't usually test. There are also some problems with 
> UB on signed overflow on Qt 4.8 and in early Qt 5 versions (I think I fixed it 
> in 5.4 or 5.5). 

In general, are applications using Qt supposed to sanity-check the sizes
to be significantly below 2 GiB before passing such data on to Qt?

Thanks again,

Alexander