Apply a sanity limit to objects with width & height.

Apply a sanity limit to objects with width & height.

TEST=NONE
BUG=NONE


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45797

[Chris Evans, 2010-04-11 04:03:15]

Defense in depth...

[cevans, 2010-04-11 07:50:21]

Hang fire on this one Justin. I want to make sure "width + start position"
and similar can never overflow either. Updated patch to follow.

On Sat, Apr 10, 2010 at 9:03 PM, <cevans@chromium.org> wrote:

> Reviewers: jschuh,
>
> Message:
> Defense in depth...
>
> Description:
> Apply a sanity limit to objects with width & height.
>
> TEST=NONE
> BUG=NONE
>
>
> Please review this at http://codereview.chromium.org/1582023/show
>
> SVN Base: svn://chrome-svn.corp.google.com/chrome/trunk/src/
>
> Affected files:
>  M     chrome/common/common_param_traits.cc
>
>
> Index: chrome/common/common_param_traits.cc
> ===================================================================
> --- chrome/common/common_param_traits.cc        (revision 44106)
> +++ chrome/common/common_param_traits.cc        (working copy)
> @@ -147,6 +147,8 @@
>       !m->ReadInt(iter, &w) ||
>       !m->ReadInt(iter, &h))
>     return false;
> +  if (w < 0 || h < 0 || h >= ((INT_MAX / 4) / (w ? w : 1)))
> +    return false;
>   r->set_x(x);
>   r->set_y(y);
>   r->set_width(w);
> @@ -170,6 +172,8 @@
>   if (!m->ReadInt(iter, &w) ||
>       !m->ReadInt(iter, &h))
>     return false;
> +  if (w < 0 || h < 0 || h >= ((INT_MAX / 4) / (w ? w : 1)))
> +    return false;
>   r->set_width(w);
>   r->set_height(h);
>   return true;
>
>
>

[Chris Evans, 2010-04-12 15:07:56]

Ok. Now with more thorough validation. Ready for review.

[jschuh, 2010-04-12 16:56:56]

I'm very confused, because I haven't followed this particular code path. It
looks like wraps can still occur for a large width, when the width and the x
coordinate are added, or when the height and the y coordinate are added. 

Also, what's with the division by 16? is that intended to account for the pixel
depth (in which case it should be four)? And there's no reason to check for a
negative plus checking for a max; just cast it unsigned and check against the
max to save that cost of the negative check.

My other thought concerns whether a coordinate transformation can occur after
this point. If that's the case, then any checks here can be bypassed.

[cevans, 2010-04-15 09:41:55]

Thanks for the schooling :) Inline:

On Mon, Apr 12, 2010 at 9:56 AM, <jschuh@chromium.org> wrote:

> I'm very confused, because I haven't followed this particular code path. It
> looks like wraps can still occur for a large width, when the width and the
> x
> coordinate are added, or when the height and the y coordinate are added.
>

Yep, that case is now covered. Thanks.


> Also, what's with the division by 16? is that intended to account for the
> pixel
> depth (in which case it should be four)?


Yes, it must account for at least the traditional pixel depth. I went one
step further to prevent ridiculous sized rects that are right on the limit
of INT_MAX.

And there's no reason to check for a
> negative plus checking for a max; just cast it unsigned and check against
> the
> max to save that cost of the negative check.
>

I prefer to check the natural types in this case. Feels cleaner than
casting. Let me know if you have a strong opinion and I can change it.


> My other thought concerns whether a coordinate transformation can occur
> after
> this point. If that's the case, then any checks here can be bypassed.


Yeah. This is a nice centralized, low-level check to reject clearly crazy
rects and sizes as a defense-in-depth measure. If someone starts doing
matrix transforms on their rects, they will need to adopt their own
additional validation :)

Cheers
Chris


>
>
>
> http://codereview.chromium.org/1582023/show
>

[jschuh, 2010-04-16 05:18:31]

I swear, I'm not trying to be difficult...

On 2010/04/15 09:41:55, cevans_google.com wrote:
> I prefer to check the natural types in this case. Feels cleaner than
> casting. Let me know if you have a strong opinion and I can change it.

I'd probably use something like the following:

#define COORD_MAX (1 << 15)
#define BAD_COORD_MASK (~(COORD_MAX - 1))

...

  if ((x | y | w | h | (x + w) | (y + h)) & BAD_COORD_MASK)
    return false;

...

  if ((w | h) & BAD_COORD_MASK)
    return false;


Here's my reasoning. You know that you're going to cap each dimension at 32768
because that's where Skia and other graphics libs cap int coords. (And even if
the cap is larger in the future, it will always be a power of two narrower than
half a native word.) So, bit operations are cleaner and a lot faster, which
strikes me as important in a graphics path.


> Yeah. This is a nice centralized, low-level check to reject clearly crazy
> rects and sizes as a defense-in-depth measure. If someone starts doing
> matrix transforms on their rects, they will need to adopt their own
> additional validation :)

I guess that makes sense, but I'm just wondering if there's somewhere else we
should be checking. Guess I'll poke around at the code when I get some free
time.

[cevans, 2010-04-18 20:51:18]

On Thu, Apr 15, 2010 at 10:18 PM, <jschuh@chromium.org> wrote:

> I swear, I'm not trying to be difficult...


Thorough reviews encouraged :)


>
> On 2010/04/15 09:41:55, cevans_google.com wrote:
>
>> I prefer to check the natural types in this case. Feels cleaner than
>> casting. Let me know if you have a strong opinion and I can change it.
>>
>
> I'd probably use something like the following:
>
> #define COORD_MAX (1 << 15)
> #define BAD_COORD_MASK (~(COORD_MAX - 1))
>
> ...
>
>  if ((x | y | w | h | (x + w) | (y + h)) & BAD_COORD_MASK)
>    return false;
>
> ...
>
>  if ((w | h) & BAD_COORD_MASK)
>    return false;
>

Ugh! re: this paradigm. It takes me several readings to understand this all,
which is bad for a security check. My brain wants to work in terms of max
and min values. Not bitmasks...


>
> Here's my reasoning. You know that you're going to cap each dimension at
> 32768
> because that's where Skia and other graphics libs cap int coords.


Hmm - do we really never handle > 32767? What about a 100000x10 PNG? (A wide
but manageable amount of data). I must admit, I just created such a PNG and
it loads without error but does not seem to display anything. I'd like
independent confirmation before I go capping a dimension that low. And for
safety, I probably prefer a more conservative approach such as my most
recent proposal...

(And even if
> the cap is larger in the future, it will always be a power of two narrower
> than
> half a native word.) So, bit operations are cleaner and a lot faster, which
> strikes me as important in a graphics path.


This is a deserialization path. We likely just took a context switch plus a
syscall to read from a pipe. Counting cycles doesn't seem productive in this
code path.

Cheers
Chris


>
>
>  Yeah. This is a nice centralized, low-level check to reject clearly crazy
>> rects and sizes as a defense-in-depth measure. If someone starts doing
>> matrix transforms on their rects, they will need to adopt their own
>> additional validation :)
>>
>
> I guess that makes sense, but I'm just wondering if there's somewhere else
> we
> should be checking. Guess I'll poke around at the code when I get some free
> time.
>
>
>
> http://codereview.chromium.org/1582023/show
>

[DO NOT USE (jschuh), 2010-04-20 18:42:34]

Fair enough. LGTM.

On 2010/04/18 20:51:18, cevans_google.com wrote:
> On Thu, Apr 15, 2010 at 10:18 PM, <mailto:jschuh@chromium.org> wrote:
> 
> > I swear, I'm not trying to be difficult...
> 
> 
> Thorough reviews encouraged :)
> 
> 
> >
> > On 2010/04/15 09:41:55, http://cevans_google.com wrote:
> >
> >> I prefer to check the natural types in this case. Feels cleaner than
> >> casting. Let me know if you have a strong opinion and I can change it.
> >>
> >
> > I'd probably use something like the following:
> >
> > #define COORD_MAX (1 << 15)
> > #define BAD_COORD_MASK (~(COORD_MAX - 1))
> >
> > ...
> >
> >  if ((x | y | w | h | (x + w) | (y + h)) & BAD_COORD_MASK)
> >    return false;
> >
> > ...
> >
> >  if ((w | h) & BAD_COORD_MASK)
> >    return false;
> >
> 
> Ugh! re: this paradigm. It takes me several readings to understand this all,
> which is bad for a security check. My brain wants to work in terms of max
> and min values. Not bitmasks...
> 
> 
> >
> > Here's my reasoning. You know that you're going to cap each dimension at
> > 32768
> > because that's where Skia and other graphics libs cap int coords.
> 
> 
> Hmm - do we really never handle > 32767? What about a 100000x10 PNG? (A wide
> but manageable amount of data). I must admit, I just created such a PNG and
> it loads without error but does not seem to display anything. I'd like
> independent confirmation before I go capping a dimension that low. And for
> safety, I probably prefer a more conservative approach such as my most
> recent proposal...
> 
> (And even if
> > the cap is larger in the future, it will always be a power of two narrower
> > than
> > half a native word.) So, bit operations are cleaner and a lot faster, which
> > strikes me as important in a graphics path.
> 
> 
> This is a deserialization path. We likely just took a context switch plus a
> syscall to read from a pipe. Counting cycles doesn't seem productive in this
> code path.
> 
> Cheers
> Chris
> 
> 
> >
> >
> >  Yeah. This is a nice centralized, low-level check to reject clearly crazy
> >> rects and sizes as a defense-in-depth measure. If someone starts doing
> >> matrix transforms on their rects, they will need to adopt their own
> >> additional validation :)
> >>
> >
> > I guess that makes sense, but I'm just wondering if there's somewhere else
> > we
> > should be checking. Guess I'll poke around at the code when I get some free
> > time.
> >
> >
> >
> > http://codereview.chromium.org/1582023/show
> >
>