Examining https://github.com/XenocodeRCE/Noisette-Obfuscator...
Let's pick a random class:
class RenamingProtection
{
public static void RenameModule(ModuleDefMD module)
{
List<String> Methname = new List<string>(Noisette.Renaming.Method1);
List<String> Typename = new List<string>(Noisette.Renaming.Type1);
Random random = new Random();
foreach (TypeDef type in module.Types)
{
if (type.IsGlobalModuleType) continue;
string new_name_type = Typename[random.Next(0, Typename.Count)];
Typename.Remove(new_name_type);
type.Name = new_name_type;
foreach (MethodDef method in type.Methods)
{
foreach (Parameter arg in method.Parameters)
{
string new_name_param = Methname[random.Next(0, Methname.Count)];
Methname.Remove(new_name_param);
arg.Name = new_name_param;
}
if (!method.Body.HasVariables) continue;
foreach (var variable in method.Body.Variables)
{
string new_name_var = Methname[random.Next(0, Methname.Count)];
Methname.Remove(new_name_var);
variable.Name = new_name_var;
}
}
foreach (PropertyDef prop in type.Properties)
{
string new_name_property = Methname[random.Next(0, Methname.Count)];
Methname.Remove(new_name_property);
prop.Name = new_name_property;
}
foreach (FieldDef field in type.Fields)
{
string new_name_fields = Methname[random.Next(0, Methname.Count)];
Methname.Remove(new_name_fields);
field.Name = new_name_fields;
}
}
}
}###What is wrong?
- holy nested foreach loops!
- high cyclomatic complexity
- pattern of getting random element from list repeats itself over and over again
- not testable code
- very likely (I didn't look) this pattern of enumerating over the types and methods and variables and properties and so on repeats itself in other classes
- appears to have at least one bug I notice (properties with local variables)
###How to fix.
Let's start with introducing a base class for a visitor pattern that we can use here and in other places:
public class Visitor
{
public virtual void Visit(ModuleDefMD module)
{
VisitTypes(module.Types)
}
public virtual void VisitTypes(IList<PropertyDef> types)
{
foreach (TypeDef type in types)
{
VisitType(type);
}
}
public virtual void VisitType(TypeDef type)
{
VisitMethods(type.Methods);
VisitProperties(type.Properties);
VisitFields(type.Fields);
}
public virtual void VisitMethods(IList<Methods> methods)
{
foreach (var method in methods)
{
VisitMethod(method);
}
}
public virtual void VisitProperties(IList<PropertyDef> properties)
{
foreach (var prop in properties)
{
VisitProperty(prop);
}
}
public virtual void VisitFields(IList<FieldDef> fields)
{
foreach (var field in fields)
{
VisitField(field);
}
}
public virtual void VisitMethod(MethodDef method)
{
VisitParameters(method.Parameters);
VisitBody(method.Body)
}
public virtual void VisitParameters(ParameterList parameters)
{
foreach (Parameter arg in parameters)
{
VisitParameter(arg);
}
}
public virtual void VisitBody(CilBody body)
{
VisitVariables(body.Variables);
}
public virtual void VisitVariables(LocalList variables)
{
foreach (var variable in variables)
{
VisitVariable(variable);
}
}
public virtual void VisitParameter(Parameter arg)
{
}
public virtual void VisitVariable(VariableDef variable)
{
}
public virtual void VisitProperty(PropertyDef prop)
{
}
public virtual void VisitField(FieldDef field)
{
}
}Ideally you would have written that with associated unit tests for each method:
public class VisitorTests
{
[Test]
public void ACallToVisitShouldVisitTypes()
{
var visitor = A.Fake<Visitor>();
var module = A.Fake<ModuleDefMD>();
var types = A.Fake<IList<TypeDef>>();
A.CallTo(() => visitor.VisitTypes(A<IList<TypeDef>>._)).DoesNothing();
A.CallTo(() => module.Types).Returns(types);
A.CallTo(() => visitor.Visit(A<ModuleDefMD>._)).CallsBaseMethod();
visitor.Visit(module);
A.CallTo(() => visitor.VisitTypes(types)).MustHaveHappened();
}
[Test]
public void ACallToVisitTypesShouldVisitEachType()
{
var visitor = A.Fake<Visitor>();
var types = new[] { A.Fake<TypeDef>(), A.Fake<TypeDef>(), A.Fake<TypeDef>() };
A.CallTo(() => visitor.VisitType(A<TypeDef>._)).DoesNothing();
A.CallTo(() => visitor.VisitTypes(A<IList<TypeDef>>._)).CallsBaseMethod();
visitor.VisitTypes(types);
A.CallTo(() => visitor.VisitType(types[0])).MustHaveHappened();
A.CallTo(() => visitor.VisitType(types[1])).MustHaveHappened();
A.CallTo(() => visitor.VisitType(types[2])).MustHaveHappened();
}
}except... dnlib is difficult to mock, so you really need an adapter pattern around dnlib that fixes this... moving on...
###Now with visitor pattern
Single Responsibility Principle FTW.
class RenamingProtection
{
public static void RenameModule(ModuleDefMD module)
{
new RenamingProtectionVisitor(Noisette.Renaming.Method1, Noisette.Renaming.Type1, new Random()).Visit(module);
}
}
public class RenamingProtectionVisitor : Visitor
{
readonly List<string> _memberNames;
readonly List<string> __typeNamess;
readonly Random _random;
public RenamingProtectionVisitor(IEnumerable<string> memberNames, IEnumerable<string> typeNames, Random random)
{
//using constructor dependencies because that makes life easier
_memberNames = new List<string>(memberNames);
_typeNames = new List<string>(typeNames);
_random = random;
}
public override void VisitType(TypeDef type)
{
if (type.IsGlobalModuleType) return;
string new_name_type = _typeNames[random.Next(0, _typeNames.Count)];
_typeNames.Remove(new_name_type);
type.Name = new_name_type;
base.VisitType();
}
public override void VisitMethod(MethodDef method)
{
if (method.IsConstructor) continue;
if (!method.HasBody) continue;
if (method.FullName.Contains("My.")) continue; //VB gives cancer anyway
string new_name_meth = _memberNames[random.Next(0, _memberNames.Count)];
_memberNames.Remove(new_name_meth);
method.Name = new_name_meth;
base.VisitMethod(method);
}
public override void VisitParameter(Parameter arg)
{
string new_name_param = _memberNames[random.Next(0, _memberNames.Count)];
_memberNames.Remove(new_name_param);
arg.Name = new_name_param;
}
public override void VisitVariable(VariableDef variable)
{
string new_name_var = _memberNames[random.Next(0, _memberNames.Count)];
_memberNames.Remove(new_name_var);
variable.Name = new_name_var;
}
public override void VisitProperty(PropertyDef prop)
{
string new_name_property = _memberNames[random.Next(0, _memberNames.Count)];
_memberNames.Remove(new_name_property);
prop.Name = new_name_property;
}
public override void VisitField(FieldDef field)
{
string new_name_fields = _memberNames[random.Next(0, _memberNames.Count)];
_memberNames.Remove(new_name_fields);
field.Name = new_name_fields;
}
}Yeah that code is really not DRY :/
###Don't Repeat Yourself
The whole point of this visitor appears to be to set names with this bit:
var newname = collection[random.Next(0, collection.Count)];
collection.Remove(newname);
obj.Name = newname;Ignoring the fact this is a shitty way to do it (O(n^2), fails when Count is 0, etc), I'm counting the same 3 lines 6 times. Lets introduce an abstraction to make this better:
public interface IProducer<out T>
{
T Produce();
}
public class RandomElementProducer<T> : IProducer<T>
{
readonly List<T> _items;
int _element;
public RandomElementProducer(IEnumerable<T> items)
{
_items = new List<T>(items);
//https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle
var rng = new Random();
int n = _items.Count;
while (n > 1)
{
int k = rng.Next(n);
n--;
T value = _items[k];
_items[k] = _items[n];
_items[n] = value;
}
_element = -1;
}
public T Produce()
{
if(_element >= _items.Count)
{
throw new InvalidOperationException("No more elements to return");
}
_element++;
return _items[_element];
}
}I leave it to you to decide to use a cryptographically secure random number generator (http://stackoverflow.com/questions/3049467/is-c-sharp-random-number-generator-thread-safe/24648788#24648788).
class RenamingProtection
{
public static void RenameModule(ModuleDefMD module)
{
new RenamingProtectionVisitor(new RandomElementProducer(Noisette.Renaming.Method1), new RandomElementProducer(Noisette.Renaming.Type1)).Visit(module);
}
}
public class RenamingProtectionVisitor : Visitor
{
readonly IProducer<string> _memberNames;
readonly IProducer<string> _typeNames;
public RenamingProtectionVisitor(IProducer<string> memberNames, IProducer<string> typeNames)
{
_memberNames = memberNames;
_typeNames = typeNames;
}
public override void VisitType(TypeDef type)
{
if (type.IsGlobalModuleType) return;
type.Name = _typeNames.Produce();
base.VisitType();
}
public override void VisitMethod(MethodDef method)
{
if (method.IsConstructor) continue;
if (!method.HasBody) continue;
if (method.FullName.Contains("My.")) continue; //VB gives cancer anyway
method.Name = _memberNames.Produce();
base.VisitMethod(method);
}
public override void VisitParameter(Parameter arg)
{
arg.Name = _memberNames.Produce();
}
public override void VisitVariable(VariableDef variable)
{
variable.Name = _memberNames.Produce();
}
public override void VisitProperty(PropertyDef prop)
{
prop.Name = _memberNames.Produce();
}
public override void VisitField(FieldDef field)
{
field.Name = _memberNames.Produce();
}
}now, about that bug:
public class Visitor
{
//...
public virtual void VisitProperty(PropertyDef prop)
{
var m = prop.GetMethod;
if (m != null)
{
VisitMethod(m);
}
m = prop.SetMethod;
if (m != null)
{
VisitMethod(prop.SetMethod);
}
}
}
public class RenamingProtectionVisitor : Visitor
{
//...
public override void VisitProperty(PropertyDef prop)
{
prop.Name = _memberNames.Produce();
base.VisitProperty(prop);
}
}
Ok I'll reply here because Reddit is not my thing (I hope you don't mind!!)
•except... dnlib is difficult to mock, so you really need an adapter pattern around dnlib that fixes this... moving on...
->Ok you are not the first to talk about U.T but I do still think that U.T are useless when you are doing an obfuscator for .NET assemblies because everytime you code an actualy features you test it on numerous 'test-app' by running the Obfuscator.
•I leave it to you to decide to use a cryptographically secure random number generator
->I don't think having the same name twice is a security breach since ANYWAY you will not be able to recover original Types/Methods/NameSpaces names