[PATCH v5 0/1] MR7047: xmllite/tests: Avoid buffer overflow by using LONG_PTR. (ASan)
This appeared by running tests with PE-side ASan enabled, just with x86_64. The enum DtdProcessing seems to have just a size of four bytes, but dereferencing `LONG_PTR *value` writes 8 bytes. ``` ================================================================= ==560==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffffe1ff8c0 at pc 0x6ffff7b33f83 bp 0x7ffffe1fed00 sp 0x7ffffe1fed48 WRITE of size 8 at 0x7ffffe1ff8c0 thread T0 05c0:fixme:file:server_get_file_info Unsupported info class e #0 0x6ffff7b33f82 in xmlreader_GetProperty .../wine/dlls/xmllite/reader.c:2807:20 #1 0x000140002254 in IXmlReader_GetProperty .../obj\include\xmllite.h:331:12 #2 0x000140002254 in test_reader_create .../wine/dlls/xmllite/tests/reader.c:582:10 #3 0x000140002254 in func_reader .../wine/dlls/xmllite/tests/reader.c:2676:5 #4 0x00014006bad1 in run_test .../wine/include/wine/test.h:765:5 #5 0x00014006bad1 in main .../wine/include/wine/test.h:884:12 #6 0x00014006d94f in mainCRTStartup .../wine/dlls/msvcrt/crt_main.c:58:11 #7 0x6ffffbdc4808 in BaseThreadInitThunk .../wine/dlls/kernel32\thread.c:61:5 #8 0x6ffffad1fa1a in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x17000fa1a) Address 0x7ffffe1ff8c0 is located in stack of thread T0 at offset 2784 in frame #0 0x00014000100f in func_reader .../wine/dlls/xmllite/tests/reader.c:2675 This frame has 113 object(s): [32, 36) 'd.i439.i' (line 824) [48, 52) 'd.i412.i' (line 824) [64, 68) 'd.i385.i' (line 824) [80, 84) 'd.i358.i' (line 824) ... [2672, 2676) 'nodetype.i2' (line 620) [2688, 2696) 'resolver.i' (line 515) [2720, 2728) 'unk.i' (line 516) [2752, 2760) 'reader.i' (line 517) [2784, 2788) 'dtd.i' (line 518) <== Memory access at offset 2784 partially overflows this variable [2800, 2804) 'nodetype.i' (line 519) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp, SEH and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow .../wine/dlls/xmllite/reader.c:2807:20 in xmlreader_GetProperty Shadow bytes around the buggy address: 0x7ffffe1ff600: f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 0x7ffffe1ff680: f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 f8 f2 0x7ffffe1ff700: f8 f2 f2 f2 f8 f2 f8 f2 f2 f2 f8 f2 f8 f2 f8 f2 0x7ffffe1ff780: f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 0x7ffffe1ff800: f2 f2 f8 f2 f2 f2 f8 f2 f2 f2 f8 f2 00 f2 f2 f2 =>0x7ffffe1ff880: 00 f2 f2 f2 00 f2 f2 f2[04]f2 04 f3 00 00 00 00 0x7ffffe1ff900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffffe1ff980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffffe1ffa00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffffe1ffa80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7ffffe1ffb00: 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 ==560==ABORTING 05c0:fixme:kernelbase:AppPolicyGetProcessTerminationMethod FFFFFFFFFFFFFFFA, 00007FFFFEA8FE80 make: *** [Makefile:471569: dlls/xmllite/tests/x86_64-windows/reader.ok] Fehler 1 ``` -- v5: xmllite/tests: Avoid buffer overflow by using LONG_PTR. (ASan) https://gitlab.winehq.org/wine/wine/-/merge_requests/7047
From: Bernhard Übelacker <bernhardu(a)mailbox.org> --- dlls/xmllite/tests/reader.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/xmllite/tests/reader.c b/dlls/xmllite/tests/reader.c index 6c784826232..0dd490ff989 100644 --- a/dlls/xmllite/tests/reader.c +++ b/dlls/xmllite/tests/reader.c @@ -515,7 +515,7 @@ static void test_reader_create(void) IXmlResolver *resolver; IUnknown *input, *unk; IXmlReader *reader; - DtdProcessing dtd; + LONG_PTR dtd; XmlNodeType nodetype; HRESULT hr; @@ -578,10 +578,10 @@ static void test_reader_create(void) hr = IXmlReader_SetProperty(reader, XmlReaderProperty_XmlResolver, 0); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - dtd = 2; - hr = IXmlReader_GetProperty(reader, XmlReaderProperty_DtdProcessing, (LONG_PTR*)&dtd); + dtd = -1; + hr = IXmlReader_GetProperty(reader, XmlReaderProperty_DtdProcessing, &dtd); ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); - ok(dtd == DtdProcessing_Prohibit, "got %d\n", dtd); + ok(dtd == DtdProcessing_Prohibit, "got %Id\n", dtd); dtd = 2; hr = IXmlReader_SetProperty(reader, XmlReaderProperty_DtdProcessing, dtd); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7047
Thanks again, yes, it is no longer needed. v4: - Removed no longer needed cast to LONG_PTR*. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7047#note_90906
On Thu Dec 19 19:31:44 2024 +0000, Alfred Agrell wrote:
I'd change line 581 to something with the high half nonzero (-1, for example), to verify how many bytes native actually writes. (It writes a LONG_PTR, I checked.) It's a natural assumption that argument type width is how many bytes it will write. You wouldn't check if e.g. 4-byte integer pointer argument updates all 4 bytes.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7047#note_90917
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7047
participants (2)
-
Bernhard Übelacker -
Nikolay Sivov (＠nsivov)