[API Proposal]: Allow Hook for Custom Resource Deserialization (BinaryFormatter)

[lonitra]

Background and motivation

When deserializing resources, there is no way to allow custom deserialization of embedded binary formatted resources leaving BinaryFormatter as the option to deserialize, which is known to be unsafe. Providing a hook to allow custom deserialization can help move away from the need for BinaryFormatter. In WinForms scenario, this would be helpful as it is expected that most WinForms apps will not be able to run without BinaryFormatter, so this would allow an opportunity to take care of resource deserialization without BinaryFormatter.

API Proposal

The hook would be a new configuration that would allow getting in early enough to provide deserialization support. The configuration would be something like switch name: System.Resources.BinaryFormat.Deserializer switch value: CustomBinaryFormatDeserializerFullyQualifiedName

API Usage

Custom binary deserializer implementation:

namespace System.Windows.Forms;

class BinaryFormatDeserializer
{
    // Needs parameter Type and cannot be a generic since type information is only known at runtime.
    // type may also be null in cases where resources did not have type information in the resx
    public object? Deserialize(Stream stream, Type? type) { }
}

runtimeconfig.json:

"configProperties": {
    "System.Resources.BinaryFormat.Deserializer": "BinaryFormatDeserializerFullyQualifiedName",
}

As payloads are being encountered during deserialization process in both ResourceReader/DeserializingResourceReader the custom deserializer type would be created via Activator.CreateInstance and reflected to get Deserialize() and call it. Note that if custom binary deserializer is present, ResourceReader/DeserializingResourceReader should not use BinaryFormatter as a fallback.

Alternative Designs

Some other design considered were:

• Instance API. This would require code changes in the caller and ultimately would not help with existing assemblies
• Static API. This could avoid code changes, but there is a risk it might not get called early enough to provide deserialization support

Risks

No response

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

[jkotas]

cc @adamsitnik @GrabYourPitchforks

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-resources See info in area-owners.md if you want to be subscribed.

[adamsitnik]

cc @bartonjs

[steveharter]

Prototype at https://github.com/dotnet/runtime/compare/main...steveharter:runtime:AddHook

[jkotas]

Prototype at https://github.com/dotnet/runtime/compare/main...steveharter:runtime:AddHook

Should this rather be a public API and the callback to be per instance?

Having config switch that affects are ResourceReaders in the app is inherently less secure since you do not know what all possible ResourceReaders the app can use.

[ericstj]

@jkotas I think @lonitra already mentioned public instance API in the alternatives and why that doesn't help here. I don't think folks are looking at this as making a significant change in how secure ResourceReaders use of BinaryForrmatter is. Instead it's a way to plug-in something that understands the BinaryFormatted resources without enabling BinaryFormatter for everyone. The callback is only called for BinaryFormatted resources - it's not a new general purpose hook.

[jkotas]

I think @lonitra already mentioned public instance API in the alternatives and why that doesn't help here.

Yes, it says that the instance method does not help. I would like to see more detailed explanation of why it does not help, and discussion of the down-sides of the proposed design.

Based on offline discussion, my understanding is that this hook is specifically targeted at enabling deserialization of fully-trusted embedded resources auto generated by WinForms designer. Could you please share an example of what the auto-generated code that we are concerned about looks like to show that it is not possible to attach the behavior via an instance method?

[JeremyKuhne]

I think @lonitra already mentioned public instance API in the alternatives and why that doesn't help here.

Yes, it says that the instance method does not help. I would like to see more detailed explanation of why it does not help, and discussion of the down-sides of the proposed design.

@jkotas the problem with an instance method is that we don't have a good place to inject it. We don't control when applications will create control instances and, more importantly, we can't dependably get ahead of other embedded resource access an app or it's dependencies might do.

[jkotas]

my understanding is that this hook is specifically targeted at enabling deserialization of fully-trusted embedded resources auto generated by WinForms designer

Is this correct statement?

Could you please share a typical example of this WinForms designer generated code? The examples of WinForms designer generated code that I was able to find start with some sort of user provided context or Type. We may be able to use it to attach the deserialization callback.

BTW: What would a trim compatible variant of this problematic pattern look like? I am wondering whether it can inform the design.

[JeremyKuhne]

@lonitra can you provide a code sample?

@jkotas if you're suggesting that this be some sort of mechanism that the designer plugs into that won't really work as it presumes that all existing forms and other controls are able to be reopened, regenerated, and recompiled. Not really attainable for many LOB applications.

As far as trimming is concerned, I'm investigating various options. One possibility discussed with the linker team is to have a source generator that crawls for [Serializable] types at compile time to generate the necessary references to keep enough rooted to deserialize correctly (and potentially in the form of a binder that "registers" <T>s and only allows those types to be reconstituted).

[lonitra]

The designer's auto generated code depends on what is on the form, but typically it looks something like the following:

partial class Form1
{
    private System.ComponentModel.IContainer components = null;

    private void InitializeComponent()
    {
        components = new System.ComponentModel.Container();
        System.ComponentModel.ComponentResourceManager resources = new System.ComponentModel.ComponentResourceManager(typeof(Form1));
        ListViewItem listViewItem1 = new ListViewItem("This is a ListViewItem with an image", 0);
        imageList1 = new ImageList(components);
        listView1 = new ListView();
        SuspendLayout();
        // 
        // imageList1
        // 
        imageList1.ColorDepth = ColorDepth.Depth32Bit;
        imageList1.ImageStream = (ImageListStreamer)resources.GetObject("imageList1.ImageStream");

        // more initialization code omitted for brevity
    }

    private ImageList imageList1;
    private ListView listView1;
}

InitializeComponent() will run when a form starts up. In this example the form had an ImageListStreamer which is one of the known types that gets serialized into the resources using the BinaryFormatter. Designer deserializes it from resources via ResourceManager.GetObject()

[jkotas]

System.ComponentModel.ComponentResourceManager(typeof(Form1));

Can ComponentResourceManager get the custom deserializer from Form1, e.g. via a new interface? This interface can be implemented by Control and potentially overridden by user forms or controls (with trim compatible implementation).

[jkotas]

@ericstj mentioned to me offline that the actual motivation behind this proposal is do minimal change possible to make WinForms private copy of BF work. I can get behind that. It would be nice if the OP mentioned that.

If we are going with minimal change possible, I do not see why we need to invent a new contract between the resource reader and the private binary formatter. The minimal change in CoreLib can be something like this: https://github.com/dotnet/runtime/compare/main...jkotas:runtime:config

[JeremyKuhne]

If we are going with minimal change possible, I do not see why we need to invent a new contract between the resource reader and the private binary formatter.

We want to be able to plug in any implementation and fully customize what it is doing. The defaults for the BinaryFormatter are not the safest and locking it down some (binder, rejecting unsafe records, etc.) will still work for the vast majority of our scenarios. We can iterate over time to provide less and less of a default handler state.

One example: Our prescribed, safer, way to do things is with TypeConverters. You can't add or change [TypeConverter] on existing types, even if you control them, as you'll break things all over. One of the workarounds we're investigating is still using type converters (via project specification or somesuch) and smuggling the context to rehydrate in an IObjectReference type that would rehydrate. That could be a configurable thing, where we only allow a fixed set of known .NET types (such as List<int>) and this special type converter rehydrator.

Can ComponentResourceManager get the custom deserializer from Form1, e.g. via a new interface? This interface can be implemented by Control and potentially overridden by user forms or controls (with trim compatible implementation).

It wouldn't address the inability to redesign everything in an app's compilation.

As far as trimming goes, while it would be nice to express things for it in InitializeComponent, it isn't particularly simple. The linker's analysis isn't recursive. We would have to walk the entire object graph and "root" every type in it.

[jkotas]

The defaults for the BinaryFormatter are not the safest and locking it down some (binder, rejecting unsafe records, etc.) will still work for the vast majority of our scenarios. We can iterate over time to provide less and less of a default handler state.

Why do you want to do all this for fully trusted resources? To me, it sounds like a ton of unnecessary perf overhead and source of compatibility issues.

[JeremyKuhne]

Why do you want to do all this for fully trusted resources? To me, it sounds like a ton of unnecessary perf overhead and source of compatibility issues.

So far, it looks like there is no perf overhead, and there is always the full package to fall back on.

It gives us defense-in-depth and allows us to move away from unbounded deserialization. It gives us more flexibility in implementing new features (such as trim support) and potentially implement compat or other security fixes. It allows customers to provide their own policy if they really want to.

What we're suggesting here isn't asking for complicated new API, it is just "give us the context you have, and we'll give you the results". I don't know that restricting it to the BinaryFormatter type buys enough to exclude flexibility.

[jkotas]

I do not see the defense in depth angle. What is the threat that you would be protecting against when deserializing embedded resources?

The proposed config switch is incompatible with trimming. It will produce trim warnings by design. It won't be usable for a production quality trimming support.

I understand that passing the extra type is not super complicated, but it is more complicated than it needs to be to make the private copy of BF in WinForms work. I would like to keep it as simple as possible unless there is a good reason to make it more complicated. What are you going to do with the extra type?

[JeremyKuhne]

I do not see the defense in depth angle. What is the threat that you would be protecting against when deserializing embedded resources?

Having binary formatted data that doesn't contain member type info is dangerous, for example. This would prevent that. It would allow guarding against supply-chain attacks- harder to have resources given back to you with embedded malicious objects.

The proposed config switch is incompatible with trimming. It will produce trim warnings by design. It won't be usable for a production quality trimming support.

Is that any different than any other solution? Our intent is to not allow any of the infra here to be trimmed. Just too important for WinForms projects.

I understand that passing the extra type is not super complicated, but it is more complicated than it needs to be to make the private copy of BF in WinForms work. I would like to keep it as simple as possible unless there is a good reason to make it more complicated. What are you going to do with the extra type?

Pre-vet that we're actually creating the expected type at the minimum. We'd ideally want all of our code to express intent here and be able to actually follow through on it. Emitting GetResource<T> in InitializeComponent for example. We don't have APIs like that on the ResourceManager yet, and I might not get buy-in for that, but not doing this here would require an additional change if I do.

[bartonjs]

I believe this is no longer necessary, as we've moved off of BinaryFormatter for reading back objects from resources (provided the caller uses DeserializingResourceReader), so I'm going to go ahead and close it.

If I'm wrong, and there's still a need, we can reopen it.