diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S4834.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S4834.json new file mode 100644 index 00000000000..6d6089da86a --- /dev/null +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S4834.json @@ -0,0 +1,69 @@ +{ +"issues": [ +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy\Owin\NancyMiddleware.cs", +"region": { +"startLine": 253, +"startColumn": 57, +"endLine": 253, +"endColumn": 73 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs", +"region": { +"startLine": 18, +"startColumn": 28, +"endLine": 18, +"endColumn": 43 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs", +"region": { +"startLine": 29, +"startColumn": 28, +"endLine": 29, +"endColumn": 37 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs", +"region": { +"startLine": 40, +"startColumn": 28, +"endLine": 40, +"endColumn": 39 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy\Security\ClaimsPrincipalExtensions.cs", +"region": { +"startLine": 52, +"startColumn": 28, +"endLine": 52, +"endColumn": 42 +} +} +} +] +} diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication-{5F2DD52D-471B-4946-8F7B-F050C88EFC52}-S4834.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication-{5F2DD52D-471B-4946-8F7B-F050C88EFC52}-S4834.json new file mode 100644 index 00000000000..0085d68febd --- /dev/null +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication-{5F2DD52D-471B-4946-8F7B-F050C88EFC52}-S4834.json @@ -0,0 +1,30 @@ +{ +"issues": [ +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication\AuthenticationBootstrapper.cs", +"region": { +"startLine": 27, +"startColumn": 39, +"endLine": 27, +"endColumn": 116 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication\AuthenticationBootstrapper.cs", +"region": { +"startLine": 27, +"startColumn": 59, +"endLine": 27, +"endColumn": 115 +} +} +} +] +} diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Basic-{911196F4-B88F-4940-B1FD-D30F5290D06D}-S4834.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Basic-{911196F4-B88F-4940-B1FD-D30F5290D06D}-S4834.json new file mode 100644 index 00000000000..7a813f2872b --- /dev/null +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Basic-{911196F4-B88F-4940-B1FD-D30F5290D06D}-S4834.json @@ -0,0 +1,30 @@ +{ +"issues": [ +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Basic\UserValidator.cs", +"region": { +"startLine": 14, +"startColumn": 24, +"endLine": 14, +"endColumn": 74 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Basic\UserValidator.cs", +"region": { +"startLine": 14, +"startColumn": 44, +"endLine": 14, +"endColumn": 73 +} +} +} +] +} diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Forms-{98940A30-1B48-4F71-A6BA-85F0AAF31A2F}-S4834.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Forms-{98940A30-1B48-4F71-A6BA-85F0AAF31A2F}-S4834.json new file mode 100644 index 00000000000..ce30e3ce37e --- /dev/null +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Forms-{98940A30-1B48-4F71-A6BA-85F0AAF31A2F}-S4834.json @@ -0,0 +1,30 @@ +{ +"issues": [ +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Forms\UserDatabase.cs", +"region": { +"startLine": 27, +"startColumn": 26, +"endLine": 27, +"endColumn": 84 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Forms\UserDatabase.cs", +"region": { +"startLine": 27, +"startColumn": 46, +"endLine": 27, +"endColumn": 83 +} +} +} +] +} diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Stateless-{BAE74CD5-57C2-40E3-8F7A-EDE5721C2ACC}-S4834.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Stateless-{BAE74CD5-57C2-40E3-8F7A-EDE5721C2ACC}-S4834.json new file mode 100644 index 00000000000..9eb5c5af70f --- /dev/null +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Demo.Authentication.Stateless-{BAE74CD5-57C2-40E3-8F7A-EDE5721C2ACC}-S4834.json @@ -0,0 +1,30 @@ +{ +"issues": [ +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Stateless\UserDatabase.cs", +"region": { +"startLine": 30, +"startColumn": 20, +"endLine": 30, +"endColumn": 91 +} +} +}, +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Demo.Authentication.Stateless\UserDatabase.cs", +"region": { +"startLine": 30, +"startColumn": 40, +"endLine": 30, +"endColumn": 90 +} +} +} +] +} diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Hosting.Self-{AA7F66EB-EC2C-47DE-855F-30B3E6EF2134}-S4834.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Hosting.Self-{AA7F66EB-EC2C-47DE-855F-30B3E6EF2134}-S4834.json new file mode 100644 index 00000000000..65da8131e69 --- /dev/null +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy.Hosting.Self-{AA7F66EB-EC2C-47DE-855F-30B3E6EF2134}-S4834.json @@ -0,0 +1,17 @@ +{ +"issues": [ +{ +"id": "S4834", +"message": "Make sure that permissions are controlled safely here.", +"location": { +"uri": "sources\Nancy\src\Nancy.Hosting.Self\NancyHost.cs", +"region": { +"startLine": 213, +"startColumn": 19, +"endLine": 213, +"endColumn": 47 +} +} +} +] +} diff --git a/sonaranalyzer-dotnet/rspec/cs/S4834_c#.html b/sonaranalyzer-dotnet/rspec/cs/S4834_c#.html new file mode 100644 index 00000000000..1f58876d597 --- /dev/null +++ b/sonaranalyzer-dotnet/rspec/cs/S4834_c#.html @@ -0,0 +1,80 @@ +

Controlling permissions is security-sensitive. It has led in the past to the following vulnerabilities:

+ +

Attackers can only damage what they have access to. Thus limiting their access is a good way to prevent them from wreaking havoc, but it has to be +done properly.

+

This rule flags code that controls the access to resources and actions or configures this access. The goal is to guide security code reviews.

+

Ask Yourself Whether

+ +

You are at risk if you answered yes to the first question and any of the following ones.

+

Recommended Secure Coding Practices

+

The first step is to restrict all sensitive actions to authenticated users.

+

Each user should have the lowest privileges possible. The access control granularity should match the sensitivity of each resource or action. The +more sensitive it is, the less people should have access to it.

+

Do not base the access control on a user input or on a value which might have been tampered with. For example, the developer should not read a +user's permissions from an HTTP cookie as it can be modified client-side.

+

Check that the access to each action and resource is properly restricted.

+

Enable administrators to swiftly remove permissions when necessary. This enables them to reduce the time an attacker can have access to your +systems when a breach occurs.

+

Log and monitor refused access requests as they can reveal an attack.

+

Questionable Code Example

+
+using System.Threading;
+using System.Security.Permissions;
+using System.Security.Principal;
+using System.IdentityModel.Tokens;
+
+class SecurityPrincipalDemo
+{
+    class MyIdentity : IIdentity // Questionable, custom IIdentity implementations should be reviewed
+    {
+        // ...
+    }
+
+    class MyPrincipal : IPrincipal // Questionable, custom IPrincipal implementations should be reviewed
+    {
+        // ...
+    }
+    [System.Security.Permissions.PrincipalPermission(SecurityAction.Demand, Role = "Administrators")] // Questionable. The access restrictions enforced by this attribute should be reviewed.
+    static void CheckAdministrator()
+    {
+        WindowsIdentity MyIdentity = WindowsIdentity.GetCurrent(); // Questionable
+        HttpContext.User = ...; // Questionable: review all reference (set and get) to System.Web HttpContext.User
+        AppDomain domain = AppDomain.CurrentDomain;
+        domain.SetPrincipalPolicy(PrincipalPolicy.WindowsPrincipal); // Questionable
+        MyIdentity identity = new MyIdentity(); // Questionable
+        MyPrincipal MyPrincipal = new MyPrincipal(MyIdentity); // Questionable
+        Thread.CurrentPrincipal = MyPrincipal; // Questionable
+        domain.SetThreadPrincipal(MyPrincipal); // Questionable
+
+        // All instantiation of PrincipalPermission should be reviewed.
+        PrincipalPermission principalPerm = new PrincipalPermission(null, "Administrators"); // Questionable
+        principalPerm.Demand();
+
+        SecurityTokenHandler handler = ...;
+        // Questionable: this creates an identity.
+        ReadOnlyCollection<ClaimsIdentity> identities = handler.ValidateToken(…);
+    }
+
+     // Questionable: review how this function uses the identity and principal.
+    void modifyPrincipal(MyIdentity identity, MyPrincipal principal)
+    {
+        // ...
+    }
+}
+
+

See

+ + diff --git a/sonaranalyzer-dotnet/rspec/cs/S4834_c#.json b/sonaranalyzer-dotnet/rspec/cs/S4834_c#.json new file mode 100644 index 00000000000..575c781753f --- /dev/null +++ b/sonaranalyzer-dotnet/rspec/cs/S4834_c#.json @@ -0,0 +1,21 @@ +{ + "title": "Controlling permissions is security-sensitive", + "type": "SECURITY_HOTSPOT", + "status": "ready", + "tags": [ + "sans-top25-porous", + "owasp-a5" + ], + "standards": [ + "OWASP Top Ten" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-4834", + "sqKey": "S4834", + "scope": "Main", + "securityStandards": { + "OWASP": [ + "A5" + ] + } +} diff --git a/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json b/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json index 825507be2b5..8e3c1accd1c 100644 --- a/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json +++ b/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json @@ -233,6 +233,7 @@ "S4818", "S4823", "S4825", - "S4829" + "S4829", + "S4834" ] } diff --git a/sonaranalyzer-dotnet/rspec/vbnet/S4834_vb.net.html b/sonaranalyzer-dotnet/rspec/vbnet/S4834_vb.net.html new file mode 100644 index 00000000000..3f0c62bbd89 --- /dev/null +++ b/sonaranalyzer-dotnet/rspec/vbnet/S4834_vb.net.html @@ -0,0 +1,76 @@ +

Controlling permissions is security-sensitive. It has led in the past to the following vulnerabilities:

+ +

Attackers can only damage what they have access to. Thus limiting their access is a good way to prevent them from wreaking havoc, but it has to be +done properly.

+

This rule flags code that controls the access to resources and actions or configures this access. The goal is to guide security code reviews.

+

Ask Yourself Whether

+ +

You are at risk if you answered yes to the first question and any of the following ones.

+

Recommended Secure Coding Practices

+

The first step is to restrict all sensitive actions to authenticated users.

+

Each user should have the lowest privileges possible. The access control granularity should match the sensitivity of each resource or action. The +more sensitive it is, the less people should have access to it.

+

Do not base the access control on a user input or on a value which might have been tampered with. For example, the developer should not read a +user's permissions from an HTTP cookie as it can be modified client-side.

+

Check that the access to each action and resource is properly restricted.

+

Enable administrators to swiftly remove permissions when necessary. This enables them to reduce the time an attacker can have access to your +systems when a breach occurs.

+

Log and monitor refused access requests as they can reveal an attack.

+

Questionable Code Example

+
+Imports System.Threading
+Imports System.Security.Permissions
+Imports System.Security.Principal
+Imports System.IdentityModel.Tokens
+
+Class SecurityPrincipalDemo
+    Class MyIdentity
+        Implements IIdentity ' Questionable, custom IIdentity implementations should be reviewed
+    End Class
+
+    Class MyPrincipal
+        Implements IPrincipal ' Questionable, custom IPrincipal implementations should be reviewed
+    End Class
+
+    <System.Security.Permissions.PrincipalPermission(SecurityAction.Demand, Role:="Administrators")> ' Questionable. The access restrictions enforced by this attribute should be reviewed.
+    Private Shared Sub CheckAdministrator()
+        Dim MyIdentity As WindowsIdentity = WindowsIdentity.GetCurrent() ' Questionable
+
+        HttpContext.User = ... ' Questionable: review all reference (set and get) to System.Web HttpContext.User
+
+        Dim domain As AppDomain = AppDomain.CurrentDomain
+        domain.SetPrincipalPolicy(PrincipalPolicy.WindowsPrincipal) ' Questionable
+
+        Dim identity As MyIdentity = New MyIdentity() ' Questionable
+        Dim MyPrincipal As MyPrincipal = New MyPrincipal(MyIdentity) ' Questionable
+        Thread.CurrentPrincipal = MyPrincipal ' Questionable
+        domain.SetThreadPrincipal(MyPrincipal) ' Questionable
+
+        Dim principalPerm As PrincipalPermission = New PrincipalPermission(Nothing, "Administrators")  ' Questionable
+        principalPerm.Demand()
+
+        Dim handler As SecurityTokenHandler = ...
+        Dim identities As ReadOnlyCollection(Of ClaimsIdentity) = handler.ValidateToken()  ' Questionable, this creates identity
+    End Sub
+
+    ' Questionable: review how this function uses the identity and principal.
+    Private Sub modifyPrincipal(ByVal identity As MyIdentity, ByVal principal As MyPrincipal)
+    End Sub
+End Class
+
+

See

+ + diff --git a/sonaranalyzer-dotnet/rspec/vbnet/S4834_vb.net.json b/sonaranalyzer-dotnet/rspec/vbnet/S4834_vb.net.json new file mode 100644 index 00000000000..575c781753f --- /dev/null +++ b/sonaranalyzer-dotnet/rspec/vbnet/S4834_vb.net.json @@ -0,0 +1,21 @@ +{ + "title": "Controlling permissions is security-sensitive", + "type": "SECURITY_HOTSPOT", + "status": "ready", + "tags": [ + "sans-top25-porous", + "owasp-a5" + ], + "standards": [ + "OWASP Top Ten" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-4834", + "sqKey": "S4834", + "scope": "Main", + "securityStandards": { + "OWASP": [ + "A5" + ] + } +} diff --git a/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile.json b/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile.json index 7d11037403f..c4c2818bf92 100644 --- a/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile.json +++ b/sonaranalyzer-dotnet/rspec/vbnet/Sonar_way_profile.json @@ -87,6 +87,7 @@ "S4818", "S4823", "S4825", - "S4829" + "S4829", + "S4834" ] } diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodDeclarationTracker.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodDeclarationTracker.cs index 0235dab29cf..240024ba08c 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodDeclarationTracker.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodDeclarationTracker.cs @@ -51,7 +51,8 @@ public override MethodDeclarationCondition ParameterAtIndexIsUsed(int index) => } var methodDeclaration = context.MethodSymbol.DeclaringSyntaxReferences - .Select(r => (MethodDeclarationSyntax)r.GetSyntax()) + .Select(r => r.GetSyntax()) + .OfType() .FirstOrDefault(declaration => declaration.HasBodyOrExpressionBody()); if (methodDeclaration == null) @@ -74,8 +75,27 @@ public override MethodDeclarationCondition ParameterAtIndexIsUsed(int index) => }); }; - - protected override SyntaxToken GetMethodIdentifier(SyntaxNode methodDeclaration) => - ((MethodDeclarationSyntax)methodDeclaration).Identifier; + protected override SyntaxToken? GetMethodIdentifier(SyntaxNode methodDeclaration) + { + switch (methodDeclaration?.Kind()) + { + case SyntaxKind.MethodDeclaration: + return ((MethodDeclarationSyntax)methodDeclaration).Identifier; + case SyntaxKind.ConstructorDeclaration: + return ((ConstructorDeclarationSyntax)methodDeclaration).Identifier; + case SyntaxKind.DestructorDeclaration: + return ((DestructorDeclarationSyntax)methodDeclaration).Identifier; + case SyntaxKind.AddAccessorDeclaration: + case SyntaxKind.RemoveAccessorDeclaration: + return ((EventDeclarationSyntax)methodDeclaration.Parent.Parent).Identifier; + case SyntaxKind.GetAccessorDeclaration: + case SyntaxKind.SetAccessorDeclaration: + return ((PropertyDeclarationSyntax)methodDeclaration.Parent.Parent).Identifier; + case SyntaxKind.OperatorDeclaration: + return ((OperatorDeclarationSyntax)methodDeclaration).OperatorToken; + default: + return null; + } + } } } diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/Hotspots/ControllingPermissions.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/Hotspots/ControllingPermissions.cs new file mode 100644 index 00000000000..3dc8b085502 --- /dev/null +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/Hotspots/ControllingPermissions.cs @@ -0,0 +1,55 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2018 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; +using SonarAnalyzer.Common; +using SonarAnalyzer.Helpers; + +namespace SonarAnalyzer.Rules.CSharp +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + [Rule(DiagnosticId)] + public sealed class ControllingPermissions : ControllingPermissionsBase + { + private static readonly DiagnosticDescriptor rule = + DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager) + .WithNotConfigurable(); + + public override ImmutableArray SupportedDiagnostics { get; } = + ImmutableArray.Create(rule); + + public ControllingPermissions() + : this(new DefaultAnalyzerConfiguration()) + { + } + + internal /*for testing*/ ControllingPermissions(IAnalyzerConfiguration analysisConfiguration) + { + ObjectCreationTracker = new CSharpObjectCreationTracker(analysisConfiguration, rule); + InvocationTracker = new CSharpInvocationTracker(analysisConfiguration, rule); + PropertyAccessTracker = new CSharpPropertyAccessTracker(analysisConfiguration, rule); + MethodDeclarationTracker = new CSharpMethodDeclarationTracker(analysisConfiguration, rule); + BaseTypeTracker = new CSharpBaseTypeTracker(analysisConfiguration, rule); + } + } +} diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/BaseTypeTracker.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/BaseTypeTracker.cs index e6c1a504b31..003ce478b26 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/BaseTypeTracker.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/BaseTypeTracker.cs @@ -103,7 +103,7 @@ internal BaseClassCondition WhenDerivesFrom(KnownType type) => { foreach(var baseTypeNode in context.AllBaseTypeNodes) { - if (context.Model.GetTypeInfo(baseTypeNode).Type?.DerivesFrom(type) ?? false) + if (context.Model.GetTypeInfo(baseTypeNode).Type?.DerivesOrImplements(type) ?? false) { issueLocation = baseTypeNode.GetLocation(); return true; // assume there won't be more than one matching node diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/KnownType.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/KnownType.cs index 8dd1485737b..54850732b14 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/KnownType.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/KnownType.cs @@ -86,6 +86,7 @@ internal sealed class KnownType internal static readonly KnownType System_Action_T1_T2_T3_T4 = new KnownType("System.Action"); internal static readonly KnownType System_Activator = new KnownType("System.Activator"); internal static readonly KnownType System_ApplicationException = new KnownType("System.ApplicationException"); + internal static readonly KnownType System_AppDomain = new KnownType("System.AppDomain"); internal static readonly KnownType System_ArgumentException = new KnownType("System.ArgumentException"); internal static readonly KnownType System_ArgumentNullException = new KnownType("System.ArgumentNullException"); internal static readonly KnownType System_ArgumentOutOfRangeException = new KnownType("System.ArgumentOutOfRangeException"); @@ -202,6 +203,7 @@ internal sealed class KnownType internal static readonly KnownType System_Guid = new KnownType("System.Guid"); internal static readonly KnownType System_IComparable = new KnownType("System.IComparable"); internal static readonly KnownType System_IComparable_T = new KnownType("System.IComparable"); + internal static readonly KnownType System_IdentityModel_Tokens_SecurityTokenHandler = new KnownType("System.IdentityModel.Tokens.SecurityTokenHandler"); internal static readonly KnownType System_IDisposable = new KnownType(SpecialType.System_IDisposable, "System.IDisposable"); internal static readonly KnownType System_IEquatable_T = new KnownType("System.IEquatable"); internal static readonly KnownType System_IFormatProvider = new KnownType("System.IFormatProvider"); @@ -297,7 +299,12 @@ internal sealed class KnownType internal static readonly KnownType System_Security_Cryptography_SymmetricAlgorithm = new KnownType("System.Security.Cryptography.SymmetricAlgorithm"); internal static readonly KnownType System_Security_Cryptography_TripleDES = new KnownType("System.Security.Cryptography.TripleDES"); internal static readonly KnownType System_Security_Permissions_CodeAccessSecurityAttribute = new KnownType("System.Security.Permissions.CodeAccessSecurityAttribute"); + internal static readonly KnownType System_Security_Permissions_PrincipalPermission = new KnownType("System.Security.Permissions.PrincipalPermission"); + internal static readonly KnownType System_Security_Permissions_PrincipalPermissionAttribute = new KnownType("System.Security.Permissions.PrincipalPermissionAttribute"); internal static readonly KnownType System_Security_PermissionSet = new KnownType("System.Security.PermissionSet"); + internal static readonly KnownType System_Security_Principal_IIdentity = new KnownType("System.Security.Principal.IIdentity"); + internal static readonly KnownType System_Security_Principal_IPrincipal = new KnownType("System.Security.Principal.IPrincipal"); + internal static readonly KnownType System_Security_Principal_WindowsIdentity = new KnownType("System.Security.Principal.WindowsIdentity"); internal static readonly KnownType System_Security_SecurityCriticalAttribute = new KnownType("System.Security.SecurityCriticalAttribute"); internal static readonly KnownType System_Security_SecuritySafeCriticalAttribute = new KnownType("System.Security.SecuritySafeCriticalAttribute"); internal static readonly KnownType System_SerializableAttribute = new KnownType("System.SerializableAttribute"); @@ -328,6 +335,7 @@ internal sealed class KnownType internal static readonly KnownType System_ValueType = new KnownType(SpecialType.System_ValueType, "ValueType"); internal static readonly KnownType System_Web_HttpApplication = new KnownType("System.Web.HttpApplication"); internal static readonly KnownType System_Web_HttpCookie = new KnownType("System.Web.HttpCookie"); + internal static readonly KnownType System_Web_HttpContext = new KnownType("System.Web.HttpContext"); internal static readonly KnownType System_Web_Http_ApiController = new KnownType("System.Web.Http.ApiController"); internal static readonly KnownType System_Web_HttpRequestBase = new KnownType("System.Web.HttpRequestBase"); internal static readonly KnownType System_Web_HttpResponseBase = new KnownType("System.Web.HttpResponseBase"); diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/MethodDeclarationTracker.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/MethodDeclarationTracker.cs index 995bc42cce4..a5d2ccb3836 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/MethodDeclarationTracker.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/MethodDeclarationTracker.cs @@ -18,6 +18,7 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; @@ -35,7 +36,7 @@ protected MethodDeclarationTracker(IAnalyzerConfiguration analysisConfiguration, { } - protected abstract SyntaxToken GetMethodIdentifier(SyntaxNode methodDeclaration); + protected abstract SyntaxToken? GetMethodIdentifier(SyntaxNode methodDeclaration); public void Track(SonarAnalysisContext context, params MethodDeclarationCondition[] conditions) { @@ -55,7 +56,10 @@ void TrackMethodDeclaration(SymbolAnalysisContext c) foreach (var declaration in c.Symbol.DeclaringSyntaxReferences) { var methodIdentifier = GetMethodIdentifier(declaration.GetSyntax()); - c.ReportDiagnosticWhenActive(Diagnostic.Create(Rule, methodIdentifier.GetLocation())); + if (methodIdentifier.HasValue) + { + c.ReportDiagnosticWhenActive(Diagnostic.Create(Rule, methodIdentifier.Value.GetLocation())); + } } } } @@ -67,12 +71,28 @@ bool IsTrackedMethod(IMethodSymbol methodSymbol, Compilation compilation) } } + internal MethodDeclarationCondition AnyParameterIsOfType(params KnownType[] types) + { + var typesArray = types.ToImmutableArray(); + return (context) => + context.MethodSymbol.Parameters.Length > 0 && + context.MethodSymbol.Parameters.Any(parameter => parameter.Type.DerivesOrImplementsAny(typesArray)); + } + public abstract MethodDeclarationCondition ParameterAtIndexIsUsed(int index); + internal MethodDeclarationCondition DecoratedWithAnyAttribute(params KnownType[] attributeTypes) => + (context) => + context.MethodSymbol.GetAttributes().Any(a => a.AttributeClass.IsAny(attributeTypes)); + public MethodDeclarationCondition MatchSimpleNames(params string[] methodNames) => (context) => methodNames.Contains(context.MethodSymbol.Name); + public MethodDeclarationCondition IsOrdinaryMethod() => + (context) => + context.MethodSymbol.MethodKind == MethodKind.Ordinary; + public MethodDeclarationCondition IsMainMethod() => (context) => context.MethodSymbol.IsMainMethod(); diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/ObjectCreationTracker.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/ObjectCreationTracker.cs index 0190dc205b9..1d426d4b03e 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/ObjectCreationTracker.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/ObjectCreationTracker.cs @@ -19,6 +19,7 @@ */ using System; +using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; @@ -72,6 +73,15 @@ internal ObjectCreationCondition FirstArgumentIs(KnownType type) => internal abstract ObjectCreationCondition FirstArgumentIsConstant(); + internal ObjectCreationCondition MatchAnyTypeThatImplements(params KnownType[] types) + { + var typesArray = types.ToImmutableArray(); + return (context) => + context.InvokedConstructorSymbol.Value != null && + context.InvokedConstructorSymbol.Value.IsConstructor() && + context.InvokedConstructorSymbol.Value.ContainingType.DerivesOrImplementsAny(typesArray); + } + internal ObjectCreationCondition MatchConstructors(params KnownType[] types) { // We cannot do a syntax check first because a type name can be aliased with diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Rules/Hotspots/ControllingPermissionsBase.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Rules/Hotspots/ControllingPermissionsBase.cs new file mode 100644 index 00000000000..97554d8fd3e --- /dev/null +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Rules/Hotspots/ControllingPermissionsBase.cs @@ -0,0 +1,79 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2018 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using SonarAnalyzer.Helpers; + +namespace SonarAnalyzer.Rules +{ + public abstract class ControllingPermissionsBase : SonarDiagnosticAnalyzer + where TSyntaxKind : struct + { + protected const string DiagnosticId = "S4834"; + protected const string MessageFormat = "Make sure that permissions are controlled safely here."; + + protected ObjectCreationTracker ObjectCreationTracker { get; set; } + protected InvocationTracker InvocationTracker { get; set; } + protected PropertyAccessTracker PropertyAccessTracker { get; set; } + protected MethodDeclarationTracker MethodDeclarationTracker { get; set; } + protected BaseTypeTracker BaseTypeTracker { get; set; } + + protected override void Initialize(SonarAnalysisContext context) + { + ObjectCreationTracker.Track(context, + ObjectCreationTracker.MatchConstructors( + KnownType.System_Security_Permissions_PrincipalPermission)); + + ObjectCreationTracker.Track(context, + ObjectCreationTracker.MatchAnyTypeThatImplements( + KnownType.System_Security_Principal_IIdentity, + KnownType.System_Security_Principal_IPrincipal)); + + InvocationTracker.Track(context, + InvocationTracker.MatchSimpleNames( + new MethodSignature(KnownType.System_Security_Principal_WindowsIdentity, "GetCurrent"), + new MethodSignature(KnownType.System_IdentityModel_Tokens_SecurityTokenHandler, "ValidateToken"), + new MethodSignature(KnownType.System_AppDomain, "SetPrincipalPolicy"), + new MethodSignature(KnownType.System_AppDomain, "SetThreadPrincipal"))); + + PropertyAccessTracker.Track(context, + PropertyAccessTracker.MatchSimpleNames( + new MethodSignature(KnownType.System_Web_HttpContext, "User"), + new MethodSignature(KnownType.System_Threading_Thread, "CurrentPrincipal"))); + + MethodDeclarationTracker.Track(context, + MethodDeclarationTracker.AnyParameterIsOfType( + KnownType.System_Security_Principal_IIdentity, + KnownType.System_Security_Principal_IPrincipal), + MethodDeclarationTracker.IsOrdinaryMethod()); + + MethodDeclarationTracker.Track(context, + MethodDeclarationTracker.DecoratedWithAnyAttribute( + KnownType.System_Security_Permissions_PrincipalPermissionAttribute)); + + BaseTypeTracker.Track(context, + BaseTypeTracker.WhenDerivesFrom( + KnownType.System_Security_Principal_IIdentity)); + + BaseTypeTracker.Track(context, + BaseTypeTracker.WhenDerivesFrom( + KnownType.System_Security_Principal_IPrincipal)); + } + } +} diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S4834_cs.html b/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S4834_cs.html new file mode 100644 index 00000000000..1f58876d597 --- /dev/null +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S4834_cs.html @@ -0,0 +1,80 @@ +

Controlling permissions is security-sensitive. It has led in the past to the following vulnerabilities:

+ +

Attackers can only damage what they have access to. Thus limiting their access is a good way to prevent them from wreaking havoc, but it has to be +done properly.

+

This rule flags code that controls the access to resources and actions or configures this access. The goal is to guide security code reviews.

+

Ask Yourself Whether

+
    +
  • at least one accessed action or resource is security-sensitive.
    • +
  • there is no access control in place or it does not cover all sensitive actions and resources.
    • +
  • users have permissions they don't need.
    • +
  • the access control is based on a user input or on some other unsafe data.
    • +
  • permissions are difficult to remove or take a long time to be updated.
    • +
+

You are at risk if you answered yes to the first question and any of the following ones.

+

Recommended Secure Coding Practices

+

The first step is to restrict all sensitive actions to authenticated users.

+

Each user should have the lowest privileges possible. The access control granularity should match the sensitivity of each resource or action. The +more sensitive it is, the less people should have access to it.

+

Do not base the access control on a user input or on a value which might have been tampered with. For example, the developer should not read a +user's permissions from an HTTP cookie as it can be modified client-side.

+

Check that the access to each action and resource is properly restricted.

+

Enable administrators to swiftly remove permissions when necessary. This enables them to reduce the time an attacker can have access to your +systems when a breach occurs.

+

Log and monitor refused access requests as they can reveal an attack.

+

Questionable Code Example

+
+using System.Threading;
+using System.Security.Permissions;
+using System.Security.Principal;
+using System.IdentityModel.Tokens;
+
+class SecurityPrincipalDemo
+{
+    class MyIdentity : IIdentity // Questionable, custom IIdentity implementations should be reviewed
+    {
+        // ...
+    }
+
+    class MyPrincipal : IPrincipal // Questionable, custom IPrincipal implementations should be reviewed
+    {
+        // ...
+    }
+    [System.Security.Permissions.PrincipalPermission(SecurityAction.Demand, Role = "Administrators")] // Questionable. The access restrictions enforced by this attribute should be reviewed.
+    static void CheckAdministrator()
+    {
+        WindowsIdentity MyIdentity = WindowsIdentity.GetCurrent(); // Questionable
+        HttpContext.User = ...; // Questionable: review all reference (set and get) to System.Web HttpContext.User
+        AppDomain domain = AppDomain.CurrentDomain;
+        domain.SetPrincipalPolicy(PrincipalPolicy.WindowsPrincipal); // Questionable
+        MyIdentity identity = new MyIdentity(); // Questionable
+        MyPrincipal MyPrincipal = new MyPrincipal(MyIdentity); // Questionable
+        Thread.CurrentPrincipal = MyPrincipal; // Questionable
+        domain.SetThreadPrincipal(MyPrincipal); // Questionable
+
+        // All instantiation of PrincipalPermission should be reviewed.
+        PrincipalPermission principalPerm = new PrincipalPermission(null, "Administrators"); // Questionable
+        principalPerm.Demand();
+
+        SecurityTokenHandler handler = ...;
+        // Questionable: this creates an identity.
+        ReadOnlyCollection<ClaimsIdentity> identities = handler.ValidateToken(…);
+    }
+
+     // Questionable: review how this function uses the identity and principal.
+    void modifyPrincipal(MyIdentity identity, MyPrincipal principal)
+    {
+        // ...
+    }
+}
+
+

See

+
    +
  • OWASP Top 10 2017 Category A5 - Broken Access Control
    • +
  • SANS Top 25 - Porous Defenses
    • +
+ diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S4834_vb.html b/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S4834_vb.html new file mode 100644 index 00000000000..3f0c62bbd89 --- /dev/null +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S4834_vb.html @@ -0,0 +1,76 @@ +

Controlling permissions is security-sensitive. It has led in the past to the following vulnerabilities:

+ +

Attackers can only damage what they have access to. Thus limiting their access is a good way to prevent them from wreaking havoc, but it has to be +done properly.

+

This rule flags code that controls the access to resources and actions or configures this access. The goal is to guide security code reviews.

+

Ask Yourself Whether

+
    +
  • at least one accessed action or resource is security-sensitive.
    • +
  • there is no access control in place or it does not cover all sensitive actions and resources.
    • +
  • users have permissions they don't need.
    • +
  • the access control is based on a user input or on some other unsafe data.
    • +
  • permissions are difficult to remove or take a long time to be updated.
    • +
+

You are at risk if you answered yes to the first question and any of the following ones.

+

Recommended Secure Coding Practices

+

The first step is to restrict all sensitive actions to authenticated users.

+

Each user should have the lowest privileges possible. The access control granularity should match the sensitivity of each resource or action. The +more sensitive it is, the less people should have access to it.

+

Do not base the access control on a user input or on a value which might have been tampered with. For example, the developer should not read a +user's permissions from an HTTP cookie as it can be modified client-side.

+

Check that the access to each action and resource is properly restricted.

+

Enable administrators to swiftly remove permissions when necessary. This enables them to reduce the time an attacker can have access to your +systems when a breach occurs.

+

Log and monitor refused access requests as they can reveal an attack.

+

Questionable Code Example

+
+Imports System.Threading
+Imports System.Security.Permissions
+Imports System.Security.Principal
+Imports System.IdentityModel.Tokens
+
+Class SecurityPrincipalDemo
+    Class MyIdentity
+        Implements IIdentity ' Questionable, custom IIdentity implementations should be reviewed
+    End Class
+
+    Class MyPrincipal
+        Implements IPrincipal ' Questionable, custom IPrincipal implementations should be reviewed
+    End Class
+
+    <System.Security.Permissions.PrincipalPermission(SecurityAction.Demand, Role:="Administrators")> ' Questionable. The access restrictions enforced by this attribute should be reviewed.
+    Private Shared Sub CheckAdministrator()
+        Dim MyIdentity As WindowsIdentity = WindowsIdentity.GetCurrent() ' Questionable
+
+        HttpContext.User = ... ' Questionable: review all reference (set and get) to System.Web HttpContext.User
+
+        Dim domain As AppDomain = AppDomain.CurrentDomain
+        domain.SetPrincipalPolicy(PrincipalPolicy.WindowsPrincipal) ' Questionable
+
+        Dim identity As MyIdentity = New MyIdentity() ' Questionable
+        Dim MyPrincipal As MyPrincipal = New MyPrincipal(MyIdentity) ' Questionable
+        Thread.CurrentPrincipal = MyPrincipal ' Questionable
+        domain.SetThreadPrincipal(MyPrincipal) ' Questionable
+
+        Dim principalPerm As PrincipalPermission = New PrincipalPermission(Nothing, "Administrators")  ' Questionable
+        principalPerm.Demand()
+
+        Dim handler As SecurityTokenHandler = ...
+        Dim identities As ReadOnlyCollection(Of ClaimsIdentity) = handler.ValidateToken()  ' Questionable, this creates identity
+    End Sub
+
+    ' Questionable: review how this function uses the identity and principal.
+    Private Sub modifyPrincipal(ByVal identity As MyIdentity, ByVal principal As MyPrincipal)
+    End Sub
+End Class
+
+

See

+
    +
  • OWASP Top 10 2017 Category A5 - Broken Access Control
    • +
  • SANS Top 25 - Porous Defenses
    • +
+ diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodDeclarationTracker.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodDeclarationTracker.cs index 793fc73cf91..95f8dcd44aa 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodDeclarationTracker.cs +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodDeclarationTracker.cs @@ -75,7 +75,26 @@ public override MethodDeclarationCondition ParameterAtIndexIsUsed(int index) => private static bool HasImplementation(MethodBlockSyntax methodBlock) => methodBlock.Statements.Count > 0; - protected override SyntaxToken GetMethodIdentifier(SyntaxNode methodDeclaration) => - ((MethodStatementSyntax)methodDeclaration).Identifier; + protected override SyntaxToken? GetMethodIdentifier(SyntaxNode methodDeclaration) + { + switch (methodDeclaration?.Kind()) + { + case SyntaxKind.SubStatement: + case SyntaxKind.FunctionStatement: + return ((MethodStatementSyntax)methodDeclaration).Identifier; + case SyntaxKind.SubNewStatement: + return ((SubNewStatementSyntax)methodDeclaration).NewKeyword; + case SyntaxKind.AddHandlerAccessorBlock: + case SyntaxKind.RemoveHandlerAccessorBlock: + return ((EventBlockSyntax)methodDeclaration.Parent.Parent).EventStatement?.Identifier; + case SyntaxKind.GetAccessorBlock: + case SyntaxKind.SetAccessorBlock: + return ((PropertyBlockSyntax)methodDeclaration.Parent.Parent).PropertyStatement?.Identifier; + case SyntaxKind.OperatorStatement: + return ((OperatorStatementSyntax)methodDeclaration).OperatorToken; + default: + return null; + } + } } } diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.VisualBasic/Rules/Hotspots/ControllingPermissions.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.VisualBasic/Rules/Hotspots/ControllingPermissions.cs new file mode 100644 index 00000000000..49035adce28 --- /dev/null +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.VisualBasic/Rules/Hotspots/ControllingPermissions.cs @@ -0,0 +1,55 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2018 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.VisualBasic; +using SonarAnalyzer.Common; +using SonarAnalyzer.Helpers; + +namespace SonarAnalyzer.Rules.VisualBasic +{ + [DiagnosticAnalyzer(LanguageNames.VisualBasic)] + [Rule(DiagnosticId)] + public sealed class ControllingPermissions : ControllingPermissionsBase + { + private static readonly DiagnosticDescriptor rule = + DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager) + .WithNotConfigurable(); + + public override ImmutableArray SupportedDiagnostics { get; } = + ImmutableArray.Create(rule); + + public ControllingPermissions() + : this(new DefaultAnalyzerConfiguration()) + { + } + + internal /*for testing*/ ControllingPermissions(IAnalyzerConfiguration analysisConfiguration) + { + ObjectCreationTracker = new VisualBasicObjectCreationTracker(analysisConfiguration, rule); + InvocationTracker = new VisualBasicInvocationTracker(analysisConfiguration, rule); + PropertyAccessTracker = new VisualBasicPropertyAccessTracker(analysisConfiguration, rule); + MethodDeclarationTracker = new VisualBasicMethodDeclarationTracker(analysisConfiguration, rule); + BaseTypeTracker = new VisualBasicBaseTypeTracker(analysisConfiguration, rule); + } + } +} diff --git a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/FrameworkMetadataReference.cs b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/FrameworkMetadataReference.cs index 4b14e617660..44d1c96670b 100644 --- a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/FrameworkMetadataReference.cs +++ b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/FrameworkMetadataReference.cs @@ -80,6 +80,9 @@ private static IEnumerable Create(string assemblyName) => internal static IEnumerable SystemGlobalization { get; } = Create("System.Globalization.dll"); + internal static IEnumerable SystemIdentityModel { get; } + = Create("System.IdentityModel.dll"); + internal static IEnumerable SystemReflection { get; } = Create("System.Reflection.dll"); diff --git a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/CsRuleTypeMapping.cs b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/CsRuleTypeMapping.cs index 6fd5823acf1..3c98c8624fe 100644 --- a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/CsRuleTypeMapping.cs +++ b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/CsRuleTypeMapping.cs @@ -4831,7 +4831,7 @@ internal static class CsRuleTypeMapping //["4831"], //["4832"], //["4833"], - //["4834"], + ["4834"] = "SECURITY_HOTSPOT", //["4835"], //["4836"], //["4837"], diff --git a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/VbRuleTypeMapping.cs b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/VbRuleTypeMapping.cs index 9220c63faa8..62d07cf7642 100644 --- a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/VbRuleTypeMapping.cs +++ b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/PackagingTests/VbRuleTypeMapping.cs @@ -4831,7 +4831,7 @@ internal static class VbRuleTypeMapping //["4831"], //["4832"], //["4833"], - //["4834"], + ["4834"] = "SECURITY_HOTSPOT", //["4835"], //["4836"], //["4837"], diff --git a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/Rules/ControllingPermissionsTest.cs b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/Rules/ControllingPermissionsTest.cs new file mode 100644 index 00000000000..2234fc12d05 --- /dev/null +++ b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/Rules/ControllingPermissionsTest.cs @@ -0,0 +1,78 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2018 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +extern alias csharp; +extern alias vbnet; + +using System.Collections.Generic; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using CSharp = csharp::SonarAnalyzer.Rules.CSharp; +using VisualBasic = vbnet::SonarAnalyzer.Rules.VisualBasic; + +namespace SonarAnalyzer.UnitTest.Rules +{ + [TestClass] + public class ControllingPermissionsTest + { + private static readonly IEnumerable AdditionalReferences = + Enumerable.Empty() + .Concat(FrameworkMetadataReference.SystemIdentityModel) + .Concat(FrameworkMetadataReference.SystemWeb); + + [TestMethod] + [TestCategory("Rule")] + public void ControllingPermissions_CS() + { + Verifier.VerifyAnalyzer(@"TestCases\ControllingPermissions.cs", + new CSharp.ControllingPermissions(new TestAnalyzerConfiguration(null, "S4834")), + additionalReferences: AdditionalReferences); + } + + [TestMethod] + [TestCategory("Rule")] + public void ControllingPermissions_CS_Disabled() + { + Verifier.VerifyNoIssueReported(@"TestCases\ControllingPermissions.cs", + new CSharp.ControllingPermissions(), + additionalReferences: AdditionalReferences); + } + + [TestMethod] + [TestCategory("Rule")] + public void ControllingPermissions_VB() + { + Verifier.VerifyAnalyzer(@"TestCases\ControllingPermissions.vb", + new VisualBasic.ControllingPermissions(new TestAnalyzerConfiguration(null, "S4834")), + additionalReferences: AdditionalReferences); + } + + [TestMethod] + [TestCategory("Rule")] + public void ControllingPermissions_VB_Disabled() + { + Verifier.VerifyNoIssueReported(@"TestCases\ControllingPermissions.vb", + new VisualBasic.ControllingPermissions(), + additionalReferences: AdditionalReferences); + } + } +} + diff --git a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ControllingPermissions.cs b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ControllingPermissions.cs new file mode 100644 index 00000000000..b43b4d6128a --- /dev/null +++ b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ControllingPermissions.cs @@ -0,0 +1,111 @@ +using System; +using System.IdentityModel.Tokens; +using System.Security.Permissions; +using System.Security.Principal; +using System.Threading; +using System.Web; + +namespace Tests.Diagnostics +{ + class Program + { + class MyIdentity : IIdentity // Noncompliant {{Make sure that permissions are controlled safely here.}} +// ^^^^^^^^^ + { + public string Name => throw new NotImplementedException(); + public string AuthenticationType => throw new NotImplementedException(); + public bool IsAuthenticated => throw new NotImplementedException(); + } + + class MyPrincipal : IPrincipal // Noncompliant + { + public IIdentity Identity => throw new NotImplementedException(); + public bool IsInRole(string role) => throw new NotImplementedException(); + } + + // Indirectly implementing IIdentity + class MyWindowsIdentity : WindowsIdentity // Noncompliant + { + public MyWindowsIdentity() : base("") { } + } + + [PrincipalPermission(SecurityAction.Demand, Role = "Administrators")] + void SecuredMethod() { } // Noncompliant, decorated with PrincipalPermission +// ^^^^^^^^^^^^^ + + void ValidateSecurityToken(SecurityTokenHandler handler, SecurityToken securityToken) + { + handler.ValidateToken(securityToken); // Noncompliant + } + + void CreatingPermissions() + { + WindowsIdentity.GetCurrent(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + + // All instantiations of PrincipalPermission + PrincipalPermission principalPermission; + principalPermission = new PrincipalPermission(PermissionState.None); // Noncompliant + principalPermission = new PrincipalPermission("", ""); // Noncompliant + principalPermission = new PrincipalPermission("", "", true); // Noncompliant + } + + void HttpContextUser(HttpContext httpContext) + { + var user = httpContext.User; // Noncompliant +// ^^^^^^^^^^^^^^^^ + httpContext.User = user; // Noncompliant +// ^^^^^^^^^^^^^^^^ + } + + void AppDomainSecurity(AppDomain appDomain, IPrincipal principal) // Noncompliant, IPrincipal parameter, see another section with tests + { + appDomain.SetPrincipalPolicy(PrincipalPolicy.WindowsPrincipal); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + appDomain.SetThreadPrincipal(principal); // Noncompliant + appDomain.ExecuteAssembly(""); // Compliant, not one of the tracked methods + } + + void ThreadSecurity(IPrincipal principal) // Noncompliant, IPrincipal parameter, see another section with tests + { + Thread.CurrentPrincipal = principal; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^ + principal = Thread.CurrentPrincipal; // Noncompliant + } + + void CreatingPrincipalAndIdentity(WindowsIdentity windowsIdentity) // Noncompliant, IIdentity parameter, see another section with tests + { + IIdentity identity; + identity = new MyIdentity(); // Noncompliant, creation of type that implements IIdentity +// ^^^^^^^^^^^^^^^^ + identity = new WindowsIdentity(""); // Noncompliant + IPrincipal principal; + principal = new MyPrincipal(); // Noncompliant, creation of type that implements IPrincipal + principal = new WindowsPrincipal(windowsIdentity); // Noncompliant + } + + // Method declarations that accept IIdentiry or IPrincipal + void AcceptIdentity(MyIdentity identity) { } // Noncompliant +// ^^^^^^^^^^^^^^ + void AcceptIdentity(IIdentity identity) { } // Noncompliant + void AcceptPrincipal(MyPrincipal principal) { } // Noncompliant + void AcceptPrincipal(IPrincipal principal) { } // Noncompliant + } + + public class Properties + { + private IIdentity identity; + + public IIdentity Identity + { + get + { + return identity; + } + set // Compliant, we do not raise for property accessors + { + identity = value; + } + } + } +} diff --git a/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ControllingPermissions.vb b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ControllingPermissions.vb new file mode 100644 index 00000000000..688288acd18 --- /dev/null +++ b/sonaranalyzer-dotnet/tests/SonarAnalyzer.UnitTest/TestCases/ControllingPermissions.vb @@ -0,0 +1,126 @@ +Imports System +Imports System.IdentityModel.Tokens +Imports System.Security.Permissions +Imports System.Security.Principal +Imports System.Threading +Imports System.Web + +Namespace Tests.Diagnostics + Class Program + Class MyIdentity + Implements IIdentity ' Noncompliant {{Make sure that permissions are controlled safely here.}} +' ^^^^^^^^^ + Public ReadOnly Property Name As String + Get + Throw New NotImplementedException() + End Get + End Property + + Public ReadOnly Property AuthenticationType As String + Get + Throw New NotImplementedException() + End Get + End Property + + Public ReadOnly Property IsAuthenticated As Boolean + Get + Throw New NotImplementedException() + End Get + End Property + End Class + + Class MyPrincipal + Implements IPrincipal ' Noncompliant + + Public ReadOnly Property Identity As IIdentity + Get + Throw New NotImplementedException() + End Get + End Property + + Public Function IsInRole(ByVal role As String) As Boolean + Throw New NotImplementedException() + End Function + End Class + + Class MyWindowsIdentity + Inherits WindowsIdentity ' Noncompliant + + Public Sub New() + MyBase.New("") + End Sub + End Class + + + Private Sub SecuredMethod() ' Noncompliant, decorated with PrincipalPermission +' ^^^^^^^^^^^^^ + End Sub + + Private Sub ValidateSecurityToken(ByVal handler As SecurityTokenHandler, ByVal securityToken As SecurityToken) + handler.ValidateToken(securityToken) ' Noncompliant + End Sub + + Private Sub CreatingPermissions() + WindowsIdentity.GetCurrent() ' Noncompliant +' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + + ' All instantiations of PrincipalPermission + Dim principalPermission As PrincipalPermission + principalPermission = New PrincipalPermission(PermissionState.None) ' Noncompliant + principalPermission = New PrincipalPermission("", "") ' Noncompliant + principalPermission = New PrincipalPermission("", "", True) ' Noncompliant + End Sub + + Private Sub HttpContextUser(ByVal httpContext As HttpContext) + Dim user = httpContext.User ' Noncompliant + httpContext.User = user ' Noncompliant + End Sub + + Private Sub AppDomainSecurity(ByVal appDomain As AppDomain, ByVal principal As IPrincipal) ' Noncompliant, IPrincipal parameter, see another section with tests + appDomain.SetPrincipalPolicy(PrincipalPolicy.WindowsPrincipal) ' Noncompliant + appDomain.SetThreadPrincipal(principal) ' Noncompliant + appDomain.ExecuteAssembly("") ' Compliant, not one of the tracked methods + End Sub + + Private Sub ThreadSecurity(ByVal principal As IPrincipal) ' Noncompliant, IPrincipal parameter, see another section with tests + Thread.CurrentPrincipal = principal ' Noncompliant + principal = Thread.CurrentPrincipal ' Noncompliant + End Sub + + Private Sub CreatingPrincipalAndIdentity(ByVal windowsIdentity As WindowsIdentity) ' Noncompliant, IIdentity parameter, see another section with tests + Dim identity As IIdentity + identity = New MyIdentity() ' Noncompliant, creation of type that implements IIdentity + identity = New WindowsIdentity("") ' Noncompliant + Dim principal As IPrincipal + principal = New MyPrincipal() ' Noncompliant, creation of type that implements IPrincipal + principal = New WindowsPrincipal(windowsIdentity) ' Noncompliant + End Sub + + ' Method declarations that accept IIdentiry or IPrincipal + Private Sub AcceptIdentity(ByVal identity As MyIdentity) ' Noncompliant + End Sub + + Private Sub AcceptIdentity(ByVal identity As IIdentity) ' Noncompliant + End Sub + + Private Sub AcceptPrincipal(ByVal principal As MyPrincipal) ' Noncompliant + End Sub + + Private Sub AcceptPrincipal(ByVal principal As IPrincipal) ' Noncompliant + End Sub + End Class + + Public Class Properties + Private identity As IIdentity + + Public Property Identity As IIdentity + Get + Return identity + End Get + Set(ByVal value As IIdentity) ' Compliant, we do not raise for property accessors + identity = value + End Set + End Property + End Class + +End Namespace