Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve reversible renamer protection with random password #393

Merged

Conversation

KvanTTT
Copy link

@KvanTTT KvanTTT commented Aug 1, 2021

Enhancement and changes (the wiki should be updated):

  • Output seed file is generated in the output directory every obfuscation run

  • seed="" now is hardcoded seed, not random

  • For decoding stack traces with reversible renaming mode you need both password and seed. If you don't want to use random seed just add seed="" (or any other hardcoded value) to .crproj config.

  • New boolean option generatePassword that forces obfuscator to generate and use a new password on every run. The password is saved to password file in the output directory. Also, it's being generated if mode is reversible and password is not specified.

fix #383

@AppVeyorBot
Copy link

Build ConfuserEx 843 completed (commit 7c944b2047 by @KvanTTT)

@KvanTTT KvanTTT force-pushed the feature/ReversibleRenamerSeed branch from e9632e3 to 88f4413 Compare August 1, 2021 12:18
@AppVeyorBot
Copy link

Build ConfuserEx 844 completed (commit 796ec8440b by @KvanTTT)

@mkaring
Copy link
Owner

mkaring commented Aug 1, 2021

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
Copy link
Author

KvanTTT commented Aug 5, 2021

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
Copy link

griknok commented Aug 6, 2021

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
Copy link
Author

KvanTTT commented Aug 6, 2021

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

@mkaring
Copy link
Owner

mkaring commented Aug 8, 2021

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
Copy link
Author

KvanTTT commented Aug 9, 2021

Ok, will remake it closer to next weekend.

@KvanTTT KvanTTT force-pushed the feature/ReversibleRenamerSeed branch from 88f4413 to 8c8d1ea Compare August 13, 2021 18:23
@KvanTTT KvanTTT changed the title Improve reversible renamer protection with random seed Improve reversible renamer protection with random password Aug 13, 2021
@AppVeyorBot
Copy link

Build ConfuserEx 847 completed (commit 180c2ee598 by @KvanTTT)

@KvanTTT
Copy link
Author

KvanTTT commented Aug 13, 2021

Done.

Confuser.Renamer/ReversibleRenamer.cs Outdated Show resolved Hide resolved
Tests/Confuser.UnitTest/TestBase.cs Show resolved Hide resolved
Confuser.Renamer/AnalyzePhase.cs Outdated Show resolved Hide resolved
@KvanTTT
Copy link
Author

KvanTTT commented Aug 14, 2021

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.

@KvanTTT KvanTTT force-pushed the feature/ReversibleRenamerSeed branch from 8c8d1ea to de0ac4c Compare August 15, 2021 11:26
@AppVeyorBot
Copy link

Build ConfuserEx 849 completed (commit 057a7e7395 by @KvanTTT)

@KvanTTT KvanTTT force-pushed the feature/ReversibleRenamerSeed branch from de0ac4c to 4aa63d0 Compare August 15, 2021 11:31
@AppVeyorBot
Copy link

Build ConfuserEx 850 completed (commit 12ab7b938f by @KvanTTT)

@AppVeyorBot
Copy link

Build ConfuserEx 852 completed (commit 625e9dcbb6 by @KvanTTT)

@mkaring
Copy link
Owner

mkaring commented Aug 16, 2021

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

@KvanTTT
Copy link
Author

KvanTTT commented Aug 16, 2021

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

@wmjordan
Copy link

wmjordan commented Aug 16, 2021

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
Copy link
Author

KvanTTT commented Aug 16, 2021

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
Copy link
Owner

mkaring commented Aug 20, 2021

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.

@mkaring mkaring merged commit 41f448f into mkaring:master Aug 20, 2021
@KvanTTT KvanTTT deleted the feature/ReversibleRenamerSeed branch August 20, 2021 10:31
@mkaring mkaring added this to the 1.6 milestone Dec 23, 2021
@mkaring mkaring added the enhancement New feature or request label Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make initialization vector dependant on password or random seed in ReversibleRenamer instead of input data
5 participants