Fontconfig - Feb 2006 - [PATCH] fix crashes on malformed fonts.cache

Hi, 

This one fixes a memory overrun when a fonts.cache file is slightly malformed. 
other places probably need similiar fixes. 

Thanks,
Dirk

2006-02-01  Dirk Mueller  <dmueller@suse.de>

	* fcfs.c (FcFontSetUnserialize): Make sure there is no
	integer overflow when reading the cache file.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: seife-crash.diff
Type: text/x-diff
Size: 1580 bytes
Desc: not available
Url :
http://lists.freedesktop.org/archives/fontconfig/attachments/20060201/097606a9/seife-crash.bin

Dirk Mueller wrote:
> Hi, 
> 
> This one fixes a memory overrun when a fonts.cache file is slightly
malformed.
> other places probably need similiar fixes. 
Other places probably ought to be fixed similarly, although this is
unlikely to happen by chance.
> -    if (nfont > 0)
> +    if (nfont > 0 && s->nfont < s->nfont + nfont)
I''m about to commit the correct version of this patch, which
doesn''t
include the inadvertently wholly redundant check...

pat

On Saturday 04 February 2006 00:05, Patrick Lam wrote:
> Other places probably ought to be fixed similarly, although this is
> unlikely to happen by chance.
I''ll submit patches once I get around to do it. 
> > -    if (nfont > 0)
> > +    if (nfont > 0 && s->nfont < s->nfont + nfont)
> I''m about to commit the correct version of this patch, which
doesn''t
> include the inadvertently wholly redundant check...
I''m sorry, but the check is not redundant. nfont is signed integer, and
above
protects against an integer overflow. 

A whole better check would be to sanitize it to be < bytes_left_in_the_cache,
since one can assume that each font eats at least one byte, but I found that 
much harder to check for..


Dirk

Dirk Mueller wrote:
> 
>>Anyway, the real fix would be to drag the bytes_left_to_read parameter
>>around and verify against that one, since otherwise block_ptr will run
out
>>of bounds (outside the mmaped area) and then crash.
> 
> Turns out this is easier than I thought. although metadata.count
doesn''t seem
> to be verified either. Anyway, this should work (yet untested): 
Yes, this is much better.  I''ve committed it.

Other places do not read a count directly from the file; instead, they
use the counts stored in the metadata struct.  I''m not sure why I
didn''t
store the number of patterns in metadata, but it must have seemed like a
good idea at the time.  It might have to do with the fact that we
actually alloc the FcPattern * array...

So if you want to make the rest of fontconfig''s treatment of input
sizes
robust, you need to check metadata when it gets read in fccache.c. 
It''s
sort of less of an issue, though, because those other input sizes don''t
trigger any memory allocation.  They''re just within the mmapped chunk.

pat

On Monday 06 February 2006 15:17, Patrick Lam wrote:
> Yes, this is much better.  I''ve committed it.
Thanks. On second thought (sorry ;)), this would be better (also, it even 
compiles, wow!): 


Dirk


Index: fcfs.c
==================================================================RCS file:
/cvs/fontconfig/fontconfig/src/fcfs.c,v
retrieving revision 1.4.4.10
diff -u -3 -d -p -r1.4.4.10 fcfs.c
--- fcfs.c	6 Feb 2006 14:14:21 -0000	1.4.4.10
+++ fcfs.c	6 Feb 2006 14:40:27 -0000
@@ -159,7 +159,10 @@ FcFontSetUnserialize(FcCache * metadata,
     nfont = *(int *)block_ptr;
     block_ptr = (int *)block_ptr + 1;
 
-    if (nfont > 0 && nfont < metadata.count)
+    /* comparing nfont and metadata.count is a bit like comparing
+       apples and oranges. Its just for rejecting totally insane
+       nfont values, and for that its good enough */
+    if (nfont > 0 && nfont < metadata->count / sizeof(void*))
     {
 	FcPattern * p = (FcPattern *)block_ptr;

Dirk Mueller wrote:
> On Monday 06 February 2006 15:17, Patrick Lam wrote:
> 
> 
>>Yes, this is much better.  I''ve committed it.
> 
> 
> Thanks. On second thought (sorry ;)), this would be better (also, it even 
> compiles, wow!): 
Hey, that''s a much better patch (committed)

pat