From 585e4931730833bb425c493ae528f8a2b33f0be2 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Fri, 26 Apr 2024 15:43:16 -0400 Subject: [PATCH 01/18] Add tests to identify rules with missing or incomplete guidance --- .../DefaultRulesTests.cs | 104 ++++++++++++++++++ .../Microsoft.DevSkim/DevSkimRule.cs | 5 + 2 files changed, 109 insertions(+) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs index a4a60986..1006e7e4 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs @@ -1,3 +1,5 @@ +using System.Reflection; + namespace Microsoft.DevSkim.Tests; [TestClass] @@ -80,4 +82,106 @@ public void DenamespacedRule() IEnumerable analysis = analyzer.Analyze(content, "thing.xml"); Assert.AreEqual(1, analysis.Count()); } + + public static IEnumerable DefaultRules + { + get + { + DevSkimRuleSet devSkimRuleSet = DevSkimRuleSet.GetDefaultRuleSet(); + foreach (DevSkimRule rule in devSkimRuleSet) + { + yield return new object[] { rule }; + } + } + } + + [TestMethod] + [DynamicData(nameof(DefaultRules))] + public void Rule_should_specify_guidance(DevSkimRule rule) + { + if (rule.Disabled) + { + Assert.Inconclusive("Rule is disabled."); + } + + if (string.IsNullOrEmpty(rule.RuleInfo)) + { + Assert.Fail("Rule does not specify guidance file."); + } + } + + [TestMethod] + [DynamicData(nameof(DefaultRules))] + public void Rule_guidance_file_should_exist(DevSkimRule rule) + { + if (rule.Disabled) + { + Assert.Inconclusive("Rule is disabled."); + } + + if (string.IsNullOrEmpty(rule.RuleInfo)) + { + Assert.Inconclusive("Rule does not specify guidance file."); + } + + string guidanceDir = GetGuidanceDirectory(); + string guidanceFile = Path.Combine(guidanceDir, rule.RuleInfo); + Assert.IsTrue(File.Exists(guidanceFile), $"Guidance file {guidanceFile} does not exist."); + } + + [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 guidanceDir = GetGuidanceDirectory(); + string guidanceFile = Path.Combine(guidanceDir, rule.RuleInfo); + if(!File.Exists(guidanceFile)) + { + Assert.Inconclusive("Guidance file does not exist"); + } + + string guidance = File.ReadAllText(guidanceFile); + if(guidance.Contains("TODO")) + { + Assert.Fail($"Guidance file {guidanceFile} contains TODO."); + } + } + + private static string GetGuidanceDirectory() + { + string directory = Directory.GetCurrentDirectory(); + + /* Given a directory, like: "C:\src\DevSkim\DevSkim-DotNet\Microsoft.DevSkim.Tests\bin\Debug\net8.0" + * we want to find: "C:\src\DevSkim\guidance" + */ + + var currentDirInfo = new DirectoryInfo(directory); + while (currentDirInfo != null && currentDirInfo.Name != "DevSkim") + { + currentDirInfo = currentDirInfo.Parent; + } + + if (currentDirInfo == null) + { + throw new Exception("Could not find DevSkim-DotNet directory."); + } + + string guidanceDir = Path.Combine(currentDirInfo.FullName, "guidance"); + if (!Directory.Exists(guidanceDir)) + { + throw new Exception($"Guidance directory {guidanceDir} does not exist."); + } + + return guidanceDir; + } } \ No newline at end of file diff --git a/DevSkim-DotNet/Microsoft.DevSkim/DevSkimRule.cs b/DevSkim-DotNet/Microsoft.DevSkim/DevSkimRule.cs index f183606f..6148d7c8 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim/DevSkimRule.cs +++ b/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}'"; + } } } \ No newline at end of file From bfdfe3434294cff57bbb24902ae1996a120f44ce Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Fri, 26 Apr 2024 16:11:08 -0400 Subject: [PATCH 02/18] Also consider guidance with "TO DO" incomplete --- DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs index 1006e7e4..553ef812 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs @@ -151,7 +151,8 @@ public void Rule_guidance_should_be_complete(DevSkimRule rule) } string guidance = File.ReadAllText(guidanceFile); - if(guidance.Contains("TODO")) + if(guidance.Contains("TODO", StringComparison.OrdinalIgnoreCase) + || guidance.Contains("TO DO", StringComparison.OrdinalIgnoreCase)) { Assert.Fail($"Guidance file {guidanceFile} contains TODO."); } From f2380a5c0800d0c34f577a3b7affec88666aea1d Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Fri, 10 May 2024 16:42:30 -0400 Subject: [PATCH 03/18] Point DES rules at same guidance --- rules/default/security/cryptography/ciphers.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rules/default/security/cryptography/ciphers.json b/rules/default/security/cryptography/ciphers.json index 98bc9947..5e526983 100644 --- a/rules/default/security/cryptography/ciphers.json +++ b/rules/default/security/cryptography/ciphers.json @@ -90,7 +90,7 @@ ], "confidence": "medium", "severity": "critical", - "rule_info": "DS106863.md", + "rule_info": "DS106864.md", "patterns": [ { "pattern": "DES", @@ -181,7 +181,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS106865.md", + "rule_info": "DS106864.md", "patterns": [ { "pattern": "['\"](?:DES(?:-(?:C(?:BC|FB[18]?)|E(?:CB|DE(?:-(?:C(?:BC|FB)|OFB))?)|OFB))?)['\"]", @@ -220,7 +220,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS106866.md", + "rule_info": "DS106864.md", "patterns": [ { "pattern": "DESEngine|DESedeEngine", From 7462d8873dfad116324131d90a77f62e44a115eb Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Fri, 10 May 2024 16:42:41 -0400 Subject: [PATCH 04/18] Add guidance for .NET TLS config --- guidance/DS112835.md | 33 +++++++++++++++++++ guidance/DS440000.md | 8 ++--- rules/default/security/TLS/tls_appconfig.json | 2 +- .../default/security/TLS/tls_appcontext.json | 2 +- .../default/security/TLS/tls_sslprotocol.json | 8 ++--- 5 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 guidance/DS112835.md diff --git a/guidance/DS112835.md b/guidance/DS112835.md new file mode 100644 index 00000000..ae178e99 --- /dev/null +++ b/guidance/DS112835.md @@ -0,0 +1,33 @@ +# 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://owasp.org/www-project-cheat-sheets/cheatsheets/Transport_Layer_Protection_Cheat_Sheet) +* [OWASP: TLS Cipher String Cheat Sheet](https://owasp.org/www-project-cheat-sheets/cheatsheets/TLS_Cipher_String_Cheat_Sheet.html) + diff --git a/guidance/DS440000.md b/guidance/DS440000.md index 982258e1..febd1b81 100644 --- a/guidance/DS440000.md +++ b/guidance/DS440000.md @@ -15,18 +15,18 @@ document for additional information and best practices. ### Use TLS Everywhere -*TLS Everywhere* is a monicker the really means "encrypt all network +*TLS Everywhere* is a moniker the really means "encrypt all network traffic whenever it crosses a trust boundary". A trust boundary is just a fancy phrase meaning, "where you trust one side differently than the other". However, like all things, there's some grey-area in here. -Common scenarios that definitely cross a trust bounary include: +Common scenarios that definitely cross a trust boundary include: * Connecting to a location on the Internet. * Connecting to a location on another service, owned by another team at your company. -Scenarios where you almost certainly don't cross a trust bounary include: +Scenarios where you almost certainly don't cross a trust boundary include: * Connecting to a service running on the same operating system. @@ -36,7 +36,7 @@ Scenarios somewhere in the middle include: * Connections between services within a virtual network, your own hardware rack, or through a VPN. -Its up to you to decice what your trust model looks like, but we recommend +Its up to you to decide what your trust model looks like, but we recommend encrypting network traffic as much as possible. HTTPS (TLS over HTTP) is the most common application-layer communication diff --git a/rules/default/security/TLS/tls_appconfig.json b/rules/default/security/TLS/tls_appconfig.json index 2dc6264e..1917405f 100644 --- a/rules/default/security/TLS/tls_appconfig.json +++ b/rules/default/security/TLS/tls_appconfig.json @@ -12,7 +12,7 @@ ], "confidence": "high", "severity": "ManualReview", - "rule_info": "DS112838.md", + "rule_info": "DS112835.md", "patterns": [ { "pattern": "Switch.System.Net.DontEnableSchUseStrongCrypto", diff --git a/rules/default/security/TLS/tls_appcontext.json b/rules/default/security/TLS/tls_appcontext.json index c399d0cb..4bb248be 100644 --- a/rules/default/security/TLS/tls_appcontext.json +++ b/rules/default/security/TLS/tls_appcontext.json @@ -15,7 +15,7 @@ ], "confidence": "high", "severity": "ManualReview", - "rule_info": "DS440000.md", + "rule_info": "DS112835.md", "patterns": [ { "pattern": "TestSwitch.LocalAppContext.DisableCaching", diff --git a/rules/default/security/TLS/tls_sslprotocol.json b/rules/default/security/TLS/tls_sslprotocol.json index ddc0547c..02f81b6c 100644 --- a/rules/default/security/TLS/tls_sslprotocol.json +++ b/rules/default/security/TLS/tls_sslprotocol.json @@ -8,7 +8,7 @@ "DS440000" ], "applies_to": [ - "csharp", + "csharp", "powershell", "vb", "fsharp" @@ -18,7 +18,7 @@ ], "confidence": "high", "severity": "ManualReview", - "rule_info": "DS440000.md", + "rule_info": "DS112835.md", "patterns": [ { "pattern": "SslProtocolsExtensions.Tls11", @@ -89,14 +89,14 @@ "scopes": [ "code" ] - }, + }, { "pattern": "SslProtocols.Tls", "type": "string", "scopes": [ "code" ] - }, + }, { "pattern": "(SslProtocols)192", "type": "string", From d2dfa1e43615ce66a1d4dc1b8d7f41df5b522c72 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Tue, 21 May 2024 10:09:07 -0400 Subject: [PATCH 05/18] Add guidance for .NET Framework 4.7.2 rule --- .../DefaultRulesTests.cs | 2 -- guidance/DS450000.md | 21 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 guidance/DS450000.md diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs index 553ef812..6f9d5538 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs @@ -1,5 +1,3 @@ -using System.Reflection; - namespace Microsoft.DevSkim.Tests; [TestClass] diff --git a/guidance/DS450000.md b/guidance/DS450000.md new file mode 100644 index 00000000..b3ff1638 --- /dev/null +++ b/guidance/DS450000.md @@ -0,0 +1,21 @@ +# .NET Framework Version + +## Summary + +* New applications should target a supported version of .NET (formerly .NET Core) if possible. +* .NET Framework applications should target at least .NET Framework 4.7.2. + +## Details + +Older versions of .NET framework have reached end-of-life state and will no longer receive security patches. .NET Framework applications should target supported versions. + +Furthermore, .NET Framework 4.7.2 offers a large number of cryptographic enhancements necessary to prevent against modern cryptographic attacks. Therefore, .NET Framework applications should target at least version 4.7.2. Note that transport layer security (TLS) security guidance advises developers to target .NET Framework 4.8 or later for TLS 1.3 support. + +If possible, new applications should target a supported version of .NET (formerly .NET Core) because the .NET Framework is no longer under active development and is only receiving security patches. + +## References + +* [.NET Framework Lifecycle](https://learn.microsoft.com/en-us/lifecycle/products/microsoft-net-framework) +* [.NET Framework 4.7.2](https://learn.microsoft.com/en-us/dotnet/framework/whats-new/#v472) +* [TLS best practices with .NET Framework](https://learn.microsoft.com/en-us/dotnet/framework/network-programming/tls) +* [.NET Support Policy](https://dotnet.microsoft.com/platform/support/policy) \ No newline at end of file From 72a3f51947eaaccd5ff171b09cb33b7785891b36 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Tue, 21 May 2024 11:32:55 -0400 Subject: [PATCH 06/18] Add guidance for .NET Core advisory 4021279 --- guidance/DS300001.md | 7 +++++++ .../security/vulnerable_libraries/microsoft_nuget.json | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 guidance/DS300001.md diff --git a/guidance/DS300001.md b/guidance/DS300001.md new file mode 100644 index 00000000..4a1ae4b6 --- /dev/null +++ b/guidance/DS300001.md @@ -0,0 +1,7 @@ +# Vulnerabilities in .NET Core, ASP.NET Core Could Allow Elevation of Privilege + +A vulnerable version of a .NET Core or ASP.NET Core dependency was found. Update the affected dependency to a newer version. See the related [advisory](https://github.com/dotnet/announcements/issues/12) for more information. + +## References + +* [Microsoft Security Advisory 4021279: Vulnerabilities in .NET Core, ASP.NET Core Could Allow Elevation of Privilege](https://github.com/dotnet/announcements/issues/12) diff --git a/rules/default/security/vulnerable_libraries/microsoft_nuget.json b/rules/default/security/vulnerable_libraries/microsoft_nuget.json index 355d4e05..92546659 100644 --- a/rules/default/security/vulnerable_libraries/microsoft_nuget.json +++ b/rules/default/security/vulnerable_libraries/microsoft_nuget.json @@ -12,7 +12,7 @@ ], "confidence": "high", "severity": "moderate", - "rule_info": "4021279", + "rule_info": "DS300001.md", "patterns": [ { "pattern": " Date: Tue, 21 May 2024 11:38:17 -0400 Subject: [PATCH 07/18] Add guidance for Microsoft.IdentityModel.Tokens rule --- guidance/DS300005.md | 7 +++++++ .../security/vulnerable_libraries/microsoft_nuget.json | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 guidance/DS300005.md diff --git a/guidance/DS300005.md b/guidance/DS300005.md new file mode 100644 index 00000000..16161ad0 --- /dev/null +++ b/guidance/DS300005.md @@ -0,0 +1,7 @@ +# Vulnerabilities in Identity Model Extensions Token Signing Verification Could Allow Elevation of Privilege + +A vulnerable version of Identity Model Extensions was found. Update the affected dependency to a newer, unaffected version. See the related [advisory](https://learn.microsoft.com/en-us/security-updates/SecurityAdvisories/2017/3214296) for more information. + +## References + +* [Microsoft Security Advisory 3214296](https://learn.microsoft.com/en-us/security-updates/SecurityAdvisories/2017/3214296) diff --git a/rules/default/security/vulnerable_libraries/microsoft_nuget.json b/rules/default/security/vulnerable_libraries/microsoft_nuget.json index 92546659..66bbaa64 100644 --- a/rules/default/security/vulnerable_libraries/microsoft_nuget.json +++ b/rules/default/security/vulnerable_libraries/microsoft_nuget.json @@ -171,7 +171,7 @@ ], "confidence": "high", "severity": "moderate", - "rule_info": "3214296", + "rule_info": "DS300005.md", "patterns": [ { "pattern": " Date: Tue, 21 May 2024 19:50:49 -0400 Subject: [PATCH 08/18] Add guidance for unsafe keyword rule --- guidance/DS172412.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 guidance/DS172412.md diff --git a/guidance/DS172412.md b/guidance/DS172412.md new file mode 100644 index 00000000..ed61f5fa --- /dev/null +++ b/guidance/DS172412.md @@ -0,0 +1,10 @@ +# Unsafe Keyword + +The `unsafe` keyword denotes an unsafe context, which is required for any operation involving pointers. Unsafe code in is not necessarily dangerous; it is just code whose safety cannot be verified by the CLR. + +Using unsafe code introduces security and stability risks. Special attention should be paid to any user-controlled data that is used within unsafe code. + +## References + +* [unsafe C# Reference](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/unsafe) +* [Unsafe code, pointer types, and function pointers](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/unsafe-code) From b32858cc48d13b6c270481419a66aa204853a2af Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Wed, 22 May 2024 08:16:49 -0400 Subject: [PATCH 09/18] Add guidance for JS setTimeout rule --- guidance/DS172411.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 guidance/DS172411.md diff --git a/guidance/DS172411.md b/guidance/DS172411.md new file mode 100644 index 00000000..74ca053e --- /dev/null +++ b/guidance/DS172411.md @@ -0,0 +1,17 @@ +# Review setTimeout for untrusted data + +## Summary + +* Avoid use of untrusted data within `setTimeout` + +## Description + +Several DOM functions such as `setTimeout` and `eval` will execute a string parameter as code. If untrusted data (data from HTTP requests, user submitted files, etc.) is included in a `setTimeout` call it can allow an attacker to execute their own code. + +## Recommendation + +Edit the `setTimeout` call so that untrusted data is NOT included. If untrusted data is absolutely necessary a great deal of care should be taken to ensure it is properly escaped so that it cannot be executed. This is not as simple as just escaping quotes. + +## References + +* [CWE-676: Use of Potentially Dangerous Function](https://cwe.mitre.org/data/definitions/676.html) From 04298bc495ceaf55c2fe26e0e6645d59993285cb Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Wed, 22 May 2024 10:34:31 -0400 Subject: [PATCH 10/18] Add guidance for weak/broken hash algo rule --- guidance/DS106864.md | 17 ++++++----- guidance/DS108647.md | 11 ------- guidance/DS126858.md | 30 +++++++++++-------- guidance/DS168931.md | 11 ------- guidance/DS196098.md | 11 ------- guidance/DS197800.md | 11 ------- .../security/cryptography/hash_algorithm.json | 14 ++++----- 7 files changed, 34 insertions(+), 71 deletions(-) delete mode 100644 guidance/DS108647.md delete mode 100644 guidance/DS168931.md delete mode 100644 guidance/DS196098.md delete mode 100644 guidance/DS197800.md diff --git a/guidance/DS106864.md b/guidance/DS106864.md index 074ea9e9..a173b8b9 100644 --- a/guidance/DS106864.md +++ b/guidance/DS106864.md @@ -1,9 +1,11 @@ -## 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. @@ -11,9 +13,9 @@ 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. diff --git a/guidance/DS108647.md b/guidance/DS108647.md deleted file mode 100644 index de6270e3..00000000 --- a/guidance/DS108647.md +++ /dev/null @@ -1,11 +0,0 @@ -## Do not use broken/weak cryptographic hash algorithms - -### Summary -Avoid using broken or weak hash algorithms. - -### Details -TO DO - put more details of problem and solution here - -### Severity Considerations -TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem? - diff --git a/guidance/DS126858.md b/guidance/DS126858.md index 9b789bf0..b5ddcce8 100644 --- a/guidance/DS126858.md +++ b/guidance/DS126858.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/). diff --git a/guidance/DS168931.md b/guidance/DS168931.md deleted file mode 100644 index f361900d..00000000 --- a/guidance/DS168931.md +++ /dev/null @@ -1,11 +0,0 @@ -## Do not use broken/weak cryptographic hash algorithms - -### Summary -A potentially weak hashing algorithm was used. - -### Details -TO DO - put more details of problem and solution here - -### Severity Considerations -TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem? - diff --git a/guidance/DS196098.md b/guidance/DS196098.md deleted file mode 100644 index de6270e3..00000000 --- a/guidance/DS196098.md +++ /dev/null @@ -1,11 +0,0 @@ -## Do not use broken/weak cryptographic hash algorithms - -### Summary -Avoid using broken or weak hash algorithms. - -### Details -TO DO - put more details of problem and solution here - -### Severity Considerations -TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem? - diff --git a/guidance/DS197800.md b/guidance/DS197800.md deleted file mode 100644 index 2fe76e14..00000000 --- a/guidance/DS197800.md +++ /dev/null @@ -1,11 +0,0 @@ -## Weak/Broken Hash Algorithm - -### Summary -A weak or broken hash algorithm was detected. - -### Details -TO DO - put more details of problem and solution here - -### Severity Considerations -TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem? - diff --git a/rules/default/security/cryptography/hash_algorithm.json b/rules/default/security/cryptography/hash_algorithm.json index cdbd4bc0..6b98d740 100644 --- a/rules/default/security/cryptography/hash_algorithm.json +++ b/rules/default/security/cryptography/hash_algorithm.json @@ -99,7 +99,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS197800.md", + "rule_info": "DS126858.md", "patterns": [ { "pattern": "CC_(MD2|MD4|MD5|SHA1)", @@ -159,7 +159,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS128420.md", + "rule_info": "DS126858.md", "patterns": [ { "pattern": "hash\\s*\\(\\s*[\\'\\\"](md2|md4|md5|sha1|sha224|ripemd128|ripemd160|ripemd256|ripemd320)[\\'\\\"](.*)\\)", @@ -205,19 +205,19 @@ "name": "Do not use broken/weak cryptographic hash algorithms", "id": "DS108647", "description": "Avoid using broken or weak hash algorithms.", - "recommendation": "Use Digest::SHA256 or Digest::SHA512", + "recommendation": "Use Digest::SHA256 or Digest::SHA512", "applies_to": [ "ruby" ], "overrides": [ "DS126858" - ], + ], "tags": [ "Cryptography.HashAlgorithm.BrokenOrWeak" ], "confidence": "high", "severity": "critical", - "rule_info": "DS108647.md", + "rule_info": "DS126858.md", "patterns": [ { "pattern": "Digest::(MD5|RMD160|SHA1)", @@ -281,7 +281,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS196098.md", + "rule_info": "DS126858.md", "patterns": [ { "pattern": "(md5|sha)\\.new\\(", @@ -342,7 +342,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS168931.md", + "rule_info": "DS126858.md", "patterns": [ { "pattern": "MD5CryptoServiceProvider", From 3de0493b328819a1cc02c3ddcbe6280456fe3b99 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Wed, 22 May 2024 14:56:08 -0400 Subject: [PATCH 11/18] Add guidance for disabling cert validation rule --- guidance/DS112835.md | 8 ++-- guidance/DS172411.md | 2 +- guidance/DS181865.md | 39 ++++++++++++++++--- guidance/DS440000.md | 4 +- .../security/cryptography/certificate.json | 20 +++++----- 5 files changed, 48 insertions(+), 25 deletions(-) diff --git a/guidance/DS112835.md b/guidance/DS112835.md index ae178e99..cf39f98f 100644 --- a/guidance/DS112835.md +++ b/guidance/DS112835.md @@ -10,13 +10,13 @@ ### 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. +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 +#### .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`. @@ -28,6 +28,4 @@ Software with hardcoded cryptographic algorithms may require code changes in the * [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://owasp.org/www-project-cheat-sheets/cheatsheets/Transport_Layer_Protection_Cheat_Sheet) -* [OWASP: TLS Cipher String Cheat Sheet](https://owasp.org/www-project-cheat-sheets/cheatsheets/TLS_Cipher_String_Cheat_Sheet.html) - +* [OWASP: Transport Layer Protection Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Security_Cheat_Sheet.html) diff --git a/guidance/DS172411.md b/guidance/DS172411.md index 74ca053e..e75ec843 100644 --- a/guidance/DS172411.md +++ b/guidance/DS172411.md @@ -6,7 +6,7 @@ ## Description -Several DOM functions such as `setTimeout` and `eval` will execute a string parameter as code. If untrusted data (data from HTTP requests, user submitted files, etc.) is included in a `setTimeout` call it can allow an attacker to execute their own code. +Several DOM functions such as `setTimeout` and `eval` will execute a string parameter as code. If untrusted data (data from HTTP requests, user submitted files, etc.) is included in a `setTimeout` call it can allow an attacker to inject and execute their own code. ## Recommendation diff --git a/guidance/DS181865.md b/guidance/DS181865.md index bb922aeb..3095d125 100644 --- a/guidance/DS181865.md +++ b/guidance/DS181865.md @@ -1,11 +1,38 @@ -## Disabled certificate validation +# Certificate Validation Disabled + +## Summary -### Summary Extend default certificate validation, but do not disable or override default rules. -### Details -TO DO - put more details of problem and solution here +## Details + +Certificate validation ensures that the client application is communicating with the intended server, that is, +the client's connection to the server has not been compromised by a man-in-the-middle attack. + +When a client application disables certificate validation, there is no guarantee that the client is communicating with the intended server and +any sensitive or confidential information submitted can be read by an attacker. + +TLS certificates should be fully verified including domain name, validity dates, revocation status, and certification authority trust chain. + +## Severity Considerations + +Clients should not continue to communicate with servers when a certificate cannot be validated. +Without certificate validation, there is no guarantee that the server has not been impersonated by an attacker who will then have access to any sensitive information sent to the server. + +## Solution + +### .NET + +Never disable certificate validation. + +* Remove `ServerCertificateValidationCallback = return true` +* Do NOT set `X509CertificateValidationMode = X509CertificateValidationMode.None` +* Do NOT set `SkipTlsVerify=true` + +### Python + +Never use `verify=False` when making a request. See [CodeQL Guidance on Certificate Validation](https://codeql.github.com/codeql-query-help/python/py-request-without-cert-validation/) for more information. -### Severity Considerations -TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem? +### JavaScript +Never disable certificate validation for TLS connections by setting `rejectUnauthorized: false`. See [CodeQL Guidance on Certificate Validation](https://codeql.github.com/codeql-query-help/javascript/js-disabling-certificate-validation/) for more information. diff --git a/guidance/DS440000.md b/guidance/DS440000.md index febd1b81..f24367d1 100644 --- a/guidance/DS440000.md +++ b/guidance/DS440000.md @@ -105,6 +105,4 @@ context is highly sensitive or if the network has low trust. * [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://owasp.org/www-project-cheat-sheets/cheatsheets/Transport_Layer_Protection_Cheat_Sheet) -* [OWASP: TLS Cipher String Cheat Sheet](https://owasp.org/www-project-cheat-sheets/cheatsheets/TLS_Cipher_String_Cheat_Sheet.html) - +* [OWASP: Transport Layer Protection Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Security_Cheat_Sheet.html) diff --git a/rules/default/security/cryptography/certificate.json b/rules/default/security/cryptography/certificate.json index d5a56be4..90390603 100644 --- a/rules/default/security/cryptography/certificate.json +++ b/rules/default/security/cryptography/certificate.json @@ -123,7 +123,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "OpenSSL::SSL::VERIFY_NONE", @@ -152,7 +152,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS130822.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "\\.check_hostname\\s*=\\s*False", @@ -193,7 +193,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "SECURITY_FLAG_IGNORE_CERT_CN_INVALID", @@ -255,7 +255,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "certificateValidationMode\\s*=\\s*\"None\"", @@ -305,7 +305,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "NODE_TLS_REJECT_UNAUTHORIZED|rejectUnauthorized", @@ -332,7 +332,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "(TrustManager|getInsecure|HostnameVerifier|AbstractVerifier|AllowAllHostnameVerifier|BrowserCompatHostnameVerifier|StrictHostnameVerifier|onReceivedSslError|insecuresocketfactory|customhostnameverifier)", @@ -359,7 +359,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "::ServerCertificateValidationCallback\\s*=\\s*{\\s*\\$true\\s*}", @@ -386,7 +386,7 @@ ], "confidence": "medium", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "verify\\s*=\\s*False", @@ -429,7 +429,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "ServerCertificateValidationCallback\\s*=\\s*delegate\\s*{\\s*return true;\\s*}", @@ -506,7 +506,7 @@ ], "confidence": "high", "severity": "critical", - "rule_info": "DS114352.md", + "rule_info": "DS181865.md", "patterns": [ { "pattern": "--insecure-skip-tls-verify(=true|$|[^=])", From 279e88aea5f8daa57f0d27f0f30542b4226912d6 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Wed, 22 May 2024 15:35:42 -0400 Subject: [PATCH 12/18] Add guidance for avoid $_REQUEST rule --- guidance/DS144886.md | 13 +++++++++++++ guidance/DS450000.md | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 guidance/DS144886.md diff --git a/guidance/DS144886.md b/guidance/DS144886.md new file mode 100644 index 00000000..ae08d205 --- /dev/null +++ b/guidance/DS144886.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) diff --git a/guidance/DS450000.md b/guidance/DS450000.md index b3ff1638..54de5516 100644 --- a/guidance/DS450000.md +++ b/guidance/DS450000.md @@ -18,4 +18,4 @@ If possible, new applications should target a supported version of .NET (formerl * [.NET Framework Lifecycle](https://learn.microsoft.com/en-us/lifecycle/products/microsoft-net-framework) * [.NET Framework 4.7.2](https://learn.microsoft.com/en-us/dotnet/framework/whats-new/#v472) * [TLS best practices with .NET Framework](https://learn.microsoft.com/en-us/dotnet/framework/network-programming/tls) -* [.NET Support Policy](https://dotnet.microsoft.com/platform/support/policy) \ No newline at end of file +* [.NET Support Policy](https://dotnet.microsoft.com/platform/support/policy) From db94608fef318e428cdc3f05046d8a38a56b5ecf Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Wed, 22 May 2024 15:44:28 -0400 Subject: [PATCH 13/18] Add guidance for PHP XSS rule --- guidance/DS163877.md | 13 +++++++++++++ rules/default/security/frameworks/php.json | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 guidance/DS163877.md diff --git a/guidance/DS163877.md b/guidance/DS163877.md new file mode 100644 index 00000000..f7f36226 --- /dev/null +++ b/guidance/DS163877.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) \ No newline at end of file diff --git a/rules/default/security/frameworks/php.json b/rules/default/security/frameworks/php.json index fa83080a..d6652811 100644 --- a/rules/default/security/frameworks/php.json +++ b/rules/default/security/frameworks/php.json @@ -72,7 +72,7 @@ { "name": "XSS: Do not echo unencoded GET/POST/COOKIE values", "id": "DS163877", - "description": "When using $_GET/POST/COOKIE values via echo, failure to encode the values will lead to Cross Site Scription (XSS), where a malicious party can inject script into the webpage.", + "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.", "recommendation": "HTML Entity Encode (for content going into HTML) or URL Encode (for content going into JavaScript variables) the data", "applies_to": [ "php" From 59477e8564bbe3a2f7b7c1c48ed048f320a00d8a Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Wed, 22 May 2024 16:10:04 -0400 Subject: [PATCH 14/18] Add guidance for strlen rule --- guidance/DS140021.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 guidance/DS140021.md diff --git a/guidance/DS140021.md b/guidance/DS140021.md new file mode 100644 index 00000000..6d064736 --- /dev/null +++ b/guidance/DS140021.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) From eac6aa67d327d8ce888403f1dbfd28d0611e9d0d Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Wed, 22 May 2024 16:22:45 -0400 Subject: [PATCH 15/18] Add guidance for Python datetime rule --- .../Microsoft.DevSkim.Tests/DefaultRulesTests.cs | 1 + guidance/DS163877.md | 2 +- guidance/DS600100.md | 10 ++++++++++ rules/default/correctness/datetime.json | 2 +- 4 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 guidance/DS600100.md diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs index 6f9d5538..c33f8e08 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs @@ -127,6 +127,7 @@ public void Rule_guidance_file_should_exist(DevSkimRule rule) 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) diff --git a/guidance/DS163877.md b/guidance/DS163877.md index f7f36226..73a2b359 100644 --- a/guidance/DS163877.md +++ b/guidance/DS163877.md @@ -10,4 +10,4 @@ HTML Entity Encode (for content going into HTML) or URL Encode (for content goin ## References -- [OWASP XSS Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html) \ No newline at end of file +- [OWASP XSS Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html) diff --git a/guidance/DS600100.md b/guidance/DS600100.md new file mode 100644 index 00000000..95b7c2e9 --- /dev/null +++ b/guidance/DS600100.md @@ -0,0 +1,10 @@ +# Possible incorrect datetime format + +## Summary + +* The `%M` format is 'minute' but used like 'month' +* Resolve this issue by changing the format to `%m`. + +## References + +* [PHP Manual: strftime](https://www.php.net/manual/en/function.strftime.php) diff --git a/rules/default/correctness/datetime.json b/rules/default/correctness/datetime.json index 340d783f..d3e75e9c 100644 --- a/rules/default/correctness/datetime.json +++ b/rules/default/correctness/datetime.json @@ -11,7 +11,7 @@ "Correctness.DateTime.Format" ], "severity": "moderate", - "rule_info": "", + "rule_info": "DS600100.md", "patterns": [ { "confidence": "high", From 4e5796e5d572f53f5a3649224b621111d5e91a53 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Thu, 23 May 2024 11:03:07 -0400 Subject: [PATCH 16/18] Add changelog for guidance changes --- Changelog.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index e3e7d708..24111a3f 100644 --- a/Changelog.md +++ b/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. From 4e743420e05e07e37b8b8abaa57859d1b57c0d36 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Thu, 23 May 2024 12:46:57 -0400 Subject: [PATCH 17/18] Add debug info to guidance tests to troubleshoot CI --- .../DefaultRulesTests.cs | 87 ++++++++----------- 1 file changed, 37 insertions(+), 50 deletions(-) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs index c33f8e08..b11c3c9f 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs @@ -3,12 +3,44 @@ 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" + * we want to find: "C:\src\DevSkim\guidance" + */ + + var currentDirInfo = new DirectoryInfo(directory); + while (currentDirInfo != null && currentDirInfo.Name != "DevSkim") + { + currentDirInfo = currentDirInfo.Parent; + } + + if (currentDirInfo == null) + { + string message = $"Could not find DevSkim 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() }); @@ -95,7 +127,7 @@ public static IEnumerable DefaultRules [TestMethod] [DynamicData(nameof(DefaultRules))] - public void Rule_should_specify_guidance(DevSkimRule rule) + public void Rule_guidance_file_should_be_specified_and_exist(DevSkimRule rule) { if (rule.Disabled) { @@ -106,24 +138,8 @@ public void Rule_should_specify_guidance(DevSkimRule rule) { Assert.Fail("Rule does not specify guidance file."); } - } - [TestMethod] - [DynamicData(nameof(DefaultRules))] - public void Rule_guidance_file_should_exist(DevSkimRule rule) - { - if (rule.Disabled) - { - Assert.Inconclusive("Rule is disabled."); - } - - if (string.IsNullOrEmpty(rule.RuleInfo)) - { - Assert.Inconclusive("Rule does not specify guidance file."); - } - - string guidanceDir = GetGuidanceDirectory(); - string guidanceFile = Path.Combine(guidanceDir, rule.RuleInfo); + string guidanceFile = Path.Combine(_guidanceDirectory, rule.RuleInfo); Assert.IsTrue(File.Exists(guidanceFile), $"Guidance file {guidanceFile} does not exist."); } @@ -142,8 +158,7 @@ public void Rule_guidance_should_be_complete(DevSkimRule rule) Assert.Inconclusive("Rule does not specify guidance file."); } - string guidanceDir = GetGuidanceDirectory(); - string guidanceFile = Path.Combine(guidanceDir, rule.RuleInfo); + string guidanceFile = Path.Combine(_guidanceDirectory, rule.RuleInfo); if(!File.Exists(guidanceFile)) { Assert.Inconclusive("Guidance file does not exist"); @@ -156,32 +171,4 @@ public void Rule_guidance_should_be_complete(DevSkimRule rule) Assert.Fail($"Guidance file {guidanceFile} contains TODO."); } } - - private static string GetGuidanceDirectory() - { - string directory = Directory.GetCurrentDirectory(); - - /* Given a directory, like: "C:\src\DevSkim\DevSkim-DotNet\Microsoft.DevSkim.Tests\bin\Debug\net8.0" - * we want to find: "C:\src\DevSkim\guidance" - */ - - var currentDirInfo = new DirectoryInfo(directory); - while (currentDirInfo != null && currentDirInfo.Name != "DevSkim") - { - currentDirInfo = currentDirInfo.Parent; - } - - if (currentDirInfo == null) - { - throw new Exception("Could not find DevSkim-DotNet directory."); - } - - string guidanceDir = Path.Combine(currentDirInfo.FullName, "guidance"); - if (!Directory.Exists(guidanceDir)) - { - throw new Exception($"Guidance directory {guidanceDir} does not exist."); - } - - return guidanceDir; - } } \ No newline at end of file From 9a698cde18993a344f47ff9f78b6eae81d2cbdf1 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Thu, 23 May 2024 13:18:47 -0400 Subject: [PATCH 18/18] Fix finding guidance for DevSkim CLI in CI --- .../Microsoft.DevSkim.Tests/DefaultRulesTests.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs index b11c3c9f..057d431e 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs @@ -10,19 +10,24 @@ 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" + /* 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") + while (currentDirInfo != null && currentDirInfo.Name != "DevSkim-DotNet") { currentDirInfo = currentDirInfo.Parent; } + currentDirInfo = currentDirInfo?.Parent; + if (currentDirInfo == null) { - string message = $"Could not find DevSkim directory from: {directory}."; + string message = $"Could not find DevSkim-DotNet directory from: {directory}."; throw new Exception(message); }