Skip to content

Conversation

@silijon
Copy link
Contributor

@silijon silijon commented Nov 24, 2019

PR Summary

Creates a dotnet core native implementation of the CryptoUtils key exchange enabling cross-platform remote sessions to successfully serialize SecureString.

PR Context

Currently, after importing a remote session between a Windows and a Linux host (in either direction), any imported cmdlets parameterized with SecureString are broken-by-design. There is a lazily triggered operation that happens during a SecureString serialization that uses the windows native CAPI crypto functions (e.g. CryptGenKey) to do the following:

  1. Generate a public/private keypair.
  2. Export/import the public key.
  3. Generate a symmetric session key.
  4. Encrypt and exchange the session key.
  5. Encrypt the SecureString for transit.

The implementation of this protocol is conditioned on the UNIX compile flag and, for non-Windows builds, it simply throws a PSCryptoException. The result is that any and all cmdlets that use a SecureString parameter are broken for cross-platform importation/invocation.

This PR fixes this problem by doing the following:

  1. Extracts an interface from PSRSACryptoServiceProvider.

  2. Re-names the current PSRSACryptoServiceProvider to Win32PSRSACryptoServiceProvider.

  3. Implements CorePSRSACryptoServiceProvider with the same external semantics as Win32PSRSACryptoServiceProvider but uses the core-compatible AES and RSA classes to do key generation, importation, exportation, encrypt, and decrypt.

  4. Uses a small set of conversions to enable CorePSRSACryptoServiceProvider to return/digest the key blob formats to/from the documented CAPI formats so as to retain compatibility with native windows systems.

  5. Conditions the use of CorePSRSACryptoServiceProvider on the UNIX compile flag.

Notes

  • I looked for an issue specifically for this problem and, while there are a number of closed issues relating to more general SecureString problems (having to do with the DPAPI and other things) I didn't see one directly related to this. Would be happy to tag an issue in here if I just missed it.

  • This PR fixes the same issue raised with the PS team in discussion with the SkyKick team. Specifically @PaulHigin (via email) and @TylerLeonhardt (on his livestream).

PR Checklist

@ghost ghost assigned TravisEz13 Nov 24, 2019
@msftclas
Copy link

msftclas commented Nov 24, 2019

CLA assistant check
All CLA requirements met.

@TylerLeonhardt
Copy link
Member

This will likely fix the local debugging experience for Azure Functions on macOS and Linux which catastrophically fails when you store a SecureString to a variable which triggers the key exchange.

Im excited at the impact this change could have!

@PaulHigin
Copy link
Contributor

SecureString is not implemented with encryption on non-Windows platforms. So encryption with the symmetric key over the wire seems unneeded. It may be better to remove it entirely. Adding committee review tag.

@PaulHigin PaulHigin added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 25, 2019
@silijon
Copy link
Contributor Author

silijon commented Nov 25, 2019

Yeah, session encryption during serialization is entirely redundant if the transport is encrypted. Here I was trying to make the smallest change possible while maintaining compatibility with older version remote shells.

Another thing for consideration: I think the core AES/RSA classes fall back on the windows native libraries in their implementation? If so, you may consider just dropping Win32PSRSACryptoServiceProvider altogether.

@PaulHigin
Copy link
Contributor

Yeah, I am beginning to see your point. This may be the best way moving forward and keeping backward compatibility. There is still the deadlock issue when server initiates key exchange, but I think that can be fixed separately.

I would like to get rid of Win32PSRSACryptoServiceProvider. Even if dotNet core implementation does not fall back, is there a reason to keep it? Isn't the core version compatible ... it would need to be for full x-plat compatibility to older versions of PowerShell.

@silijon
Copy link
Contributor Author

silijon commented Nov 26, 2019

Personally, I think getting rid of Win32PSRSACryptoServiceProvider is the right move but I wasn't sure if you guys had a reason to leave the native windows crypto stuff in. So I tried to do as light a touch as possible.

I'd be happy to refactor it out. It'll allow a lot of code and compiler conditions to be removed from CryptoUtils and simplify the whole thing. I think it's a good trade-off to ensure backward compat but clean some of this stuff up.

@silijon silijon force-pushed the allow-keyexchange-for-linux-remote-session-v7 branch from a311102 to 7d3b364 Compare November 29, 2019 21:35
@silijon
Copy link
Contributor Author

silijon commented Nov 29, 2019

I updated this PR to completely remove the native windows dll references and change the entire key exchange over to using dotnet core compatible aes and rsa classes. I also consolidated the CAPI conversion code into CryptoUtils.cs and used the existing pattern of byte constant naming to be more explicit about the structure of the keyblob format conversions. Let me know what you think.

@PaulHigin
Copy link
Contributor

Thanks @silijon, I'll take a look this week.

@TravisEz13 TravisEz13 added this to the rc.1-consider milestone Dec 3, 2019
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me. I have tested this on Windows platform and SecureString objects are passed correctly between PowerShell versions. However, key exchange is not currently working on non-Windows platforms, but I don't think it is due to these changes and is just not supported since it was not needed before. I'll continue to look into it.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 4, 2019
@SteveL-MSFT SteveL-MSFT removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 4, 2019
@SteveL-MSFT
Copy link
Member

Removing Review - Committee, after discussing with @PaulHigin this seems like a good change to have, just need security sign-off

@PaulHigin
Copy link
Contributor

Key exchange was broken because of remaining #ifdefs in code for non-Windows platform. Just need to change OutOfProcServerMediator.cs and serverremotesession.cs files to get non-Windows support.
We can do that in this PR or a separate one.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 4, 2019
@PaulHigin
Copy link
Contributor

@silijon I have created a PR that addresses my CR comments above, and also enables key exchange for non-Windows platforms. If you can merge my PR I think we can accept this PR.

SkyKick#1

Thanks for your work on this!

@SteveL-MSFT
Copy link
Member

@PoshChan please retry linux

@PoshChan
Copy link
Collaborator

PoshChan commented Dec 9, 2019

@SteveL-MSFT, successfully started retry of PowerShell-CI-Linux

@SteveL-MSFT SteveL-MSFT removed this from the rc.1-consider milestone Dec 10, 2019
@SteveL-MSFT SteveL-MSFT added this to the 7.1-Consider milestone Dec 10, 2019
@silijon
Copy link
Contributor Author

silijon commented Dec 10, 2019

@PoshChan please retry windows

@PoshChan
Copy link
Collaborator

@silijon, you are not authorized to request a rebuild

@vexx32
Copy link
Collaborator

vexx32 commented Dec 11, 2019

@PoshChan please retry windows

@PoshChan
Copy link
Collaborator

@vexx32, successfully started retry of PowerShell-CI-Windows

@silijon silijon force-pushed the allow-keyexchange-for-linux-remote-session-v7 branch from e6d2fc2 to 68646a8 Compare December 18, 2019 06:34
Copy link

@mathewcathey mathewcathey left a comment

Choose a reason for hiding this comment

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

Hi

Copy link

@mathewcathey mathewcathey left a comment

Choose a reason for hiding this comment

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

Yes

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 7, 2020
Copy link

@mathewcathey mathewcathey left a comment

Choose a reason for hiding this comment

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

Yes

@PaulHigin
Copy link
Contributor

@TravisEz13 Where are we on this PR? Is there anything preventing the merge?

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

approving as a maintainer

@TravisEz13 TravisEz13 merged commit 895d4b3 into PowerShell:master Feb 19, 2020
@iSazonov iSazonov added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Feb 20, 2020
@iSazonov
Copy link
Collaborator

@silijon Thanks for your contribution!

@ghost
Copy link

ghost commented Mar 26, 2020

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.