From 85f568b5f0e222437d3d2018031289bf3c7b03ba Mon Sep 17 00:00:00 2001 From: Grant Cleary Date: Thu, 19 Apr 2018 11:37:59 -0400 Subject: [PATCH 1/3] Add new analyzer to prevent injection and instantiation of ICorsHeaderAppender/CorsHeaderAppender --- .../CorsHeaderAppenderUsageAnalyzer.cs | 122 ++++++++++++++++++ .../D2L.CodeStyle.Analyzers.csproj | 1 + src/D2L.CodeStyle.Analyzers/Diagnostics.cs | 10 ++ .../D2L.CodeStyle.Analyzers.Tests.csproj | 3 +- .../Specs/CorsHeaderAppenderUsageAnalyzer.cs | 97 ++++++++++++++ 5 files changed, 232 insertions(+), 1 deletion(-) create mode 100644 src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs create mode 100644 tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs diff --git a/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs b/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs new file mode 100644 index 000000000..8bc5ce0cc --- /dev/null +++ b/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs @@ -0,0 +1,122 @@ +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using D2L.CodeStyle.Analyzers.Extensions; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace D2L.CodeStyle.Analyzers.ApiUsage { + + [DiagnosticAnalyzer( LanguageNames.CSharp )] + internal sealed class CorsHeaderAppenderUsageAnalyzer : DiagnosticAnalyzer { + + private const string CorsInterface = "D2L.LP.Web.Cors.ICorsHeaderAppender"; + private const string CorsClass = "D2L.LP.Web.Cors.CorsHeaderAppender"; + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create( + Diagnostics.DangerousUsageOfCorsHeaderAppender + ); + + public override void Initialize( AnalysisContext context ) { + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction( RegisterCorsHeadersUsageAnalyzer ); + } + + public void RegisterCorsHeadersUsageAnalyzer( CompilationStartAnalysisContext context ) { + INamedTypeSymbol interfaceType = context.Compilation.GetTypeByMetadataName( CorsInterface ); + + if( !interfaceType.IsNullOrErrorType() ) { + context.RegisterSyntaxNodeAction( + ctx => PreventInjection( + ctx, + interfaceType + ), + SyntaxKind.ConstructorDeclaration + ); + } + + INamedTypeSymbol classType = context.Compilation.GetTypeByMetadataName( CorsClass ); + + if( !classType.IsNullOrErrorType() ) { + context.RegisterSyntaxNodeAction( + ctx => PreventManualInstantiation( + ctx, + classType + ), + SyntaxKind.ObjectCreationExpression + ); + } + + } + + /// + /// Prevent injection of the ICorsHttpHeaderHelper interface except in a specific whitelist of classes. + /// + private static void PreventInjection( + SyntaxNodeAnalysisContext context, + INamedTypeSymbol interfaceType + ) { + ConstructorDeclarationSyntax constructor = context.Node as ConstructorDeclarationSyntax; + if( constructor == null ) { + return; + } + + foreach( var parameter in constructor.ParameterList.Parameters ) { + INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( parameter.Type ).Type as INamedTypeSymbol; + + if( baseType.IsNullOrErrorType() || !baseType.Equals( interfaceType ) ) { + return; + } + + var parentClasses = context.Node.Ancestors().Where( a => a.IsKind( SyntaxKind.ClassDeclaration ) ); + var parentSymbols = parentClasses.Select( c => context.SemanticModel.GetDeclaredSymbol( c ) ); + + if( parentSymbols.Any( s => IsClassWhitelisted( s.ToString() ) ) ) { + return; + } + + context.ReportDiagnostic( + Diagnostic.Create( Diagnostics.DangerousUsageOfCorsHeaderAppender, parameter.GetLocation() ) + ); + } + } + + /// + /// We never want to allow manual instantiation of this object, so no whitelist here. + /// + private static void PreventManualInstantiation( + SyntaxNodeAnalysisContext context, + INamedTypeSymbol classType + ) { + ObjectCreationExpressionSyntax instantiation = context.Node as ObjectCreationExpressionSyntax; + if( instantiation == null ) { + return; + } + + INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( instantiation ).Type as INamedTypeSymbol; + + if( baseType.IsNullOrErrorType() || !baseType.Equals( classType ) ) { + return; + } + + context.ReportDiagnostic( + Diagnostic.Create( Diagnostics.DangerousUsageOfCorsHeaderAppender, context.Node.GetLocation() ) + ); + } + + /// + /// A list of classes that have been cleared to implement these headers + /// + private static readonly ImmutableHashSet WhitelistedClasses = new HashSet { + "D2L.LP.Web.ContentHandling.Handlers.ContentHttpHandler", + "D2L.LP.Web.Files.FileViewing.StreamFileViewerResult", + "D2L.LP.Web.Files.FileViewing.Default.StreamFileViewerResultFactory" + }.ToImmutableHashSet(); + + private static bool IsClassWhitelisted( string className ) { + return WhitelistedClasses.Contains( className ); + } + } +} diff --git a/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj b/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj index a4df7f736..77ea96fc0 100644 --- a/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj +++ b/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj @@ -31,6 +31,7 @@ 4 + diff --git a/src/D2L.CodeStyle.Analyzers/Diagnostics.cs b/src/D2L.CodeStyle.Analyzers/Diagnostics.cs index 155a183ce..1a86367c8 100644 --- a/src/D2L.CodeStyle.Analyzers/Diagnostics.cs +++ b/src/D2L.CodeStyle.Analyzers/Diagnostics.cs @@ -271,5 +271,15 @@ public static class Diagnostics { isEnabledByDefault: true, description: "Unnecessary mutability annotations should be removed to keep the code base clean" ); + + public static readonly DiagnosticDescriptor DangerousUsageOfCorsHeaderAppender = new DiagnosticDescriptor( + id: "D2L0031", + title: "ICorsHeaderAppender should not be used, as it can introduce security vulnerabilities.", + messageFormat: "ICorsHeaderAppender should not be used, as it can introduce security vulnerabilities.", + category: "Safety", + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: "ICorsHeaderAppender should not be used, as it can introduce security vulnerabilities." + ); } } diff --git a/tests/D2L.CodeStyle.Analyzers.Test/D2L.CodeStyle.Analyzers.Tests.csproj b/tests/D2L.CodeStyle.Analyzers.Test/D2L.CodeStyle.Analyzers.Tests.csproj index 31008d687..f76c3b9b4 100644 --- a/tests/D2L.CodeStyle.Analyzers.Test/D2L.CodeStyle.Analyzers.Tests.csproj +++ b/tests/D2L.CodeStyle.Analyzers.Test/D2L.CodeStyle.Analyzers.Tests.csproj @@ -47,6 +47,7 @@ + @@ -167,4 +168,4 @@ --> - + \ No newline at end of file diff --git a/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs b/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs new file mode 100644 index 000000000..1834545b0 --- /dev/null +++ b/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs @@ -0,0 +1,97 @@ +// analyzer: D2L.CodeStyle.Analyzers.ApiUsage.CorsHeaderAppenderUsageAnalyzer + +using D2L.LP.Web.Cors; + +namespace D2L.LP.Web.Cors { + + public interface ICorsHeaderAppender { } + + public class CorsHeaderAppender : ICorsHeaderAppender { + + public CorsHeaderAppender() { } + + public CorsHeaderAppender( int i ) { } + + } + + public interface ISomeOtherSafeInterface { } + + public class SomeOtherSafeClass : ISomeOtherSafeInterface { } + +} + +namespace D2L.LP.Web.ContentHandling.Handlers { + + public class ContentHttpHandler { // Whitelisted by necessity + + public ContentHttpHandler( + ICorsHeaderAppender corsHelper + ) { } + + } + +} + +namespace D2L.LP.Web.Files.FileViewing { // Whitelisted by necessity + + public class StreamFileViewerResult { + + public StreamFileViewerResult( + ICorsHeaderAppender corsHelper + ) { } + + } + +} + +namespace D2L.LP.Web.Files.FileViewing.Default { + + public class StreamFileViewerResultFactory { // Whitelisted by necessity + + public StreamFileViewerResultFactory( + ICorsHeaderAppender corsHelper + ) { } + + } + +} + +namespace D2L.CodeStyle.Analyzers.CorsHeaderAppenderUsageAnalyzer.Examples { + + public sealed class BadClass { + + public BadClass( + /* DangerousUsageOfCorsHeaderAppender */ ICorsHeaderAppender corsHelper /**/ + ) { } + + public void UsesCorsHelper_ManualInstantiation_NoParams() { + ICorsHeaderAppender corsHelper = /* DangerousUsageOfCorsHeaderAppender */ new CorsHeaderAppender() /**/; + } + + public void UsesCorsHelper_ManualInstantiation_WithParams() { + const int i = 10; + ICorsHeaderAppender corsHelper = /* DangerousUsageOfCorsHeaderAppender */ new CorsHeaderAppender( i ) /**/; + } + + public void UsesCorsHelper_ManualInstantiation_OnlyInstantiationTriggersDiagnostic() { + CorsHeaderAppender corsHelper = /* DangerousUsageOfCorsHeaderAppender */ new CorsHeaderAppender() /**/; + } + } + + public sealed class GoodClass { + + ISomeOtherSafeInterface m_safeInterface; + + public GoodClass( + ISomeOtherSafeInterface safeInterface + ) { + m_safeInterface = safeInterface; + } + + public void SafeMethod() { + ISomeOtherSafeInterface safeInterface = new SomeOtherSafeClass(); + } + + } + +} From 85c3775815c6a4aac850e26be7b91e5c663fa3c1 Mon Sep 17 00:00:00 2001 From: Grant Cleary Date: Thu, 19 Apr 2018 14:28:24 -0400 Subject: [PATCH 2/3] Switching IsClassWhiteListed to take in ISymbol; replacing INamedTypeSymbol.IsNullOrError checks with simple null checks --- .../CorsHeaderAppenderUsageAnalyzer.cs | 68 +++++++++++++------ 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs b/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs index 8bc5ce0cc..0b7d82e1c 100644 --- a/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs +++ b/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs @@ -15,17 +15,23 @@ internal sealed class CorsHeaderAppenderUsageAnalyzer : DiagnosticAnalyzer { private const string CorsInterface = "D2L.LP.Web.Cors.ICorsHeaderAppender"; private const string CorsClass = "D2L.LP.Web.Cors.CorsHeaderAppender"; - public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create( - Diagnostics.DangerousUsageOfCorsHeaderAppender - ); + public override ImmutableArray SupportedDiagnostics + => ImmutableArray.Create( + Diagnostics.DangerousUsageOfCorsHeaderAppender + ); public override void Initialize( AnalysisContext context ) { context.EnableConcurrentExecution(); - context.RegisterCompilationStartAction( RegisterCorsHeadersUsageAnalyzer ); + context.RegisterCompilationStartAction( + RegisterCorsHeadersUsageAnalyzer + ); } - public void RegisterCorsHeadersUsageAnalyzer( CompilationStartAnalysisContext context ) { - INamedTypeSymbol interfaceType = context.Compilation.GetTypeByMetadataName( CorsInterface ); + public void RegisterCorsHeadersUsageAnalyzer( + CompilationStartAnalysisContext context + ) { + INamedTypeSymbol interfaceType = context.Compilation + .GetTypeByMetadataName( CorsInterface ); if( !interfaceType.IsNullOrErrorType() ) { context.RegisterSyntaxNodeAction( @@ -37,7 +43,8 @@ internal sealed class CorsHeaderAppenderUsageAnalyzer : DiagnosticAnalyzer { ); } - INamedTypeSymbol classType = context.Compilation.GetTypeByMetadataName( CorsClass ); + INamedTypeSymbol classType = context.Compilation + .GetTypeByMetadataName( CorsClass ); if( !classType.IsNullOrErrorType() ) { context.RegisterSyntaxNodeAction( @@ -52,57 +59,74 @@ internal sealed class CorsHeaderAppenderUsageAnalyzer : DiagnosticAnalyzer { } /// - /// Prevent injection of the ICorsHttpHeaderHelper interface except in a specific whitelist of classes. + /// Prevent injection of the ICorsHttpHeaderHelper interface except in + /// a specific whitelist of classes. /// private static void PreventInjection( SyntaxNodeAnalysisContext context, INamedTypeSymbol interfaceType ) { - ConstructorDeclarationSyntax constructor = context.Node as ConstructorDeclarationSyntax; + var constructor = context.Node as ConstructorDeclarationSyntax; if( constructor == null ) { return; } foreach( var parameter in constructor.ParameterList.Parameters ) { - INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( parameter.Type ).Type as INamedTypeSymbol; + INamedTypeSymbol paramType = context.SemanticModel + .GetTypeInfo( parameter.Type ) + .Type as INamedTypeSymbol; - if( baseType.IsNullOrErrorType() || !baseType.Equals( interfaceType ) ) { + if( paramType == null || !paramType.Equals( interfaceType ) ) { return; } - var parentClasses = context.Node.Ancestors().Where( a => a.IsKind( SyntaxKind.ClassDeclaration ) ); - var parentSymbols = parentClasses.Select( c => context.SemanticModel.GetDeclaredSymbol( c ) ); + var parentClasses = context.Node + .Ancestors() + .Where( a => a.IsKind( SyntaxKind.ClassDeclaration ) ); + + var parentSymbols = parentClasses.Select( + c => context.SemanticModel.GetDeclaredSymbol( c ) + ); - if( parentSymbols.Any( s => IsClassWhitelisted( s.ToString() ) ) ) { + if( parentSymbols.Any( IsClassWhitelisted ) ) { return; } context.ReportDiagnostic( - Diagnostic.Create( Diagnostics.DangerousUsageOfCorsHeaderAppender, parameter.GetLocation() ) + Diagnostic.Create( + Diagnostics.DangerousUsageOfCorsHeaderAppender, + parameter.GetLocation() + ) ); } } /// - /// We never want to allow manual instantiation of this object, so no whitelist here. + /// We never want to allow manual instantiation of this object, so no + /// whitelist here. /// private static void PreventManualInstantiation( SyntaxNodeAnalysisContext context, INamedTypeSymbol classType ) { - ObjectCreationExpressionSyntax instantiation = context.Node as ObjectCreationExpressionSyntax; + var instantiation = context.Node as ObjectCreationExpressionSyntax; if( instantiation == null ) { return; } - INamedTypeSymbol baseType = context.SemanticModel.GetTypeInfo( instantiation ).Type as INamedTypeSymbol; + INamedTypeSymbol baseType = context.SemanticModel + .GetTypeInfo( instantiation ) + .Type as INamedTypeSymbol; - if( baseType.IsNullOrErrorType() || !baseType.Equals( classType ) ) { + if( baseType == null || !baseType.Equals( classType ) ) { return; } context.ReportDiagnostic( - Diagnostic.Create( Diagnostics.DangerousUsageOfCorsHeaderAppender, context.Node.GetLocation() ) + Diagnostic.Create( + Diagnostics.DangerousUsageOfCorsHeaderAppender, + context.Node.GetLocation() + ) ); } @@ -115,8 +139,8 @@ INamedTypeSymbol classType "D2L.LP.Web.Files.FileViewing.Default.StreamFileViewerResultFactory" }.ToImmutableHashSet(); - private static bool IsClassWhitelisted( string className ) { - return WhitelistedClasses.Contains( className ); + private static bool IsClassWhitelisted( ISymbol classSymbol ) { + return WhitelistedClasses.Contains( classSymbol.ToString() ); } } } From f0a556d14b34b5de54bcc67e0f9a15083ebdc25e Mon Sep 17 00:00:00 2001 From: Grant Cleary Date: Thu, 19 Apr 2018 14:57:37 -0400 Subject: [PATCH 3/3] Move CorsHeaderAppenderUsageAnalyzer to the new D2L.CodeStyle.Analyzers.Visibility namespace (for ease of future generalization) --- src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj | 2 +- .../{ApiUsage => Visibility}/CorsHeaderAppenderUsageAnalyzer.cs | 2 +- .../Specs/CorsHeaderAppenderUsageAnalyzer.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename src/D2L.CodeStyle.Analyzers/{ApiUsage => Visibility}/CorsHeaderAppenderUsageAnalyzer.cs (95%) diff --git a/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj b/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj index 77ea96fc0..af314a3e1 100644 --- a/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj +++ b/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj @@ -31,7 +31,7 @@ 4 - + diff --git a/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs b/src/D2L.CodeStyle.Analyzers/Visibility/CorsHeaderAppenderUsageAnalyzer.cs similarity index 95% rename from src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs rename to src/D2L.CodeStyle.Analyzers/Visibility/CorsHeaderAppenderUsageAnalyzer.cs index 0b7d82e1c..e10a35185 100644 --- a/src/D2L.CodeStyle.Analyzers/ApiUsage/CorsHeaderAppenderUsageAnalyzer.cs +++ b/src/D2L.CodeStyle.Analyzers/Visibility/CorsHeaderAppenderUsageAnalyzer.cs @@ -7,7 +7,7 @@ using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; -namespace D2L.CodeStyle.Analyzers.ApiUsage { +namespace D2L.CodeStyle.Analyzers.Visibility { [DiagnosticAnalyzer( LanguageNames.CSharp )] internal sealed class CorsHeaderAppenderUsageAnalyzer : DiagnosticAnalyzer { diff --git a/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs b/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs index 1834545b0..75342c0a4 100644 --- a/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs +++ b/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs @@ -1,4 +1,4 @@ -// analyzer: D2L.CodeStyle.Analyzers.ApiUsage.CorsHeaderAppenderUsageAnalyzer +// analyzer: D2L.CodeStyle.Analyzers.Visibility.CorsHeaderAppenderUsageAnalyzer using D2L.LP.Web.Cors;