Suggestion: New analyzer rule that warns when using cryptographic hardware intrinsics

[GrabYourPitchforks]

In .NET Core 3.0, we added support for x86 hardware intrinsics, including for AES-NI (via the type System.Runtime.Intrinsics.X86.Aes). We added these types because they may have valid use cases for a very limited set of callers. However, the number of people who should be calling these APIs is extremely limited. There exist numerous pitfalls for these APIs - far too numerous for me to list here - that aren't immediately apparent. And even expert developers who are intimately familiar with cryptographic algorithms are likely to get tripped up by these.

We should introduce an analyzer which looks for references to this type and which says "hey, you really should be using the System.Security.Cryptography.Aes type instead."

Category: Security

[mavasani]

@dotpaul for review.

[dotpaul]

Sounds good to me. @GrabYourPitchforks, I (or whoever implements this) will probably ask your advice for the rule messages / documentation guidance.

[dotpaul]

If anyone else wants to pick this up before I get to it, please target the 2.9.x branch.

[GrabYourPitchforks]

Sure, I'm willing to help with messaging. Just send a ping when ready. :)

[stephentoub]

I thought we'd agreed not to do this. We just added these APIs and now we're saying don't use them. We shouldn't add them in the first place if we then immediately discourage anyone from using them.

[dotpaul]

In this case, isn't it too late for "We shouldn't add them in the first place"? 🙂

[GrabYourPitchforks]

@stephentoub My understanding was that we had agreed not to obsolete them. Am I misremembering?

There are some valid (mainly academic) use cases for calling these APIs. But on the whole that's an extreme minority of our developer audience, with the bulk of our audience better served by the normal APIs.

[stephentoub]

My understanding was that we had agreed not to obsolete them. Am I misremembering?

I don't see a meaningful difference between marking an API as obsolete so that the compiler says "don't use this" and writing an analyzer that fires on that API so that the compiler says "don't use this".

isn't it too late for "We shouldn't add them in the first place"?

The arguments against these methods were made when they were introduced, and they were still added. To my knowledge, nothing has changed.

If there are valid uses for the APIs, we shouldn't be warning on their use. If there aren't valid uses, we shouldn't be adding them just in the name of completeness.

[GrabYourPitchforks]

I don't see a meaningful difference between marking an API as obsolete so that the compiler says "don't use this" and writing an analyzer that fires on that API so that the compiler says "don't use this".

The analyzer shouldn't say "don't use this." We wouldn't have introduced the API if it had zero legitimate use cases.

Instead, the analyzer should say "I'm 99.999% confident you wanted to call xyz method instead of this one. But if you really did intend to call this API, suppress the rule and party on!"

Philosophically, I see an analyzer as alerting developers to a potential trap, allowing them to use their better judgment to overrule the alert and continue working. Seems like this proposed analyzer rule fits within that philosophy? (Contrast this with an obsoletion, which is philosophically akin to a hard "no, seriously, stop.")

[stephentoub]

Contrast this with an obsoletion, which is philosophically akin to a hard "no, seriously, stop."

I don't see such a distinction. How is the user experience so different between such an analyzer and adding this to the API?

[Obsolete("I'm 99.999% confident you wanted to call xyz method instead of this one. But if you really did intend to call this API, suppress the rule and party on!", DiagnosticId = "CA12345"]

and we already agreed not to do that.

On top of that, these instrinsics are hard to use. First, you have to find yourself including and using things from the System.Runtime.Intrinsics.X86 namespace, at which point you're already in super advanced territory. Then you need to figure out how to create a Vector128<byte> from your data. Either you really know what you're doing and an analyzer telling you otherwise isn't going to change your mind, or you're going to have looked at the docs, which can highlight whatever we want them to highlight.

[GrabYourPitchforks]

One problem we're encountering is that the IDE is directing people to these APIs.

What if we instead focused on fixing that behavior? (How?) If we can address it there, then maybe there would be no need for an analyzer.

[mavasani]

Tagging @genlu - can we teach unimported type completion to exclude certain namespaces from its list?

[mavasani]

I agree with @stephentoub that having an analyzer that flags every usage site of certain symbol is not the right approach - with the analyzers in the box, there is no experience difference for end users between seeing a CSxxxx obsolete warning and a CAxxxx obsolete warning on all usages. Having a CA warning does not mean any less importance/critical then CS warning if they are both in the box and on by default.

[GrabYourPitchforks]

I don't see the equivalence between an obsoletion and an analyzer. IMO they're geared toward different scenarios. Maybe I'm looking at this from too nuanced a view?

We already have analyzers that flag every usage of certain symbols. For example, CA1307 flags every usage of string.Compare(string, string), string.IndexOf(string), and so on. But we clearly aren't obsoleting those APIs, nor is there a desire to do so.

As a consumer, an analyzer tells me "There is a pitfall to using this API. Go learn about the pitfall to see if it impacts your scenario. If you're confident this is still the right API for you, suppress the alert." So the analyzer is more cautionary than anything else.

But an obsoletion tells me "The library author really wants you to stop using this API. You must take the action of reading up on alternatives. If you ignore this alert you risk your code breaking in a future release." It's a proper warning rather than a caution flag.

But again, I'm willing to cede that an analyzer for this particular issue might not be needed if we can get the IDE to stop recommending it to people. Then it's unlikely that anybody would ever stumble across the API in the first place. :)

[dotpaul]

Not that I've noticed any insecure (or any at all) usages of these intrinsics APIs, but if we did see people manage to use them in a security-critical context when they ought to be using something under System.Security.Cryptography instead, then I would want to add an analyzer.

[stephentoub]

Maybe I'm looking at this from too nuanced a view?

I'm looking at it from a consumer point of view. I build my project and I get this:

warning CA1234: 'Program.Foo()' is obsolete: 'Don't use this'

Did that come from an analyzer or an obsoletion attribute? It could have come from either.

[GrabYourPitchforks]

Ok, so how do we make progress with this, then? The ultimate goal is to discourage people from using these APIs. I see two possible ways to accomplish this.

The first mechanism is to prevent the APIs from even showing up in Intellisense (see my earlier comment above). I don't know if this is viable given the current Intellisense design, and I do not know how to make forward progress with this proposal. If this is the preferred path forward then somebody else on this thread needs to drive this proposal.

The second mechanism is to generate some type of warning "hey, you're almost certainly doing something incorrect and probably wanted to use this other API instead." I do know how to make forward progress with this proposal, so I can help drive it. But it sounds like this is leaving a sour taste in peoples' mouths.

Are there other alternatives that I'm missing?

[mavasani]

The first mechanism is to prevent the APIs from even showing up in Intellisense (see my earlier comment above). I don't know if this is viable given the current Intellisense design, and I do not know how to make forward progress with this proposal. If this is the preferred path forward then somebody else on this thread needs to drive this proposal.

If we decide to go this route, @genlu would most likely be the one driving this work.

[genlu]

The first mechanism is to prevent the APIs from even showing up in Intellisense (see my earlier comment above). I don't know if this is viable given the current Intellisense design, and I do not know how to make forward progress with this proposal. If this is the preferred path forward then somebody else on this thread needs to drive this proposal.

For completion, can we mark the type as [EditorBrowsable(EditorBrowsableState.Never)]? I believe it should serve your purpose well. In terms of the "add import" feature shown in your screenshot above, I don't think it currently checks this attribute, but it might make sense to do so. @CyrusNajmabadi what do you think?

[GrabYourPitchforks]

@stephentoub Think using EditorBrowsable.Never on these types would work? I know historically we've shied away from using them except for APIs that were truly meant to be called by compilers (e.g., GetPinnableReference). It solves the problem of hiding the APIs from Intellisense while not producing a warning for consumers who know of their existence and are intending to use them. It does mean that the API's consumers won't get any Intellisense benefit at all. So it'll discourage perhaps a grand total of 7 people worldwide who really wanted to use this API. :)

[stephentoub]

Think using EditorBrowsable.Never on these types would work?

I'm ok with it.

[CyrusNajmabadi]

One problem we're encountering is that the IDE is directing people to these APIs.

We're directing because it's an exact match. If you want to push people away, i would recommend naming things accordingly. i.e. AesAdvanced, or AesSpecialized, or AesIntrinsics. That will help advise users that this is really not the mainline case they should be using. Naming the types the same actively makes it seem as if they're effectively the same and should be interchangeable.

@stephentoub Think using EditorBrowsable.Never on these types would work?

This is def the recommended approach for APIs that are extremely specialized and should not be used by most developers.

[CyrusNajmabadi]

In terms of the "add import" feature shown in your screenshot above, I don't think it currently checks this attribute, but it might make sense to do so. @CyrusNajmabadi what do you think?

Discussed this offline. In general we would not go do this. Add-Import is explicitly designed so that if you name your thing properly you can use it to add the import for it. i.e. users may have editorbrowsable stuff already that they can still add imports for themselves.

In general, the major factor here is the 'Browsable' portoin of 'EditorBrowsable'. i.e. we don't show it in UIs that are intended for browsing the graph of symbols. But we would still include it UIs intended for direct navigation/access.

[stephentoub]

We're directing because it's an exact match. If you want to push people away, i would recommend naming things accordingly. i.e. AesAdvanced, or AesSpecialized, or AesIntrinsics

That's in the namespace; it'd be duplicative to also include it in the type name.

[CyrusNajmabadi]

name duplication isn't really something that bothers me between namespaces and types. Roslyn follows that pattern heavily for many of our types. However, if it's not your Cup<T> that's ok :) I'd just go with EB(never) then.

[Youssef1313]

I'm lost as to what the final decision is here 😄

Should the analyzer be implemented?