Add some bounds for gfx ipc deserialization.

content/public/common/common_param_traits.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/public/common/common_param_traits.h"
 
+#include <limits>
+
 #include "content/public/common/content_constants.h"
 #include "content/public/common/referrer.h"
 #include "net/base/host_port_pair.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/rect_f.h"
 
 namespace {
 
 struct SkBitmap_Data {
@@ skipping 144 matching lines @@
     return false;
   r->set_x(x);
   r->set_y(y);
   return true;
 }
 
 void ParamTraits<gfx::PointF>::Log(const gfx::PointF& v, std::string* l) {
   l->append(base::StringPrintf("(%f, %f)", v.x(), v.y()));
 }
 
 void ParamTraits<gfx::Size>::Write(Message* m, const gfx::Size& p) {

[piman, 2013/01/07 19:05:56]

could we add a DCHECK that the serialized values pass the checks of the
deserializer? It is very painful to debug issues on the receiving side.

[jschuh, 2013/01/07 22:24:14]

Yep.

   m->WriteInt(p.width());
   m->WriteInt(p.height());
 }
 
 bool ParamTraits<gfx::Size>::Read(const Message* m,
                                   PickleIterator* iter,
                                   gfx::Size* r) {
   int w, h;
-  if (!m->ReadInt(iter, &w) ||
-      !m->ReadInt(iter, &h))
+  if (!m->ReadInt(iter, &w) || w < 0 ||
+      !m->ReadInt(iter, &h) || h < 0 ||
+      (h && w > ((std::numeric_limits<int>::max() / 4) / h)))

[danakj, 2013/01/07 19:19:03]

We already DCHECK that sizes have positive values inside the Size class itself.
So checking that w and h >= 0 here makes lots of sense.

But, while this upper bound might make sense is some contexts, there's no reason
why people might not want to use std::numeric_limits<int>::max() in a size. I
don't think we should have any upper bound in here. It is too general of a
class, and there is nothing that says a large size isn't a valid use of the
class.

[danakj, 2013/01/07 19:21:52]

Actually maybe we don't. I landed that CL but it was reverted due to some
android problems, and I never got back to relanding it. I should do that.

But it would be nice to DCHECK the size values >= 0 on the serialization side
now, then, as piman said as well. That is the intended use of the class.

[jschuh, 2013/01/07 22:24:14]

I understand that generally, but in security sensitive areas like the IPC layer
we're usually best served by identifying and enforcing reasonable restrictions.
I ran the test and manually searched for places where this assumption would
fail, and didn't find any. so I'm pretty strongly inclined to leave it in unless
we're aware of anything that will break.

[jschuh, 2013/01/07 22:24:14]

Yep.

[danakj, 2013/01/07 22:56:37]

Sure, I'm just not sure why you see something like this as more reasonable than
"an integer". I'd much prefer upper-bounding size values at the message or
containing-struct level, where you have some context what is valid. This just
seems pretty arbitrary and something people will just have to remove some day
when they want to use a size to do something new. Or, worse off, make them use 2
ints when they were planning to use a size, because of an arbitrary limit.

[jschuh, 2013/01/08 00:08:45]

I appreciate that it seems arbitrary, but it's the most common error we see when
handling image buffers (overflow in h * w * pixels), and this is the gfx::Size
class. If someone wants to use the type for something new, it seems that they'd
need to make it larger, which means rewriting the message serializer anyway.

If it makes it clearer, I'm happy to put a comment explaining the rationale for
the check.

[jschuh, 2013/01/08 00:43:32]

Antoine provided some context in was lacking (in the future please assume I know
less about graphics ;). I'll post another patch shortly.

     return false;
   r->set_width(w);
   r->set_height(h);
   return true;
 }
 
 void ParamTraits<gfx::Size>::Log(const gfx::Size& p, std::string* l) {
   l->append(base::StringPrintf("(%d, %d)", p.width(), p.height()));
 }
 
@@ skipping 66 matching lines @@
   m->WriteInt(p.width());
   m->WriteInt(p.height());
 }
 
 bool ParamTraits<gfx::Rect>::Read(const Message* m,
                                   PickleIterator* iter,
                                   gfx::Rect* r) {
   int x, y, w, h;
   if (!m->ReadInt(iter, &x) ||
       !m->ReadInt(iter, &y) ||
-      !m->ReadInt(iter, &w) ||
-      !m->ReadInt(iter, &h))
+      !m->ReadInt(iter, &w) || w < 0 ||
+      !m->ReadInt(iter, &h) || h < 0 ||
+      (h && w > ((std::numeric_limits<int>::max() / 4) / h)))

[piman, 2013/01/07 19:05:56]

nit: it would be even better to serialize p.origin() and p.size() and restore
them through them, it's more concise, and avoids the duplication of the logic.

[jschuh, 2013/01/07 22:24:14]

Yep.

     return false;
   r->set_x(x);
   r->set_y(y);
   r->set_width(w);
   r->set_height(h);
   return true;
 }
 
 void ParamTraits<gfx::Rect>::Log(const gfx::Rect& p, std::string* l) {
   l->append(base::StringPrintf("(%d, %d, %d, %d)", p.x(), p.y(),
@@ skipping 84 matching lines @@
 #undef CONTENT_PUBLIC_COMMON_COMMON_PARAM_TRAITS_MACROS_H_
 #include "content/public/common/common_param_traits_macros.h"
 }  // namespace IPC
 
 // Generate param traits log methods.
 #include "ipc/param_traits_log_macros.h"
 namespace IPC {
 #undef CONTENT_PUBLIC_COMMON_COMMON_PARAM_TRAITS_MACROS_H_
 #include "content/public/common/common_param_traits_macros.h"
 }  // namespace IPC