[PATCH] wineps.drv: check for GDI_ERROR in LoadTable()
Signed-off-by: Wolfgang Walter <wine(a)stwm.de> --- If GetFontData returns GDI_ERROR do not treat it as length. With wine32 this worked by luck. Set the table length to zero. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts
On 11/9/18 4:21 PM, Wolfgang Walter wrote:
if(table->MS_tag == MS_MAKE_TAG('g','d','i','r')) return TRUE; table->len = GetFontData(hdc, table->MS_tag, 0, NULL, 0); + table->check = 0; + if(table->len == GDI_ERROR) { + table->len = 0; + return TRUE; + } + if(table->len > (0xfffffffflu - 3)) { + table->len = 0; + return FALSE; + }
What is the second condition for?
On Friday, 9 November 2018 18:50:43 CET Nikolay Sivov wrote:
On 11/9/18 4:21 PM, Wolfgang Walter wrote:
if(table->MS_tag == MS_MAKE_TAG('g','d','i','r')) return TRUE; table->len = GetFontData(hdc, table->MS_tag, 0, NULL, 0);
+ table->check = 0; + if(table->len == GDI_ERROR) { + table->len = 0; + return TRUE; + } + if(table->len > (0xfffffffflu - 3)) { + table->len = 0; + return FALSE; + }
What is the second condition for?
The code which follows is: table->data = HeapAlloc(GetProcessHeap(), 0, (table->len + 3) & ~3 ); memset(table->data + ((table->len - 1) & ~3), 0, sizeof(DWORD)); GetFontData(hdc, table->MS_tag, 0, table->data, table->len); for(i = 0; i < (table->len + 3) / 4; i++) table->check += FLIP_ORDER(*((DWORD*)(table->data) + i)); If table->len (which itself is a DWORD) gets bigger than 0xfffffffflu - 3 it will overflow in (table->len + 3) and HeapAlloc does not allocate as much memory as expected. The whole thing will then be inconsistent and I thought therefor one should no rely that a) wine will handle that gracefully and b) that there is no such font embedded in pdfs. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Recht
On Sat, Nov 10, 2018 at 12:33:30AM +0100, Wolfgang Walter wrote:
On Friday, 9 November 2018 18:50:43 CET Nikolay Sivov wrote:
On 11/9/18 4:21 PM, Wolfgang Walter wrote:
if(table->MS_tag == MS_MAKE_TAG('g','d','i','r')) return TRUE; table->len = GetFontData(hdc, table->MS_tag, 0, NULL, 0);
+ table->check = 0; + if(table->len == GDI_ERROR) { + table->len = 0; + return TRUE; + } + if(table->len > (0xfffffffflu - 3)) { + table->len = 0; + return FALSE; + }
What is the second condition for?
The code which follows is:
table->data = HeapAlloc(GetProcessHeap(), 0, (table->len + 3) & ~3 ); memset(table->data + ((table->len - 1) & ~3), 0, sizeof(DWORD)); GetFontData(hdc, table->MS_tag, 0, table->data, table->len); for(i = 0; i < (table->len + 3) / 4; i++) table->check += FLIP_ORDER(*((DWORD*)(table->data) + i));
If table->len (which itself is a DWORD) gets bigger than 0xfffffffflu - 3 it will overflow in (table->len + 3) and HeapAlloc does not allocate as much memory as expected.
I don't think that's worth covering. I've sent in a cleaner version. Thanks! Huw.
Am Montag, 12. November 2018, 10:57:59 schrieb Huw Davies:
On Sat, Nov 10, 2018 at 12:33:30AM +0100, Wolfgang Walter wrote:
On Friday, 9 November 2018 18:50:43 CET Nikolay Sivov wrote:
On 11/9/18 4:21 PM, Wolfgang Walter wrote:
if(table->MS_tag == MS_MAKE_TAG('g','d','i','r')) return TRUE; table->len = GetFontData(hdc, table->MS_tag, 0, NULL, 0);
+ table->check = 0; + if(table->len == GDI_ERROR) { + table->len = 0; + return TRUE; + } + if(table->len > (0xfffffffflu - 3)) { + table->len = 0; + return FALSE; + }
What is the second condition for?
The code which follows is:
table->data = HeapAlloc(GetProcessHeap(), 0, (table->len + 3) & ~3 ); memset(table->data + ((table->len - 1) & ~3), 0, sizeof(DWORD)); GetFontData(hdc, table->MS_tag, 0, table->data, table->len); for(i = 0; i < (table->len + 3) / 4; i++)
table->check += FLIP_ORDER(*((DWORD*)(table->data) + i));
If table->len (which itself is a DWORD) gets bigger than 0xfffffffflu - 3 it will overflow in (table->len + 3) and HeapAlloc does not allocate as much memory as expected.
I don't think that's worth covering. I've sent in a cleaner version.
Wouldn't it possible that someone sends a PDF with a table len of say 0xfffffffe? Then the allocated memory is small and the later memset writes 0 beyond. Actually I found this problem with PDF embedded font so if this is not checked elsewhere it is scary. But of one should then also check that len is not to small? It should be at least 1 so that table->len - 1 does not underflow. I don't know if plausibilty of the table lenght is already checked elsewhere, of course. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts
On Mon, 12 Nov 2018 at 14:28, Huw Davies <huw(a)codeweavers.com> wrote:
If table->len (which itself is a DWORD) gets bigger than 0xfffffffflu - 3 it will overflow in (table->len + 3) and HeapAlloc does not allocate as much memory as expected.
I don't think that's worth covering. I've sent in a cleaner version.
I'm inclined to side somewhat with Wolfgang here. I.e., fonts are essentially untrusted data, and it seems plausible enough that someone may set unreasonable values on purpose.
On Mon, Nov 12, 2018 at 04:04:32PM +0330, Henri Verbeet wrote:
On Mon, 12 Nov 2018 at 14:28, Huw Davies <huw(a)codeweavers.com> wrote:
If table->len (which itself is a DWORD) gets bigger than 0xfffffffflu - 3 it will overflow in (table->len + 3) and HeapAlloc does not allocate as much memory as expected.
I don't think that's worth covering. I've sent in a cleaner version.
I'm inclined to side somewhat with Wolfgang here. I.e., fonts are essentially untrusted data, and it seems plausible enough that someone may set unreasonable values on purpose.
Sure, it's easy enough to send in a follow-up patch after the missing table patch is in. Huw.
Henri Verbeet <hverbeet(a)gmail.com> writes:
On Mon, 12 Nov 2018 at 14:28, Huw Davies <huw(a)codeweavers.com> wrote:
If table->len (which itself is a DWORD) gets bigger than 0xfffffffflu - 3 it will overflow in (table->len + 3) and HeapAlloc does not allocate as much memory as expected.
I don't think that's worth covering. I've sent in a cleaner version.
I'm inclined to side somewhat with Wolfgang here. I.e., fonts are essentially untrusted data, and it seems plausible enough that someone may set unreasonable values on purpose.
It seems to me that this should be caught by the lower layers, ideally in FreeType or else in gdi32. -- Alexandre Julliard julliard(a)winehq.org
Am Montag, 12. November 2018, 13:49:26 schrieb Alexandre Julliard:
Henri Verbeet <hverbeet(a)gmail.com> writes:
On Mon, 12 Nov 2018 at 14:28, Huw Davies <huw(a)codeweavers.com> wrote:
If table->len (which itself is a DWORD) gets bigger than 0xfffffffflu - 3 it will overflow in (table->len + 3) and HeapAlloc does not allocate as much memory as expected.
I don't think that's worth covering. I've sent in a cleaner version.
I'm inclined to side somewhat with Wolfgang here. I.e., fonts are essentially untrusted data, and it seems plausible enough that someone may set unreasonable values on purpose.
It seems to me that this should be caught by the lower layers, ideally in FreeType or else in gdi32.
A table lenght of 0xffffffff could be valid (at least in theory). Though the length of a table must be a multiple of 4 bytes including padding, the padding should not ne recorded: "The length of all tables should be recorded in the table record with their actual length (not their padded length)." https://docs.microsoft.com/de-de/typography/opentype/spec/otff On the other hand the example code given there for calculating the checksum would not work with 0xffffffff. So a very large font with > 4G could contain such a table and freetype could accept it? But probably wineps has other parts where it does not check for overflows or malicious data, i.e. when accessing the glyph table. Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts
On Tue, 13 Nov 2018 at 19:43, Wolfgang Walter <wine(a)stwm.de> wrote:
But probably wineps has other parts where it does not check for overflows or malicious data, i.e. when accessing the glyph table.
Yeah, but you have to start somewhere. If someone had some free time and wanted to have some fun, it may be interesting to run AFL [1] on some of that code. At the very least they'd gain some insight into the various ways in which things can break. [1] http://lcamtuf.coredump.cx/afl/
participants (5)
-
Alexandre Julliard -
Henri Verbeet -
Huw Davies -
Nikolay Sivov -
Wolfgang Walter