Add tests to identify rules with missing or incomplete guidance (#613)

* Add tests to identify rules with missing or incomplete guidance

* Also consider guidance with "TO DO" incomplete

* Point DES rules at same guidance

* Add guidance for .NET TLS config

* Add guidance for .NET Framework 4.7.2 rule

* Add guidance for .NET Core advisory 4021279

* Add guidance for Microsoft.IdentityModel.Tokens rule

* Add guidance for unsafe keyword rule

* Add guidance for JS setTimeout rule

* Add guidance for weak/broken hash algo rule

* Add guidance for disabling cert validation rule

* Add guidance for avoid $_REQUEST rule

* Add guidance for PHP XSS rule

* Add guidance for strlen rule

* Add guidance for Python datetime rule

* Add changelog for guidance changes

* Add debug info to guidance tests to troubleshoot CI

* Fix finding guidance for DevSkim CLI in CI

Author: danfiedler-msft

Changelog.md

@@ -4,7 +4,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## [1.0.35] - 2024-05-21
+## [1.0.37] - 2024-5-23
+### Missing Guidance
+Added guidance for several rules such as weak hash algorithm, disabling certificate validation, and TLS client configuration.
+
+## [1.0.36] - 2024-05-21
 ### Rules
 Fix substitution pattern in PHP Request rule.
 

DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs

@@ -3,12 +3,49 @@ namespace Microsoft.DevSkim.Tests;
 [TestClass]
 public class DefaultRulesTests
 {
+    private static string _guidanceDirectory = string.Empty;
+
+    [ClassInitialize]
+    public static void ClassInitialize(TestContext testContext)
+    {
+        string directory = Directory.GetCurrentDirectory();
+
+        /* Given a directory, like: "C:\src\DevSkim\DevSkim-DotNet\Microsoft.DevSkim.Tests\bin\Debug\net8.0"     - local dev
+         * OR                       "/mnt/vss/_work/1/s/DevSkim-DotNet/Microsoft.DevSkim.Tests/bin/Debug/net8.0" - CI for DevSkim CLI
+         * we want to find:         "C:\src\DevSkim\guidance"
+         * 
+         * so we look for DevSkim-DotNet and then go up one more level to find guidance.
+         */
+
+        var currentDirInfo = new DirectoryInfo(directory);
+        while (currentDirInfo != null && currentDirInfo.Name != "DevSkim-DotNet")
+        {
+            currentDirInfo = currentDirInfo.Parent;
+        }
+
+        currentDirInfo = currentDirInfo?.Parent;
+
+        if (currentDirInfo == null)
+        {
+            string message = $"Could not find DevSkim-DotNet directory from: {directory}.";
+            throw new Exception(message);
+        }
+
+        string guidanceDir = Path.Combine(currentDirInfo.FullName, "guidance");
+        if (!Directory.Exists(guidanceDir))
+        {
+            throw new Exception($"Guidance directory {guidanceDir} does not exist.");
+        }
+
+        _guidanceDirectory = guidanceDir;
+    }
+
     [TestMethod]
     public void ValidateDefaultRules()
     {
-        DevSkimRuleSet devSkimRuleSet = DevSkim.DevSkimRuleSet.GetDefaultRuleSet();
+        DevSkimRuleSet devSkimRuleSet = DevSkimRuleSet.GetDefaultRuleSet();
         Assert.AreNotEqual(0, devSkimRuleSet.Count());
-        DevSkimRuleVerifier validator = new DevSkim.DevSkimRuleVerifier(new DevSkimRuleVerifierOptions()
+        var validator = new DevSkimRuleVerifier(new DevSkimRuleVerifierOptions()
         {
             LanguageSpecs = DevSkimLanguages.LoadEmbedded()
         });
@@ -80,4 +117,63 @@ public void DenamespacedRule()
         IEnumerable<Issue> analysis = analyzer.Analyze(content, "thing.xml");
         Assert.AreEqual(1, analysis.Count());
     }
+
+    public static IEnumerable<object[]> DefaultRules
+    {
+        get
+        {
+            DevSkimRuleSet devSkimRuleSet = DevSkimRuleSet.GetDefaultRuleSet();
+            foreach (DevSkimRule rule in devSkimRuleSet)
+            {
+                yield return new object[] { rule };
+            }
+        }
+    }
+
+    [TestMethod]
+    [DynamicData(nameof(DefaultRules))]
+    public void Rule_guidance_file_should_be_specified_and_exist(DevSkimRule rule)
+    {
+        if (rule.Disabled)
+        {
+            Assert.Inconclusive("Rule is disabled.");
+        }
+
+        if (string.IsNullOrEmpty(rule.RuleInfo))
+        {
+            Assert.Fail("Rule does not specify guidance file.");
+        }
+
+        string guidanceFile = Path.Combine(_guidanceDirectory, rule.RuleInfo);
+        Assert.IsTrue(File.Exists(guidanceFile), $"Guidance file {guidanceFile} does not exist.");
+    }
+
+    [Ignore] // TODO: temporary to get missing guidance in.
+    [TestMethod]
+    [DynamicData(nameof(DefaultRules))]
+    public void Rule_guidance_should_be_complete(DevSkimRule rule)
+    {
+        if (rule.Disabled)
+        {
+            Assert.Inconclusive("Rule is disabled.");
+        }
+
+        if(string.IsNullOrEmpty(rule.RuleInfo))
+        {
+            Assert.Inconclusive("Rule does not specify guidance file.");
+        }
+
+        string guidanceFile = Path.Combine(_guidanceDirectory, rule.RuleInfo);
+        if(!File.Exists(guidanceFile))
+        {
+            Assert.Inconclusive("Guidance file does not exist");
+        }
+
+        string guidance = File.ReadAllText(guidanceFile);
+        if(guidance.Contains("TODO", StringComparison.OrdinalIgnoreCase) 
+            || guidance.Contains("TO DO", StringComparison.OrdinalIgnoreCase))
+        {
+            Assert.Fail($"Guidance file {guidanceFile} contains TODO.");
+        }
+    }
 }

DevSkim-DotNet/Microsoft.DevSkim/DevSkimRule.cs

@@ -22,5 +22,10 @@ public class DevSkimRule : Rule
         [JsonProperty(PropertyName = "rule_info")]
         [JsonPropertyName("rule_info")]
         public string? RuleInfo { get; set; }
+
+        public override string ToString()
+        {
+            return $"'{Id}: {Name}'";
+        }
     }
 }

guidance/DS106864.md

@@ -1,19 +1,21 @@
-## Do not use the DES symmetric block cipher.
+# Do not use the DES symmetric block cipher
+
+## Summary
 
-### Summary
 The DES cipher was found, which is widely considered to be broken.
 
-### Details
+## Details
+
 The [Data Encryption Standard](https://en.wikipedia.org/wiki/Data_Encryption_Standard), or DES, 
 is a symmetric block cipher created in the 1970s. It has since been broken and should not be used
 anywhere.
 
 In general, the [Advanced Encryption Standard](https://en.wikipedia.org/wiki/Advanced_Encryption_Standard), 
 or AES, algorithm, is preferred for all cases where symmetric encryption is needed.
 
-#### Solution
+### Solution
 
-##### .NET
+#### .NET
 
 Use the following method: `System.Security.Cryptography.Aes.Create()`
 
@@ -36,7 +38,7 @@ It can also be accessed through the following factory methods:
 On Windows (C/C++), DES is available through the following:
 
 * The algorithm identifier `CALG_DES` can be passed to various methods, including
-  [CryptDeriveKey](https://msdn.microsoft.com/en-us/library/windows/desktop/aa379916(v=vs.85).aspx). 
+  [CryptDeriveKey](https://msdn.microsoft.com/en-us/library/windows/desktop/aa379916(v=vs.85).aspx).
 * The algorithm identifier `BCRYPT_DES_ALGORITHM` can be passed to 
   [BCryptOpenAlgorithmProvider](https://msdn.microsoft.com/en-us/library/windows/desktop/aa375479(v=vs.85).aspx).
 
@@ -52,5 +54,4 @@ OpenSSL) makes the DES algorithm available through the following calls:
 
 ### Severity Considerations
 
-Any use of the DES algorithm should be very carefully reviewed by a security expert or cryptographer
-to understand the risks involved.
+Any use of the DES algorithm should be very carefully reviewed by a security expert or cryptographer to understand the risks involved.

guidance/DS108647.md

@@ -0,0 +1,31 @@
+# SSL/TLS Cryptographic Agility Guidance
+
+## Summary
+
+* .NET Framework applications should target .NET Framework 4.8 or later for TLS 1.3 support.
+* Do not specify the TLS version explicitly. Configure your code to let the OS decide on the TLS version.
+* Do not disable the use of strong cryptography for TLS via configuration.
+
+## Details
+
+### Defer Choice of TLS Version to OS
+
+The Transport Layer Security (TLS) protocol is a cryptographic standard designed to help protect the privacy of information communicated over the Internet. Cryptographic algorithms once considered secure may become insecure due to advances in computing power or discovery of subtle flaws in the algorithms.
+
+Software with hardcoded cryptographic algorithms may require code changes in the future if those algorithms become insecure. A better alternative is to defer control of the cryptographic algorithm to the operating system.
+
+### Implementation
+
+#### .NET
+
+* When `Switch.System.ServiceModel.DontEnableSystemDefaultTlsVersions` is `false`, the application will use TLS protocol chosen by the operating system. Do not set `DontEnableSystemDefaultTlsVersions` to `false`.
+* When `Switch.System.Net.DontEnableSchUseStrongCrypto` is `false`, the application will use more secure network protocols. Do not set `DontEnableSchUseStrongCrypto` to `true`.
+* When application code sets a value for `System.Net.ServicePointManager.SecurityProtocol`, the application will override the TLS protocol chosen by the operating system. This makes the application less crypto-agile and may make the application less secure. Do not set a value for `System.Net.ServicePointManager.SecurityProtocol` in application code.
+
+## References
+
+* [TLS Best Practices with .NET Framework](https://learn.microsoft.com/en-us/dotnet/framework/network-programming/tls)
+* [Security Briefs - Cryptographic Agility](https://learn.microsoft.com/en-us/archive/msdn-magazine/2009/august/cryptographic-agility)
+* [Microsoft SDL Cryptographic Recommendations](http://download.microsoft.com/download/6/3/A/63AFA3DF-BB84-4B38-8704-B27605B99DA7/Microsoft%20SDL%20Cryptographic%20Recommendations.pdf)
+* [SSL Labs: SSL and TLS Deployment Best Practices](https://github.com/ssllabs/research/wiki/SSL-and-TLS-Deployment-Best-Practices)
+* [OWASP: Transport Layer Protection Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Security_Cheat_Sheet.html)

guidance/DS112835.md

@@ -1,19 +1,25 @@
-## Weak/Broken Hash Algorithm
+# Weak/Broken Hash Algorithm
+
+## Summary
 
-### Summary
 A weak or broken hash algorithm was detected.
+Any usage of MD2, MD4, MD5 or SHA-1 is considered insecure.
+Replace the use of insecure hashing algorithms with more secure alternatives such as an algorithm from the SHA-2 family (SHA256, SHA384, and SHA512).
+
+## Details
+
+Hash collisions are computationally feasible for older, weak hash algorithms such as MD2, MD4, MD5, and SHA-1.
+A hash collision allows an attacker to substitute an alternative input that results in the same hash value.
+Collision attacks allow attackers to undermine the security of systems using an insecure hash algorithm (e.g., by forging digital signatures, concealing data tampering, or cracking passwords).
+
+## Solution
 
-### Details
-The use of signers like `MD5WithRSAEncryption` in cryptography providers like BouncyCastle
-is susceptible to colission attacks. Anything that uses MD2, MD4, MD5 or SHA-1 is considered
-insecure.
+### .NET
 
-Replace the use of insecure hashing algorithms with more secure alternatives, from SHA256 onward.
-See the list of available BouncyCastle signers here:
-https://github.com/neoeinstein/bouncycastle/blob/master/crypto/src/security/SignerUtilities.cs.
+Replace usages of insecure hash algorithms with `System.Security.Cryptography.SHA512Cng`, `System.Security.Cryptography.SHA384Cng`, or `System.Security.Cryptography.SHA256Cng`.
 
-For more information, see https://codeql.github.com/codeql-query-help/python/py-weak-sensitive-data-hashing/.
+### Python
 
-### Severity Considerations
-Data signed using broken hash algorithms like MD2, MD4, MD5 and SHA1 can be broken using specially designed hardware/software.
+The use of signers like `MD5WithRSAEncryption` in cryptography providers like BouncyCastle is susceptible to collision attacks. See the list of available [BouncyCastle signers](https://github.com/neoeinstein/bouncycastle/blob/master/crypto/src/security/SignerUtilities.cs).
 
+For more information, see [CodeQL Python Hash Algorithm Guidance](https://codeql.github.com/codeql-query-help/python/py-weak-sensitive-data-hashing/).

guidance/DS126858.md

@@ -0,0 +1,26 @@
+# Banned C function detected (strlen)
+
+## Summary
+
+* Use of the `strlen` function to determine the length of a string can lead to a buffer overrun vulnerability.
+* Use secure versions such as `strlen_s` or `strnlen` to help prevent buffer overruns.
+
+## Details
+
+The `strlen` function counts characters until the null terminator is encountered.
+When a string is missing the null terminator, the resulting value returned is larger than the string.
+Code that relies on the result of `strlen` can suffer from a buffer overrun vulnerability.
+
+## Severity Considerations
+
+In the worst case, a buffer overrun vulnerability can provide an attacker the ability to execute arbitrary code leading to complete system compromise.
+
+## Solution
+
+Use secure versions such as `strlen_s` or `strnlen` to help prevent buffer overruns. See [Microsoft C Runtime Reference: strnlen](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strnlen-strnlen-s) for more information.
+
+## References
+
+* [Avoiding Buffer Overruns](https://learn.microsoft.com/en-us/windows/win32/SecBP/avoiding-buffer-overruns)
+* [Microsoft C Runtime Reference: strlen](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strlen-wcslen-mbslen-mbslen-l-mbstrlen-mbstrlen-l)
+* [Microsoft C Runtime Reference: strnlen](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strnlen-strnlen-s)

guidance/DS140021.md

@@ -0,0 +1,13 @@
+# $_REQUEST should be avoided
+
+## Description
+
+`$_REQUEST` combines POST, GET, and cookie values in one array, making it easy for an attacker to modify a POST or cookie value by instead putting it in a querystring parameter and sending the URL to the victim.
+
+## Solution
+
+Use $_POST, $_GET, $_COOKIE to scope to the expected delivery method for a value.
+
+## References
+
+* [OWASP PHP Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/PHP_Configuration_Cheat_Sheet.html#Use_of_.24_REQUEST)

guidance/DS144886.md

@@ -0,0 +1,13 @@
+# Do not echo unencoded GET/POST/COOKIE values
+
+## Description
+
+When using $_GET/POST/COOKIE values via echo, failure to  encode the values will lead to Cross Site Scripting (XSS), where a malicious party can inject script into the webpage.
+
+## Solution
+
+HTML Entity Encode (for content going into HTML) or URL Encode (for content going into JavaScript variables) the data.
+
+## References
+
+- [OWASP XSS Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html)