mkaring / ConfuserEx

An open-source, free protector for .NET applications
https://mkaring.github.io/ConfuserEx/
MIT License
2.31k stars 350 forks source link

System.ArgumentNullException at System.Linq.Enumerable.ToList after maximum obfuscation #472

Closed bukkideme closed 2 years ago

bukkideme commented 2 years ago

Steps to Reproduce:

  1. Create a Winform project with Linq in it
  2. Build it
  3. Use Confuser with maximum level

When I run my Winform app, I get the following error:

System.ArgumentNullException: Value cannot be null. Parameter name: source at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source) at ZT:1<\&MNqBHXyn>AN|C!\&S60(.‫‮‌‏‪‮‏‌‏​‍‍‎‭‪‏‪‍‎‪‍‌‏‌‏‫‬‏‏‮(Object , MessageReceivedEventArgs )

As a workaround and acceptable protection, I use for now only these options, and I do not get error anymore running my EXE:

image

mkaring commented 2 years ago

It's very difficult to know what is going on here without more details on your application. What happens in the event handler that is causing the issue for your? Can you provide a minimal example to reproduce the issue?

bukkideme commented 2 years ago

Hi! As I see, the public static T DeserializeObject(string value) method from Nuget package Newtonsoft.Json 13.0.1 fails to deserialize my Class object from the json string if I use the ConfuserEx with Maximum protection settings.

ServerDataContainer serverData = JsonConvert.DeserializeObject(combined_D_andME_FileFromServer); Using ConfuserEx Maximum protection, serverData.DfileProperties becomes NULL, otherwise fine.

Then, this code line throws the error: DfileProperties = serverData.DfileProperties.ToList(); Where List DfileProperties = new List();. DFileProps is a class with a few properties, with Browsable props Componentmodel: [DisplayName("Idő"), Browsable(true)] public DateTime Timestamp { get; set; }

I cannot share my actual project, but I will try to make a minimalistic test project which shows the bug and can be shared. Thanks very much!

mkaring commented 2 years ago

Obfuscating the names of the properties may interfere with the serialization of Newtonsoft Json, unless the names of the properties are specified using the JsonProperty attribute. So this may very well be a hint to the issue already.

bukkideme commented 2 years ago

OK, I managed to find the problem. It is nothing to do with Newtonsoft.Json. I get data via TCP from a server, so I need to convert the incoming byte array to the Json string. Somehow ConfuserEx maximum protection ruins proper working of the Encoding.GetBytes(string s) method...So the Json deserializer gets null as result.

Code to reproduce the issue. Run the code below without protection, it works fine. Run ConfuserEx with Maximum protection, and you get NULL as result:

public class Data
    {
        [DisplayName("Idő"), Browsable(true)]
        public DateTime MyTime { get; set; }
        public List<int> AxleMasses { get; set; }

        public Data()
        {
            AxleMasses = new List<int>();
        }
    }
Data inputData = new Data();
            inputData.MyTime = DateTime.Now;

            //serverside process
            string jsonstringInput = JsonConvert.SerializeObject(inputData, Formatting.None);

            //simulate TCP server/client data transfer!
            Encoding enc = Encoding.GetEncoding("iso-8859-2");
            byte[] byteData = enc.GetBytes(jsonstringInput);
            string jsonstringOutput = enc.GetString(byteData);

            Data outputData = JsonConvert.DeserializeObject<Data>(jsonstringOutput);
            MessageBox.Show($"Simulate TCP incoming server data to client (this will be NULL): {outputData.MyTime}");

            Data outputDataOK = JsonConvert.DeserializeObject<Data>(jsonstringInput);
            MessageBox.Show($"Local jsonSerialize_deserialize test: {outputDataOK.MyTime}");
mkaring commented 2 years ago

Okay, so the problem is that Newtonsoft.Json determines based on the names of the attributes, what property of the source json file needs to be put in what property. The issue is that ConfuserEx renames the properties, meaning the names don't match with the properties anymore, breaking the serialization.

Your Json in the example looks something like this:

{
  "MyTime": "2022-04-06",
  "AxleMasses": []
}

Those property names have to line up with the names in the classes. ConfuserEx creates property names that aren't readable and not legal for property names in Json files. Even if Newtonsoft Json manages to create a file from this, it fails when deserializing the classes.

There are two ways to resolve that issue:

  1. Disable the name protection for all the fields and properties of the classes used for serialization. If they are all in one namespace, you can do this for the entire namespace.
  2. Alternatively you can add the JsonProperty attribute to the properties. Newtonsoft Json will prefer using those to determine the name. In that case the code would need to be altered like this:

    public class Data
    {
    [DisplayName("Idő"), Browsable(true)]
    [JsonProperty(nameof(MyTime))]
    public DateTime MyTime { get; set; }
    
    [JsonProperty(nameof(AxleMasses))]
    public List<int> AxleMasses { get; set; }
    
    public Data()
    {
    AxleMasses = new List<int>();
    }
    }
bukkideme commented 2 years ago

Thanks for the excellent explanation! Everything is clear :) Option number 1. is just fine for me, so I will go with it! This is very easy to use, and now my test program works just fine :)

image

mkaring commented 2 years ago

That is a working solution, how ever I would not recommend just disabling the the renaming completely. It's a strong protection as it irreversible alters your assembly.

If you for example have all your serialization classes in a namespace called My.Serialization.Classes you could a second rule with the pattern: namespace('My.Serialization.Classes') and remove the rename protection only for this namespace.

github-actions[bot] commented 2 years ago

This issue needs more information and has not had recent activity. Please provide the missing information or it will be closed in 7 days. Thanks!

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.