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 — Don't use Win32 native APIs on non-Windows for crypto of secure string over remoting by SteveL-MSFT · Pull Request #8746 · PowerShell/PowerShell

Conversation

@SteveL-MSFT

@SteveL-MSFT Steve Lee (SteveL-MSFT) commented Jan 25, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

PR Summary

Remoting relies on Windows native crypto APIs to pass a secure string. Of course these APIs don't work on non-Windows. However, the remoting code does not expect this to fail so ends up in a hang waiting for an event that never happens.

Have not figured out how to return an error to the user in the case Get-Credential is called within a PSSession. Currently it just quietly closes the remote connection. Paul Higinbotham (@PaulHigin) can you give a pointer to where this code would exist? Debugging the current code, it has a bunch of error handlers which just result in the pssession being closed and not sure where throwing is appropriate.

Fix #8723

PR Context

Fix is to provide implementations of the Win32 native pinvoke apis that throw an exception. The reason to do it this way is that there's a bunch of code that calls some of the APIs of the wrapper .Net class to setup structures that eventually get passed to the win32 api. Throwing in the managed class causes other things to fail early that should work on non-Windows.

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 .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.
  • 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
      • Add [feature] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

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

All reactions
@SteveL-MSFT
[feature] don't use Win32 native APIs on non-Windows for crypto
411c39a
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Travis Plunk (TravisEz13) Jan 25, 2019
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Paul Higinbotham (PaulHigin) January 25, 2019 06:40
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested review from Bruce Payette (BrucePay), Dongbo Wang (daxian-dbw) and Mike Richmond (mirichmo) as code owners January 25, 2019 06:40
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jan 25, 2019

Copy link
Copy Markdown
Member Author

The CodeFactor issues are all on existing code complaining about Hungarian notation which none of them are

All reactions

Sorry, something went wrong.

Uh oh!

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

PaulHigin
Paul Higinbotham (PaulHigin) suggested changes Jan 25, 2019
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 seems Ok as a temporary change to prevent a hang, although the result for a client initiated key exchange is the termination of the remoting connection. Only slightly better. For a server initiated exchange, a Windows DPAPI dll exception is generated and returned to the user, but the connection remains. We can easily change this (in serialization->HandleSecureString) to throw a better exception, but I see that the original exception gets lost and the user gets an obscure "Root element is missing" error, probably because the remoting packet XML is malformed (serializer is not supposed to throw but simply return nothing on errors). But this may be Ok until we get this working correctly.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread src/System.Management.Automation/utils/CryptoUtils.cs Resolved

Uh oh!

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

Comment thread src/System.Management.Automation/engine/serialization.cs Resolved

Uh oh!

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

@SteveL-MSFT
[feature] address Paul's comments
1d4260e
@SteveL-MSFT Steve Lee (SteveL-MSFT) changed the title WIP: Don't use Win32 native APIs on non-Windows for crypto of secure string over remoting Don't use Win32 native APIs on non-Windows for crypto of secure string over remoting Feb 1, 2019
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Feb 6, 2019

Copy link
Copy Markdown
Member Author

Paul Higinbotham (@PaulHigin) can you re-review?

All reactions

Sorry, something went wrong.

Uh oh!

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

PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Feb 7, 2019
View reviewed changes
TravisEz13
Travis Plunk (TravisEz13) approved these changes Feb 7, 2019
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 7, 2019
@TravisEz13
Travis Plunk (TravisEz13) merged commit 5ece96a into PowerShell:master Feb 7, 2019
@SteveL-MSFT
Steve Lee (SteveL-MSFT) deleted the remote-secure-string branch February 8, 2019 06:03
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@SteveL-MSFT @TravisEz13
Don't use Win32 native APIs on non-Windows for crypto of secure strin…
275134d
…g over remoting (PowerShell#8746)

Remoting relies on Windows native crypto APIs to pass a secure string.  Of course these APIs don't work on non-Windows.  However, the remoting code does not expect this to fail so ends up in a hang waiting for an event that never happens.

Have not figured out how to return an error to the user in the case `Get-Credential` is called within a PSSession.  Currently it just quietly closes the remote connection.  @PaulHigin can you give a pointer to where this code would exist?  Debugging the current code, it has a bunch of error handlers which just result in the pssession being closed and not sure where throwing is appropriate.

Fix PowerShell#8723

## PR Context  

Fix is to provide implementations of the Win32 native pinvoke apis that throw an exception.  The reason to do it this way is that there's a bunch of code that calls some of the APIs of the wrapper .Net class to setup structures that eventually get passed to the win32 api.  Throwing in the managed class causes other things to fail early that should work on non-Windows.
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
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw daxian-dbw is a code owner
@mirichmo Mike Richmond (mirichmo) Awaiting requested review from mirichmo
+1 more reviewer
@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

@TravisEz13 Travis Plunk (TravisEz13)

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SSH remoting Credential objects from OSX can hard-lock Powershell

Uh oh!

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

3 participants

@SteveL-MSFT @TravisEz13 @PaulHigin
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.