PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.84k stars 372 forks source link

SuppressMessageAttribute is not recognized by PSA with "using namespace" import #1295

Open oising opened 5 years ago

oising commented 5 years ago

Before submitting a bug report:

Steps to reproduce

#requires -version 6.1

using namespace Diagnostics.CodeAnalysis;

[SuppressMessage("PSAvoidUsingPlainTextForPassword", "", Justification="Azure Pipelines will obscure the password")]
[CmdletBinding()]
param(
    [Parameter(Mandatory)]
    [ValidateNotNullOrEmpty()]
    [string]$Password, # will be obfuscated by Azure Pipelines

Expected behavior

PSA should not highlight $Password as an violation.

Actual behavior

PSA highlights parameter as being in violation. Using long form [Diagnostics.CodeAnalysis.SuppressMessageAttribute(...)] works as expected.

Environment data

> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      6.2.2
PSEdition                      Core
GitCommitId                    6.2.2
OS                             Microsoft Windows 10.0.17763 
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.18.0
bergmeister commented 5 years ago

Fair point, it would be nice to be able to shorten the syntax but the documentation examples show at least the usage of the full namespace (which works), so definitely more a 'nice to have feature' rather than a bug:

using namespace Diagnostics.CodeAnalysis;

[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingPlainTextForPassword", "", Justification="Azure Pipelines does not support SecureString")]
[CmdletBinding()]
param(
    [string] $Password
)

The code that looks up the attribute is here, not sure what would be needed to make it see the using statement: https://github.com/PowerShell/PSScriptAnalyzer/blob/b1a81874fbe61ebaf1a0b9631f337b4f06296625/Engine/Generic/RuleSuppression.cs#L311

P.S. Azure Pipelines does not obscure the password, even when you use a 'secret' variable. All it does at the end is a simple string replacement of the value with stars in the log but you actually have the password in plaintext in memory. You could use this to send the password to yourself for example or just simply dump it out by inserting a space character before the final character if you want to retrieve the 'secret': "$($password.Substring(0, $password.Length-1)) $($password[$password.Length-1])" Azure Pipeline's security model is that if someone has access to source control or the build pipeline then one is implicitly an admin. I've discussed this weak approach with the team in the past, see https://github.com/microsoft/azure-pipelines-tasks/issues/4284#issuecomment-300354042

oising commented 5 years ago

Correct, it's not encrypted in memory. However, it would be naive to place your imaginary markers for a security boundary there.

I just don't want it willy-nilly emitted in logs. I'm fully aware that in an optimistic world, it would be protected somehow, but this idea - and your linked issue - make too many assumptions about the environment and real world characteristics about SecureStrings. Even if it was injected as a SecureString, if someone can inject if (!$secretKey), someone can equally inject code to decrypt it (SecureStringToBSTR). And finally, just because it's in SecureString format, that means it's encrypted for some of the time. Once the .NET process needs to actually use the value, it will be decrypted... in memory.

Anyway, yeah, I took a look at RuleSupression earllier, and it seems someone would have to add some plumbing to pull out the UsingNamespaceAST info and try to resolve shortened typenames against the list.

oising commented 5 years ago

I would also argue it is a bug, since semantically it is equivalent to the namespace qualified version. If this was a pragma in a comment, it would be open to debate; but since it's a de-facto strongly-typed attribute, then all valid expressions of it should be understood.