dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
6.92k stars 1.15k forks source link

Deprecate ObjectDataProvider #1199

Open vatsan-madhavan opened 5 years ago

vatsan-madhavan commented 5 years ago

We should consider marking ObjectDataProvider as [Obsolete] in .NET Core 3.

In addition to this, we should also consider a global policy switch (an AppContext switch, for e.g.) that will disable <ObjectDataProvider .../> in XAML.

Or perhaps this should be reversed - off by default and provide an opt-in switch? I worry that this will have a huge app-compat impact.

/cc @GrabYourPitchforks, @rladuca, @SamBent /cc @grubioe

grubioe commented 5 years ago

WPF community - will removing ObjectDataProvider cause gaps with your application? We'd like your input to help formalize our approach. Thanks!

weltkante commented 5 years ago

We aren't using it in our codebase, but do you mind sharing the motivation for getting rid of it? I see you marked it as a security related issue, why is that?

Also I don't think the WPF community is represented well enough here to base the decision for removal in 3.0 off an issue discussion. You can of course try it (assuming you don't already get major objections here) and see if anyone else will run into problems, but you might have to add it back, especially since you put Desktop Framework compatibility on your 3.0 headlines.

vatsan-madhavan commented 5 years ago

https://www.blackhat.com/docs/us-17/thursday/us-17-Munoz-Friday-The-13th-Json-Attacks.pdf

ObjectDataProvider provides a a clever approach to execute arbitrary methods.

vatsan-madhavan commented 5 years ago

There are several approaches we can consider.

  1. Remove the type (heavy handed, will definitely break compat)
  2. Functionality no-op by default (i.e., the type will be available, can be compiled against etc., but the functionality will be a no-op), can be opted-in by applications that really want to use this type and related functionality
  3. Available by default (everything works as before), provide an opt-out switch (applications can opt-out of this in a way that makes all ObjectDataProvider related functionality a no-op)

My personal preference is (2): no-op by default with an opt-in.

weltkante commented 5 years ago

Thanks for the link, and I agree with (2) being a reasonable choice.

However I'm not sure I can follow that argument about security yet, as far as I'm concerned (might be missing something) ObjectDataProvider is just another way to do data binding, I don't see anything that makes it special by e.g. consuming external data.

If this is about XAML loading, if you can load attacker controlled XAML you can already construct arbitrary objects anyways and put it e.g. into the Source of a Binding extension. So getting rid of just ObjectDataProvider doesn't make anything safer.

As far as I'm concerned if you (as an attacker) can create an ObjectDataProvider you probably also can create other objects. The primary use case for XAML in WPF is not serialization, if people use it for that there's probably plenty of security issues, I don't think dumbing everything in WPF down to make XAML a "safe" serialization format is a good idea. (Again, I might be missing something, so ignore that if thats the case.)

vatsan-madhavan commented 4 years ago

We had some more discussion internally with @GrabYourPitchforks , and have decided to hold this for .NET Core 3.0 timeframe.

There is some hardening of deserialization in place since .NET Framework (https://github.com/dotnet/wpf/issues/1132), and we will be removing BinaryFormatteruses in .NET 5(https://github.com/dotnet/wpf/issues/1131).

We will revisit whether directly addressing ObjectDataProvidermakes sense, or whether #1131 etc. is sufficient, in .NET 5.

AceCoderLaura commented 2 years ago

An application markup as powerful as XAML is inevitably going to be able to do things that could compromise security. If you're loading arbitrary XAML then you have to accept that security is going to be a trade off no matter what you do.