diff --git a/Rules/AvoidUsingBrokenHashAlgorithms.cs b/Rules/AvoidUsingBrokenHashAlgorithms.cs new file mode 100644 index 000000000..b43116020 --- /dev/null +++ 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 +{ + /// + /// AvoidUsingBrokenHashAlgorithms: Avoid using the broken algorithms MD5 or SHA-1. + /// +#if !CORECLR +[Export(typeof(IScriptRule))] +#endif + public class AvoidUsingBrokenHashAlgorithms : AvoidParameterGeneric + { + private readonly string[] brokenAlgorithms = new string[] + { + "MD5", + "SHA1", + }; + + private string algorithm; + + /// + /// Condition on the cmdlet that must be satisfied for the error to be raised + /// + /// + /// + public override bool CommandCondition(CommandAst CmdAst) + { + return true; + } + + /// + /// Condition on the parameter that must be satisfied for the error to be raised. + /// + /// + /// + /// + 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( + 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; + } + + /// + /// Retrieves the error message + /// + /// + /// + /// + 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); + } + + /// + /// GetName: Retrieves the name of this rule. + /// + /// The name of this rule + public override string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidUsingBrokenHashAlgorithmsName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public override string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingBrokenHashAlgorithmsCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public override string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingBrokenHashAlgorithmsDescription); + } + + /// + /// GetSourceType: Retrieves the type of the rule: builtin, managed or module. + /// + public override SourceType GetSourceType() + { + return SourceType.Builtin; + } + + /// + /// GetSeverity: Retrieves the severity of the rule: error, warning or information. + /// + /// + public override RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// DiagnosticSeverity: Retrieves the severity of the rule of type DiagnosticSeverity: error, warning or information. + /// + /// + public override DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Warning; + } + + /// + /// GetSourceName: Retrieves the module/assembly name the rule is from. + /// + public override string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } +} + + + + diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 858b34f28..b94192697 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -762,6 +762,42 @@ internal static string AvoidUsernameAndPasswordParamsName { } } + /// + /// Looks up a localized string similar to Avoid Using Broken Hash Algorithms. + /// + internal static string AvoidUsingBrokenHashAlgorithmsCommonName { + get { + return ResourceManager.GetString("AvoidUsingBrokenHashAlgorithmsCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Avoid using the broken algorithms MD5 or SHA-1.. + /// + internal static string AvoidUsingBrokenHashAlgorithmsDescription { + get { + return ResourceManager.GetString("AvoidUsingBrokenHashAlgorithmsDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The Algorithm parameter of cmdlet '{0}' was used with the broken algorithm '{1}'.. + /// + internal static string AvoidUsingBrokenHashAlgorithmsError { + get { + return ResourceManager.GetString("AvoidUsingBrokenHashAlgorithmsError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidUsingBrokenHashAlgorithms. + /// + internal static string AvoidUsingBrokenHashAlgorithmsName { + get { + return ResourceManager.GetString("AvoidUsingBrokenHashAlgorithmsName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using Clear-Host. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 53e78c69e..68eeeebe2 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1176,4 +1176,16 @@ AvoidMultipleTypeAttributes + + Avoid Using Broken Hash Algorithms + + + Avoid using the broken algorithms MD5 or SHA-1. + + + The Algorithm parameter of cmdlet '{0}' was used with the broken algorithm '{1}'. + + + AvoidUsingBrokenHashAlgorithms + \ No newline at end of file diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index de9e6e326..0ca6e569f 100644 --- 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 new file mode 100644 index 000000000..d6f0059c7 --- /dev/null +++ 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 + } + } +} \ No newline at end of file diff --git a/docs/Rules/AvoidUsingBrokenHashAlgorithms.md b/docs/Rules/AvoidUsingBrokenHashAlgorithms.md new file mode 100644 index 000000000..5f0406934 --- /dev/null +++ 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 index 3a838f3f9..aaac74f66 100644 --- 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 | Yes2 | | [AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md) | Error | Yes | | | [AvoidUsingConvertToSecureStringWithPlainText](./AvoidUsingConvertToSecureStringWithPlainText.md) | Error | Yes | |