Win32: map.c integer overflow crashes PIL in tobytes()

[raygard]

What did you do?

Running some tests using PIL test images, my program quit silently. Narrowed down:

import traceback
from PIL import Image

print(Image.__version__)
try:
    with Image.open(r'Pillow-master/Tests/images/l2rgb_read.bmp') as im:
        print(f'mode: {im.mode} size: {im.size}')
        im.tobytes()
except Exception as e:
    print(f'Exception: {e.args} {e}')
    traceback.print_exc()
print('ok')

What did you expect to happen?

I expected an Exception and traceback, because this is a test image of a truncated BMP that is supposed to be a huge image. It should then print 'ok'. For example, in WSL2, (Python 3.8.5, PIL 7.0.0) I do get "ValueError: buffer is not large enough" and the 'ok' prints.

What actually happened?

15:14:17.99 e:\rdg\pil>testit
8.1.0
E:\rdg\pil\PIL\Image.py:2847: DecompressionBombWarning: Image size (151587072 pixels) exceeds limit of 89478485 pixels,
could be decompression bomb DOS attack.
  warnings.warn(
mode: L size: (3158064, 48)

15:14:52.54 e:\rdg\pil>

No exception, no 'ok' printed.

What are your OS, Python and Pillow versions?

• OS: Windows 10 Pro 1909
• Python: Python 3.9
• Pillow: PIL 8.1.0, or a build from the latest source.

Discussion

Program dies in tobytes(). After considerable effort, I found that the test in map.c, mapping_readimage() line 214:

   if (mapper->offset + size > mapper->size) {
        PyErr_SetString(PyExc_OSError, "image file truncated");
        return NULL;
    }

fails (should test true and raise the OSError) due to integer overflow: mapper->offset is 2147483440, size is 75793536; the 32-bit signed sum is -2071690320.

The file is mapped, and eventually tobytes() goes to ImagingRawEncode() in RawEncode.c, which calls state->shuffle() to move the bytes using a "packer" that is simply memcpy(). The first attempt to fetch from the (non-existent) source causes the program to fail, returning to the command prompt silently, no fault or other message. Nasty.

I poke around and find that the image apparently exists for a test test_overflow() in test_map.py. This test is skipped for Win32 because "Win32 does not call map_buffer". But Win32 does call mapping_readimage() and the same file causes this crash!

Resolution:

The "image file truncated" test in mapping_readimage() needs to be guarded against integer overflow. As Victor Stinner writes, "Check if an operation will overflow is hard, especially for signed numbers." I've written a bit of C, but I'll leave this one to you folks for now. My quick and dirty fix is to use:

   if (mapper->offset + size > mapper->size || mapper->offset + size < 0)

but I suspect this is not a good fix.

[radarhere]

It seems likely that #4310 is a solution to this.

[raygard]

Thank you, you are correct. I applied cgohlke's patch and it does seem to work OK in Windows. I am new to this process. Why has #4310 not been accepted/merged into the main branch? What is needed to make that happen?

[radarhere]

One problem was that it needed to be rebased, which I've now done in #5224

The other problem is that it needs a test. Typically with PRs in this repository, a test is added at the same time - code that fails without the fix but passes with it. This allows CI testing to stop us from breaking it again in the future.

[radarhere]

Question for you - is your machine 32-bit or 64-bit?

[raygard]

It's a 64-bit processor, 64-bit Windows 10, 16GB RAM. I also have an old slow laptop with 64-bit processor and 32-bit Windows 10, but only 4GB RAM so I don't know if I can build PIL or run memory-intensive tests on it. I can try. I assume you saw my comment on #5224 about my gist https://gist.github.com/raygard/3615f737e448ba1bf7a45874772db1c7 . Let me know if there's more I can do to help.

[raygard]

Also, I am running 64-bit Python 3.9, but was using 32-bit Python 3.8.6 until about 10 days ago. I may still be able to use the old Python and PIL build setup, not sure.

[radarhere]

Ok, I've added a test using your initial code - I find your gist a bit complex in comparison.
Thanks for your effort in moving this forward.

[raygard]

Excellent, that it works for you. I didn’t suggest a test based on my original issue because I expected the “before” case to just crash back to a command prompt, as it did in my case. I wish Windows had given me a fatal exception instead of a silent failure. I wonder what’s different in your testing environment. Maybe I should try to run your test suite.