diff --git a/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj b/src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj index a4df7f736..af314a3e1 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/src/D2L.CodeStyle.Analyzers/Visibility/CorsHeaderAppenderUsageAnalyzer.cs b/src/D2L.CodeStyle.Analyzers/Visibility/CorsHeaderAppenderUsageAnalyzer.cs new file mode 100644 index 000000000..e10a35185 --- /dev/null +++ b/src/D2L.CodeStyle.Analyzers/Visibility/CorsHeaderAppenderUsageAnalyzer.cs @@ -0,0 +1,146 @@ +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.Visibility { + + [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 + ) { + var constructor = context.Node as ConstructorDeclarationSyntax; + if( constructor == null ) { + return; + } + + foreach( var parameter in constructor.ParameterList.Parameters ) { + INamedTypeSymbol paramType = context.SemanticModel + .GetTypeInfo( parameter.Type ) + .Type as INamedTypeSymbol; + + 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 ) + ); + + if( parentSymbols.Any( IsClassWhitelisted ) ) { + 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 + ) { + var instantiation = context.Node as ObjectCreationExpressionSyntax; + if( instantiation == null ) { + return; + } + + INamedTypeSymbol baseType = context.SemanticModel + .GetTypeInfo( instantiation ) + .Type as INamedTypeSymbol; + + if( baseType == null || !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( ISymbol classSymbol ) { + return WhitelistedClasses.Contains( classSymbol.ToString() ); + } + } +} 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..75342c0a4 --- /dev/null +++ b/tests/D2L.CodeStyle.Analyzers.Test/Specs/CorsHeaderAppenderUsageAnalyzer.cs @@ -0,0 +1,97 @@ +// analyzer: D2L.CodeStyle.Analyzers.Visibility.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(); + } + + } + +}