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 2 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
9 changes: 9 additions & 0 deletions docs/Analyzer Configuration.md
View file
Open in desktop
Expand Up @@ -298,3 +298,12 @@ Option Values: Integer values of System.Runtime.InteropServices.DllImportSearchP
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`

#### Configure if only look at Controller-derived classes when considering CSRF
LLLXXXCCC marked this conversation as resolved.
Show resolved Hide resolved
Option Name: `only_look_at_derived_classes_of_Controller`

Option Values: Boolean values

Default Value: Specific to each configurable rule ('true' by default for most rules)
LLLXXXCCC marked this conversation as resolved.
Show resolved Hide resolved

Example: `dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = false`
39 changes: 25 additions & 14 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.OnlyLookAtDerivedClassesOfController,
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,7 +107,7 @@ 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 onAuthorizationAsyncMethodSymbols = new ConcurrentDictionary<IMethodSymbol, bool>();
var actionMethodSymbols = new HashSet<(IMethodSymbol, string)>();
var actionMethodNeedAddingHttpVerbAttributeSymbols = new HashSet<IMethodSymbol>();

LLLXXXCCC marked this conversation as resolved.
Show resolved Hide resolved
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.ReturnType.SpecialType == SpecialType.System_Void) &&
LLLXXXCCC marked this conversation as resolved.
Show resolved Hide resolved
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 indicates that cookie-based authentication is used and thus CSRF is a concern.
Copy link
Contributor

Choose a reason for hiding this comment

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

which indicates that cookie-based authentication is used and thus CSRF is a concern. [](start = 76, length = 84)

which probably indicates views are used and maybe cookie-based authentication is used and thus CSRF is a concern.

(Can't be sure. 🙂)

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 @@ -340,7 +350,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
158 changes: 72 additions & 86 deletions src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ public void Test_GlobalAntiForgeryFilter_Add_ChildrenOfIAsyncAuthorizationFilter
using Microsoft.AspNetCore.Mvc.Filters;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

Expand All @@ -42,7 +42,7 @@ public Task OnAuthorizationAsync (AuthorizationFilterContext context)
}
}

class TestClass : ControllerBase
class TestClass : Controller
{
[HttpDelete]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
Expand Down Expand Up @@ -73,21 +73,20 @@ public void Test_GlobalAntiForgeryFilter_Add_ChildrenOfIAuthorizationFilter_NotC
using Microsoft.AspNetCore.Mvc.Filters;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

class FilterClass : IAuthorizationFilter
{
public DefaultAntiforgery defaultAntiforgery;

public Task OnAuthorization (AuthorizationFilterContext context)
public void OnAuthorization (AuthorizationFilterContext context)
{
return null;
}
}

class TestClass : ControllerBase
class TestClass : Controller
{
[HttpDelete]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
Expand All @@ -104,43 +103,21 @@ public void TestMethod ()
filterCollection.Add(typeof(FilterClass));
}
}",
GetCSharpResultAt(26, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete"));
GetCSharpResultAt(25, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete"));
}

[Fact]
public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPostAttribute_Diagnostic()
public void Test_ChildrenOfController_ActionMethodWithHttpPostAndHttpGetAttributes_Diagnostic()
{
VerifyCSharpWithDependencies(@"
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

class TestClass : ControllerBase
{
[HttpPost]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
{
return null;
}
}",
GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPost"));
}

[Fact]
public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPostAndHttpGetAttributes_Diagnostic()
{
VerifyCSharpWithDependencies(@"
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
{
}

class TestClass : ControllerBase
class TestClass : Controller
{
[HttpGet]
[HttpPost]
Expand All @@ -153,63 +130,18 @@ public AcceptedAtActionResult CustomizedActionMethod (string actionName)
}

[Fact]
public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPutAttribute_Diagnostic()
{
VerifyCSharpWithDependencies(@"
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
{
}

class TestClass : ControllerBase
{
[HttpPut]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
{
return null;
}
}",
GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPut"));
}

[Fact]
public void Test_ChildrenOfControllerBase_ActionMethodWithHttpDeleteAttribute_Diagnostic()
{
VerifyCSharpWithDependencies(@"
using System;
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
{
}

class TestClass : ControllerBase
{
[HttpDelete]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
{
return null;
}
}",
GetCSharpResultAt(13, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete"));
}

[Fact]
public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPatchAttribute_Diagnostic()
public void Test_ChildrenOfController_ActionMethodWithHttpPatchAttribute_Diagnostic()
{
VerifyCSharpWithDependencies(@"
using System;
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

class TestClass : ControllerBase
class TestClass : Controller
{
[HttpPatch]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
Expand All @@ -227,7 +159,7 @@ public void Test_ChildrenOfController_ActionMethodWithHttpPostAttribute_Diagnost
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

Expand All @@ -249,7 +181,7 @@ public void Test_ChildrenOfController_ActionMethodWithHttpPutAttribute_Diagnosti
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

Expand All @@ -271,7 +203,7 @@ public void Test_ChildrenOfController_ActionMethodWithHttpDeleteAttribute_Diagno
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

Expand All @@ -297,11 +229,11 @@ public void Test_WithoutValidateAntiForgeryAttribute_ActionMethodWithTwoHttpVerv
using Microsoft.AspNetCore.Mvc.Filters;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

class TestClass : ControllerBase
class TestClass : Controller
{
[HttpDelete]
[HttpPost]
Expand All @@ -323,19 +255,46 @@ public void Test_NoValidateAntiForgeryTokenAttribute_ActionMethodMissingHttpVerb
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Filters;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller
{
}

class TestClass : Controller
{
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
{
return null;
}
}",
GetCSharpResultAt(15, 35, UseAutoValidateAntiforgeryToken.MissHttpVerbAttributeRule, "CustomizedActionMethod"));
}

[Theory]
[InlineData("dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = false")]
public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_DefaultValue_Diagnostic(string editorConfigText)
{
VerifyCSharpAcrossTwoAssemblies(
ASPNetCoreApis.CSharp,
@"
using System;
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
{
}

class TestClass : ControllerBase
{
[HttpDelete]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
{
return null;
}
}",
GetCSharpResultAt(15, 35, UseAutoValidateAntiforgeryToken.MissHttpVerbAttributeRule, "CustomizedActionMethod"));
GetEditorConfigAdditionalFile(editorConfigText),
GetCSharpResultAt(13, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete"));
}

[Fact]
Expand Down Expand Up @@ -1355,6 +1314,33 @@ public void TestMethod ()
}");
}

[Theory]
[InlineData("")]
[InlineData("dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = true")]
public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_NonDefaultValue_NoDiagnostic(string editorConfigText)
{
VerifyCSharpAcrossTwoAssemblies(
ASPNetCoreApis.CSharp,
@"
using System;
using Microsoft.AspNetCore.Mvc;

[MyValidateAntiForgeryAttribute]
class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase
{
}

class TestClass : ControllerBase
{
[HttpDelete]
public AcceptedAtActionResult CustomizedActionMethod (string actionName)
{
return null;
}
}",
GetEditorConfigAdditionalFile(editorConfigText));
}

protected override DiagnosticAnalyzer GetBasicDiagnosticAnalyzer()
{
return new UseAutoValidateAntiforgeryToken();
Expand Down