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

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 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();

(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 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.

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

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

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.

Gustavo, Did this bug get a CVE elsewhere? Did you report this to Expat directly?

> 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 ..