diff --git a/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs b/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs new file mode 100644 index 00000000000..e396899f7bc --- /dev/null +++ b/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs @@ -0,0 +1,18 @@ +using System.Linq; +using System.Security.Cryptography; +using System.Security.Cryptography.Xml; +using System.Xml; + +namespace IntentionalFindings; + +public class S6377 +{ + public void CheckSignature(XmlDocument xmlDoc, RSACryptoServiceProvider rsaCryptoServiceProvider) + { + var signedXml = new SignedXml(xmlDoc); + signedXml.LoadXml((XmlElement)xmlDoc.GetElementsByTagName("Signature").Item(0)); + + _ = signedXml.CheckSignature(rsaCryptoServiceProvider); + _ = signedXml.CheckSignature(); // The key is missing. + } +} diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1128-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1128-IntentionalFindings-net8.0.json index 5d682c4c8fa..9bdda6ae1ed 100644 --- a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1128-IntentionalFindings-net8.0.json +++ b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1128-IntentionalFindings-net8.0.json @@ -6,6 +6,12 @@ "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S2857.cs#L9", "Location": "Line 9 Position 1-29" }, + { + "Id": "S1128", + "Message": "Remove this unnecessary \u0027using\u0027.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs#L1", + "Location": "Line 1 Position 1-19" + }, { "Id": "S1128", "Message": "Remove this unnecessary \u0027using\u0027.", diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json index faf2b668b6f..8514185dbdb 100644 --- a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json +++ b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json @@ -12,6 +12,12 @@ "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S3416.cs#L1", "Location": "Line 1 Position 1-1" }, + { + "Id": "S1451", + "Message": "Add or update the header of this file.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs#L1", + "Location": "Line 1 Position 1-1" + }, { "Id": "S1451", "Message": "Add or update the header of this file.", diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json index 1b51b9728c0..95a82af763a 100644 --- a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json +++ b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json @@ -12,6 +12,12 @@ "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S3655.cs#L13", "Location": "Line 13 Position 21-47" }, + { + "Id": "S2325", + "Message": "Make \u0027CheckSignature\u0027 a static method.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs#L10", + "Location": "Line 10 Position 17-31" + }, { "Id": "S2325", "Message": "Make \u0027Method\u0027 a static method.", diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S3242-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S3242-IntentionalFindings-net8.0.json new file mode 100644 index 00000000000..39902030823 --- /dev/null +++ b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S3242-IntentionalFindings-net8.0.json @@ -0,0 +1,10 @@ +{ + "Issues": [ + { + "Id": "S3242", + "Message": "Consider using more general type \u0027System.Security.Cryptography.AsymmetricAlgorithm\u0027 instead of \u0027System.Security.Cryptography.RSACryptoServiceProvider\u0027.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs#L10", + "Location": "Line 10 Position 77-101" + } + ] +} \ No newline at end of file diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S3900-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S3900-IntentionalFindings-net8.0.json index bfea651a2df..d8828bfeb7e 100644 --- a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S3900-IntentionalFindings-net8.0.json +++ b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S3900-IntentionalFindings-net8.0.json @@ -6,6 +6,12 @@ "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S3416.cs#L9", "Location": "Line 9 Position 13-20" }, + { + "Id": "S3900", + "Message": "Refactor this method to add validation of parameter \u0027xmlDoc\u0027 before using it.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs#L13", + "Location": "Line 13 Position 39-45" + }, { "Id": "S3900", "Message": "Refactor this method to add validation of parameter \u0027traceSwitch\u0027 before using it.", diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6377-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6377-IntentionalFindings-net8.0.json new file mode 100644 index 00000000000..52e56189f6c --- /dev/null +++ b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6377-IntentionalFindings-net8.0.json @@ -0,0 +1,10 @@ +{ + "Issues": [ + { + "Id": "S6377", + "Message": "Change this code to only accept signatures computed from a trusted party.", + "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6377.cs#L16", + "Location": "Line 16 Position 13-39" + } + ] +} \ No newline at end of file diff --git a/analyzers/rspec/cs/S6377.html b/analyzers/rspec/cs/S6377.html new file mode 100644 index 00000000000..ff03d44193e --- /dev/null +++ b/analyzers/rspec/cs/S6377.html @@ -0,0 +1,80 @@ +

XML signatures are a method used to ensure the integrity and authenticity of XML documents. However, if XML signatures are not validated securely, +it can lead to potential vulnerabilities.

+

Why is this an issue?

+

XML can be used for a wide variety of purposes. Using a signature on an XML message generally indicates this message requires authenticity and +integrity. However, if the signature validation is not properly implemented this authenticity can not be guaranteed.

+

What is the potential impact?

+

By not enforcing secure validation, the XML Digital Signature API is more susceptible to attacks such as signature spoofing and injections.

+

Increased Vulnerability to Signature Spoofing

+

By disabling secure validation, the application becomes more susceptible to signature spoofing attacks. Attackers can potentially manipulate the +XML signature in a way that bypasses the validation process, allowing them to forge or tamper with the signature. This can lead to the acceptance of +invalid or maliciously modified signatures, compromising the integrity and authenticity of the XML documents.

+

Risk of Injection Attacks

+

Disabling secure validation can expose the application to injection attacks. Attackers can inject malicious code or entities into the XML document, +taking advantage of the weakened validation process. In some cases, it can also expose the application to denial-of-service attacks. Attackers can +exploit vulnerabilities in the validation process to cause excessive resource consumption or system crashes, leading to service unavailability or +disruption.

+

How to fix it in ASP.NET Core

+

Code examples

+

The following noncompliant code example verifies an XML signature without providing a trusted public key. This code will validate the signature +against the embedded public key, accepting any forged signature.

+

Noncompliant code example

+
+XmlDocument xmlDoc = new()
+{
+    PreserveWhitespace = true
+};
+xmlDoc.Load("/data/login.xml");
+SignedXml signedXml = new(xmlDoc);
+XmlNodeList nodeList = xmlDoc.GetElementsByTagName("Signature");
+signedXml.LoadXml((XmlElement?)nodeList[0]);
+if (signedXml.CheckSignature()) {
+    // Process the XML content
+} else {
+    // Raise an error
+}
+
+

Compliant solution

+
+CspParameters cspParams = new()
+{
+    KeyContainerName = "MY_RSA_KEY"
+};
+RSACryptoServiceProvider rsaKey = new(cspParams);
+
+XmlDocument xmlDoc = new()
+{
+    PreserveWhitespace = true
+};
+xmlDoc.Load("/data/login.xml");
+SignedXml signedXml = new(xmlDoc);
+XmlNodeList nodeList = xmlDoc.GetElementsByTagName("Signature");
+signedXml.LoadXml((XmlElement?)nodeList[0]);
+if (signedXml.CheckSignature(rsaKey)) {
+    // Process the XML content
+} else {
+    // Raise an error
+}
+
+

How does this work?

+

Here, the compliant solution provides an RSA public key to the signature validation function. This will ensure only signatures computed with the +associated private key will be accepted. This prevents signature forgery attacks.

+

Resources

+

Documentation

+ +

Standards

+ + diff --git a/analyzers/rspec/cs/S6377.json b/analyzers/rspec/cs/S6377.json new file mode 100644 index 00000000000..36f02110118 --- /dev/null +++ b/analyzers/rspec/cs/S6377.json @@ -0,0 +1,35 @@ +{ + "title": "XML signatures should be validated securely", + "type": "VULNERABILITY", + "code": { + "impacts": { + "SECURITY": "MEDIUM" + }, + "attribute": "CONVENTIONAL" + }, + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "15min" + }, + "tags": [], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6377", + "sqKey": "S6377", + "scope": "Main", + "securityStandards": { + "CWE": [ + 347 + ], + "OWASP": [ + "A3" + ], + "OWASP Top 10 2021": [ + "A2" + ], + "STIG ASD 2023-06-08": [ + "V-222608" + ] + }, + "quickfix": "unknown" +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index b4ff4ab92e1..124867bd2c3 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -277,6 +277,7 @@ "S5766", "S5773", "S5856", + "S6377", "S6419", "S6420", "S6422", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/XMLSignatureCheck.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/XMLSignatureCheck.cs new file mode 100644 index 00000000000..a8c710bd1fb --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/XMLSignatureCheck.cs @@ -0,0 +1,44 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 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. + */ + +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class XmlSignatureCheck : SonarDiagnosticAnalyzer +{ + private const string DiagnosticId = "S6377"; + private const string MessageFormat = "Change this code to only accept signatures computed from a trusted party."; + + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + protected override void Initialize(SonarAnalysisContext context) + { + var tracker = CSharpFacade.Instance.Tracker.Invocation; + tracker.Track( + new TrackerInput(context, AnalyzerConfiguration.AlwaysEnabled, Rule), + tracker.Or( + tracker.And( + tracker.MethodHasParameters(0), + tracker.MatchMethod(new MemberDescriptor(KnownType.System_Security_Cryptography_Xml_SignedXml, "CheckSignature"))), + tracker.MatchMethod(new MemberDescriptor(KnownType.System_Security_Cryptography_Xml_SignedXml, "CheckSignatureReturningKey")))); + } +} diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs index 4e0e9834b1a..38841ef5399 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs @@ -517,6 +517,7 @@ public sealed partial class KnownType public static readonly KnownType System_Security_Cryptography_TripleDES = new("System.Security.Cryptography.TripleDES"); public static readonly KnownType System_Security_Cryptography_X509Certificates_X509Certificate2 = new("System.Security.Cryptography.X509Certificates.X509Certificate2"); public static readonly KnownType System_Security_Cryptography_X509Certificates_X509Chain = new("System.Security.Cryptography.X509Certificates.X509Chain"); + public static readonly KnownType System_Security_Cryptography_Xml_SignedXml = new("System.Security.Cryptography.Xml.SignedXml"); public static readonly KnownType System_Security_Permissions_CodeAccessSecurityAttribute = new("System.Security.Permissions.CodeAccessSecurityAttribute"); public static readonly KnownType System_Security_Permissions_PrincipalPermission = new("System.Security.Permissions.PrincipalPermission"); public static readonly KnownType System_Security_Permissions_PrincipalPermissionAttribute = new("System.Security.Permissions.PrincipalPermissionAttribute"); diff --git a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs index ba7bd4005c3..fe50700def8 100644 --- a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs @@ -6301,7 +6301,7 @@ internal static class RuleTypeMappingCS // ["S6374"], // ["S6375"], // ["S6376"], - // ["S6377"], + ["S6377"] = "VULNERABILITY", // ["S6378"], // ["S6379"], // ["S6380"], diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/XMLSignatureCheckTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/XMLSignatureCheckTest.cs new file mode 100644 index 00000000000..cec1a2c76f6 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/XMLSignatureCheckTest.cs @@ -0,0 +1,36 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 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.Rules.CSharp; + +namespace SonarAnalyzer.Test.Rules; + +[TestClass] +public class XmlSignatureCheckTest +{ + private readonly VerifierBuilder builder = new VerifierBuilder() + .AddReferences(MetadataReferenceFacade.SystemXml) + .AddReferences(MetadataReferenceFacade.SystemSecurityCryptography) + .AddReferences(NuGetMetadataReference.SystemSecurityCryptographyXml()); + + [TestMethod] + public void XmlSignatureCheck_CS() => + builder.AddPaths("XMLSignatureCheck.cs").Verify(); +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/XMLSignatureCheck.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/XMLSignatureCheck.cs new file mode 100644 index 00000000000..44537e7a996 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/XMLSignatureCheck.cs @@ -0,0 +1,35 @@ +using System.Security.Cryptography; +using System.Security.Cryptography.Xml; +using System.Xml; +using System.Linq; + +public class XMLSignatures +{ + public void TestCases(RSACryptoServiceProvider rsaCryptoServiceProvider) + { + XmlDocument xmlDoc = new XmlDocument() { PreserveWhitespace = true }; + xmlDoc.Load("/data/login.xml"); + SignedXml signedXml = new SignedXml(xmlDoc); + signedXml.LoadXml((XmlElement)xmlDoc.GetElementsByTagName("Signature").Item(0)); + + _ = signedXml.CheckSignature(rsaCryptoServiceProvider); // A key is provided. + _ = signedXml.CheckSignature("other"); // Custom defined extension method. + _ = signedXml.CheckSignatureReturningKey("other"); // Custom defined extension method. + _ = new[] { rsaCryptoServiceProvider }.Select(signedXml.CheckSignature); // Used as an Func, parameter is provided. + + _ = signedXml.CheckSignature(); // Noncompliant {{Change this code to only accept signatures computed from a trusted party.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^ + _ = signedXml.CheckSignatureReturningKey(out var signingKey); // Noncompliant {{Change this code to only accept signatures computed from a trusted party.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + + } +} + +public static class SignedXmlExtensions +{ + public static bool CheckSignature(this SignedXml signedXml, string other) => + true; + + public static bool CheckSignatureReturningKey(this SignedXml signedXml, string other) => + true; +} diff --git a/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/NuGetMetadataReference.cs b/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/NuGetMetadataReference.cs index 4bee23cc8c1..7c1e50a05d8 100644 --- a/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/NuGetMetadataReference.cs +++ b/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/NuGetMetadataReference.cs @@ -162,6 +162,7 @@ public static References MongoDBDriver(string packageVersion = Constants.NuGetLa public static References SystemDrawingCommon(string packageVersion = "4.7.0") => Create("System.Drawing.Common", packageVersion); public static References SystemNetHttp(string packageVersion = Constants.NuGetLatestVersion) => Create("System.Net.Http", packageVersion); public static References SystemSecurityCryptographyOpenSsl(string packageVersion = "4.7.0") => Create("System.Security.Cryptography.OpenSsl", packageVersion); + public static References SystemSecurityCryptographyXml(string packageVersion = Constants.NuGetLatestVersion) => Create("System.Security.Cryptography.Xml", packageVersion); public static References SystemSecurityPermissions(string packageVersion = "4.7.0") => Create("System.Security.Permissions", packageVersion); public static References SystemPrivateServiceModel(string packageVersion = "4.7.0") => Create("System.Private.ServiceModel", packageVersion); public static References SystemServiceModelPrimitives(string packageVersion = "4.7.0") => Create("System.ServiceModel.Primitives", packageVersion);