mkaring / ConfuserEx

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

Improve reversible renamer protection with random password #393

Closed KvanTTT closed 3 years ago

KvanTTT commented 3 years ago

Enhancement and changes (the wiki should be updated):

fix #383

AppVeyorBot commented 3 years ago

:white_check_mark: Build ConfuserEx 843 completed (commit https://github.com/mkaring/ConfuserEx/commit/7c944b2047 by @KvanTTT)

AppVeyorBot commented 3 years ago

:white_check_mark: Build ConfuserEx 844 completed (commit https://github.com/mkaring/ConfuserEx/commit/796ec8440b by @KvanTTT)

mkaring commented 3 years ago

I still do not see the added benefit of mixing the seed in there. Requiring both the seed and the password to decrypt the names feels clumsy. Why don't you just use the password itself (or some hash value of it) as seed for the renamer?

Changing the behavior of the "seed" also something I'd like to avoid for the 1.x branch. It may break existing projects without the need for it.

KvanTTT commented 3 years ago

It allows hackers to reuse symbols map extracted from the first version of the software in further versions. It already works for other renaming modes except of reversible because they use random generator internally.

With random seed, hacker have to build the map after every obfuscation run (after the next version). It compilated the process and consumes more time (the more time is required to deobfuscate → the more successful obfuscation).

Also, I've implemented it in such a way that seed is not required if you don't want to use it: just add seed="" option to config and there is no need to fill seed field in UI during stack-trace deobfuscation. But by default, the seed is random because I think options should be strongest by default if they don't break anything.

Maybe somebody else should express his point about the current seed and reversible renaming implementation. @ElektroKill what do you think about it?

griknok commented 3 years ago

I love the concept of reversible renaming introducing randomness to protect against hackers who've mapped out previous versions. But not using this seed concept.

The seed attribute seems like it would need documentation explaining the difference between not supplying it, supplying it with the value "", or supplying it with a value but then needing to use both the password and the seed to reverse the renaming.

If the end result is make the password random, why not have a random password generator? Either use password="myPassword" or generate-password="true" and that writes a generated password (a hash or a GUID or whatever) to a file like your implementation is doing with the generated seed value. Now reversing is straight forward, and the attribute name is self-documenting (except for explaining the password will be output to a file, or to the console)

KvanTTT commented 3 years ago

Great, your suggestion looks more clear, convenient, and back compatible. I mean just the new option like generatePassword.

mkaring commented 3 years ago

I like @griknok idea. We still got one password that may be used to reverse the renaming but sufficient randomness to get a good rename. Adding a single new parameter to the rename protection isn't a big deal and doesn't break compatibility in any direction.

KvanTTT commented 3 years ago

Ok, will remake it closer to next weekend.

AppVeyorBot commented 3 years ago

:white_check_mark: Build ConfuserEx 847 completed (commit https://github.com/mkaring/ConfuserEx/commit/180c2ee598 by @KvanTTT)

KvanTTT commented 3 years ago

Done.

KvanTTT commented 3 years ago

I have one question: how to name the new option? genPassword would be consistent with the current naming (renPublic, renPdb), generatePassword would be more cleaner.

AppVeyorBot commented 3 years ago

:white_check_mark: Build ConfuserEx 849 completed (commit https://github.com/mkaring/ConfuserEx/commit/057a7e7395 by @KvanTTT)

AppVeyorBot commented 3 years ago

:white_check_mark: Build ConfuserEx 850 completed (commit https://github.com/mkaring/ConfuserEx/commit/12ab7b938f by @KvanTTT)

AppVeyorBot commented 3 years ago

:white_check_mark: Build ConfuserEx 852 completed (commit https://github.com/mkaring/ConfuserEx/commit/625e9dcbb6 by @KvanTTT)

mkaring commented 3 years ago

I like generatePassword for the name of the new operation. It's clear and descriptive.

KvanTTT commented 3 years ago

Ok, then it's done, I've fixed your notes.

wmjordan commented 3 years ago

I suggest changing the following to log-like appandage for the password file.

File.WriteAllText(path, password);

to something like the following:

File.AppendAllText(path, $"{outputFileTime.ToString("yyyy-MM-dd hh:mm:ss")}\t{outputFileName}\t{outputFileVersion}\t{outputFileSize}\t{password}");

So we can keep passwords for various versions in a single file. If multiple output files are placed into the same folder, it is also possible to distinguish them in the entries.

KvanTTT commented 3 years ago

Not sure about your suggestion:

  1. symbols.map is also being created every run and it does not contain date/version info. The current scheme with password is consistent with symbols.map.
  2. Plain password just can be read by a third-party tool without parsing.
mkaring commented 3 years ago

The way it's working now matches the symbols.map. So it's good the way it is. Aggregating the passwords into a single file can be done outside of ConfuserEx very easily, splitting the file to extract the required password is significantly more difficult.