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.

Sign up for GitHub

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

Jump to bottom

Fix IndexOutOfRangeException bug of CA5391. #2857

Merged
merged 11 commits into from Sep 30, 2019
Merged

Fix IndexOutOfRangeException bug of CA5391. #2857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
53bfa2f
Fix the IndexOutOfRangeException warning and add a option for configi…
LingxiaChen Sep 18, 2019
3dfaf04
Merge branch 'UseAutoValidateAntiforgeryToken' of https://github.com/…
LingxiaChen Sep 18, 2019
7de05dd
Address review feedback.
LingxiaChen Sep 19, 2019
ddb7bf3
Update docs/Analyzer Configuration.md
LLLXXXCCC Sep 27, 2019
411d52f
Update docs/Analyzer Configuration.md
LLLXXXCCC Sep 27, 2019
1331d0b
Update src/Utilities/Compiler/Options/EditorConfigOptionNames.cs
LLLXXXCCC Sep 27, 2019
a949bfd
Add test for concurrency issue.
LingxiaChen Sep 27, 2019
c36045d
Merge branch 'UseAutoValidateAntiforgeryToken' of https://github.com/…
LingxiaChen Sep 27, 2019
c99c8ec
Merge branch 'master' into UseAutoValidateAntiforgeryToken
LLLXXXCCC Sep 27, 2019
70f46f5
Update.
LingxiaChen Sep 27, 2019
860ec1c
Add more controllers for the concurrency test.
LingxiaChen Sep 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 22 additions & 9 deletions docs/Analyzer Configuration.md
View file
Open in desktop
Expand Up @@ -213,6 +213,28 @@ Examples:
|`dotnet_code_quality.excluded_type_names_with_derived_types = MyType1\|MyType2` | Matches all types named either 'MyType1' or 'MyType2' and all of their derived types in the compilation
|`dotnet_code_quality.excluded_type_names_with_derived_types = M:NS.MyType` | Matches specific type 'MyType' with given fully qualified name and all of its derived types
|`dotnet_code_quality.excluded_type_names_with_derived_types = M:NS1.MyType1\|M:NS2.MyType2` | Matches specific types 'MyType1' and 'MyType2' with respective fully qualified names and all of their derived types

### Unsafe DllImportSearchPath bits when using DefaultDllImportSearchPaths attribute
Option Name: `unsafe_DllImportSearchPath_bits`

Configurable Rules: CA5393

Option Values: Integer values of System.Runtime.InteropServices.DllImportSearchPath

Default Value: '770', which is AssemblyDirectory | UseDllDirectoryForDependencies | ApplicationDirectory

Example: `dotnet_code_quality.CA5393.unsafe_DllImportSearchPath_bits = 770`

### Exclude ASP.NET Core MVC ControllerBase when considering CSRF
Option Name: `exclude_aspnet_core_mvc_controllerbase`

Configurable Rules: CA5391

Option Values: Boolean values

Default Value: `true`

Example: `dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controllerbase = false`

### Disallowed symbol names
Option Name: `disallowed_symbol_names`
Expand Down Expand Up @@ -370,12 +392,3 @@ Option Values: integral values
Default Value: Specific to each configurable rule ('100000' by default for most rules)

Example: `dotnet_code_quality.CA5387.sufficient_IterationCount_for_weak_KDF_algorithm = 100000`

#### Configure unsafe DllImportSearchPath bits when using DefaultDllImportSearchPaths attribute
Option Name: `unsafe_DllImportSearchPath_bits`

Option Values: Integer values of System.Runtime.InteropServices.DllImportSearchPath

Default Value: Specific to each configurable rule ('770', which is AssemblyDirectory | UseDllDirectoryForDependencies | ApplicationDirectory, by default for most rules)

Example: `dotnet_code_quality.CA5392.unsafe_DllImportSearchPath_bits = 770`
54 changes: 33 additions & 21 deletions src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs
View file
Open in desktop
Expand Up @@ -25,7 +25,7 @@ public sealed class UseAutoValidateAntiforgeryToken : DiagnosticAnalyzer
typeof(MicrosoftNetCoreAnalyzersResources),
nameof(MicrosoftNetCoreAnalyzersResources.UseAutoValidateAntiforgeryToken),
nameof(MicrosoftNetCoreAnalyzersResources.UseAutoValidateAntiforgeryTokenMessage),
DiagnosticHelpers.EnabledByDefaultIfNotBuildingVSIX,
false,
helpLinkUri: null,
descriptionResourceStringName: nameof(MicrosoftNetCoreAnalyzersResources.UseAutoValidateAntiforgeryTokenDescription),
customTags: WellKnownDiagnosticTags.Telemetry);
Expand All @@ -49,12 +49,13 @@ public sealed class UseAutoValidateAntiforgeryToken : DiagnosticAnalyzer
WellKnownTypeNames.MicrosoftAspNetCoreMvcHttpDeleteAttribute,
WellKnownTypeNames.MicrosoftAspNetCoreMvcHttpPatchAttribute);

// It is used to translate ConcurrentDictionary into ConcurrentHashset, which is not provided.
private const bool placeholder = true;

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(
UseAutoValidateAntiforgeryTokenRule,
MissHttpVerbAttributeRule);

public delegate bool RequirementsOfValidateMethod(IMethodSymbol methodSymbol);

public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
Expand Down Expand Up @@ -90,8 +91,14 @@ public override void Initialize(AnalysisContext context)
return;
}

var cancellationToken = compilationStartAnalysisContext.CancellationToken;
var onlyLookAtDerivedClassesOfController = compilationStartAnalysisContext.Options.GetBoolOptionValue(
optionName: EditorConfigOptionNames.ExcludeAspnetCoreMvcControllerBase,
rule: UseAutoValidateAntiforgeryTokenRule,
defaultValue: true,
cancellationToken: cancellationToken);

// A dictionary from method symbol to set of methods calling it directly.
// The bool value in the sub ConcurrentDictionary is not used, use ConcurrentDictionary rather than HashSet just for the concurrency security.
var inverseGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>();

// Ignore cases where a global anti forgery filter is in use.
Expand All @@ -100,9 +107,9 @@ public override void Initialize(AnalysisContext context)
// Verify that validate anti forgery token attributes are used somewhere within this project,
// to avoid reporting false positives on projects that use an alternative approach to mitigate CSRF issues.
var usingValidateAntiForgeryAttribute = false;
var onAuthorizationAsyncMethodSymbols = new HashSet<IMethodSymbol>();
var actionMethodSymbols = new HashSet<(IMethodSymbol, string)>();
var actionMethodNeedAddingHttpVerbAttributeSymbols = new HashSet<IMethodSymbol>();
var onAuthorizationAsyncMethodSymbols = new ConcurrentDictionary<IMethodSymbol, bool>();
var actionMethodSymbols = new ConcurrentDictionary<(IMethodSymbol, string), bool>();
var actionMethodNeedAddingHttpVerbAttributeSymbols = new ConcurrentDictionary<IMethodSymbol, bool>();

LLLXXXCCC marked this conversation as resolved.
Show resolved Hide resolved
// Constructing inverse callGraph.
// When it comes to delegate function assignment Del handler = DelegateMethod;, inverse call Graph will add:
Expand Down Expand Up @@ -152,7 +159,7 @@ public override void Initialize(AnalysisContext context)
}

callers = inverseGraph.GetOrAdd(calledSymbol, (_) => new ConcurrentDictionary<ISymbol, bool>());
callers.TryAdd(owningSymbol, true);
callers.TryAdd(owningSymbol, placeholder);
}, OperationKind.Invocation, OperationKind.FieldReference);
});

Expand Down Expand Up @@ -190,16 +197,18 @@ public override void Initialize(AnalysisContext context)
else if (potentialAntiForgeryFilter.AllInterfaces.Contains(iAsyncAuthorizationFilterTypeSymbol) ||
potentialAntiForgeryFilter.AllInterfaces.Contains(iAuthorizationFilterTypeSymbol))
{
onAuthorizationAsyncMethodSymbols.Add(
onAuthorizationAsyncMethodSymbols.TryAdd(
potentialAntiForgeryFilter
.GetMembers()
.OfType<IMethodSymbol>()
.FirstOrDefault(
s => (s.Name == "OnAuthorizationAsync" ||
s.Name == "OnAuthorization") &&
s.ReturnType.Equals(taskTypeSymbol) &&
s => (s.Name == "OnAuthorizationAsync" &&
s.ReturnType.Equals(taskTypeSymbol) ||
s.Name == "OnAuthorization" &&
s.ReturnsVoid) &&
s.Parameters.Length == 1 &&
s.Parameters[0].Type.Equals(authorizationFilterContextTypeSymbol)));
s.Parameters[0].Type.Equals(authorizationFilterContextTypeSymbol)),
placeholder);
}
}
}
Expand All @@ -215,9 +224,10 @@ public override void Initialize(AnalysisContext context)
var derivedControllerTypeSymbol = (INamedTypeSymbol)symbolAnalysisContext.Symbol;
var baseTypes = derivedControllerTypeSymbol.GetBaseTypes();

// An subtype of `Microsoft.AspNetCore.Mvc.Controller` or `Microsoft.AspNetCore.Mvc.ControllerBase`).
// An subtype of `Microsoft.AspNetCore.Mvc.Controller`, which probably indicates views are used and maybe cookie-based authentication is used and thus CSRF is a concern.
if (baseTypes.Contains(controllerTypeSymbol) ||
baseTypes.Contains(controllerBaseTypeSymbol))
(!onlyLookAtDerivedClassesOfController &&
baseTypes.Contains(controllerBaseTypeSymbol)))
LLLXXXCCC marked this conversation as resolved.
Show resolved Hide resolved
{
// The controller class is not protected by a validate anti forgery token attribute.
if (!IsUsingAntiFogeryAttribute(derivedControllerTypeSymbol))
Expand Down Expand Up @@ -259,13 +269,14 @@ public override void Initialize(AnalysisContext context)
if (httpVerbAttributeTypeSymbolAbleToModify != null)
{
var attributeName = httpVerbAttributeTypeSymbolAbleToModify.AttributeClass.Name;
actionMethodSymbols.Add(
actionMethodSymbols.TryAdd(
(actionMethodSymbol,
attributeName.EndsWith("Attribute", StringComparison.Ordinal) ? attributeName.Remove(attributeName.Length - "Attribute".Length) : attributeName));
attributeName.EndsWith("Attribute", StringComparison.Ordinal) ? attributeName.Remove(attributeName.Length - "Attribute".Length) : attributeName),
placeholder);
}
else if (!actionMethodSymbol.GetAttributes().Any(s => s.AttributeClass.GetBaseTypes().Contains(httpMethodAttributeTypeSymbol)))
{
actionMethodNeedAddingHttpVerbAttributeSymbols.Add((actionMethodSymbol));
actionMethodNeedAddingHttpVerbAttributeSymbols.TryAdd(actionMethodSymbol, placeholder);
}
}
}
Expand Down Expand Up @@ -300,7 +311,7 @@ public override void Initialize(AnalysisContext context)
}
}

foreach (var (methodSymbol, attributeName) in actionMethodSymbols)
foreach (var (methodSymbol, attributeName) in actionMethodSymbols.Keys)
{
compilationAnalysisContext.ReportDiagnostic(
methodSymbol.CreateDiagnostic(
Expand All @@ -309,7 +320,7 @@ public override void Initialize(AnalysisContext context)
attributeName));
}

foreach (var methodSymbol in actionMethodNeedAddingHttpVerbAttributeSymbols)
foreach (var methodSymbol in actionMethodNeedAddingHttpVerbAttributeSymbols.Keys)
{
compilationAnalysisContext.ReportDiagnostic(
methodSymbol.CreateDiagnostic(
Expand Down Expand Up @@ -340,7 +351,8 @@ void FindAllTheSpecifiedCalleeMethods(ISymbol methodSymbol, HashSet<ISymbol> vis

foreach (var child in callingMethods.Keys)
{
if (onAuthorizationAsyncMethodSymbols.Contains(child))
if (child is IMethodSymbol childMethodSymbol &&
onAuthorizationAsyncMethodSymbols.ContainsKey(childMethodSymbol))
Copy link
Member

Choose a reason for hiding this comment

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

Do we know where the IndexOutOfRangeException was being thrown? I presume it is in the indexer access below results[methodSymbol]? If so, I am not sure how this PR would fix that exception.

Copy link
Contributor

@dotpaul dotpaul Sep 18, 2019 •
edited

Choose a reason for hiding this comment

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

@LLLXXXCCC based on your guess of the exception is a thread safety issue, I've come up with an unreliable repro (get the rule to trigger when there are multiple controllers in the project).

https://github.com/dotpaul/repro2844

Replace the Microsoft.NetCore.Analyzers.dll with your own build (maybe with Debugger.Launch() somewhere so you can catch the exceptions), and try building / rebuilding multiple times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, don't remember if I got IndexOutOfRangeExceptions, but definitely ran into some others like:

warning AD0001: Analyzer 'Microsoft.NetCore.Analyzers.Security.UseAutoValidateAntiforgeryToken' threw an exception of type 'System.ArgumentException' with message 'Destination array was not long enough. Check destIndex and length, and the array's lower bounds.'.

warning AD0001: Analyzer 'Microsoft.NetCore.Analyzers.Security.UseAutoValidateAntiforgeryToken' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'.

Copy link
Member

Choose a reason for hiding this comment

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

I think these might all be likely due to use of non-concurrent data structure in this analyzer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@LLLXXXCCC were you able to repro some exceptions being thrown, and verify your changes don't have exceptions?

Would probably be a good idea to see if a unit test with multiple controllers can repro (and should have one anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can reproduce the two warnings you mentioned and the IndexOutOfRangeException. For now, the fix works well.

Copy link
Contributor

@dotpaul dotpaul Sep 27, 2019 •
edited

Choose a reason for hiding this comment

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

Please add a unit test that contains multiple (like 4 or 5) controllers and produces diagnostics, so that we have a test committed that reproduces the original problem.

{
results[methodSymbol].Add(child);
}
Expand Down