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

Consider ignoring SuppressIldasmAttribute in ILDASM #13341

Closed
vcsjones opened this issue Aug 29, 2019 · 8 comments · Fixed by #50951
Closed

Consider ignoring SuppressIldasmAttribute in ILDASM #13341

vcsjones opened this issue Aug 29, 2019 · 8 comments · Fixed by #50951
Assignees
Milestone

Comments

@vcsjones
Copy link
Member

The SuppressIldasmAttribute instructs the ildasm tool to prevent working on assemblies or modules that have this attribute over here.

I'd like to ask re-considering if this is necessary still and if the check can be removed, and documenting the attribute as obsolete.

Some motivation:

The attribute is, as many know, trivial to bypass. Practically every other disassembly tool ignores this attribute. As for why "Well why not keep using that tool" is because ildasm offers some benefits over other tools, such as quickly dumping the IL-text to disk and re-assembling.

The attribute conveys as sense of protection that is really not existent. If the desired outcome is to protect intellectual property, then they should be steered toward 3rd party solutions like a commercial obfuscator.

@Symbai
Copy link

Symbai commented Aug 29, 2019

The attribute conveys as sense of protection that is really not existent. If the desired outcome is to protect intellectual property, then they should be steered toward 3rd party solutions like a commercial obfuscator.

Which can also bypassed easily so the protection is not really existent as well. Instead .NET should offer a way to generative native assemblies and this wouldn't be a problem at all. On top of that, it would also provide performance improvements.

@mgravell
Copy link
Member

Instead .NET should offer a way to generative native assemblies and this wouldn't be a problem at all.

...because as we all know, native assemblies can't be disassembled, decompiled, etc! 🤦‍♂️ (my point: that also just gives a false sense of security)

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@BruceForstall BruceForstall modified the milestones: 5.0.0, Future Jul 6, 2020
@GrabYourPitchforks GrabYourPitchforks self-assigned this Apr 7, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 8, 2021
@dotMorten
Copy link

dotMorten commented Apr 9, 2021

The attribute is, as many know, trivial to bypass. Practically every other disassembly tool ignores this attribute. As for why "Well why not keep using that tool" is because ildasm offers some benefits over other tools, such as quickly dumping the IL-text to disk and re-assembling.

I think this is the wrong argument. The attribute is not about security or completely preventing people from decompiling. It's about helping to keep the honest people honest. A lot of commercial software (if not all of it), has a license that explicitly forbids your to decompile their code - regardless of whether it's possible or not. This attribute is useful for those products as it's another indicator that acts in support of this paper license. This especially becomes true with Visual Studio now offering users to decompile libraries when navigating to a definition. This attribute is there to prevent that from happening, as clicking yes to decompile instantly makes you violate that license agreement.

If producers of class libraries wants to make it easy for users to decompile their stuff, all they have to do is not set this attribute on their assembly. Easy peasy. But don't obsolete this attribute, or have ildasm ignore it, or have VS's go-to-definition bypass it.

image

I could perhaps see an argument for letting ildasm ignore this attribute (or a dialog to allow you to override it), but the Visual Studio scenario is still very much valid.

@mletterle
Copy link
Contributor

The attribute is, as many know, trivial to bypass. Practically every other disassembly tool ignores this attribute. As for why "Well why not keep using that tool" is because ildasm offers some benefits over other tools, such as quickly dumping the IL-text to disk and re-assembling.

I think this is the wrong argument. The attribute is not about security or completely preventing people from decompiling. It's about helping to keep the honest people honest.

It may be better to define a standardized AssemblyMetadataAttribute for this scenario, something like [AssemblyMetadata("PlsNoDecompile")] :P

@dotMorten
Copy link

dotMorten commented Apr 9, 2021

@mletterle Aren't we then just replacing one attribute with another to cover the exact same scenario?

@mletterle
Copy link
Contributor

@mletterle Aren't we then just replacing one attribute with another to cover the exact same scenario?

Yes, but it would not be ildasm specific so it would not seem wrong for ildasm to ignore it ;)

@GrabYourPitchforks
Copy link
Member

FWIW, I believe ildasm and VS don't look for this specific attribute (as it exists in System.Private.CoreLib). I think you can define your own attribute with the same namespace & name in your own assembly, and the behavior today is that those tools will honor the attribute.

In practice, what this means is that if the only change we made was to remove this attribute from the reference assemblies, with no other functional changes, the tooling would continue to work as it does today.

@mletterle
Copy link
Contributor

On a relevant note, maybe a good compromise is to add an opt-out argument to ildasm and have it match just on the attribute's simplename (ignoring namespace) and remove the attribute from CorLib. Users and protection tools could then inject their own versions of SuppressIldasmAttribute and "honest" developers could simply never use the opt-out argument.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 19, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants