Remove BinaryFormatter usage from System.ComponentModel.TypeConverter

[GrabYourPitchforks]

Ref:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesigntimeLicenseContextSerializer.cs

Lines 26 to 45 in af828ae

	public static void Serialize(Stream o, string cryptoKey, DesigntimeLicenseContext context)
	{
	IFormatter formatter = new BinaryFormatter();
	formatter.Serialize(o, new object[] { cryptoKey, context._savedLicenseKeys });
	}
	internal static void Deserialize(Stream o, string cryptoKey, RuntimeLicenseContext context)
	{
	IFormatter formatter = new BinaryFormatter();
	object obj = formatter.Deserialize(o);
	if (obj is object[] value)
	{
	if (value[0] is string && (string)value[0] == cryptoKey)
	{
	context._savedLicenseKeys = (Hashtable)value[1];
	}
	}
	}

This issue tracks the removal and replacement of this code per the BinaryFormatter obsoletion plan.

[ghost]

Tagging subscribers to this area: @safern
Notify danmosemsft if you want to be subscribed.

[pgovind]

I looked at the BinaryFormatter usage in this class and think this should be easily convertable to S.T.Json. Here's what I have working locally:

        public static void Serialize(Stream o, string cryptoKey, DesigntimeLicenseContext context)
        {
            if (AppContext.TryGetSwitch("System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization", out bool isEnabled) && isEnabled)
            {
                IFormatter formatter = new BinaryFormatter();
#pragma warning disable SYSLIB0011 // Issue https://github.com/dotnet/runtime/issues/39293 tracks finding an alternative to BinaryFormatter
                formatter.Serialize(o, new object[] { cryptoKey, context._savedLicenseKeys });
#pragma warning restore SYSLIB0011
            }
            else
            {
                using (BinaryWriter writer = new BinaryWriter(o, encoding: Text.Encoding.UTF8, leaveOpen: true))
                {
                    writer.Write(cryptoKey);
                    string licenseKeys = System.Text.Json.JsonSerializer.Serialize(context._savedLicenseKeys);
                    writer.Write(licenseKeys);
                }
            }
        }


        internal static void Deserialize(Stream o, string cryptoKey, RuntimeLicenseContext context)
        {
            if (AppContext.TryGetSwitch("System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization", out bool isEnabled) && isEnabled)
            {
#pragma warning disable SYSLIB0011 // Issue https://github.com/dotnet/runtime/issues/39293 tracks finding an alternative to BinaryFormatter
                IFormatter formatter = new BinaryFormatter();

                object obj = formatter.Deserialize(o);
#pragma warning restore SYSLIB0011

                if (obj is object[] value)
                {
                    if (value[0] is string && (string)value[0] == cryptoKey)
                    {
                        context._savedLicenseKeys = (Hashtable)value[1];
                    }
                }
            }
            else
            {
                using (BinaryReader reader = new BinaryReader(o, encoding: Text.Encoding.UTF8, leaveOpen: true))
                {
                    string streamCryptoKey = reader.ReadString();
                    if (streamCryptoKey == cryptoKey)
                    {
                        string licenseKeys = reader.ReadString();
                        context._savedLicenseKeys = System.Text.Json.JsonSerializer.Deserialize<Hashtable>(licenseKeys);
                    }
                }
            }

The only wrinkle I can think of here is if the output Stream from a Serialize call (that uses binary formatter) is stored on disk and later another exe tries to Deserialize that stream without the EnableUnsafeBinaryFormatterSerialization flag set. Setting that flag is an easy fix though, so this should be ok IMO. @GrabYourPitchforks: See anything else I'm missing here?

Also, to test the Deserialize method here, I'd like to use InternalsVisibleTo. To access the Deserialize method from public API, I'd have to go 4-5 layers up to LicenseManager and even then it's not very straightforward. Other options are Reflection or #if defs. Anyone prefer that I don't use InternalsVisibleTo to test this change?
cc @maryamariyan @tarekgh @safern @michaelgsharp

[safern]

Also, to test the Deserialize method here, I'd like to use InternalsVisibleTo. To access the Deserialize method from public API, I'd have to go 4-5 layers up to LicenseManager and even then it's not very straightforward. Other options are Reflection or #if defs. Anyone prefer that I don't use InternalsVisibleTo to test this change?

@ericstj what are the guidelines for InternalsVisibleTo. I've seen we use them for extensions tests for example, but I don't know if Reflection is preferred?

[ericstj]

Extensions tests use it for legacy reasons. In general we try to avoid testing using internals.

Are you sure that changing the format here is even legitimate? It looks to me like the entire scenario for this type is to consume external data, which is presumably only of that specific format. If you want to change the format you need to somehow record that in the data itself. One way of doing this is with some sort of header on the data that can inform the consuming code which implementation to use. Here you might be able to use a sentinel data that is unlikely/impossible to show up in the binaryFormatted data, use that as the header and probe for it.

I see:

                       context._savedLicenseKeys = System.Text.Json.JsonSerializer.Deserialize<Hashtable>(licenseKeys);

HashTable is a loosely typed collection. It looks to me like this might be string/string, but you should double check that. I think there are better ways to serialize this /cc @layomia

Finally, who calls serialize? I seem to remember it's a commandline tool that might only exist on desktop. Will customers have a way to use this technology and leverage your change of format?

[pgovind]

If you want to change the format you need to somehow record that in the data itself

Why though? IIUC, customers use Binary Formatter right now for both Serialize and Deserialize. From .NET 6, we can turn on JsonSerializer as the default. Those who want to stick to BinaryFormatter can just set the EnableUnsafeBinaryFormatterSerialization flag in their csproj right? I'm missing how this change breaks someone?

but you should double check that

Yup, I did double check that the HashTable can only contain strings.

who calls serialize

(Caveat that I don't understand this library very well). No idea. AFAICT, there are no calls to Serialize in the code base. It must be public API that customers call directly.

Will customers have a way to use this technology and leverage your change of format?
I think so. We're providing a way to go back to the old behavior for the customers that need it, so I don't see why not.

[GrabYourPitchforks]

I agree with Eric's comment that there's no need to use System.Text.Json for this scenario. Looking through the code, the hash set is a collection of string keys with string values. So this could be done as:

public void Serialize(string cryptoKey, Hashset data, BinaryWriter writer)
{
    writer.Write(cryptoKey);
    writer.Write(data.Count);
    foreach (var kvp in data)
    {
        writer.Write(kvp.Key);
        writer.Write(kvp.Value);
    }
}

public void Deserialize(BinaryReader reader, string cryptoKey, Hashset data)
{
    if (reader.ReadString() != cryptoKey) { /* FAIL */ }
    int numEntries = reader.ReadInt32();
    while (int i = 0; i < numEntries; i++)
    {
        data.Add(reader.ReadString(), reader.ReadString();
    }
}

Those who want to stick to BinaryFormatter can just set the EnableUnsafeBinaryFormatterSerialization flag in their csproj right? I'm missing how this change breaks someone?

The EnableUnsafeBinaryFormatterSerialization flag should only control whether BinaryFormatter is allowed to run within the context of the current application or whether it throws an exception on use. It shouldn't control whether any .NET component calls into it. They're separate concerns. I'd recommend using a different switch for this.

One way of doing this is with some sort of header on the data that can inform the consuming code which implementation to use. Here you might be able to use a sentinel data that is unlikely/impossible to show up in the binaryFormatted data, use that as the header and probe for it.

Careful with this approach. We mustn't end up in a situation where the payload itself can say "I'm BinaryFormatter-formatted!" and activate the insecure code paths without the app developer having explicitly stated "I consent to allowing LicenseManager to do this."

[pgovind]

no need to use System.Text.Json for this scenario. Looking through the code, the hash set is a collection of string keys with string values. So this could be done as

Ah indeed, we can serialize the individual values in the HashTable directly. I was only putting up a quick first draft.

They're separate concerns. I'd recommend using a different switch for this

Interesting. I didn't consider that, but it makes sense. Would defining a flag like this work? (copied from S.P.CoreLib)

    internal static partial class LocalAppContextSwitches
    {
        private static int s_enableUnsafeUTF7Encoding; // rename to something like enableBinaryFormatterUse
        public static bool EnableUnsafeUTF7Encoding
        {
            [MethodImpl(MethodImplOptions.AggressiveInlining)]
            get => GetCachedSwitchValue("System.Text.Encoding.EnableUnsafeUTF7Encoding", ref s_enableUnsafeUTF7Encoding);
        }
}

[ericstj]

We mustn't end up in a situation where the payload itself can say "I'm BinaryFormatter-formatted!" and activate the insecure code paths without the app developer having explicitly stated "I consent to allowing LicenseManager to do this."

Of course the developer would have to consent to this same as any other BF usage. I’m pointing out that you are really only changing one side of a data format without thinking about updating whatever writer is out their or helping people with migration.

I'm missing how this change breaks someone?

Have you understood the end to end use case for this type? Where do people get the files they are feeding into it? Perhaps you can look at some of the usage in desktop to get a better idea. Cc @OliaG

[GrabYourPitchforks]

If there's a scenario where core writes the data file and netfx reads it, then absolutely we should consider porting to netfx whatever changes we make here. Regardless, it's fine for the payload to say "I'm using <xyz> serialization technology" (there are well-known markers for BinaryFormatter), and for the consumer to switch on that marker, as long as the consumer has opted in to understanding it. The point of these exercises is that the BinaryFormatter-related code paths remain completely deactivated unless the consumer has opted in to reactivating them. Here, the "consumer" is the app developer. The payload itself may be untrusted and must not be the component in charge of this decision.

[ericstj]

Right if the payload said it needed BinaryFormatter and the app didn’t allow it that should be an exception. That’s the whole premise of this work.

[ericstj]

Just to drop some pointers, the caller to this Serialize method is
https://docs.microsoft.com/en-us/dotnet/framework/tools/lc-exe-license-compiler
I don't believe this is in reference-source or in the open anywhere, but you can find it under ndp/fx/src/runtimetools/LicenseCompiler/LicenseCompiler.cs in a .NETFramework enlistment. The tool ships with .NETFramework SDK tools.

The tool is not supported on .NETCore:
dotnet/winforms#1462

The current workaround is for folks to build the lcix file on .NETframework and use it on core, as mentioned in that issue.

There is MSBuild integration:
https://github.com/dotnet/msbuild/blob/83cd7d4e36b71d5b2cefd02cb9a5a58d27dd6a75/src/Tasks/Microsoft.Common.CurrentVersion.targets#L3269

And it is passed to the compiler and embedded into the assembly:
https://github.com/dotnet/roslyn/blob/afd10305a37c0ffb2cfb2c2d8446154c68cfa87a/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L127

So, we have a compatibility issue here consuming binaries which embedded the binary-formatted format. We also don't have a path forward since we aren't building a new version of this tool on .NETCore in order to produce the new format.

[pgovind]

it's fine for the payload to say "I'm using serialization technology" (there are well-known markers for BinaryFormatter)

Just to be clear, the UX in my mind here was : Call Deserialize in an app => it throws an error saying "BF is not supported by default, turn on EnableUnsafeBinaryFormatterSerialization" => dev turns on the EnableUnsafeBinaryFormatterSerialization flag. We don't really need to parse the payload here for BF markers right?

So, we have a compatibility issue here consuming binaries which embedded the binary-formatted format. We also don't have a path forward since we aren't building a new version of this tool on .NETCore in order to produce the new format.

Ah I see, nice find. That begs the question, is deprecating BF considered a big enough ask to make a change in .NET Framework code?

If we assume that .NETFramework remains untouched, we only need to consider the case where .NET Core tries to consume a resource from Framework that is binary-formatted. If EnableUnsafeBinaryFormatterSerialization flag is set, .NET Core will be able to consume the resource, so this should be ok for .NET 6.

The real concern is what we want to do when BF support is completely dropped for .NET 7? It would be simple to write a console app/tool that converts a Binary-Formatted resource into something Deserialize can consume. Have we done something like this before?

[ericstj]

We don't really need to parse the payload here for BF markers right?

No, that was only if we decided to change the format that's written. If we did that we'd need to be able to see if the payload was BF or something else.

is deprecating BF considered a big enough ask to make a change in .NET Framework code?

Not likely, we aren't deprecating there.

so this should be ok for .NET 6.

Yeah, this thing is effectively broken unless you turn on that flag. That's probably OK for now.

what we want to do when BF support is completely dropped for .NET 7?

I don't think we'll touch existing binaries. Those will need the flag. Also, I've heard folks say that we will pull BF but may have a package that folks can use to add it back if they choose to. This might just be a rumor ;)

Have we done something like this before?

This is sort of like the path we are on for resources. I think the complete solution here would be to bring back the code for the LC task to .NETCore: porting it to use a different serialization technology (eg: maybe just write those key-value pairs). This is similar to the work we did in 5.0 for resources in MSBuild. The nice thing about this technology is that it seems the file checked into the user's source is not a BinaryFormatted payload. According to the docs it is an .licx text file. This of course would need some amount of support from WinForms team who owns the LicenseCompiler on .NETFramework, but we could probably help.