1349390 - Integer overflow in dom/xslt/xslt/txNodeSorter.cpp, potentially leading to double-free or uninitialized memory

Status: RESOLVED INVALID

(Core :: XSLT, enhancement)

Description

[Christian Holler (:decoder)]

The following code performs unchecked arithmetic operations on integral data types with the result being used in functions related to memory allocation:

https://dxr.mozilla.org/mozilla-central/rev/7ebcd45634eef3711dccf68e4e1390134d48b63b/dom/xslt/xslt/txNodeSorter.cpp#162

Analysis:

On 64 bit, it seems that overflow can't happen in the call to memset due to the constraints above:

>mNKeys > (UINT32_MAX - sizeof(uint32_t)) / sizeof(txObject*)
>len >= UINT32_MAX / itemSize

So 

> (UINT32_MAX - sizeof(uint32_t)) * ((UINT32_MAX / itemSize) - 1)

isn't large enough to overflow size_t on 64 bit. However, I think on 32 bit it should be possible to overflow the call to memset without overflowing len * mNKeys. I think the result would be that memset doesn't reset all the memory, but we later on iterate over all of the memory and call delete on it. That would be potentially exploitable. If you overflow both values, it might be possible that some of the memory remains uninitialized. In any case, it doesn't look good to me and it is probably sane to use CheckedInt (found in mfbt) to perform overflow checking to fix this.

If I made a mistake in the analysis and this code is really safe, then we should add some annotation to make static analysis ignore this issue.

(This is part of a custom static analysis we have been working on in bug 1279569)

Comment 1

[Peter Van der Beken [:peterv]]

We make sure that |len < UINT32_MAX / itemSize|. So |len * itemSize < UINT32_MAX|. So |len * (sizeof(uint32_t) + mNKeys * sizeof(txObject*)) < UINT32_MAX|. |(len * sizeof(uint32_t)) + (len * mNKeys * sizeof(txObject*)) < UINT32_MAX|. So it seems to me that len * mNKeys * sizeof(txObject*) can not overflow.

An item contains one uint32_t and mNKeys txObject pointers.
|mNKeys > (UINT32_MAX - sizeof(uint32_t)) / sizeof(txObject*)| means that the number of keys doesn't cause one item to be bigger than UINT32_MAX bytes.
|len >= UINT32_MAX / itemSize| means that the number of items doesn't cause the array of items to be equal or bigger than UINT32_MAX bytes.
If those 2 hold then I don't see how the size of the total number of txObject pointers can be bigger than UINT32_MAX.

Comment 2

[Christian Holler (:decoder)]

I think what I missed is that the first constraint (len < UINT32_MAX / itemSize) is influenced by mNKeys because itemSize is derived from that. The constrains certainly look complicated enough to confuse people. Maybe this could be simplified with CheckedInt, but I don't know and would leave that up you entirely :)

Comment 3

[Eric Rahm [:erahm]]

(In reply to Christian Holler (:decoder) from comment #2)
> I think what I missed is that the first constraint (len < UINT32_MAX /
> itemSize) is influenced by mNKeys because itemSize is derived from that. The
> constrains certainly look complicated enough to confuse people. Maybe this
> could be simplified with CheckedInt, but I don't know and would leave that
> up you entirely :)

I agree it should be made obvious that this is okay. I'm going to close this sec bug and file a follow up to switch over to CheckedInt and possibly clean up the function a bit.

Comment 4

[Eric Rahm [:erahm]]

I filed bug 1349695 for the follow up.