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

Create CryptographicHardwareIntrinsicsAnalyzer.cs #4005

Closed
wants to merge 12 commits into from

Conversation

Youssef1313
Copy link
Member

@mavasani @GrabYourPitchforks, I need an initial review on this.

Fixes #3646

@Youssef1313 Youssef1313 requested a review from a team as a code owner August 13, 2020 18:23
@mavasani
Copy link
Contributor

Tagging @dotpaul as this is a security rule.

@Youssef1313 Youssef1313 changed the base branch from master to 2.9.x August 13, 2020 18:32
@Youssef1313 Youssef1313 changed the base branch from 2.9.x to master August 13, 2020 18:33
@Youssef1313 Youssef1313 changed the base branch from master to 2.9.x August 13, 2020 18:45
@dotpaul
Copy link
Contributor

dotpaul commented Aug 13, 2020

Thank you @Youssef1313 ! Could you please target the 2.9.x branch for this PR? For the time being, we're shipping new security rules and updates in v2.9 releases as well.

Edit: Oh, I see you already did that. 😄

@dotpaul dotpaul self-requested a review August 13, 2020 19:10

compilationStartContext.RegisterOperationAction(
context => AnalyzeObjectCreation(context, tooGenericExceptionSymbols, reservedExceptionSymbols),
OperationKind.ObjectCreation);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I use OperationKind.Invocation here instead? I'm unsure.

Copy link
Contributor

@dotpaul dotpaul Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. System.Runtime.Intrinsics.X86.Aes looks like a static class, i.e. it can't be instantiated, so you wouldn't see ObjectCreation operations.

Edit: I mean, a lot of the methods inside System.Runtime.Intrinsics.X86.Aes are static, so should catch those cases as well.

Try playing around the System.Runtime.Intrinsics.X86.Aes API, and then you'll have an idea of what unit tests to write.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since it's an abstract class, we should also make sure no one is deriving from it.

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to worry about deriving from it. All of the intrinsics are abstract classes with internal ctors, meaning that only the runtime itself can subclass it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, cool.

Comment on lines +30 to +35
ReferenceAssemblies = new ReferenceAssemblies(
"netcoreapp3.1",
new PackageIdentity(
"Microsoft.NETCore.App.Ref",
"3.1.0"),
Path.Combine("ref", "netcoreapp3.1")),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ I had to do it that way because ReferenceAssemblies.NetCore.NetCoreApp31 doesn't exist. It's probably because an older version of roslyn SDK being used in 2.9.x branch.

@mavasani, Do you want to update the sdk version instead? If so, please inform me where the version should be updated.

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking this as blocked until #3646 (comment) is resolved.

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Aug 14, 2020

Don't forget these classes as well:

  • System.Runtime.Intrinsics.Arm.Aes
  • System.Runtime.Intrinsics.Arm.Sha1
  • System.Runtime.Intrinsics.Arm.Sha256

Note: there may also be nested classes within these types, such as Arm.Aes.Arm64. Instance members on those nested classes should also trigger this analyzer rule.

@Youssef1313
Copy link
Member Author

Closing as the discussion in the issue has moved towards using EditorBrowsable for these APIs.

@Youssef1313 Youssef1313 deleted the patch-6 branch September 17, 2020 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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