Skip to content

Allow cross-platform CAPI-compatible remote key exchange - #11185

Merged
Travis Plunk (TravisEz13) merged 7 commits into
PowerShell:masterfrom
SkyKick:allow-keyexchange-for-linux-remote-session-v7
Feb 19, 2020
Merged

Travis Plunk (TravisEz13) merged 7 commits into
PowerShell:masterfrom
SkyKick:allow-keyexchange-for-linux-remote-session-v7

Conversation

@silijon

Copy link
Copy Markdown
Contributor

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 Paul Higinbotham (@PaulHigin) (via email) and TylerLeonhardt (on his livestream).

PR Checklist

@msftclas

Microsoft Contribution License Agreements (msftclas) commented Nov 24, 2019 •

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@TylerLeonhardt

Copy link
Copy Markdown
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
Copy Markdown
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 Paul Higinbotham (PaulHigin) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 25, 2019
@silijon

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
Contributor Author

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
John Dennis (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
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

Thanks John Dennis (@silijon), I'll take a look this week.

Copy link
Copy Markdown
Contributor

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 Steve Lee (SteveL-MSFT) removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 4, 2019
@SteveL-MSFT

Copy link
Copy Markdown
Member

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

@PaulHigin

Copy link
Copy Markdown
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
Copy Markdown
Contributor

John Dennis (@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
Copy Markdown
Member

PoshChan-Bot (@PoshChan) please retry linux

@PoshChan

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT), successfully started retry of PowerShell-CI-Linux

@SteveL-MSFT Steve Lee (SteveL-MSFT) removed this from the rc.1-consider milestone Dec 10, 2019
@SteveL-MSFT Steve Lee (SteveL-MSFT) added this to the 7.1-Consider milestone Dec 10, 2019
@silijon

Copy link
Copy Markdown
Contributor Author

PoshChan-Bot (@PoshChan) please retry windows

@PoshChan

Copy link
Copy Markdown
Collaborator

John Dennis (@silijon), you are not authorized to request a rebuild

@vexx32

Copy link
Copy Markdown
Collaborator

PoshChan-Bot (@PoshChan) please retry windows

@PoshChan

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32), successfully started retry of PowerShell-CI-Windows

Comment thread test/xUnit/csharp/test_CryptoUtils.cs Resolved
Comment thread test/xUnit/csharp/test_CryptoUtils.cs Resolved
Comment thread test/xUnit/csharp/test_CryptoUtils.cs Resolved
Comment thread test/xUnit/csharp/test_CryptoUtils.cs Resolved
@silijon
John Dennis (silijon) force-pushed the allow-keyexchange-for-linux-remote-session-v7 branch from e6d2fc2 to 68646a8 Compare December 18, 2019 06:34

@mathewcathey mathewcathey left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi

@mathewcathey mathewcathey left a comment

Copy link
Copy Markdown

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

@mathewcathey mathewcathey left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes

@PaulHigin

Copy link
Copy Markdown
Contributor

Travis Plunk (@TravisEz13) Where are we on this PR? Is there anything preventing the merge?

@TravisEz13 Travis Plunk (TravisEz13) left a comment

Copy link
Copy Markdown
Member

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
Travis Plunk (TravisEz13) merged commit 895d4b3 into PowerShell:master Feb 19, 2020
@iSazonov Ilya (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
Copy Markdown
Collaborator

John Dennis (@silijon) Thanks for your contribution!

@ghost

Copy link
Copy Markdown

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