1274777 - (CVE-2016-9063) Possible integer overflow to fix inside XML_Parse in expat

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Gustavo Grieco]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160425115534

Steps to reproduce:

Possible integer overflow in XML_Parse:

https://github.com/mozilla/gecko-dev/blob/7b9ea8afc579378606ecf3593b04fc2aaba7daec/parser/expat/lib/xmlparse.c#L1568

Comment 1

[Gustavo Grieco]

Added erahm, since he told me to create a different report for this possible issue here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1236923#c41

Comment 2

[Eric Rahm [:erahm]]

Attachment: Check for oveflow

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8755538 [details] [diff] [review]
Check for oveflow

Review of attachment 8755538 [details] [diff] [review]:
-----------------------------------------------------------------

It seems that there are other checks to do in this file. I just gave a quick glance, but there are many sizes done like:
    int neededSize = len + (int)(bufferEnd - bufferPtr);

Maybe would be nice to fix/check/analyze them too in a follow up.

::: parser/expat/lib/xmlparse.c
@@ +1568,3 @@
>          char *temp;
> +/* BEGIN MOZILLA CHANGE (check for overflow) */
> +        int newLen = len * 2;

CheckedInt<int>

@@ +1586,5 @@
>            eventPtr = eventEndPtr = NULL;
>            processor = errorProcessor;
>            return XML_STATUS_ERROR;
>          }
> +        bufferLim = buffer + newLen;

Also this can overflow. We should use:

CheckedInt<int> buffer;
buffer += newLen;
if (!buffer.isValid()) {
  something;
}

bufferLim = buffer.value();

Comment 4

[Eric Rahm [:erahm]]

(In reply to Andrea Marchesini (:baku) from comment #3)
> Comment on attachment 8755538 [details] [diff] [review]
> Check for oveflow
> 
> Review of attachment 8755538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems that there are other checks to do in this file. I just gave a quick
> glance, but there are many sizes done like:
>     int neededSize = len + (int)(bufferEnd - bufferPtr);
> 
> Maybe would be nice to fix/check/analyze them too in a follow up.
> 
> ::: parser/expat/lib/xmlparse.c
> @@ +1568,3 @@
> >          char *temp;
> > +/* BEGIN MOZILLA CHANGE (check for overflow) */
> > +        int newLen = len * 2;
> 
> CheckedInt<int>

This is C.

> 
> @@ +1586,5 @@
> >            eventPtr = eventEndPtr = NULL;
> >            processor = errorProcessor;
> >            return XML_STATUS_ERROR;
> >          }
> > +        bufferLim = buffer + newLen;
> 
> Also this can overflow. We should use:
> 
> CheckedInt<int> buffer;
> buffer += newLen;
> if (!buffer.isValid()) {
>   something;
> }
> 
> bufferLim = buffer.value();

I thought about this, I can't imagine a situation where the allocator can allocate a block of memory that extends past the addressable heap.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 8755538 [details] [diff] [review]
Check for oveflow

Review of attachment 8755538 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/expat/lib/xmlparse.c
@@ +1576,3 @@
>          temp = (buffer == NULL
> +                ? (char *)MALLOC(newLen)
> +                : (char *)REALLOC(buffer, newLen));

Hmm, I think we normally put all changes inside the MOZILLA CHANGE comments and ifdef out the old code. So something like:

/* BEGIN MOZILLA CHANGE (check for overflow) */
#if 0
        temp = (buffer == NULL
                ? (char *)MALLOC(len * 2)
                : (char *)REALLOC(buffer, len * 2));
#else
        int newLen = len * 2;
        if (newLen < 0) {
          errorCode = XML_ERROR_NO_MEMORY;
          return XML_STATUS_ERROR;
        }
        temp = (buffer == NULL
                ? (char *)MALLOC(newLen)
                : (char *)REALLOC(buffer, newLen));
#endif
/* END MOZILLA CHANGE */

That way if the change does get merged back from upstream the diff shoul donly show removal.

@@ +1586,5 @@
>            eventPtr = eventEndPtr = NULL;
>            processor = errorProcessor;
>            return XML_STATUS_ERROR;
>          }
> +        bufferLim = buffer + newLen;

buffer and bufferLim are const char*, so I'm not sure what you mean.

Comment 6

[Eric Rahm [:erahm]]

Oh that pattern makes sense now, I'll update with the ifdef.

Comment 7

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7926661e4a89b25210539d3f4485e7fad75f9a
Bug 1274777 - Check for oveflow. r=peterv

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b7926661e4a

Comment 10

[Daniel Veditz [:dveditz]]

Minusing for a bounty without demonstration. If expat has had a FIXME for years and no one's exploited it or filed a bug it may not be reachable in practice.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Gustavo,
Did this bug get a CVE elsewhere?

Did you report this to Expat directly?

Comment 12

[Gustavo Grieco]

> Did this bug get a CVE elsewhere? 

Nop

> Did you report this to Expat directly?

Yes, but for some reason they haven't changed the code. I think they said it was unreachable ..