GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add AvoidUsingBrokenHashAlgorithms by MJVL · Pull Request #1787 · PowerShell/PSScriptAnalyzer

Conversation

@MJVL

@MJVL Michael Van Leeuwen (MJVL) commented Apr 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

PR Summary

This adds a new rule: AvoidUsingBrokenHashAlgorithms.

This rule searches for use of SHA-1 or MD5 within -Algorithm parameters. This mainly serves to flag use with Get-FileHash, but also works for other cmdlets which may use the same parameter scheme. Should other algorithms in the future be deemed insecure, it would be trivial to add them.

At this point both of these algorithms are broken (Microsoft SDL has labeled them as such since 2009), so I think it would be worthwhile to flag these, such that new code doesn't use these algorithms except when needed for backwards compatability.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thomasrayner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Michael Van Leeuwen (MJVL) added 5 commits April 7, 2022 21:38
@MJVL
Add AvoidUsingBrokenHashAlgorithms rule
93a3ada
@MJVL
Add AvoidUsingBrokenHashAlgorithms strings
458d761
@MJVL
Add AvoidUsingBrokenhashAlgorithms test suite
d9fae07
@MJVL
Fix tests and improve style
157c162
@MJVL
Add AvoidUsingBrokenHashAlgorithms to docs
de7a0d0
@ghost

Deleted user (ghost) commented Apr 9, 2022 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 17, 2022
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution and very good idea. I'll be giving it a test run but from a high level looks good otherwise. I might suggest something minor around the parameter parsin

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@MJVL

Michael Van Leeuwen (MJVL) commented May 23, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the contribution and very good idea. I'll be giving it a test run but from a high level looks good otherwise. I might suggest something minor around the parameter parsin

Cool, thanks! I'll fix anything as needed when you get a chance to review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 30, 2022
View reviewed changes
Comment thread Rules/AvoidUsingBrokenHashAlgorithms.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 30, 2022
View reviewed changes
Comment thread Rules/AvoidUsingBrokenHashAlgorithms.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 30, 2022
View reviewed changes
Comment thread docs/Rules/AvoidUsingBrokenHashAlgorithms.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 30, 2022
View reviewed changes
Comment thread docs/Rules/README.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 30, 2022
View reviewed changes
Comment thread Tests/Engine/GetScriptAnalyzerRule.tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 30, 2022
View reviewed changes
Comment thread Tests/Engine/GetScriptAnalyzerRule.tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

bergmeister
Christoph Bergmeister (bergmeister) reviewed May 30, 2022
View reviewed changes
Comment thread Tests/Rules/AvoidUsingBrokenHashAlgorithms.tests.ps1
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

@bergmeister Christoph Bergmeister (bergmeister) May 30, 2022

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An optional improvement would be to make it also work when no named parameters are used: Invoke-ScriptAnalyzer -ScriptDefinition 'Get-FileHash foo SHA1'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@MJVL Michael Van Leeuwen (MJVL) May 30, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Can you see of any cases where this might false flag? Ie, someone opening a file that's named the same as a flagged hashing algorithm?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@bergmeister Christoph Bergmeister (bergmeister) May 31, 2022

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because what you would do is check whether the first and second item in the array are not named parameters, i.e. do not start with dash and therefore you know for sure that the first one has to be the file name and the second one the algorithm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
bergmeister
Christoph Bergmeister (bergmeister) approved these changes May 30, 2022
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I would just change severity from Warning. An optional suggestion on improving parsing but this could also just be a follow up PR as I'd rather want to wrap this up and ship it quickly in the next version of PSSA.
Docs question to Sean Wheeler (@sdwheeler) What do we do with the docs/Rules folder in this repo now? Do we have to replicate the same change here: https://github.com/MicrosoftDocs/PowerShell-Docs-Modules/tree/main/reference/docs-conceptual/PSScriptAnalyzer/Rules

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@MJVL @bergmeister
Fix severity and unit tests
fc09bd8
Co-authored-by: Christoph Bergmeister [MVP] <[email protected]>
@MJVL

Michael Van Leeuwen (MJVL) commented May 30, 2022

Copy link
Copy Markdown
Contributor Author

Looks good, I would just change severity from Warning. An optional suggestion on improving parsing but this could also just be a follow up PR as I'd rather want to wrap this up and ship it quickly in the next version of PSSA. Docs question to Sean Wheeler (@sdwheeler) What do we do with the docs/Rules folder in this repo now? Do we have to replicate the same change here: https://github.com/MicrosoftDocs/PowerShell-Docs-Modules/tree/main/reference/docs-conceptual/PSScriptAnalyzer/Rules

Thanks for the input! Fixed severities and unit tests based on your review comments.

I lack the bandwidth at the moment to improve the parsing, but it's something I could look into in the future. I think we should leave that as a follow-up PR like you said to get this to ship faster.

bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MJVL
Fix error severity count in unit test
1a35e45
@sdwheeler

Sean Wheeler (sdwheeler) commented May 31, 2022

Copy link
Copy Markdown
Contributor

Docs question to Sean Wheeler (@sdwheeler) What do we do with the docs/Rules folder in this repo now? Do we have to replicate the same change here: https://github.com/MicrosoftDocs/PowerShell-Docs-Modules/tree/main/reference/docs-conceptual/PSScriptAnalyzer/Rules

Yes. I just need to know when you are going to release and then I will copy the updated docs from the source repository to the docs repository. Eventually we want a CI job to do that automatically at release time. But for now, I can do it manually.

bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sean Wheeler (sdwheeler) and others added 2 commits May 31, 2022 08:29
@sdwheeler
Adding frontmatter
b79a48e
@bergmeister
Merge branch 'master' into hashRule
5f0f9a6
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jul 25, 2022
View reviewed changes
Comment thread Tests/Engine/GetScriptAnalyzerRule.tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

Christoph Bergmeister (bergmeister) added 2 commits July 25, 2022 15:17
@bergmeister
Update Tests/Engine/GetScriptAnalyzerRule.tests.ps1
f252ecd
@bergmeister
Merge branch 'master' into hashRule
62d9ae9
@JamesWTruher James Truher (JamesWTruher) added this to the 1.21 milestone Jul 25, 2022
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Aug 1, 2022
View reviewed changes
@bergmeister Christoph Bergmeister (bergmeister) mentioned this pull request Aug 11, 2022
Changelog for 1.21.0 #1828
Merged
5 of 6 tasks
JamesWTruher
James Truher (JamesWTruher) approved these changes Aug 11, 2022
View reviewed changes
@JamesWTruher
James Truher (JamesWTruher) merged commit a154270 into PowerShell:master Aug 11, 2022
@michaeltlombardi Mikey Lombardi (He/Him) (michaeltlombardi) mentioned this pull request Aug 11, 2022
Document PSAvoidUsingBrokenHashAlgorithms MicrosoftDocs/PowerShell-Docs-Modules#90
Closed
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergmeister Christoph Bergmeister (bergmeister) bergmeister approved these changes
+1 more reviewer
@JamesWTruher James Truher (JamesWTruher) JamesWTruher approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.21

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@MJVL @sdwheeler @bergmeister @JamesWTruher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.