[PATCH 0/1] MR8141: gdiplus: Add check of type parameter to be positive (ASan).
Function GdipGetImageAttributesAdjustedPalette gets [called with type = -1](https://gitlab.winehq.org/wine/wine/-/blob/22af42ac22279e6c0c671f033661f95c1...) and therefore accesses several arrays at index -1. This patch adds a helper which additionally checks for `type >= ColorAdjustTypeDefault`. <details> <summary>ASan output</summary> ``` ================================================================= ==gdiplus_test.exe==308==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f21342c57f8 at pc 0x6ffffadfd25b bp 0x7ffffe1ff750 sp 0x7ffffe1ff798 READ of size 4 at 0x7f21342c57f8 thread T0 #0 0x6ffffadfd25a in apply_image_attributes .../dlls/gdiplus/graphics.c:929:41 #1 0x6ffffae5c2f2 in GdipGetImageAttributesAdjustedPalette .../dlls/gdiplus/imageattributes.c:123:5 #2 0x000140084b59 in test_getadjustedpalette .../dlls/gdiplus/tests/image.c:5649:12 #3 0x000140062be6 in func_image .../dlls/gdiplus/tests/image.c:6719:5 #4 0x0001400c9132 in run_test .../include/wine/test.h:765:5 #5 0x0001400c8b50 in main .../include/wine/test.h:884:12 #6 0x0001400cad3a in mainCRTStartup .../dlls/msvcrt/crt_main.c:60:11 #7 0x6fffffc45aa0 in BaseThreadInitThunk .../dlls/kernel32/thread.c:61:24 #8 0x6fffffdcc776 in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x17004c776) 0x7f21342c57f8 is located 144 bytes after 1256-byte region [0x7f21342c5280,0x7f21342c5768) freed by thread T0 here: #0 0x6ffffe88aab1 in free C:/llvm-mingw/llvm-mingw/llvm-project/compiler-rt/lib/asan\asan_malloc_win.cpp:71:3 #1 0x6ffffae5c21d in GdipDisposeImageAttributes .../dlls/gdiplus/imageattributes.c:109:5 #2 0x000140083598 in test_colorkey .../dlls/gdiplus/tests/image.c:3709:5 #3 0x000140062bd7 in func_image .../dlls/gdiplus/tests/image.c:6716:5 #4 0x0001400c9132 in run_test .../include/wine/test.h:765:5 #5 0x0001400c8b50 in main .../include/wine/test.h:884:12 #6 0x0001400cad3a in mainCRTStartup .../dlls/msvcrt/crt_main.c:60:11 #7 0x6fffffc45aa0 in BaseThreadInitThunk .../dlls/kernel32/thread.c:61:24 #8 0x6fffffdcc776 in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x17004c776) previously allocated by thread T0 here: #0 0x6ffffe88ace6 in calloc C:/llvm-mingw/llvm-mingw/llvm-project/compiler-rt/lib/asan\asan_malloc_win.cpp:91:3 #1 0x6ffffae5c0fc in GdipCreateImageAttributes .../dlls/gdiplus/imageattributes.c:87:18 #2 0x000140082bf9 in test_colorkey .../dlls/gdiplus/tests/image.c:3629:12 #3 0x000140062bd7 in func_image .../dlls/gdiplus/tests/image.c:6716:5 #4 0x0001400c9132 in run_test .../include/wine/test.h:765:5 #5 0x0001400c8b50 in main .../include/wine/test.h:884:12 #6 0x0001400cad3a in mainCRTStartup .../dlls/msvcrt/crt_main.c:60:11 #7 0x6fffffc45aa0 in BaseThreadInitThunk .../dlls/kernel32/thread.c:61:24 #8 0x6fffffdcc776 in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x17004c776) SUMMARY: AddressSanitizer: heap-buffer-overflow .../dlls/gdiplus/graphics.c:929:41 in apply_image_attributes Shadow bytes around the buggy address: 0x7f21342c5500: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x7f21342c5580: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x7f21342c5600: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x7f21342c5680: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x7f21342c5700: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa =>0x7f21342c5780: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa] 0x7f21342c5800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7f21342c5880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f21342c5900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f21342c5980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f21342c5a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==gdiplus_test.exe==308==ABORTING ``` </details> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8141
From: Bernhard Übelacker <bernhardu(a)mailbox.org> --- dlls/gdiplus/gdiplus_private.h | 7 +++++++ dlls/gdiplus/graphics.c | 3 +++ dlls/gdiplus/imageattributes.c | 14 +++++++------- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h index 9d0a166fdeb..50a642a5364 100644 --- a/dlls/gdiplus/gdiplus_private.h +++ b/dlls/gdiplus/gdiplus_private.h @@ -710,4 +710,11 @@ static inline void set_rect(GpRectF *rect, REAL x, REAL y, REAL width, REAL heig rect->Height = height; } +static inline int is_valid_ColorAdjustType(ColorAdjustType type) +{ + if (type >= ColorAdjustTypeDefault && type < ColorAdjustTypeCount) + return TRUE; + return FALSE; +} + #endif diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index a2b5649222f..d2abfdac9ad 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -854,6 +854,9 @@ PixelFormat apply_image_attributes(const GpImageAttributes *attributes, LPBYTE d UINT x, y; INT i; + if (!is_valid_ColorAdjustType(type)) + return InvalidParameter; + if ((attributes->noop[type] == IMAGEATTR_NOOP_UNDEFINED && attributes->noop[ColorAdjustTypeDefault] == IMAGEATTR_NOOP_SET) || (attributes->noop[type] == IMAGEATTR_NOOP_SET)) diff --git a/dlls/gdiplus/imageattributes.c b/dlls/gdiplus/imageattributes.c index 349e52f5be9..c5328a37209 100644 --- a/dlls/gdiplus/imageattributes.c +++ b/dlls/gdiplus/imageattributes.c @@ -117,7 +117,7 @@ GpStatus WINGDIPAPI GdipGetImageAttributesAdjustedPalette(GpImageAttributes *ima TRACE("(%p,%p,%u)\n", imageattr, palette, type); if (!imageattr || !palette || !palette->Count || - type >= ColorAdjustTypeCount || type == ColorAdjustTypeDefault) + !is_valid_ColorAdjustType(type) || type == ColorAdjustTypeDefault) return InvalidParameter; apply_image_attributes(imageattr, (LPBYTE)palette->Entries, palette->Count, 1, 0, @@ -131,7 +131,7 @@ GpStatus WINGDIPAPI GdipSetImageAttributesColorKeys(GpImageAttributes *imageattr { TRACE("(%p,%u,%i,%08lx,%08lx)\n", imageattr, type, enableFlag, colorLow, colorHigh); - if(!imageattr || type >= ColorAdjustTypeCount) + if(!imageattr || !is_valid_ColorAdjustType(type)) return InvalidParameter; imageattr->colorkeys[type].enabled = enableFlag; @@ -148,7 +148,7 @@ GpStatus WINGDIPAPI GdipSetImageAttributesColorMatrix(GpImageAttributes *imageat TRACE("(%p,%u,%i,%p,%p,%u)\n", imageattr, type, enableFlag, colorMatrix, grayMatrix, flags); - if(!imageattr || type >= ColorAdjustTypeCount || flags > ColorMatrixFlagsAltGray) + if(!imageattr || !is_valid_ColorAdjustType(type) || flags > ColorMatrixFlagsAltGray) return InvalidParameter; if (enableFlag) @@ -206,7 +206,7 @@ GpStatus WINGDIPAPI GdipSetImageAttributesGamma(GpImageAttributes *imageAttr, { TRACE("(%p,%u,%i,%0.2f)\n", imageAttr, type, enableFlag, gamma); - if (!imageAttr || (enableFlag && gamma <= 0.0) || type >= ColorAdjustTypeCount) + if (!imageAttr || (enableFlag && gamma <= 0.0) || !is_valid_ColorAdjustType(type)) return InvalidParameter; imageAttr->gamma_enabled[type] = enableFlag; @@ -220,7 +220,7 @@ GpStatus WINGDIPAPI GdipSetImageAttributesNoOp(GpImageAttributes *imageAttr, { TRACE("(%p,%u,%i)\n", imageAttr, type, enableFlag); - if (type >= ColorAdjustTypeCount) + if (!is_valid_ColorAdjustType(type)) return InvalidParameter; imageAttr->noop[type] = enableFlag ? IMAGEATTR_NOOP_SET : IMAGEATTR_NOOP_CLEAR; @@ -263,7 +263,7 @@ GpStatus WINGDIPAPI GdipSetImageAttributesRemapTable(GpImageAttributes *imageAtt TRACE("(%p,%u,%i,%u,%p)\n", imageAttr, type, enableFlag, mapSize, map); - if(!imageAttr || type >= ColorAdjustTypeCount) + if(!imageAttr || !is_valid_ColorAdjustType(type)) return InvalidParameter; if (enableFlag) @@ -325,7 +325,7 @@ GpStatus WINGDIPAPI GdipResetImageAttributes(GpImageAttributes *imageAttr, { TRACE("(%p,%u)\n", imageAttr, type); - if(!imageAttr || type >= ColorAdjustTypeCount) + if(!imageAttr || !is_valid_ColorAdjustType(type)) return InvalidParameter; memset(&imageAttr->colormatrices[type], 0, sizeof(imageAttr->colormatrices[type])); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8141
Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphics.c:
UINT x, y; INT i;
+ if (!is_valid_ColorAdjustType(type)) + return InvalidParameter; +
We shouldn't need this for an internal function (and also it doesn't return a GpStatus). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8141#note_104485
participants (2)
-
Bernhard Übelacker -
Esme Povirk (＠madewokherd)