342017 - expat buffer overflow from invalid utf8

Status: RESOLVED WORKSFORME

(Core :: XML, defect)

Description

[Daniel Veditz [:dveditz]]

from jackerror via email:
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

http://lxr.mozilla.org/mozilla/source/parser/expat/lib/xmltok.c#334

334 static void PTRCALL
335 utf8_toUtf16(const ENCODING *enc,
336              const char **fromP, const char *fromLim,
337              unsigned short **toP, const unsigned short *toLim)
338 {
339   unsigned short *to = *toP;
340   const char *from = *fromP;
341   while (from != fromLim && to != toLim) {
342     switch (((struct normal_encoding *)enc)->type[(unsigned char)*from]) {
343     case BT_LEAD2:
344       *to++ = (unsigned short)(((from[0] & 0x1f) << 6) | (from[1] & 0x3f));
345       from += 2;
346       break;
347     case BT_LEAD3:
348       *to++ = (unsigned short)(((from[0] & 0xf) << 12)
349                                | ((from[1] & 0x3f) << 6) | (from[2] & 0x3f));
350       from += 3;
351       break;
352     case BT_LEAD4:
353       {
354         unsigned long n;
355         if (to + 1 == toLim)
356           goto after;
357         n = ((from[0] & 0x7) << 18) | ((from[1] & 0x3f) << 12)
358             | ((from[2] & 0x3f) << 6) | (from[3] & 0x3f);
359         n -= 0x10000;
360         to[0] = (unsigned short)((n >> 10) | 0xD800);
361         to[1] = (unsigned short)((n & 0x3FF) | 0xDC00);
362         to += 2;
363         from += 4;
364       }
365       break;
366     default:
367       *to++ = *from++;
368       break;
369     }
370   }
371 after:
372   *fromP = from;
373   *toP = to;
374 }
375 

Line 345, 350 and 363 the content of the utf8 string is trusted to be
valid utf8, which is off course an error. This lead to set from > fromLim
and the while at line 341 is cheated. However you can't directly overflow
the 'to' destination buffer, but you can set *fromP to an out of bound
ponter at line 372, thus offering potential exploitable issue depending
on the calling function.

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

http://lxr.mozilla.org/mozilla/source/parser/expat/lib/xmltok.c#532

532 #define DEFINE_UTF16_TO_UTF8(E) \
533 static void  PTRCALL \
534 E ## toUtf8(const ENCODING *enc, \
535             const char **fromP, const char *fromLim, \
536             char **toP, const char *toLim) \
537 { \
538   const char *from; \
539   for (from = *fromP; from != fromLim; from += 2) { \

The same bug exist in the DEFINE_UTF16_TO_UTF8 macro, defined in :
But this macro suffers from another bug : due to the for boucle, if
(strlen (from) % 2) == 1, from will be set out of bound because of the
from += 2.
Thus a string like '\128\0' could trigger an overflow.

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Comment 1

[Daniel Veditz [:dveditz]]

Peter: can you look at this? jackerror's right that this code can't deal with imperfect input, is there anything upstream that sanitizes this? Instead of yet another utf8 implementation can we reuse one of the non-broken ones here?

Comment 2

[Peter Van der Beken [:peterv]]

I thought we'd already gone over this somewhere (either in mail or in a bug): we only use Expat in UTF-16 mode. See http://lxr.mozilla.org/seamonkey/source/parser/htmlparser/src/nsExpatDriver.cpp#713 and http://lxr.mozilla.org/seamonkey/source/parser/htmlparser/src/nsExpatDriver.cpp#1177
I think we should just mark this invalid and I'll make sure the Expat team knows about it.

Comment 3

[Daniel Veditz [:dveditz]]

> we only use Expat in UTF-16 mode.

That's good to know, thanks! Can we "#if 0" the broken conversion code to make this clear and possibly save code space?

Comment 4

[Daniel Veditz [:dveditz]]

mkaply: CCK seems to have its own copy of expat, and it *does* use encodings (from where?) when it instantiates the parser. I assume these are configuration/build files and aren't shared around?

Comment 5

[Mike Kaply [:mkaply] (PTO until Sept 14)]

(In reply to comment #4)
> mkaply: CCK seems to have its own copy of expat, and it *does* use encodings
> (from where?) when it instantiates the parser. I assume these are
> configuration/build files and aren't shared around?
> 

Not sure what you mean - I definitely don't do my own parsing of anything, and I don't include expat.

Every CCK file is in the tree...

Comment 6

[Daniel Veditz [:dveditz]]

> I don't include expat.

So what's http://lxr.mozilla.org/mozilla/source/cck/expat/ ? That's where I'm finding it. If you're not using it we should "cvs rm" it. If you *are* using it maybe we should see if there's a way to use the officially maintained mozilla/parser/expat version.

Comment 7

[Mike Kaply [:mkaply] (PTO until Sept 14)]

(In reply to comment #6)
> > I don't include expat.
> 
> So what's http://lxr.mozilla.org/mozilla/source/cck/expat/ ? That's where I'm
> finding it. If you're not using it we should "cvs rm" it. If you *are* using it
> maybe we should see if there's a way to use the officially maintained
> mozilla/parser/expat version.
> 

Ah. That's the old CCK that got originally checked in by Netscape. All my code is in extensions/cck/browser

Comment 8

[timeless]

dveditz: what's the plan for declassifying this bug? the status whiteboard says investigate, but this bug's resolved.

Comment 9

[Daniel Veditz [:dveditz]]

This one's tricky as it's a valid security issue in the expat code that just happens not to affect us, but presumably does hit other consumers of the expat library. I think we should leave it closed until the upstream fixes the conversion routines.

peterv: did you tell the expat team about it (comment 2)? Do they have a bug-link or something else we could reference so we can know when it's safe to make this public?