Skip to content

Commit

Permalink
Merge pull request #2857 from LLLXXXCCC/UseAutoValidateAntiforgeryToken
Browse files Browse the repository at this point in the history 
Fix IndexOutOfRangeException bug of CA5391.
  • Loading branch information
@dotpaul
dotpaul committed Sep 30, 2019
2 parents 188af5d + 860ec1c commit acf79e6
Show file tree
Hide file tree
Showing 5 changed files with 182 additions and 104 deletions.
31 changes: 22 additions & 9 deletions docs/Analyzer Configuration.md
View file
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
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>();
// 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)))
{
// 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))
{
results[methodSymbol].Add(child);
}
Expand Down

0 comments on commit acf79e6

Please sign in to comment.