orph3usLyre / muddy-waters

A static string obfuscation library for rust projects
Apache License 2.0
73 stars 3 forks source link

Fix: env variable decoding and added error messages #5

Closed mrauhu closed 8 months ago

mrauhu commented 8 months ago

Hello @orph3usLyre.

Fix for an error:

no method named as_bytes found for struct OsString in the current scope [E0599]

https://github.com/orph3usLyre/muddy-waters/blob/5f73dc10d65087f933a2c5fb7f5d490850917643/muddy_macros/src/internal.rs#L120

As far as I know, it's Windows related problem, because the OsStrExt trait on Windows https://doc.rust-lang.org/std/os/windows/ffi/trait.OsStrExt.html is missing as_bytes() function.

Related: https://github.com/orph3usLyre/muddy-waters/issues/2 and https://github.com/orph3usLyre/muddy-waters/pull/3.

Changes

Best wishes, Sergey.

orph3usLyre commented 8 months ago

Hey again @mrauhu,

Thanks for working on this! I'd like to have a look at the differences between the unix as_bytes() and the as_encoded_bytes() before merging this, so I'll check when I'm off work over the next few days. (But it appears as if as_encoded_bytes() is the more flexible method, which is nice.)

I'm not a big fan of the panic!() messages, however. The core idea of muddy was to provide all the information required to run the program at build-time, and to avoid (as much as possible) any indications of how to de-obfuscate the binary after it's built. This way, it would be more obscure to people who might have the binary, but did not compile it. The panic messages would be quite the big hint to anyone attempting to decrypt the strings inside the binary - I'd rather avoid that and allow the implementers to decide what messages to display at runtime.

Thanks again! :crab:

mrauhu commented 8 months ago

Thank you for the context.

orph3usLyre commented 8 months ago

Hey @mrauhu,

I checked the implementation of as_encoded_bytes() and this seems like it would be fine for the use case here. I made a few suggestions based on my comment from yesterday, and if these look good to you, I'd be happy to merge this.

Thanks again!

mrauhu commented 8 months ago

Hello @orph3usLyre,

About the changes, that you requested:

  1. In my opinion, it is better to leave information about what the developer needs to do during development, because this will be a good error message that adds a context of fix, not only indicating how to fix it.

  2. I read your suggestion comment and original message, quote:

I'm not a big fan of the panic!() messages, however. The core idea of muddy was to provide all the information required to run the program at build-time, and to avoid (as much as possible) any indications of how to de-obfuscate the binary after it's built. This way, it would be more obscure to people who might have the binary, but did not compile it.

And pushed a small update that will help developers to understand what to do when they debug code, but hide it for a release build.

In your opinion this is suitable?

Thank you.

orph3usLyre commented 8 months ago

Hey @mrauhu,

I really like the idea of using the #[cfg(debug_assertions)] to provide more info for debug builds, let's go with that. Just to double check, the text doesn't show up in the release binary if you run strings on it, right?

I also hadn't realized that the //language=sh was a RustRover thing. If that's the case, I don't mind it. :+1:

Just a final question then, is there any particular reason for the extra empty eprintln!()?

Cheers!

mrauhu commented 8 months ago

Just to double check, the text doesn't show up in the release binary if you run strings on it, right?

The only way it can be in the release build, if a developer will to change default release profile settings in Cargo.toml to:

[profile.release]
debug-assertions = true

Or set it for rustc with -C debug-assertions=true.

https://doc.rust-lang.org/cargo/reference/profiles.html#debug-assertions

Just a final question then, is there any particular reason for the extra empty eprintln!()?

@orph3usLyre thank you for noticing, removed it.