1024803 - Possible buffer overrun in nsBMPDecoder::WriteInternal

Status: RESOLVED FIXED

Description

[Eric Rahm [:erahm]]

Coverity believes there's potential for a buffer overrun in nsBMPDecoder::WriteInternal[1]. It's somewhat convoluted but if:
  #1 - mBIH.compression == BI_BITFIELDS
  #2 - mPos < WIN_V3_HEADER_LENGTH

Then we'll write to a rather large address in this memcpy:
 memcpy(mRawBuf + (mPos - WIN_V3_HEADER_LENGTH), aBuffer, toCopy);
As mPos is unsigned.

[1] http://dxr.mozilla.org/mozilla-central/source/image/decoders/nsBMPDecoder.cpp?from=nsBMPDecoder.cpp&case=true#384

Comment 1

[Andrew McCreight [:mccr8]]

Milan, is there somebody who can look at this?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Just to be clear, this is code inspection bug, we don't have a test that triggers it?

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Just to be clear, this is code inspection bug, we don't have a test that
> triggers it?

Yes, this is based on a static analysis report by Coverity.  So it may not be a real issue in practice, depending on how this method is called, etc.

Comment 4

[Walter Litwinczyk [:walter]]

On my first pass I think it might be possible to craft a bitmap to reach the case and have a buffer overrun.

mLOH is only ever set to either OS2_HEADER_LENGTH == 26 or WIN_V3_HEADER_LENGTH == 54

By line 227 mPos should be >= mLOH

If mLOH == WIN_V3_HEADER_LENGTH then everything is fine. But there is a code path that does set mLOH = OS_HEADER_LENGTH (216).

So there is the possibility of having "memcpy(mRawBuf + (26 - 54), ...);". I have to double check that it's possible to construct a bitmap that will follow that code sequence though.

Additionally, there is another possible overrun condition. If "mPos - WIN_V3_HEADER_LENGTH" >= 36 then it will be off the end of the mRawBuf array, but I don't think that can happen in the code, but will have to double-check.

Comment 5

[Walter Litwinczyk [:walter]]

Okay, after further investigation I don't think it's possible. So in my above comment, case 1 cannot happen because mBIH.compression is always == 0 when mLOH = OS2_HEADER_LENGTH. 

http://dxr.mozilla.org/mozilla-central/source/image/decoders/nsBMPDecoder.cpp#215
http://dxr.mozilla.org/mozilla-central/source/image/decoders/nsBMPDecoder.cpp#720

That means when the if statement at:
http://dxr.mozilla.org/mozilla-central/source/image/decoders/nsBMPDecoder.cpp#378

is reached mBIH.compression != BI_BITFIELDS so the block is not executed. And when the block is executed as per my above comment mPos >= mLOH && mLOH == WIN_V3_HEADER_LENGTH.

As per the second case in my above comment, the if statement contains:
    mPos < (WIN_V3_HEADER_LENGTH + BITFIELD_LENGTH)
which makes the overflow impossible as the maximal value that (mPos - WIN_V3_HEADER_LENGTH) can have is 11 which is < 36.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks, great investigation. If we decide that no code changes are necessary, see if there are comments and asserts we can put in the code to capture your investigation and make it easier for other to see what's going on, as well as save us if the code changes in the future.

Comment 7

[Walter Litwinczyk [:walter]]

(In reply to Walter Litwinczyk [:walter] from comment #4)
> On my first pass I think it might be possible to craft a bitmap to reach the
> case and have a buffer overrun.
> 
> mLOH is only ever set to either OS2_HEADER_LENGTH == 26 or
> WIN_V3_HEADER_LENGTH == 54
> 
> By line 227 mPos should be >= mLOH
> 
> If mLOH == WIN_V3_HEADER_LENGTH then everything is fine. But there is a code
> path that does set mLOH = OS_HEADER_LENGTH (216).
> 
> So there is the possibility of having "memcpy(mRawBuf + (26 - 54), ...);". I
> have to double check that it's possible to construct a bitmap that will
> follow that code sequence though.
> 
> Additionally, there is another possible overrun condition. If "mPos -
> WIN_V3_HEADER_LENGTH" >= 36 then it will be off the end of the mRawBuf
> array, but I don't think that can happen in the code, but will have to
> double-check.

I would like to make a correction. mPos does not have to be >= mLOH at line 227. It's possible that the file being read may be smaller than the header size. In this case mPos < mLOH, but it's still okay because aCount == 0 in that case and the block still won't get executed.

Comment 8

[Walter Litwinczyk [:walter]]

Attachment: Added some comments and assert statements

Wasn't sure who to add as reviewer. Added a few asserts and comments around the memcpy's to make it more clear that a buffer overflow should not happen.

Try: https://tbpl.mozilla.org/?tree=Try&rev=1a1f1c66ec66

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8443788 [details] [diff] [review]
Added some comments and assert statements

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

This is good, and most of the modifications I'd like to see are about comments - either the wording of the existing or some more.  For example, we don't really check with asserts, so I'd rather see something like "we know this is the case, because..."

::: image/decoders/nsBMPDecoder.cpp
@@ +203,5 @@
>              toCopy = aCount;
> +
> +        // mRawbuf has a fixed length. Verify that the copy
> +        // stays within that fixed length.
> +        MOZ_ASSERT(mPos < sizeof(mRawBuf));

I'm OK with this assert as long as we also assert sizeof(mRawBuf[0]) == 1.  Otherwise, you may want something that holds true even if the raw buffer stops being byte sized:
MOZ_ASSERT(sizeof(mRawBuf[0])*mPos < sizeof(mRawBuf);


I'd also like to see the comment look more like this:
// mRawBuf has WIN_V3_INTERNAL_BIH_LENGTH
// mPos is less than BFH_INTERNAL_LENGTH
// BFH_INTERNAL_LENGTH is less than WIN_V3_INTERNAL_BIH_LENGTH
// so this assert should hold.

Although, I'm not clear on why that third one is a universal truth, but it is now, so it is probably enough.

Also, it may not be a bad idea to assert sizeof(mRawBuf) == WIN_V3_INTERNAL_BIH_LENGTH, just in case somebody decides to change the size of mRawBuf in the future?

@@ +227,5 @@
>              toCopy = aCount;
> +
> +        const uint32_t offset = mPos - BFH_INTERNAL_LENGTH;
> +        MOZ_ASSERT(offset < sizeof(mRawBuf));
> +        MOZ_ASSERT(sizeof(mRawBuf) - offset >= toCopy);

Similar to the comments above; when you're asserting, since it isn't quite obvious why that happens to be the universal truth, I'd rather see us write down the logic in the comments, rather than have everybody question the code as they see it.  Also, if the asserts ever trigger, it'd be easier to find out which assumptions were wrong.

Comment 10

[Walter Litwinczyk [:walter]]

Attachment: nsbmp_overflow

Added some more comments. Will definitely need a review of the logic to make sure I didn't make any silly mistakes.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8446039 [details] [diff] [review]
nsbmp_overflow

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

I left a couple of comments, you can take care of those, but it looks good.
Asserts and comments, probably OK without a peer r+.

::: image/decoders/nsBMPDecoder.cpp
@@ +207,2 @@
>      if (mPos < BFH_INTERNAL_LENGTH) { /* In BITMAPFILEHEADER */
> +        // BFH_INTERNAL_LENGTH < sizeof(mRawBuf)

The comment above says BFH_INTERNAL_LENGTH <= sizeof(mRawBuf).  Which one is it?

@@ +209,5 @@
> +        // mPos < BFH_INTERNAL_LENGTH
> +        // BFH_INTERNAL_LENGTH - mPos < sizeof(mRawBuf)
> +        // so toCopy <= BFH_INTERNAL_LENGTH
> +        // so toCopy < sizeof(mRawBuf)
> +        // so toCopy >= 0 && toCopy <= BFH_INTERNAL_LENGTH

toCopy > 0 instead?  To be 0, would we not need mPos == BFH_INTERNAL_LENGTH, and we are in the mPos < BFH_INTERNAL_LENGTH branch?
Probably enough to just assert toCopy > 0 && toCopy <= BFH_INTERNAL_LENGTH

@@ +227,5 @@
> +        // mPos >= 0 && mPos < BFH_INTERNAL_LENGTH
> +        // sizeof(mRawBuf) >= BFH_INTERNAL_LENGTH (verified above)
> +        //
> +        // Therefore this assert should hold
> +        MOZ_ASSERT(sizeof(mRawBuf) - mPos >= toCopy);

So, what we really care about is that mRawBuf has enough space for the toCopy amount of data to come in from the buffer.  In other words, that mPos+toCopy <= sizeof(mRawBuf).  Your assert is correct, but I think it's clearer if you have mPos+toCopy rather than -mPos.

@@ +261,5 @@
> +        //
> +        //      1 to WIN_V3_INTERNAL_BIH_LENGTH
> +        // or   1 to OS2_INTERNAL_BIH_LENGTH
> +        // and  OS2_INTERNAL_BIH_LENGTH < WIN_V3_INTERNAL_BIH_LENGTH
> +        //

May be worth tossing in this as a separate assert?

@@ +300,5 @@
> +        //
> +        // As sizeof(mRawBuf) == WIN_V3_INTERNAL_BIH_LENGTH (verified above)
> +        // and WIN_V3_HEADER_LENGTH is the largest range of values. If mLOH
> +        // was equal to OS2_HEADER_LENGTH then the ranges are smaller.
> +        MOZ_ASSERT(sizeof(mRawBuf) - offset >= toCopy);

Same comment as above, it may be clearer to go offset+toCopy route.

@@ +313,5 @@
> +    // data. In the latter case aCount should be 0.
> +    if (mPos < mLOH) {
> +        MOZ_ASSERT(aCount == 0);
> +    }
> +

Probably worth going without the if just for the assert:
MOZ_ASSERT(mPos >= mLoH || aCount == 0);

Comment 12

[Walter Litwinczyk [:walter]]

Attachment: nsBMPDecoder_comments

r=milan carried forward

Comment 13

[Walter Litwinczyk [:walter]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=f7b4c1a63a46

Comment 14

[Carsten Book [:Tomcat]]

Hi Walter, could you rebase this patch, since it caused problems during commit and didn't applied cleanly

applying nsBMPDecoder_comments
patching file image/decoders/nsBMPDecoder.cpp
Hunk #2 FAILED at 459
1 out of 2 hunks FAILED -- saving rejects to file image/decoders/nsBMPDecoder.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh nsBMPDecoder_comments

Comment 15

[Walter Litwinczyk [:walter]]

Attachment: nsBMPDecoder_comments

r=milan carried forward.

The apply issue appears to have been a whitespace problem. I had a whitespace removal patch applied that I had forgotten about. I apologize, this patch should apply cleanly.

Comment 16

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c630687dad45

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c630687dad45