New rule S6377: XML signatures should be verified securely (#9277)

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.
+    }
+}

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.",

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.",

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.",

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"
+    }
+  ]
+}

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.",

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"
+    }
+  ]
+}

analyzers/rspec/cs/S6377.html

@@ -0,0 +1,80 @@
+<p>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.</p>
+<h2>Why is this an issue?</h2>
+<p>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.</p>
+<h3>What is the potential impact?</h3>
+<p>By not enforcing secure validation, the XML Digital Signature API is more susceptible to attacks such as signature spoofing and injections.</p>
+<h3>Increased Vulnerability to Signature Spoofing</h3>
+<p>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.</p>
+<h3>Risk of Injection Attacks</h3>
+<p>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.</p>
+<h2>How to fix it in ASP.NET Core</h2>
+<h3>Code examples</h3>
+<p>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.</p>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+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
+}
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+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
+}
+</pre>
+<h3>How does this work?</h3>
+<p>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.</p>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.xml">System.Security.Cryptography.Xml
+  Namespace</a> </li>
+  <li> Microsfot Learn - <a
+  href="https://learn.microsoft.com/en-us/dotnet/standard/security/how-to-verify-the-digital-signatures-of-xml-documents">How to: Verify the Digital
+  Signatures of XML Documents</a> </li>
+</ul>
+<h3>Standards</h3>
+<ul>
+  <li> OWASP - <a href="https://owasp.org/Top10/A02_2021-Cryptographic_Failures/">Top 10:2021 A02:2021 - Cryptographic Failures</a> </li>
+  <li> OWASP - <a href="https://owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure">Top 10 2017 Category A3 - Sensitive Data
+  Exposure</a> </li>
+  <li> CWE - <a href="https://cwe.mitre.org/data/definitions/347">CWE-347 - Improper Verification of Cryptographic Signature</a> </li>
+  <li> STIG Viewer - <a href="https://stigviewer.com/stig/application_security_and_development/2023-06-08/finding/V-222608">Application Security and
+  Development: V-222608</a> - The application must not be vulnerable to XML-oriented attacks. </li>
+</ul>
+

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"
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -277,6 +277,7 @@
     "S5766",
     "S5773",
     "S5856",
+    "S6377",
     "S6419",
     "S6420",
     "S6422",

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<DiagnosticDescriptor> 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"))));
+    }
+}

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");

analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs

@@ -6301,7 +6301,7 @@ internal static class RuleTypeMappingCS
         // ["S6374"],
         // ["S6375"],
         // ["S6376"],
-        // ["S6377"],
+        ["S6377"] = "VULNERABILITY",
         // ["S6378"],
         // ["S6379"],
         // ["S6380"],

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<XmlSignatureCheck>()
+        .AddReferences(MetadataReferenceFacade.SystemXml)
+        .AddReferences(MetadataReferenceFacade.SystemSecurityCryptography)
+        .AddReferences(NuGetMetadataReference.SystemSecurityCryptographyXml());
+
+    [TestMethod]
+    public void XmlSignatureCheck_CS() =>
+        builder.AddPaths("XMLSignatureCheck.cs").Verify();
+}

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<T, bool>, 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;
+}

analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/NuGetMetadataReference.cs

@@ -162,6 +162,7 @@ public static class NuGetMetadataReference
     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);