Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11617006:
Add some bounds for gfx ipc deserialization. (Closed)
|
Created:
8 years ago by jschuh Modified:
7 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd some bounds for gfx ipc deserialization.
BUG=167680
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175496
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 11
Patch Set 7 : #
Total comments: 9
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 2
Patch Set 11 : #Messages
Total messages: 17 (0 generated)
@piman - We discussed this a few weeks ago, and it looks like this bounds checking is sufficient and passes the tests.
Overall I'm ok with that, modulo nits, but I'll let Dana comment on whether the conditions are currently assumed to be true (I think we used to use negative sizes, but most, if not all have been converted to vectors). https://chromiumcodereview.appspot.com/11617006/diff/32001/content/public/com... File content/public/common/common_param_traits.cc (right): https://chromiumcodereview.appspot.com/11617006/diff/32001/content/public/com... content/public/common/common_param_traits.cc:173: void ParamTraits<gfx::Size>::Write(Message* m, const gfx::Size& p) { 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. https://chromiumcodereview.appspot.com/11617006/diff/32001/content/public/com... content/public/common/common_param_traits.cc:273: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) 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.
https://chromiumcodereview.appspot.com/11617006/diff/32001/content/public/com... File content/public/common/common_param_traits.cc (right): https://chromiumcodereview.appspot.com/11617006/diff/32001/content/public/com... content/public/common/common_param_traits.cc:184: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) 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.
https://chromiumcodereview.appspot.com/11617006/diff/32001/content/public/com... File content/public/common/common_param_traits.cc (right): https://chromiumcodereview.appspot.com/11617006/diff/32001/content/public/com... content/public/common/common_param_traits.cc:184: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) On 2013/01/07 19:19:03, danakj wrote: > 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. 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.
https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... content/public/common/common_param_traits.cc:173: void ParamTraits<gfx::Size>::Write(Message* m, const gfx::Size& p) { On 2013/01/07 19:05:56, piman wrote: > 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. Yep. https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... content/public/common/common_param_traits.cc:184: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) On 2013/01/07 19:19:03, danakj wrote: > 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. 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. https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... content/public/common/common_param_traits.cc:184: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) On 2013/01/07 19:21:52, danakj wrote: > On 2013/01/07 19:19:03, danakj wrote: > > 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. > > 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. Yep. https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... content/public/common/common_param_traits.cc:273: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) On 2013/01/07 19:05:56, piman wrote: > 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. Yep.
https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:265: m->WriteInt(p.y()); y u no do WriteParam(m, p.origin()) too ? https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:266: ParamTraits<gfx::Size>::Write(m, gfx::Size(p.width(), p.height())); WriteParam(m, p.size()); https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:276: !ParamTraits<gfx::Size>::Read(m, iter, &size)) !ReadParam(m, iter, &size) https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:280: r->set_width(size.width()); r->set_size(size);
https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... content/public/common/common_param_traits.cc:184: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) On 2013/01/07 22:24:14, Justin Schuh wrote: > On 2013/01/07 19:19:03, danakj wrote: > > 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. > > 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. 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.
https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:265: m->WriteInt(p.y()); On 2013/01/07 22:51:35, piman wrote: > y u no do WriteParam(m, p.origin()) too ? Because you weren't supposed to review it yet. :P My checkout is broken/win64 and I'm using the clang bot as a test builder. I really should have labelled this WIP.
Okay, let's try again. https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... content/public/common/common_param_traits.cc:184: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) On 2013/01/07 22:56:37, danakj wrote: > On 2013/01/07 22:24:14, Justin Schuh wrote: > > 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. > > 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. 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. https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:265: m->WriteInt(p.y()); On 2013/01/07 22:51:35, piman wrote: > y u no do WriteParam(m, p.origin()) too ? Done. https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:266: ParamTraits<gfx::Size>::Write(m, gfx::Size(p.width(), p.height())); On 2013/01/07 22:51:35, piman wrote: > WriteParam(m, p.size()); Done. https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:276: !ParamTraits<gfx::Size>::Read(m, iter, &size)) On 2013/01/07 22:51:35, piman wrote: > !ReadParam(m, iter, &size) Done. https://codereview.chromium.org/11617006/diff/31005/content/public/common/com... content/public/common/common_param_traits.cc:280: r->set_width(size.width()); On 2013/01/07 22:51:35, piman wrote: > r->set_size(size); Done.
https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... File content/public/common/common_param_traits.cc (right): https://codereview.chromium.org/11617006/diff/32001/content/public/common/com... content/public/common/common_param_traits.cc:184: (h && w > ((std::numeric_limits<int>::max() / 4) / h))) On 2013/01/08 00:08:45, Justin Schuh wrote: > On 2013/01/07 22:56:37, danakj wrote: > > On 2013/01/07 22:24:14, Justin Schuh wrote: > > > 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. > > > > 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. > > 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. Antoine provided some context in was lacking (in the future please assume I know less about graphics ;). I'll post another patch shortly.
Okay, trimmed to just checking for positive sizes. From the security end we'll see investigate additional restrictions in another CL. Everybody good?
LGTM https://codereview.chromium.org/11617006/diff/38004/content/common/cc_message... File content/common/cc_messages_unittest.cc (right): https://codereview.chromium.org/11617006/diff/38004/content/common/cc_message... content/common/cc_messages_unittest.cc:470: arbitrary_resource1.size = gfx::Size(3718, 12312); These don't seem needed then, now.
lgtm
https://codereview.chromium.org/11617006/diff/38004/content/common/cc_message... File content/common/cc_messages_unittest.cc (right): https://codereview.chromium.org/11617006/diff/38004/content/common/cc_message... content/common/cc_messages_unittest.cc:470: arbitrary_resource1.size = gfx::Size(3718, 12312); On 2013/01/08 04:06:06, danakj wrote: > These don't seem needed then, now. Good point.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/11617006/36011
Message was sent while issue was closed.
Change committed as 175496 |
