Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Sep 1, 2020

As part of building secure software, DevDiv has a set of Roslyn anaylzers dealing with security that should be run on every managed assembly. This PR adds these analyzers and fixes errors they introduce.

Running Analyzers.

In order to run the Roslyn analyzers, the NuGet package Microsoft.CodeAnalysis.FxCopAnalyzers must be added to each project. Rather than do this manually now, and for each future project, we add this to the Directory.Build.props file, which automatically adds it to all projects.

By default, adding the NuGet package runs all included analyzers at each one's default severity level. At this time, we are only concerned with the prescribed security set, so we use .editorconfig to set those analyzers as error, and all other analyzers as none.

Projects that wish to opt out of running the analyzers can set <DisableRoslynAnalyzers>True</DisableRoslynAnalyzers>.

Fixing Errors

The only errors surfaced by these analyzers is CA3075: Insecure DTD Processing. These were fixed by adding new XmlReaderSettings { XmlResolver = null } which will not attempt to resolve and download any DTD files.

Other Notes

Updating the Java.Interop.dll to the latest analyzer NuGet version triggered some errors we had marked for that specific assembly which likely did not exist in the old analyzers and thus were not being surfaced as errors. They do not appear to be rules that we are actually concerned with, so they were disabled:

  • CA1021 - Don't use out parameters
  • CA1045 - Don't use reference parameters
  • CA1822 - Mark methods static if they don't reference instance members
  • CA1002 - Don't expose generic Lists

Moved NullableAttributes.cs to the src\utils directory.

This file was added to Java.Interop.Tools.JavaCallableWrappers.csproj from ..\Java.Interop\. However, because the file resided in the directory containing the strict .editorconfig for Java.Interop.dll, it was applying those .editorconfig rules to J.I.T.JavaCallableWrappers. Moving it to a neutral directory fixed this.

@jpobst jpobst force-pushed the roslyn-analyzers branch 2 times, most recently from 6c7dd47 to bd88554 Compare September 1, 2020 16:36
@jpobst jpobst marked this pull request as ready for review September 1, 2020 16:37
@jpobst jpobst requested a review from jonpryor September 1, 2020 16:41
</PropertyGroup>

<!-- Add Roslyn analyzers NuGet to all projects -->
<ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't item groups actually belong in Directory.Build.targets?

Copy link
Contributor Author

@jpobst jpobst Sep 2, 2020

Choose a reason for hiding this comment

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

I went with the guidance here: https://docs.microsoft.com/en-us/visualstudio/msbuild/customize-your-build?view=vs-2019#choose-between-adding-properties-to-a-props-or-targets-file.

Specifically:

Include items in .props files (conditioned on a property). All properties are considered before any 
item, so user-project property customizations get picked up, and this gives the user's project the 
opportunity to Remove or Update any item brought in by the import.

I added $(DisableRoslynAnalyzers) to allow opting out of running the analyzers.

@jonpryor
Copy link
Contributor

jonpryor commented Sep 1, 2020

Commit message for review:

As part of building secure software, Microsoft DevDiv has a set of
[Roslyn anaylzers][0] dealing with security that should be run on every
managed assembly.

Adds these analyzers and fix any errors they introduce.


~~ Running Analyzers ~~

In order to run the Roslyn analyzers, the NuGet package
[`Microsoft.CodeAnalysis.FxCopAnalyzers`][1] must be added to each
project.  Rather than do this manually now, and for each new project
in the future, we instead add this to the `Directory.Build.props` file,
which automatically adds it to all projects.

By default, adding the NuGet package runs all included analyzers at
each analyzer's default severity level.  At this time, we are only
concerned with the prescribed security set, so we use
`.editorconfig` to set those analyzers as `error`, and all other
analyzers as `none`.

Projects that wish to opt out of running the analyzers can set 
`<DisableRoslynAnalyzers>True</DisableRoslynAnalyzers>`.

~~ Fixing Errors ~~

The only errors surfaced by these analyzers is
[CA3075: Insecure DTD Processing][2].  These were fixed by using
`new XmlReaderSettings { XmlResolver = null }`, which will not attempt
to resolve and download any DTD files.


~~ Move `NullableAttributes.cs` ~~

`NullableAttributes.cs` is moved to the `src\utils` directory.  

This file was added to `Java.Interop.Tools.JavaCallableWrappers.csproj`
via `..\Java.Interop\`.  However, because the file resided in the
directory containing the strict `.editorconfig` for `Java.Interop.dll`,
it was applying those `.editorconfig` rules to
`Java.Interop.Tools.JavaCallableWrappers.dll`.


Moving it to a neutral directory fixed this.

~~ Other Notes ~~

Updating the `Java.Interop.dll` to the latest analyzer NuGet version
triggered some errors we had handled for that specific assembly, which
likely did not exist in the old analyzers and thus were not being
surfaced as errors.  They do not appear to be rules that we are
actually concerned with, so they were disabled:

  * CA1021 - Don't use out parameters
  * CA1045 - Don't use reference parameters
  * CA1822 - Mark methods static if they don't reference instance members
  * CA1002 - Don't expose generic Lists

[0]: https://github.com/dotnet/roslyn-analyzers
[1]: https://www.nuget.org/packages/Microsoft.CodeAnalysis.FxCopAnalyzers/
[2]: https://docs.microsoft.com/en-us/visualstudio/code-quality/ca3075?view=vs-2019

@jonpryor jonpryor merged commit ac914ce into master Sep 8, 2020
@jonpryor jonpryor deleted the roslyn-analyzers branch September 8, 2020 18:15
@jpobst jpobst added this to the 11.1 (16.9 / 8.9) milestone Sep 22, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants