Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DH kex on Windows 10 1903 #397

Closed
wants to merge 1 commit into from
Closed

Fix DH kex on Windows 10 1903 #397

wants to merge 1 commit into from

Conversation

wez
Copy link
Contributor

@wez wez commented Jul 31, 2019

Since Windows 1903 the approach used to perform DH kex with the CNG
API has been failing.

This commit switches to using the DH algorithm provider to perform
generation of the key pair and derivation of the shared secret.

It uses a feature of CNG that is not yet documented. The sources of
information that I've found on this are:

With this change I am able to successfully connect from Windows 10 to my
ubuntu system.

Fixes: #388
Refs: alexcrichton/ssh2-rs#122

@wez wez changed the title wincng: use newer DH API for Windows 8.1+ Fix DH kex on Windows 10 1903 Aug 18, 2019
@JanFellner
Copy link

Looks promising, working on my side.
Needed to upgrade to a newer platform sdk as it failed to build with an older one (10.0.10240.0)
I did only take over wincng.c and wincng.h into my project
Suggestion (without pull request)

use:
static int is_windows_10_or_later(void)
{
return IsWindows10OrGreater();
}

(Thats the microsoft suggested way of checking versions)
https://docs.microsoft.com/en-us/windows/win32/api/versionhelpers/nf-versionhelpers-iswindowsversionorgreater
https://docs.microsoft.com/en-us/windows/win32/api/versionhelpers/nf-versionhelpers-iswindows10orgreater

@wez
Copy link
Contributor Author

wez commented Nov 21, 2019

@JanFellner see wez@3dcd743 and 7fe4a7a
The TL;DR is that IsWindows10OrGreater surprisingly returns false on Windows 10 if the embedding application hasn't compiled in a manifest that explicitly indicates that it runs on Windows 10. That would make the version check less effective when this library is linked into a simple application.

@willco007
Copy link
Member

This build failed because it it can't find versionhelpers.h. Could we get an updated PR @wez? Thanks!

@wez
Copy link
Contributor Author

wez commented Feb 5, 2020

I'm not sure that that header is even used in the latest version of this PR; can you try taking it out and building that? I don't have the right machine handy right now; it's been months since I looked at this

Since Windows 1903 the approach used to perform DH kex with the CNG
API has been failing.

This commit switches to using the `DH` algorithm provider to perform
generation of the key pair and derivation of the shared secret.

It uses a feature of CNG that is not yet documented.  The sources of
information that I've found on this are:

* https://stackoverflow.com/a/56378698/149111
* https://github.com/wbenny/mini-tor/blob/5d39011e632be8e2b6b1819ee7295e8bd9b7a769/mini/crypto/cng/dh.inl#L355

With this change I am able to successfully connect from Windows 10 to my
ubuntu system.

Fixes: libssh2#388
Refs: alexcrichton/ssh2-rs#122
Copy link
Member

@mback2k mback2k left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Since I am the original author of the WinCNG backend, I have some questions.

src/wincng.c Show resolved Hide resolved
src/wincng.h Show resolved Hide resolved
@jonathanturcotte
Copy link

This fixed the issue we were seeing in #456, but I had to add ntdll to CMakeLists to get it to build for me properly on Windows Server 2012 and Visual Studio 2017. I'm not familiar with cmake, but I just added:

image

@jonathanturcotte
Copy link

Is there anything else needed to have this PR put in? I can add those lines to the CMakeLists if needed.

@willco007
Copy link
Member

@mback2k are you OK to sign off on these changes?

@mback2k
Copy link
Member

mback2k commented Mar 20, 2020

@mback2k are you OK to sign off on these changes?

I haven't tested this myself yet. I am still uncertain about including an unpublished/undocumented API call in libssh2 before it is published/documented. I still don't understand why the existing approach fails with recent Windows versions.

My plan is to come back to libssh2 development once my current work on curl is done, but if you need to go ahead with this now, I guess you are free to do so. One last thing though: is checking for Windows 10 sufficient here? I think the new approach is only needed on Windows 1903 or later?

src/wincng.c Show resolved Hide resolved
@brandl-muc
Copy link

brandl-muc commented Apr 27, 2020

Thanks @jonathanturcotte , your diff saved me. Seems to me that it should be integrated in this PR.
Edit/Note: this is only relevant when building a shared library of course.

@jonathanturcotte
Copy link

Is there any way this can go in soon?

@mback2k
Copy link
Member

mback2k commented May 30, 2020

I will take a look into this the next 2 days. Sorry for the delay.

Edit: still looking into this and a good solution to automatically try the new and fallback to the old if the new one is unavailable. Will continue working on this during the next days and hope to have something ready soon.

@mback2k
Copy link
Member

mback2k commented Jun 7, 2020

Quick update: I have got a slightly modified version working in my test environment without the version check due to the manifest requirement. I plan to have this merge ready in the upcoming week. Thanks for your patience.

@mback2k
Copy link
Member

mback2k commented Jun 8, 2020

@mback2k
Copy link
Member

mback2k commented Jun 11, 2020

Quick update: my reworked approach regarding automatic fallback from BCRYPT_KDF_RAW_SECRET to the classic approach is now working on Windows 10 (1903) and Windows 7. I hope to have something merge ready very soon. I will keep you posted. My WIP draft PR is not yet updated.

mback2k pushed a commit to mback2k/libssh2 that referenced this pull request Jun 14, 2020
Since Windows 1903 the approach used to perform DH kex with the CNG
API has been failing.

This commit switches to using the `DH` algorithm provider to perform
generation of the key pair and derivation of the shared secret.

It uses a feature of CNG that is not yet documented.  The sources of
information that I've found on this are:

* https://stackoverflow.com/a/56378698/149111
* https://github.com/wbenny/mini-tor/blob/5d39011e632be8e2b6b1819ee7295e8bd9b7a769/mini/crypto/cng/dh.inl#L355

With this change I am able to successfully connect from Windows 10 to my
ubuntu system.

Refs: alexcrichton/ssh2-rs#122
Fixes: libssh2#388
Closes: libssh2#397
mback2k added a commit to mback2k/libssh2 that referenced this pull request Jun 14, 2020
mback2k added a commit to mback2k/libssh2 that referenced this pull request Jun 14, 2020
Avoid the use of RtlGetVersion or similar Win32 functions,
since these depend on version information from manifests.

This commit makes the WinCNG backend first try to use the
new DH algorithm API with the raw secret derivation feature.
In case this feature is not available the WinCNG backend
will fallback to the classic approach of using RSA-encrypt
to perform the required modular exponentiation of BigNums.

The feature availability test is done during the first handshake
and the result is stored in the crypto backends global state.

Follow up to libssh2#397
Closes libssh2#484
@mback2k
Copy link
Member

mback2k commented Jun 14, 2020

@wez @JanFellner @willco007 @jonathanturcotte @brandl-muc

I am so sorry that it took me so long to look into this. But I now finally have integrated the work of @wez and adapted it to a feature-testing-based fallback-approach with PR #484 that does not rely on version checking (which still did not work for me with this original PR due to the requirement of manifests). Therefore I would like to ask you to take a look at the new PR, test it on as much Windows versions you have available and give me some quick Go or No-Go feedback. Thanks in advance!

I hope that we can merge this very soon and also do a release shortly after. cc @bagder

@mback2k mback2k self-assigned this Jun 16, 2020
@jonathanturcotte
Copy link

Awesome, things are a bit hectic right now, but I'll try and give this a try in the next little while if I can!

mback2k pushed a commit to mback2k/libssh2 that referenced this pull request Jun 22, 2020
Since Windows 1903 the approach used to perform DH kex with the CNG
API has been failing.

This commit switches to using the `DH` algorithm provider to perform
generation of the key pair and derivation of the shared secret.

It uses a feature of CNG that is not yet documented.  The sources of
information that I've found on this are:

* https://stackoverflow.com/a/56378698/149111
* https://github.com/wbenny/mini-tor/blob/5d39011e632be8e2b6b1819ee7295e8bd9b7a769/mini/crypto/cng/dh.inl#L355

With this change I am able to successfully connect from Windows 10 to my
ubuntu system.

Refs: alexcrichton/ssh2-rs#122
Fixes: libssh2#388
Closes: libssh2#397
mback2k added a commit to mback2k/libssh2 that referenced this pull request Jun 22, 2020
mback2k added a commit to mback2k/libssh2 that referenced this pull request Jun 22, 2020
Avoid the use of RtlGetVersion or similar Win32 functions,
since these depend on version information from manifests.

This commit makes the WinCNG backend first try to use the
new DH algorithm API with the raw secret derivation feature.
In case this feature is not available the WinCNG backend
will fallback to the classic approach of using RSA-encrypt
to perform the required modular exponentiation of BigNums.

The feature availability test is done during the first handshake
and the result is stored in the crypto backends global state.

Follow up to libssh2#397
Closes libssh2#484
@mback2k mback2k closed this in 7a26697 Jul 6, 2020
mback2k added a commit that referenced this pull request Jul 6, 2020
mback2k added a commit that referenced this pull request Jul 6, 2020
Avoid the use of RtlGetVersion or similar Win32 functions,
since these depend on version information from manifests.

This commit makes the WinCNG backend first try to use the
new DH algorithm API with the raw secret derivation feature.
In case this feature is not available the WinCNG backend
will fallback to the classic approach of using RSA-encrypt
to perform the required modular exponentiation of BigNums.

The feature availability test is done during the first handshake
and the result is stored in the crypto backends global state.

Follow up to #397
Closes #484
@mback2k
Copy link
Member

mback2k commented Jul 6, 2020

Sorry for the delay, I was quite busy the last week. I just merged this into master and plan to do some follow ups via separate PRs.

@wez thanks a lot for the contribution!

willco007 pushed a commit to willco007/libssh2 that referenced this pull request Sep 2, 2020
Since Windows 1903 the approach used to perform DH kex with the CNG
API has been failing.

This commit switches to using the `DH` algorithm provider to perform
generation of the key pair and derivation of the shared secret.

It uses a feature of CNG that is not yet documented.  The sources of
information that I've found on this are:

* https://stackoverflow.com/a/56378698/149111
* https://github.com/wbenny/mini-tor/blob/5d39011e632be8e2b6b1819ee7295e8bd9b7a769/mini/crypto/cng/dh.inl#L355

With this change I am able to successfully connect from Windows 10 to my
ubuntu system.

Refs: alexcrichton/ssh2-rs#122
Fixes: libssh2#388
Closes: libssh2#397
willco007 pushed a commit to willco007/libssh2 that referenced this pull request Sep 2, 2020
willco007 pushed a commit to willco007/libssh2 that referenced this pull request Sep 2, 2020
Avoid the use of RtlGetVersion or similar Win32 functions,
since these depend on version information from manifests.

This commit makes the WinCNG backend first try to use the
new DH algorithm API with the raw secret derivation feature.
In case this feature is not available the WinCNG backend
will fallback to the classic approach of using RSA-encrypt
to perform the required modular exponentiation of BigNums.

The feature availability test is done during the first handshake
and the result is stored in the crypto backends global state.

Follow up to libssh2#397
Closes libssh2#484
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login to ssh server fails using wincng
6 participants