[Bug 48927] New: Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

Bug ID: 48927
           Summary: Heap buffer underflow in TiffFrameDecode_ReadTile when
                    decoding 1x1 4bpp RGBA image
           Product: Wine-staging
           Version: unspecified
          Hardware: x86
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: -unknown
          Assignee: [e-mail]
          Reporter: [e-mail]
                CC: [e-mail], [e-mail]
      Distribution: ---

Created attachment 66887
  --> https://bugs.winehq.org/attachment.cgi?id=66887
Debugger info from ReactOS

ReactOS bug for reference: https://jira.reactos.org/browse/CORE-16796

Apologies for not reproducing this on Wine; the bug & fix are pretty simple
though.

The gdiplus:image test tries to decode a 1x1 TIFF image, and
TiffFrameDecode_ReadTile assumes that the cached_tile is large enough for an
even number of output pixels (i.e. a full number of input bytes).

The issue appears to be with this Staging patch:
https://github.com/wine-staging/wine-staging/blob/master/patches/windowscode...

The attachment has a backtrace and relevant variables. The line numbers may not
match but the underflow got caught at:
            dst[0] = (b & 0x20) ? 0xff : 0; /* B */

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

--- Comment #1 from Thomas Faber  ---
Created attachment 66888
  --> https://bugs.winehq.org/attachment.cgi?id=66888
Possible fix

Attaching a possible fix (line numbers may not match but context should).

I'm not sure of the proper process for updating someone else's patch in
Staging. Happy to follow it if preferred, or someone can just fix it directly.

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #2 from Dmitry Timoshkov  ---
(In reply to Thomas Faber from comment #1)
> Created attachment 66888 [details]
> Possible fix
>
> Attaching a possible fix (line numbers may not match but context should).
>
> I'm not sure of the proper process for updating someone else's patch in
> Staging. Happy to follow it if preferred, or someone can just fix it
> directly.

While the compiler usually should be able to optimize this code it's always
preferable to use '& 1' instead of '%2'.

Does using simple
if (!(This->decode_info.tile_width & 1))
help?

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

--- Comment #3 from Thomas Faber  ---
In all but the last iteration, two bytes should be written (or we'd skip every
other pixel) -- so we'd still need the "count ||", right?

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

--- Comment #4 from Dmitry Timoshkov  ---
I don't have a TIFF image in an appropriate format to test at the moment,
however does the following implementation fix the heap corruption?

/* 1 source byte expands to 2 BGRA samples */
        count = This->decode_info.tile_width * This->decode_info.tile_height;

src = This->cached_tile + count / 2 - 1;
        dst = This->cached_tile + This->decode_info.tile_size;

while (count >= 2)
        {
            BYTE b = *src--;

dst -= 8;
            dst[2] = (b & 0x80) ? 0xff : 0; /* R */
            dst[1] = (b & 0x40) ? 0xff : 0; /* G */
            dst[0] = (b & 0x20) ? 0xff : 0; /* B */
            dst[3] = (b & 0x10) ? 0xff : 0; /* A */
            dst[6] = (b & 0x08) ? 0xff : 0; /* R */
            dst[5] = (b & 0x04) ? 0xff : 0; /* G */
            dst[4] = (b & 0x02) ? 0xff : 0; /* B */
            dst[7] = (b & 0x01) ? 0xff : 0; /* A */

count -= 2;
        }
        if (count)
        {
            BYTE b = *src--;

dst -= 4;
            dst[2] = (b & 0x80) ? 0xff : 0; /* R */
            dst[1] = (b & 0x40) ? 0xff : 0; /* G */
            dst[0] = (b & 0x20) ? 0xff : 0; /* B */
            dst[3] = (b & 0x10) ? 0xff : 0; /* A */
        }

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

--- Comment #5 from Thomas Faber  ---
Almost!

Because this no longer rounds up "count" before dividing by two, "src" ends up
one byte before "cached_tile" here:
src = This->cached_tile + count / 2 - 1;

I fixed this to be:
src = This->cached_tile + (count + 1) / 2 - 1;

With that modification, there's no more out of bounds access.

There may be other complications to consider at some point, such as whether
byte-alignment needs to happen at the end of each row (so the bottom half of
the last byte of each row would be skipped). But for the 1x1 case this seems to
work great now. I've tested this using the gdiplus:image test.

Thanks!

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

--- Comment #6 from Dmitry Timoshkov  ---
(In reply to Thomas Faber from comment #5)
> Almost!
>
> Because this no longer rounds up "count" before dividing by two, "src" ends
> up one byte before "cached_tile" here:
> src = This->cached_tile + count / 2 - 1;
>
> I fixed this to be:
> src = This->cached_tile + (count + 1) / 2 - 1;
>
> With that modification, there's no more out of bounds access.

Thanks for testing.
> There may be other complications to consider at some point, such as whether
> byte-alignment needs to happen at the end of each row (so the bottom half of
> the last byte of each row would be skipped). But for the 1x1 case this seems
> to work great now. I've tested this using the gdiplus:image test.

That would need a real test case.

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

--- Comment #7 from Dmitry Timoshkov  ---
Created attachment 66946
  --> https://bugs.winehq.org/attachment.cgi?id=66946
WIP patch

Just in case I'm attaching a patch against staged
0012-windowscodecs-Add-support-for-4bpp-RGBA-format-to-TI.patch

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

Alistair Leslie-Hughes  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66946|0                           |1
           is patch|                            |

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|unspecified                 |5.6

--- Comment #8 from Dmitry Timoshkov  ---
I've sent another version of the fix that doesn't avoid an intermediate
memory allocation:
https://source.winehq.org/patches/data/183871
along with a specific test for the 4bps BGRA TIFF:
https://source.winehq.org/patches/data/183867

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED
      Fixed by SHA1|                            |0cd8502b493e7a93df7aad08bc2
                   |                            |fdfe3fd73b5cc

--- Comment #9 from Dmitry Timoshkov  ---
(In reply to Dmitry Timoshkov from comment #8)
> I've sent another version of the fix that doesn't avoid an intermediate
> memory allocation:
> https://source.winehq.org/patches/data/183871
> along with a specific test for the 4bps BGRA TIFF:
> https://source.winehq.org/patches/data/183867

0cd8502b493e7a93df7aad08bc2fdfe3fd73b5cc contains new implementation.

Note: it's a winehq commit.

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

Zebediah Figura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|-unknown                    |windowscodecs
            Product|Wine-staging                |Wine

--- Comment #10 from Zebediah Figura  ---
(In reply to Dmitry Timoshkov from comment #9)
> (In reply to Dmitry Timoshkov from comment #8)
> > I've sent another version of the fix that doesn't avoid an intermediate
> > memory allocation:
> > https://source.winehq.org/patches/data/183871
> > along with a specific test for the 4bps BGRA TIFF:
> > https://source.winehq.org/patches/data/183867
>
> 0cd8502b493e7a93df7aad08bc2fdfe3fd73b5cc contains new implementation.
>
> Note: it's a winehq commit.

Never quite sure how to reasonably deal with these, but I'm going to move the
bug to the Wine product in that case.

[Bug 48927] Heap buffer underflow in TiffFrameDecode_ReadTile when decoding 1x1 4bpp RGBA image

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48927

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #11 from Alexandre Julliard  ---
Closing bugs fixed in 5.8.