Add AvoidUsingBrokenHashAlgorithms (#1787)

* Add AvoidUsingBrokenHashAlgorithms rule * Add AvoidUsingBrokenHashAlgorithms strings * Add AvoidUsingBrokenhashAlgorithms test suite * Fix tests and improve style * Add AvoidUsingBrokenHashAlgorithms to docs * Fix severity and unit tests Co-authored-by: Christoph Bergmeister [MVP] <[email protected]> * Fix error severity count in unit test * Adding frontmatter * Update Tests/Engine/GetScriptAnalyzerRule.tests.ps1 Co-authored-by: Christoph Bergmeister [MVP] <[email protected]> Co-authored-by: Sean Wheeler <[email protected]>
PowerShell · Aug 11, 2022 · a154270 · a154270
1 parent d7a9fa8
commit a154270
Show file tree

Hide file tree

Showing 7 changed files with 333 additions and 1 deletion.
diff --git a/Rules/AvoidUsingBrokenHashAlgorithms.cs b/Rules/AvoidUsingBrokenHashAlgorithms.cs
@@ -0,0 +1,186 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Management.Automation.Language;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+#if !CORECLR
+using System.ComponentModel.Composition;
+#endif
+using System.Globalization;
+using System.Linq;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// AvoidUsingBrokenHashAlgorithms: Avoid using the broken algorithms MD5 or SHA-1.
+    /// </summary>
+#if !CORECLR
+[Export(typeof(IScriptRule))]
+#endif
+    public class AvoidUsingBrokenHashAlgorithms : AvoidParameterGeneric
+    {
+        private readonly string[] brokenAlgorithms = new string[]
+        {
+            "MD5",
+            "SHA1",
+        };
+
+        private string algorithm;
+
+        /// <summary>
+        /// Condition on the cmdlet that must be satisfied for the error to be raised
+        /// </summary>
+        /// <param name="CmdAst"></param>
+        /// <returns></returns>
+        public override bool CommandCondition(CommandAst CmdAst)
+        {
+            return true;
+        }
+
+        /// <summary>
+        /// Condition on the parameter that must be satisfied for the error to be raised.
+        /// </summary>
+        /// <param name="CmdAst"></param>
+        /// <param name="CeAst"></param>
+        /// <returns></returns>
+        public override bool ParameterCondition(CommandAst CmdAst, CommandElementAst CeAst)
+        {
+            if (CeAst is CommandParameterAst)
+            {
+                CommandParameterAst cmdParamAst = CeAst as CommandParameterAst;
+
+                if (String.Equals(cmdParamAst.ParameterName, "algorithm", StringComparison.OrdinalIgnoreCase))
+                {
+                    Ast hashAlgorithmArgument = cmdParamAst.Argument;
+                    if (hashAlgorithmArgument is null)
+                    {
+                        hashAlgorithmArgument = GetHashAlgorithmArg(CmdAst, cmdParamAst.Extent.StartOffset);
+                        if (hashAlgorithmArgument is null)
+                        {
+                            return false;
+                        }
+                    }
+
+                    var constExprAst = hashAlgorithmArgument as ConstantExpressionAst;
+                    if (constExprAst != null)
+                    {
+                        algorithm = constExprAst.Value as string;
+                        return IsBrokenAlgorithm(algorithm);
+                    }
+                }
+            }
+
+            return false;
+        }
+
+        private bool IsBrokenAlgorithm(string algorithm)
+        {
+            if (algorithm != null)
+            {
+                return brokenAlgorithms.Contains<string>(
+                    algorithm,
+                    StringComparer.OrdinalIgnoreCase);
+            }
+
+            return false;
+        }
+
+        private Ast GetHashAlgorithmArg(CommandAst CmdAst, int StartIndex)
+        {
+            int small = int.MaxValue;
+            Ast hashAlgorithmArg = null;
+            foreach (Ast ast in CmdAst.CommandElements)
+            {
+                if (ast.Extent.StartOffset > StartIndex && ast.Extent.StartOffset < small)
+                {
+                    hashAlgorithmArg = ast;
+                    small = ast.Extent.StartOffset;
+                }
+            }
+
+            return hashAlgorithmArg;
+        }
+
+        /// <summary>
+        /// Retrieves the error message
+        /// </summary>
+        /// <param name="FileName"></param>
+        /// <param name="CmdAst"></param>
+        /// <returns></returns>
+        public override string GetError(string FileName, CommandAst CmdAst)
+        {
+            if (CmdAst is null)
+            {
+                return string.Empty;
+            }
+
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingBrokenHashAlgorithmsError, CmdAst.GetCommandName(), algorithm);
+        }
+
+        /// <summary>
+        /// GetName: Retrieves the name of this rule.
+        /// </summary>
+        /// <returns>The name of this rule</returns>
+        public override string GetName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidUsingBrokenHashAlgorithmsName);
+        }
+
+        /// <summary>
+        /// GetCommonName: Retrieves the common name of this rule.
+        /// </summary>
+        /// <returns>The common name of this rule</returns>
+        public override string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingBrokenHashAlgorithmsCommonName);
+        }
+
+        /// <summary>
+        /// GetDescription: Retrieves the description of this rule.
+        /// </summary>
+        /// <returns>The description of this rule</returns>
+        public override string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingBrokenHashAlgorithmsDescription);
+        }
+
+        /// <summary>
+        /// GetSourceType: Retrieves the type of the rule: builtin, managed or module.
+        /// </summary>
+        public override SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+
+        /// <summary>
+        /// GetSeverity: Retrieves the severity of the rule: error, warning or information.
+        /// </summary>
+        /// <returns></returns>
+        public override RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Warning;
+        }
+
+        /// <summary>
+        /// DiagnosticSeverity: Retrieves the severity of the rule of type DiagnosticSeverity: error, warning or information.
+        /// </summary>
+        /// <returns></returns>
+        public override DiagnosticSeverity GetDiagnosticSeverity()
+        {
+            return DiagnosticSeverity.Warning;
+        }
+
+        /// <summary>
+        /// GetSourceName: Retrieves the module/assembly name the rule is from.
+        /// </summary>
+        public override string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+    }
+}
+
+
+
+
diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs
diff --git a/Rules/Strings.resx b/Rules/Strings.resx
@@ -1176,4 +1176,16 @@
   <data name="AvoidMultipleTypeAttributesName" xml:space="preserve">
     <value>AvoidMultipleTypeAttributes</value>
   </data>
+  <data name="AvoidUsingBrokenHashAlgorithmsCommonName" xml:space="preserve">
+    <value>Avoid Using Broken Hash Algorithms</value>
+  </data>
+  <data name="AvoidUsingBrokenHashAlgorithmsDescription" xml:space="preserve">
+    <value>Avoid using the broken algorithms MD5 or SHA-1.</value>
+  </data>
+  <data name="AvoidUsingBrokenHashAlgorithmsError" xml:space="preserve">
+    <value>The Algorithm parameter of cmdlet '{0}' was used with the broken algorithm '{1}'.</value>
+  </data>
+  <data name="AvoidUsingBrokenHashAlgorithmsName" xml:space="preserve">
+    <value>AvoidUsingBrokenHashAlgorithms</value>
+  </data>
 </root>
diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1
@@ -63,7 +63,7 @@ Describe "Test Name parameters" {
 
         It "get Rules with no parameters supplied" {
             $defaultRules = Get-ScriptAnalyzerRule
-            $expectedNumRules = 67
+            $expectedNumRules = 68
             if ($PSVersionTable.PSVersion.Major -le 4)
             {
                 # for PSv3 PSAvoidGlobalAliases is not shipped because

diff --git a/Tests/Rules/AvoidUsingBrokenHashAlgorithms.tests.ps1 b/Tests/Rules/AvoidUsingBrokenHashAlgorithms.tests.ps1
@@ -0,0 +1,51 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+BeforeAll {
+    $settings = @{
+        IncludeRules = @('PSAvoidUsingBrokenHashAlgorithms')
+        Rules        = @{
+            PSAvoidUsingBrokenHashAlgorithms = @{
+                Enable = $true
+            }
+        }
+    }
+
+    $violationMessageMD5 = [regex]::Escape("The Algorithm parameter of cmdlet 'Get-FileHash' was used with the broken algorithm 'MD5'.")
+    $violationMessageSHA1 = [regex]::Escape("The Algorithm parameter of cmdlet 'Get-FileHash' was used with the broken algorithm 'SHA1'.")
+}
+
+Describe "AvoidUsingBrokenHashAlgorithms" {
+    Context "When there are violations" {
+        It "detects broken hash algorithms violations" {
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo -Algorithm MD5' -Settings $settings).Count | Should -Be 1
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo -Algorithm SHA1' -Settings $settings).Count | Should -Be 1
+        }
+
+        It "has the correct description message for MD5" {
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo -Algorithm MD5' -Settings $settings).Message | Should -Match $violationMessageMD5
+        }
+
+        It "has the correct description message for SHA-1" {
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo -Algorithm SHA1' -Settings $settings).Message | Should -Match $violationMessageSHA1
+        }
+
+        It "detects arbitrary cmdlets" {
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Fake-Cmdlet foo -Algorithm MD5' -Settings $settings).Count | Should -Be 1
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Fake-Cmdlet foo -Algorithm SHA1' -Settings $settings).Count | Should -Be 1
+        }
+
+    }
+
+    Context "When there are no violations" {
+        It "does not flag default algorithm" {
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo' -Settings $settings).Count | Should -Be 0
+        }
+
+        It "does not flag safe algorithms" {
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo -Algorithm SHA256' -Settings $settings).Count | Should -Be 0
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo -Algorithm SHA384' -Settings $settings).Count | Should -Be 0
+            (Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo -Algorithm SHA512' -Settings $settings).Count | Should -Be 0
+        }
+    }
+}
diff --git a/docs/Rules/AvoidUsingBrokenHashAlgorithms.md b/docs/Rules/AvoidUsingBrokenHashAlgorithms.md
@@ -0,0 +1,46 @@
+---
+description: Cmdlet Verbs
+ms.custom: PSSA v1.21.0
+ms.date: 05/31/2022
+ms.topic: reference
+title: AvoidUsingBrokenHashAlgorithms
+---
+# AvoidUsingBrokenHashAlgorithms
+
+**Severity Level: Warning**
+
+## Description
+
+Avoid using the broken algorithms MD5 or SHA-1.
+
+## How
+
+Replace broken algorithms with secure alternatives. MD5 and SHA-1 should be replaced with SHA256, SHA384, SHA512, or other safer algorithms when possible, with MD5 and SHA-1 only being utilized by necessity for backwards compatibility.
+
+## Example 1
+
+### Wrong
+
+```powershell
+Get-FileHash foo.txt -Algorithm MD5
+```
+
+### Correct
+
+```powershell
+Get-FileHash foo.txt -Algorithm SHA256
+```
+
+## Example 2
+
+### Wrong
+
+```powershell
+Get-FileHash foo.txt -Algorithm SHA1
+```
+
+### Correct
+
+```powershell
+Get-FileHash foo.txt
+```
diff --git a/docs/Rules/README.md b/docs/Rules/README.md
@@ -26,6 +26,7 @@ The PSScriptAnalyzer contains the following rule definitions.
 | [AvoidSemicolonsAsLineTerminators](./AvoidSemicolonsAsLineTerminators.md)                         | Warning     |         No         |                 |
 | [AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md)                           | Warning     |        Yes         |                 |
 | [AvoidTrailingWhitespace](./AvoidTrailingWhitespace.md)                                           | Warning     |        Yes         |                 |
+| [AvoidUsingBrokenHashAlgorithms](./AvoidUsingBrokenHashAlgorithms.md)                             | Warning     |        Yes         |                 |
 | [AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md)                                           | Warning     |        Yes         | Yes<sup>2</sup> |
 | [AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md)                           | Error       |        Yes         |                 |
 | [AvoidUsingConvertToSecureStringWithPlainText](./AvoidUsingConvertToSecureStringWithPlainText.md) | Error       |        Yes         |                 |