Added a missing filter in SkFlattenable::InitializeFlattenables()

Added a missing filter in SkFlattenable::InitializeFlattenables().

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=11110

[sugoi1, 2013-08-27 20:10:43]

This code simply closes a security gap that would allow someone to use
SkOrderedReadBuffer/SkOrderedWriteBuffer with a cross process flag, but still
send/receive a function pointer.

[mtklein, 2013-08-28 14:46:51]

On 2013/08/27 20:10:43, sugoi1 wrote:
> This code simply closes a security gap that would allow someone to use
> SkOrderedReadBuffer/SkOrderedWriteBuffer with a cross process flag, but still
> send/receive a function pointer.

I think we need to step back even further here.  None of the code above or below
SkFlattenable was written with security in mind, and it's going to be full of
holes.  It's great to force that we're not passing function pointers, yes, but
passing by name is really just as bad isn't it?  Take a look at how we decode
that factory name and then look up the factory.  Again here's the rough
pseudocode:

decode:
  name = readString
  factory = NameToFactory(name)

readString:
  ptr, len = reader32::readString()
  copy len bytes of ptr into output SkString

reader32::readString: (with some inlining)
  read 4 bytes from our buffer as an int32_t
  increment buffer 4 bytes
  in debug mode, check that we've not overrun
  return a pointer into the buffer

NameToFactory:
  strcmp each factory name with the lookup name, return the factory if found


So, reader32::readString is really unsafe.  It doesn't check that we can read 4
bytes of size.  It doesn't check that the string we're returning is actually len
bytes long.  It assumes, but doesn't check, that there's a \0 at ptr[len].  None
of the sanity checks that are there are enabled in a release build.

The result of reader32::readString goes into an SkString and that code looks
safe from there, provided len and ptr are safe.  We're pretty careful to always
obey len there inside SkString.

Now, I'm not saying that you need to patch these too.  I'm saying we need to
start from scratch if we're going to be secure, because our whole stack here is
just filled with holes like this, and mostly we want those holes there.  This
code is the way it is for Skia's convenience and because it's fast.  It's
trusted code.  We would not want, for example, to add checks to readString to
make it safe if it forced us to read the bytes of the string.  The way we use
SkString looks safe as used now, but there's every chance we'll sneak a strcpy
in there one day because it made the code clearer or faster.  I don't want to
have to trust that the Skia team maintains a vigilant security mindset.  We
certainly don't have one now.

Maybe it's not so bad to read bad data?  At worst we'll garble some drawing or
segfault the process right?  Hard to make that guarantee.  What we're sending is
effectively a serialized Skia program, which will call into jpeg, png, pdf code,
etc without much validation.  Do we trust that there's no way Skia can do
something malicious if it's told to?  I don't.

I don't trust we can do anything really secure using the existing SkFlattenable
stack.  We've got an opportunity here to start secure by breaking any dependency
SkFlattenableSerialization has on SkOrderedWriteBuffer, SkOrderedReadBuffer,
SkWriter32, and SkReader32.  Skia's been wanting to revamp our serialization
format for a while now, but the size of the task has until now dwarfed its
perceived importance.  Now that we've got security as a concern, that tips it
over to time to redesign, at least to me.

You with me here?  If so, let's meet up for an hour or so to sketch out a plan
of attack?  If not, let's meet up for an hour or so to see if we can get on the
same page?

[sugoi1, 2013-08-28 15:12:41]

On 2013/08/28 14:46:51, mtklein wrote:
> It's great to force that we're not passing function pointers, yes, but
> passing by name is really just as bad isn't it? 

I wouldn't say "as bad", but still pretty bad :)

> So, reader32::readString is really unsafe.  It doesn't check that we can read
4
> bytes of size.  It doesn't check that the string we're returning is actually
len
> bytes long.  It assumes, but doesn't check, that there's a \0 at ptr[len]. 
None
> of the sanity checks that are there are enabled in a release build.

I was thinking of adding these checks in a different cl, but I'm open to a
little re-architecturing.

> Maybe it's not so bad to read bad data?  At worst we'll garble some drawing or
> segfault the process right?  Hard to make that guarantee.  What we're sending
is
> effectively a serialized Skia program, which will call into jpeg, png, pdf
code,
> etc without much validation.  Do we trust that there's no way Skia can do
> something malicious if it's told to?  I don't.

Maybe in the general case, but in the chromium case, we're just
serializing/deserializing SkFlattenable objects. Nothing else is permitted in
that context. The code explicitly calls writeFlattenable/readFlattenable
directly in SkSerializeFlattenable()/SkDeserializeFlattenable(). I'm not saying
the rest doesn't need to be safe, I'm just saying it doesn't need to be safe now
because it can't be executed (unless you know of some way I'm unaware of).

> I don't trust we can do anything really secure using the existing
SkFlattenable
> stack.  We've got an opportunity here to start secure by breaking any
dependency
> SkFlattenableSerialization has on SkOrderedWriteBuffer, SkOrderedReadBuffer,
> SkWriter32, and SkReader32.  Skia's been wanting to revamp our serialization
> format for a while now, but the size of the task has until now dwarfed its
> perceived importance.  Now that we've got security as a concern, that tips it
> over to time to redesign, at least to me.
> 
> You with me here?  If so, let's meet up for an hour or so to sketch out a plan
> of attack?  If not, let's meet up for an hour or so to see if we can get on
the
> same page?

I'm with you. If you have a plan, I'll gladly execute it. Make sure reed@ is
also in, it should be easier for you to discuss the plan with him than for me.
Thanks.

[sugoi1, 2013-09-04 18:46:33]

Since most changes have been deprecated by the changes made here :
https://codereview.chromium.org/23021015/

I just kept the small bugfix.

[mtklein, 2013-09-04 19:09:09]

lgtm

[commit-bot: I haz the power, 2013-09-04 19:26:24]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/23444005/6001

[commit-bot: I haz the power, 2013-09-05 18:28:12]

Change committed as 11110