Analyzer: warn on use of module initializer

[stephentoub]

See discussion in #43268.

The analyzer would flag any application of [ModuleInitializer]. We would turn this on in dotnet/runtime, and prohibit any use of the attribute. It would likely be an info-level analyzer by default, suggesting not to use initializers in library-level code.

[terrajobst]

Video

We seem to believe this can even be a warning that is on by default (but it shouldn't apply to generated code). Advanced users (who this feature is for) know how to turn it off.

[sharwell]

📝 I would not be surprised to see test libraries inject these to ensure things like trace listeners are properly initialized. Roslyn is using this approach today.

[sharwell]

💭 It almost seems like this is better suited to Public API analyzer

[terrajobst]

@sharwell

📝 I would not be surprised to see test libraries inject these to ensure things like trace listeners are properly initialized. Roslyn is using this approach today.

That seems like a reasonable scenario. So long people are aware what the limitations are, that's fine. However, we know from static constructors (type initializers) that people think of these things as constructors and don't consider interdependencies. It's not a feature for the masses.

💭 It almost seems like this is better suited to Public API analyzer

What does this mean? Isn't the Roslyn specific analyzer?

[carlossanlop]

@sharwell ping... can you please clarify your last comment?

[sharwell]

We could treat module initializers like a public API, where the PublicAPI.Shipped.md and PublicAPI.Unshipped.md files list the initializers when present. This would prevent them from accidentally getting included in assemblies that track the public API surface.

[khalidabuhakmeh]

Posting this blog post by Maarten Balliauw to show how malicious parties can use source generators to theoretically do a supply chain attack using ModuleInitializer.

https://blog.maartenballiauw.be/post/2021/05/05/building-a-supply-chain-attack-with-dotnet-nuget-dns-source-generators-and-more.html

[terrajobst]

I don't see how module initializers are different from any of the other opportunities to hide code. In the end, if you take a dependency on a package, that package will contain code that can run in your process or on your machine during design time. There is an infinite amount of ways that code can execute in your process. It can start threads, it can put files on disk and schedule them using task manager etc. If you depend on code, you better either know what it does, trust the provider of the code to not be malicious, and have decent security to not be compromised themselves. Once 3rd party code runs in your process, all bets are off IMHO so I don't see a reason to single out module initializers. What about process wide events? What about access to disk? What about reflection emit? What about private reflection?

To be clear, I'm not trying to downplay the risk of supply chain attacks. Those are very real and a huge problem space. But I'm objecting to removing useful features if they are don't increase the risk envelope. The way to deal with supply chain attacks is by making it harder to accidently depend on non-trust worthy parties or be vulnerable to package swaps.

/cc @clairernovotny @jaredpar

[clairernovotny]

Quite frankly, given that a package can contain targets that run by default, you don't need a ModuleInitializer to do damage -- just the PackageReference. Put something that runs on a design time build, add the package reference, and then boom, your code runs, game over.

I agree with @terrajobst; I'm not sure that Module Initializers should be singled out.

[sharwell]

To put @terrajobst comments in concrete form: the issue described in #43328 (comment) suggests that a supply chain attack can be performed, but the argument is based on a prerequisite that a supply chain attack already occurred (introduction of a malicious source generator is considered a supply chain attack, regardless of whether the malicious output is code executed on the host or injected into the output).

[jeffhandley]

Closed via dotnet/roslyn-analyzers#5347 - CA2255 - ModuleInitializerAttributeShouldNotBeUsedInLibraries