687387 - txStylesheetCompiler::startElement could be vulnerable to integer overflow

Status: RESOLVED WORKSFORME

(Core :: XSLT, defect)

Description

[Josh Matthews [:jdm]]

Alex Miller points out that http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txStylesheetCompiler.cpp#159 indexes into an array based on a PRInt32 parameter without any kind of sanity checking. This is also presumably accessible to arbitrary content, since this is in the xslt code, so this might be particularly dangerous.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

So this would require an element to have more than 1 billion attributes? Could be a 
problem on 64 bit builds, unless expat handles that case already.

Comment 2

[Peter Van der Beken [:peterv]]

The Expat parser would hand us an array with a null at an index > PR_INT32_MAX? I'm not sure how that could happen, could you explain?

Comment 3

[Daniel Veditz [:dveditz]]

kicking this back into the triage pool by removing "[sg:?]"

Comment 4

[Daniel Veditz [:dveditz]]

Not a problem?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I haven't looked enough to know if this is exploitable or not, but sounds like it's worth fixing the code to stay on the safe side in case code changes in the future.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Here's a better link since the code has changed:

http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txStylesheetCompiler.cpp?rev=0abb7330c64c#159

Looking at this in more detail, I don't see how this could be a problem. Expat hands us an array that is twice the size of aAttrCount. So I don't see how we could end up indexing outside of that array here.

Even if expat fails to allocate the array due to OOM (or just a huge number of attributes) *and* fails to check that the allocation succeeded, this would just be a null-deref on the first iteration of the loop.

So closing this as WORKSFORME. Unless someone disagrees we should just open this bug.