Mash and aura need to validate properties before using them |
||||||||||
Issue descriptionMash has a bunch of code that assumes a shared property is of a certain type (see https://chromium.googlesource.com/chromium/src/+/master/ash/mus/property_util.cc and https://chromium.googlesource.com/chromium/src/+/master/services/ui/public/cpp/property_type_converters.cc ). The way the code is currently structured ash is likely to crash if a property is requested and the client didn't fill in the right type. For example, the type converter for a rect assumes the array size is 16 bytes, if not, it crashes. This code needs to be audited and ensure we don't crash if a bogus value is supplied. I tend to think the best way to do this is by adding a function to WindowTreeClientDelegate that is called anytime a shared property is encountered from the server. The delegate function would be responsible for validating and potentially rejecting the value. If we do this it means ash can continue assuming all values have been validated as any invalid value would have been rejected. This has security ramifications, so Tom, please tag appropriately.
,
Nov 18 2016
,
Feb 13 2017
,
Feb 16 2017
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19e1e20024109653910be72bd7ccbcc866a47c26 commit 19e1e20024109653910be72bd7ccbcc866a47c26 Author: erg <erg@chromium.org> Date: Fri Feb 24 23:24:12 2017 aura-mus: Validate incoming window properties. This adds a validator function to the integral type version of PropertyConverter::RegisterProperty() and rejects changes if the incoming value, passed to the validator, returns false. BUG= 654924 Review-Url: https://codereview.chromium.org/2702423004 Cr-Commit-Position: refs/heads/master@{#452990} [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ash/mus/window_manager.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ash/public/cpp/BUILD.gn [add] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ash/public/cpp/shelf_types.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ash/public/cpp/shelf_types.h [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ui/aura/mus/property_converter.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ui/aura/mus/property_converter.h [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ui/aura/mus/property_converter_unittest.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ui/aura/mus/window_tree_client_unittest.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ui/views/mus/mus_client.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ui/wm/core/shadow_types.cc [modify] https://crrev.com/19e1e20024109653910be72bd7ccbcc866a47c26/ui/wm/core/shadow_types.h
,
Feb 27 2017
I believe everything needed here is actually done. The two things that I thought were left after my patch on Friday were additional validation in PropertyConverter and propagating an error back to the window server so that it could terminate a bad client. Going through property_converter.cc, it looks like msw@ already added all the validation needed to prevent crashing: actually checking the size of the incoming data field. After that, we pass data to the various mojo::ConvertTo<> methods which should cleanly deal with anything. We could perform additional validation so we could explicitly reject bad sizes or rects, for instance, but the data will get normalized into valid values anyway. The other thing I was going to do but am no longer sure is correct is closing the connection of a client which sets a property that another reports as bad. The idea was that if one process set a bad value, then it is acting hostile and should be killed. But this entire property interface is supposed to be an untyped property bag! And what happens when we inevitably get some sort of version skew, where a newer client sets a value which it thinks is valid but the receiver doesn't? (It also leaves open an untrusted hostile receiver to just report that a property is invalid to kill other processes.) Does that all sound correct?
,
Feb 28 2017
I think you got it all.
,
Apr 17 2017
,
May 30 2017
,
Aug 1 2017
,
Oct 14 2017
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by sky@chromium.org
, Nov 18 2016Summary: Mash and aura need to validate properties before using them (was: Mash needs to validate properties before using them)