XenocodeRCE / neo-ConfuserEx

Updated ConfuserEX, an open-source, free obfuscator for .NET applications
http://yck1509.github.io/ConfuserEx/
Other
746 stars 89 forks source link

TypeScrambler fails when scrambling generic class #5

Open mkaring opened 6 years ago

mkaring commented 6 years ago

Describe the bug The type scrambler causes and error together with the renaming when handling methods in a generic class, because it does not handle the generic parameters of the class itself properly.

To Reproduce Steps to reproduce the behavior:

  1. Have an assembly that contains a class like this: https://github.com/mkaring/ConfuserEx/blob/feature/typescrambler_unittest/Tests/TypeScrambler/GenericClass.cs
  2. Try to have the type scrambler process it. Be sure that the Confuser.Rename assembly is loaded and active as well, because it's AnalyzePhase raises the error.

Expected behavior Should work without errors.

Additional context My best guess is that the scrambler does not consider the generic parameter of the class at this point. The result of the UnitTest, including the error is here: https://ci.appveyor.com/project/mkaring/confuserex/build/13#L289

XenocodeRCE commented 6 years ago

Error is raised by Confuser.Protections.TypeScramble.Scrambler not Confuser.Renamer ?

TypeSig.ScopeType is null and because there is no exception handling here it throw an error

mkaring commented 6 years ago

Ah yes. As an attempted work around I added a null Handling there and had the method return null. And that resulted in the error I described. I forgot about that, sorry.

Either way it should work. :wink:

XenocodeRCE commented 6 years ago

I don't think TypeScrambler can handle generics, because it transforms param and so to generic. I used a built-int Boolean method to check if Type contains generic types, this seems to work for the moment.

mkaring commented 6 years ago

I'll run it against my test cases and get back to you.

XenocodeRCE commented 6 years ago

From my tests it seems is simply not compatible with generic types at all.

class Test<T>
    {
        T _value;

        public Test(T t)
        {
            // The field has the same type as the parameter.
            this._value = t;
        }

        public void Write()
        {
            Console.WriteLine(this._value);
        }
    }

    class Program
    {
        static void Main(string[] args)
        {

            Test<int> test1 = new Test<int>(5);
            // Call the Write method.
            test1.Write();

            // Use the generic type Test with a string type parameter.
            Test<string> test2 = new Test<string>("cat");

            test2.Write();

            Console.ReadKey();
        }
    }

will inevitably fail and throw an error when TypeScrambler is selected. It seems to be a very sloppy project. I might remove it and prefer to code my own from scratch.

mkaring commented 6 years ago

Where did you get this thing from? I only got it in my repository from yours.

alexmurari commented 6 years ago

@mkaring he got it from here: https://github.com/BahNahNah/ConfuserExPlugins

mkaring commented 6 years ago

The problem is still present. The issue occurs because that ConvertToGenericIfAvalible method returns the original type signature in case in case the scrambling of the type should fail. Now the methods calling this, get the original type signature that also contains a generic type. So they assume the method is being scrambled and at this point it all fails.

I think the basic method on how this works is the problem here. The analyzing part of the scrambler needs to annotate the methods that are selected for scrambling and the rewriter phase needs to update them based on the the annotations.

Also it is absolutely required to rename the Prepair functions to Prepare.

All in all I think most of the scrambler is usable. Just the part how the phases communicate needs to be improved to some extend.

XenocodeRCE commented 6 years ago

Last commit should have fixed this bug @mkaring thanks to 2 PR

mkaring commented 6 years ago

That is still not it. It still messes up the analysis stage of the renamer, because the method references don't match anymore.

See: https://ci.appveyor.com/project/mkaring/confuserex/build/22#L384

pigeonhands commented 5 years ago

Bit late but ive started a rewrite to make it a bit more readable. https://github.com/BahNahNah/ConfuserExPlugins/tree/rewrite

For whatever reason its corrupting the assembly and debugging it isnt giving me much help as to why. Removing this line stops it crashing so its from applying the generics incorrectly. https://github.com/BahNahNah/ConfuserExPlugins/blob/rewrite/TypeScramble/ScramblePhase.cs#L24

Its late so its probably just a stupid mistake somewhere, ill try and fix it when i get time.

Edit: fixed. Working . And I've added some more stuff. On the main branch now. https://github.com/BahNahNah/ConfuserExPlugins/

XenocodeRCE commented 5 years ago

People, we might just need a Pull Request here no ? Otherwise we just might put that protection in the bin ?