bugThe FreeType Project - Bugs: bug #34139, LZW decompression unbound...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #34139: LZW decompression unbound character stack size

Submitter:  Tomas Hoger <thoger>
Submitted:  Fri 26 Aug 2011 07:28:02 AM UTC
   
 
Severity:  3 - Normal Item Group:  None
Status:  Fixed Privacy:  Public
Assigned to:  wl Open/Closed:  Closed
Planned Release:  2.4.7

Mon 12 Sep 2011 01:41:58 PM UTC, comment #5: 

Thanks for the additional samples.  I concur that I don't have to something, which is good :-)

Werner LEMBERG <wl>
Group administrator
Mon 12 Sep 2011 10:11:03 AM UTC, comment #4: 

There is still somewhat surprising behaviour that prefix[] and suffix[] tables are overwritten at clear code position.  As noted in comment #2, those values are never used though, and hence that behaviour seems completely harmless.

I'm attaching another reproducer that demonstrates the loop via subsequent clear codes.  It makes prefix[LZW_CLEAR - 256] be 256.  It is now blocked by 487913d9.  Reverting 487913d9 causes max stack size check from 83cb6c00 to prevent memory hogging.

(file #23970)

Tomas Hoger <thoger>
Sun 11 Sep 2011 07:30:10 AM UTC, comment #3: 

I've applied all your patches, thanks!

Regarding Jörg's issue: Do you think that more action is required within FreeType?

Werner LEMBERG <wl>
Group administrator
Fri 26 Aug 2011 07:53:52 AM UTC, comment #2: 

One additional minor change I did while looking at this issue...  Attached patch sightly changes clear code handling to properly handle subsequent clear codes (they probably never appear in non-corrupted/malicious LZW streams).

It also re-sets old_code and old_char to more predictable value.  They may be used if the code after clear code is >= 256 (not valid).  This change is related to the question in the code: /* why not LZW_FIRST-256 ? */  The answer to the question is that original compress code had a bug(?)/feature(?) that it used standard processing loop to handle the first code after clear, rather than using special handling that is used for the first code in the stream.  As a consequence, prefix and suffix tables got changed/corrupted at the LZW_CLEAR (- 256) position.  This did not have any real impact, as those tables are never accessed at that position (well, except of the subsequent clear codes case).

Recently Joerg Sonnenberger has written a patch for NetBSD compress that avoids the unintended change of the tables at clear position:
http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.bin/compress/zopen.c#rev1.15

This relies on having oldcode set to special value meaning "no old code" (-1 in the patch).  My reading of the FT code is that that approach would require changing old_code type, as it's FT_UInt now, which is possibly 16bit, so all possible values are required for LZW codes.  Type change would not be needed for 32bit machines.

(file #23882)

Tomas Hoger <thoger>
Fri 26 Aug 2011 07:33:08 AM UTC, comment #1: 

Optional additional patch adds explicit stack size limit to ft_lzwstate_stack_grow.  Stack larger than 1<<LZW_MAX_BITS is never needed.  Doing an explicit check in ft_lzwstate_stack_grow seems fairly cheap, and can help limit memory usage if some other way is found to create prefix loop.

(file #23881)

Tomas Hoger <thoger>
Fri 26 Aug 2011 07:28:02 AM UTC, original submission:  

While FreeType LZW decompressor is the re-write of the original BSD compress code, it has some of the problems of the original version.  It does not sufficiently validate codes read from the LZW stream, which can lead to breaking expected invariant: code < prefix[code], which allows creating loops in the prefix table.

Unlike original code, ftzopen grows character stack as needed, so the prefix loop does not lead to buffer overflow, it does lead to memory usage spikes, until ft_lzwstate_stack_grow fails to reallocate memory.

There are two ways to create loop - either via invalid first code which is read in the start phase, or via KwKwK special case handling.  Attaching reproducers for both cases, as well as proposed patch.  Issue can be reproduced with ftview (ftview 10 test[12].Z).

Tomas Hoger <thoger>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #23970:  test3.Z added by thoger (348B - application/octet-stream - Subsequent clear code loop reproducer)
file #23882:  ft-lzw-clear-code-handling.diff added by thoger (1KiB - application/octet-stream - Slightly improved LZW_CLEAR handling)
file #23881:  ft-lzw-stack-size-limit.diff added by thoger (1KiB - application/octet-stream - Explicity character stack size limit)
file #23878:  ft-lzw-code-checks.diff added by thoger (1KiB - application/octet-stream - Reproducers and patch)
file #23879:  test1.Z added by thoger (348B - application/octet-stream - Reproducers and patch)
file #23880:  test2.Z added by thoger (348B - application/octet-stream - Reproducers and patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by wl (Posted a comment)
  • -email is unavailable- added by thoger (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-09-12 thoger Attached File- Added test3.Z, #23970
    2011-09-11 wl StatusNone Fixed
        Assigned toNone wl
        Open/ClosedOpen Closed
        Planned ReleaseNone 2.4.7
    2011-08-26 thoger Attached File- Added ft-lzw-clear-code-handling.diff, #23882
    2011-08-26 thoger Attached File- Added ft-lzw-stack-size-limit.diff, #23881
    2011-08-26 thoger Attached File- Added ft-lzw-code-checks.diff, #23878
        Attached File- Added test1.Z, #23879
        Attached File- Added test2.Z, #23880

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code