1902439 - Missing status checks in WriteBitmap()

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect, P3)

Description

[mozillabugs]

WriteBitmap() (browser/components/shell/nsWindowsShellService.cpp) fails to check the status of nsIOutputStream::Write(). This means that if Write() fails, WriteBitmap() writes a malformed bitmap file to disk without detecting that fact. These files are used by nsWindowsShellService::SetDesktopBackground() to implement the "Set image as desktop background" context menu item. Thus, any exploit would have to involve some defect in Windows.

The bug that really matters is on line 473: if Write() writes the first row successfully (thus setting written to bytesPerRow), any combination of the following rows can fail without changing the value of written, thus spuriously causing the function to conclude that it successfully wrote the file.

You can verify that this is so by using a debugger to trace a Write() call down to where _PR_MD_WRITE() calls the Windows API WriteFile(), then manually skipping that call and setting rv == 0 (as WriteFile() does on failure), then tracing back out to WriteBitmap().

Code from FIREFOX_126_0_RELEASE:

    406: static nsresult WriteBitmap(nsIFile* aFile, imgIContainer* aImage) {
    407:   nsresult rv;
    460:   // write the bitmap headers and rgb pixel data to the file
    461:   rv = NS_ERROR_FAILURE;
    462:   if (stream) {
    463:     uint32_t written;
    464:     stream->Write((const char*)&bf, sizeof(BITMAPFILEHEADER), &written);
    465:     if (written == sizeof(BITMAPFILEHEADER)) {
    466:       stream->Write((const char*)&bmi, sizeof(BITMAPINFOHEADER), &written);
    467:       if (written == sizeof(BITMAPINFOHEADER)) {
    468:         // write out the image data backwards because the desktop won't
    469:         // show bitmaps with negative heights for top-to-bottom
    470:         uint32_t i = map.mStride * height;
    471:         do {
    472:           i -= map.mStride;
    473:           stream->Write(((const char*)map.mData) + i, bytesPerRow, &written);
    474:           if (written == bytesPerRow) {
    475:             rv = NS_OK;
    476:           } else {
    477:             rv = NS_ERROR_FAILURE;
    478:             break;
    479:           }
    480:         } while (i != 0);
    481:       }
    482:     }
    483: 
    484:     stream->Close();
    485:   }
    ...
    489:   return rv;
    490: }

There is also a similar bug in WinUtils::WriteBitmap(nsIFile* , SourceSurface* ) (widget/windows/WinUtils.cpp) but searchfox says that only WinUtils::WriteBitmap(nsIFile*, imgIContainer*) calls that, and that nothing calls WinUtils::WriteBitmap(nsIFile*, imgIContainer*).

Comment 1

[Nick Alexander :nalexander [he/him]]

Thanks for the report. This is obviously a bug, but it doesn't strike me as having security implications. Our sec review team will evaluate.

Comment 2

[Daniel Veditz [:dveditz]]

Agreed, functional bug but not an exploit.

Comment 3

[Max Christian Pohle [:/dev/max]]

Hi! It looks to me as if the easiest fix would be to move the FileDescriptorOutputStream.cpp#50 one line up and use the resulting negative epsilon as not so magic number. Rewriting to handle the return type NS_ERROR_FAILURE would be nicer though.

Comment 4

[Jilvin Jacob [:jilvin]]

Attachment: Bug 1902439 - Checks return of nsIOutputStream::Write() to NS_OK in WriteBitmap() to handle failures better. r=mpohle,nrishel

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 6

[Nick Rishel [:nrishel]]

Bad bot.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:jilvin, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd70967f242a
Checks return of nsIOutputStream::Write() to NS_OK in WriteBitmap() to handle failures better. r=mpohle,nrishel

Comment 9

[Serban Stanca [:SerbanS]]

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