Closed
Bug 1024803
Opened 7 years ago
Closed 7 years ago
Possible buffer overrun in nsBMPDecoder::WriteInternal
Categories
(Core :: ImageLib, defect)
Core
ImageLib
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: erahm, Assigned: wlitwinczyk)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: [CID 1157283])
Attachments
(1 file, 3 obsolete files)
9.76 KB,
patch
|
wlitwinczyk
:
review+
|
Details | Diff | Splinter Review |
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
Just to be clear, this is code inspection bug, we don't have a test that triggers it?
Assignee: nobody → wlitwinczyk
Flags: needinfo?(milan)
Comment 3•7 years ago
|
||
(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.
Assignee | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
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.
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.
Assignee | ||
Comment 7•7 years ago
|
||
(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.
Assignee | ||
Comment 8•7 years ago
|
||
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
Attachment #8443788 -
Flags: review?(milan)
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.
Updated•7 years ago
|
Group: core-security
Assignee | ||
Comment 10•7 years ago
|
||
Added some more comments. Will definitely need a review of the logic to make sure I didn't make any silly mistakes.
Attachment #8443788 -
Attachment is obsolete: true
Attachment #8443788 -
Flags: review?(milan)
Attachment #8446039 -
Flags: review?(milan)
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);
Attachment #8446039 -
Flags: review?(milan) → review+
Assignee | ||
Comment 12•7 years ago
|
||
r=milan carried forward
Attachment #8446039 -
Attachment is obsolete: true
Attachment #8449581 -
Flags: review+
Assignee | ||
Comment 13•7 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=f7b4c1a63a46
Keywords: checkin-needed
Comment 14•7 years ago
|
||
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
Keywords: checkin-needed
Assignee | ||
Comment 15•7 years ago
|
||
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.
Attachment #8449581 -
Attachment is obsolete: true
Attachment #8450322 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 17•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c630687dad45
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•3 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•