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 — Change signature validation logic to call first without the file content by JamesWTruher · Pull Request #14849 · PowerShell/PowerShell

Conversation

@JamesWTruher

@JamesWTruher James Truher (JamesWTruher) commented Feb 19, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator

Only validate the signature with the content if the first non-content validation fails

PR Summary

When ExecutionPolicy is set to require a signature, under some circumstances a signature placed in a file will not execute with a "hash mismatch" error, even though Get-AuthenticodeSignature will return that the signature is valid.
This is because Get-AuthenticodeSignature and our invocation path are not symmetrical. This PR adds the same check that we do in Get-AuthenticodeSignature into our invocation path.

PR Context

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • Breaking changes
    • None
    • OR
    • Experimental feature(s) needed
      • Experimental feature name(s):
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
  • Tooling
    • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
    • OR
    • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
      • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
        (which runs in a different PS Host).
        • Issue filed:
      • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
        • Issue filed:
      • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
        • Issue filed:
      • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
        • Issue filed:

Sorry, something went wrong.

Uh oh!

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

All reactions
@ghost ghost assigned Aditya Patwardhan (adityapatwardhan) Feb 19, 2021
@JamesWTruher
James Truher (JamesWTruher) requested review from Steve Lee (SteveL-MSFT), Travis Plunk (TravisEz13) and Rob Holt (rjmholt) February 19, 2021 01:52
@JamesWTruher James Truher (JamesWTruher) added the WG-Security security related areas such as JEA label Feb 19, 2021
iSazonov
Ilya (iSazonov) reviewed Feb 19, 2021
View reviewed changes
Comment thread src/System.Management.Automation/security/SecurityManager.cs Resolved

Uh oh!

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

rjmholt
Rob Holt (rjmholt) reviewed Feb 19, 2021
View reviewed changes
Comment thread src/System.Management.Automation/security/SecurityManager.cs Resolved

Uh oh!

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

@rjmholt
Rob Holt (rjmholt) requested a review from Paul Higinbotham (PaulHigin) February 19, 2021 18:06
TravisEz13
Travis Plunk (TravisEz13) approved these changes Feb 19, 2021
View reviewed changes

@TravisEz13 Travis Plunk (TravisEz13) left a comment

Copy link
Copy Markdown
Member

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.

I believe this is correct.
GetSignature should be called without contents first, because:

  1. The underlying validation should never return a false valid, so the call is safe.
  2. we cannot validate windows signatures, when called WITH contents. So, we should try without contents first.
  3. manipulating the contents makes it more likely to get a false invalid.

Sorry, something went wrong.

Uh oh!

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

All reactions
James Truher (JamesWTruher) and others added 4 commits February 19, 2021 13:29
@JamesWTruher
Change signature validation logic to call first without the file cont…
008e2e9
…ent.

Only validate the signature with the content if the first non-content validation fails
@JamesWTruher
remove extraneous extra line
f9bb569
@TravisEz13 @iSazonov @JamesWTruher
Update src/System.Management.Automation/security/SecurityManager.cs
c43b472
Co-authored-by: Ilya <[email protected]>
@JamesWTruher
use system.text namespace
72e4cd1
@JamesWTruher
James Truher (JamesWTruher) force-pushed the signfix001 branch from 45ba910 to 72e4cd1 Compare February 19, 2021 21:38
TravisEz13
Travis Plunk (TravisEz13) approved these changes Feb 20, 2021
View reviewed changes
rjmholt
Rob Holt (rjmholt) approved these changes Feb 20, 2021
View reviewed changes
Comment thread src/System.Management.Automation/security/SecurityManager.cs Resolved

Uh oh!

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

@JamesWTruher @rjmholt
Update src/System.Management.Automation/security/SecurityManager.cs
113064a
Co-authored-by: Robert Holt <[email protected]>
TravisEz13
Travis Plunk (TravisEz13) approved these changes Feb 22, 2021
View reviewed changes
@TravisEz13 Travis Plunk (TravisEz13) added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Feb 22, 2021
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Feb 22, 2021
View reviewed changes
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Feb 22, 2021

Copy link
Copy Markdown
Member

James Truher (@JamesWTruher) Is there a way we can add a test for the change?

All reactions

Sorry, something went wrong.

Uh oh!

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

PaulHigin
Paul Higinbotham (PaulHigin) suggested changes Feb 22, 2021
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

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.

Does this change need to be backported to Windows PowerShell?

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread src/System.Management.Automation/security/SecurityManager.cs
{
string verificationContents = System.Text.Encoding.Unicode.GetString(script.OriginalEncoding.GetPreamble()) + script.ScriptContents;
Signature signature = SignatureHelper.GetSignature(path, verificationContents);
// Invoke the SIP directly with the most simple method

@PaulHigin Paul Higinbotham (PaulHigin) Feb 22, 2021

Copy link
Copy Markdown
Contributor

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.

This comment is confusing to me, what is the simple method? I think it should say something like 'first try to get a catalog signature, by passing in null content'.

Sorry, something went wrong.

Uh oh!

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

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

@JamesWTruher James Truher (JamesWTruher) Feb 22, 2021

Copy link
Copy Markdown
Collaborator 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.

My understanding is that the missing catalog look up is only part of the problem, and that we should be letting the SIP try to validate with out our "help". Travis Plunk (@TravisEz13) can perhaps address additional points.

Sorry, something went wrong.

Uh oh!

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

All reactions

@TravisEz13 Travis Plunk (TravisEz13) Feb 22, 2021

Copy link
Copy Markdown
Member

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.

I think getting the catalog is only half of it, like James is saying. If we fail to get a catalog signature, we try to get the embedded signature without modifying the content. This should be what we try first. It is the only thing we do in Get-AuthenticodeSignature.

Sorry, something went wrong.

Uh oh!

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

All reactions
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 22, 2021
@JamesWTruher

James Truher (JamesWTruher) commented Feb 22, 2021

Copy link
Copy Markdown
Collaborator Author

Aditya Patwardhan (@adityapatwardhan) I couldn't think of one other than the tests we have, and I'm not sure why the tests we have didn't find this. It may be that Paul Higinbotham (@PaulHigin) can figure out a way

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 22, 2021
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Feb 22, 2021

Copy link
Copy Markdown
Member

Paul Higinbotham (@PaulHigin) we initially checked against WinPS and for whatever reason it doesn't fail in WinPS. I compared the code between WinPS and PS7 and don't see any differences in this specific code path so it remains a mystery why it only fails on PS7.

All reactions

Sorry, something went wrong.

Uh oh!

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

@PaulHigin

Paul Higinbotham (PaulHigin) commented Feb 22, 2021

Copy link
Copy Markdown
Contributor

Steve Lee (@SteveL-MSFT) Hmm, I don't like those kinds of mysteries. Could it be a PInvoke difference between .Net frameworks? But this seems like the right fix since it matches what Get-AuthenticodeSignature does.

All reactions

Sorry, something went wrong.

Uh oh!

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

@rjmholt

Rob Holt (rjmholt) commented Feb 22, 2021

Copy link
Copy Markdown
Collaborator

I think the difference lies here:

PowerShell/src/System.Management.Automation/engine/ExternalScriptInfo.cs

Line 518 in 3c7f4b5

Encoding defaultEncoding = ClrFacade.GetDefaultEncoding();

All reactions

Sorry, something went wrong.

Uh oh!

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

@iSazonov

Ilya (iSazonov) commented Feb 22, 2021

Copy link
Copy Markdown
Collaborator

I wonder that Get-AuthenticodeSignature -Content assumes the Content is always in Unicode.

I think the difference lies here:

PowerShell/src/System.Management.Automation/engine/ExternalScriptInfo.cs

Line 518 in 3c7f4b5

Encoding defaultEncoding = ClrFacade.GetDefaultEncoding();

It could be so if a test script file is in Unicode but without BOM otherwise StreamReader() detected right an encoding of the file.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iSazonov

Ilya (iSazonov) commented Feb 22, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator

Although it may be just the same

[System.Text.Encoding] :: Utf8.GetPreamble ()
239
187
191

I mean, now we get the default BOM for Uft8 when it shouldn't be (in GetSignatureWithEncodingRetry()).

All reactions

Sorry, something went wrong.

Uh oh!

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

@PaulHigin

Paul Higinbotham (PaulHigin) commented Feb 22, 2021

Copy link
Copy Markdown
Contributor

I see. This does seem like it could be the culprit. But the comment in GetSignatureWithEncodingRetry says the SIP will (may?) replace with a Unicode BOM when signing, in which case the code should handle it. We should investigate this more.

All reactions

Sorry, something went wrong.

Uh oh!

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

@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Feb 22, 2021

Copy link
Copy Markdown
Member

I am marking this PR as WIP as more discussion is required here.

rjmholt 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.

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) changed the title Change signature validation logic to call first without the file content WIP: Change signature validation logic to call first without the file content Feb 22, 2021
PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Feb 23, 2021
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

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.

This change looks good to me. I feel we can look into the SIP issue separately.

Sorry, something went wrong.

Uh oh!

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

TravisEz13 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) changed the title WIP: Change signature validation logic to call first without the file content Change signature validation logic to call first without the file content Feb 23, 2021
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit 907746c into PowerShell:master Feb 23, 2021
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) added this to the 7.2.0-preview.4 milestone Feb 23, 2021
@SteveL-MSFT Steve Lee (SteveL-MSFT) mentioned this pull request Mar 9, 2021
Running Authenticode-signed script is seen as changed when file is UTF8 (w/out BOM) and contains a unicode character #3466
Closed
@ghost

Deleted user (ghost) commented Mar 16, 2021

Copy link
Copy Markdown

🎉v7.2.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

  • Release Notes
All reactions

Sorry, something went wrong.

Uh oh!

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

@JamesWTruher
James Truher (JamesWTruher) deleted the signfix001 branch September 23, 2023 05:34
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@JamesWTruher
Change signature validation logic to call first without the file cont…
f4ccc09
…ent (PowerShell#14849)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@iSazonov Ilya (iSazonov) iSazonov left review comments
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
+2 more reviewers
@rjmholt Rob Holt (rjmholt) rjmholt approved these changes
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log WG-Security security related areas such as JEA

Projects

None yet

Milestone

7.2.0-preview.4

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

7 participants

@JamesWTruher @adityapatwardhan @SteveL-MSFT @PaulHigin @rjmholt @iSazonov @TravisEz13
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.