TrueType tables: fix bugs and possibly security issues

TrueType tables: fix bugs and possibly security issues

[reed, 2009-04-09 03:23:03]

http://codereview.appspot.com/33092/diff/1/2
File src/ports/SkFontHost_tables.cpp (right):

http://codereview.appspot.com/33092/diff/1/2#newcode54
Line 54: } else {
Good catch, but I think we should set it to sizeof(SkSFNTHeader), not 0, to
match the meaning of the value above.

http://codereview.appspot.com/33092/diff/1/2#newcode74
Line 74: stream->rewind();
I think this change is unnecessary with the above comment.

http://codereview.appspot.com/33092/diff/1/2#newcode162
Line 162: // length.
I get how this detects overflow (cool), but I don't see how I can overrwrite the
caller's data buffer (assuming that is the security issue you're speaking of).

Can you show some sample values that would trigger the problem?

[agl, 2009-04-09 06:02:17]

Cheers,

http://codereview.appspot.com/33092/diff/1/2
File src/ports/SkFontHost_tables.cpp (right):

http://codereview.appspot.com/33092/diff/1/2#newcode54
Line 54: } else {
On 2009/04/09 03:23:03, reed wrote:
> Good catch, but I think we should set it to sizeof(SkSFNTHeader), not 0, to
> match the meaning of the value above.

I think it /does/ match the above. From
http://www.microsoft.com/typography/otspec/otff.htm#otttables, "TTC Header
Version 1.0", offsets is described as "Array of offsets to the OffsetTable for
each font from the beginning of the file" and the OffsetTable is the struct
SkSFNTHeader. So I think we have to advance by sizeof(SkSFNTHeader) in each case
to reach the table records.

http://codereview.appspot.com/33092/diff/1/2#newcode162
Line 162: // length.
On 2009/04/09 03:23:03, reed wrote:
> I get how this detects overflow (cool), but I don't see how I can overrwrite
the
> caller's data buffer (assuming that is the security issue you're speaking of).
> 
> Can you show some sample values that would trigger the problem?

This might just be me being paranoid. Please tell me if that's so. I was just
worried that callers might assume that we'll protect them against invalid offset
values (taken from some attacker controlled source). Since we're seeking (and a
seek will fail with invalid offset values), it's not totally unreasonable.

Consider the case where size_t is 32-bits, and the length of the table is 10
bytes. The caller allocates 10 bytes and wants to read a subset of the table
(which might also be the full table; they are assuming that we'll reject invalid
offset values for them).

Now let the attacker controlled offset be -11 mod 2^32. The caller calls:
  GetTableData(fontID, tag, offset (= -11 mod 2^32), length - offset (= 21),
data)

Now, offset + length overflows (and equals 10, which is valid). Also realOffset
+ offset ends up being positive and in range (since realOffset is probably >=
11).

Now we'll read -11 mod 2^32 bytes and maybe crash. However, if the file is short
enough, we'll actually just overwrite the end of the buffer and corrupt the heap
metadata after that buffer with attacker controlled data. That'll generally get
you arbitrary code execution.

Of course, this all hinges on the caller expecting us to reject invalid offset
values for them. And maybe it's just me being paranoid :)

[reed, 2009-04-09 21:36:26]

LGTM