@willco007 and @vszakats, would you mind taking another look? Thanks!

@jpassing Would you mind investigating the failures here? Thanks.

@jpassing Would you mind investigating the failures here? Thanks.

Yes, sorry for the delay.

Added the missing declarations. I don't have a linux/mingw64 setup locally. so I have to rely on the CI build to check whether it's ok now.

Would it be possible to trigger the CI again? I hope my last commits fixed the remaining build issues.

Thanks!

Unfortunatelty some tests failed because of issues unrelated to this PR:

Error running command 'docker pull %s' (exit 256): Error response from daemon: Head "https://ghcr.io/v2/libssh2/ci_tests_openssh_server/manifests/3598cf79fbd7ded577bf": Get "https://ghcr.io/token?scope=repository%3Alibssh2%2Fci_tests_openssh_server%3Apull&service=ghcr.io": net/http: request canceled (Client.Timeout exceeded while awaiting headers)

Could you trigger it once again, please?

More intermittent docker pull issues:

Error running command 'docker pull %s' (exit 256): Error response from daemon: Head "https://ghcr.io/v2/libssh2/ci_tests_openssh_server/manifests/3598cf79fbd7ded577bf": Get "https://ghcr.io/token?scope=repository%3Alibssh2%2Fci_tests_openssh_server%3Apull&service=ghcr.io": net/http: request canceled (Client.Timeout exceeded while awaiting headers)

Let's wait a few hours and try again?

@jpassing The intermittent issue was the xz backdoor most probably. It should be okay now.
I suggest rebasing on master to include all the other recent CI fixes.

There is a what looks like a crash happening while running tests in job
VS2015, WinCNG, x64, Server 2012 R2.

The job fails consistently:
https://ci.appveyor.com/project/libssh2org/libssh2/builds/49542358 https://ci.appveyor.com/project/libssh2org/libssh2/builds/49542680 https://ci.appveyor.com/project/libssh2org/libssh2/builds/49544107 (moved the job to top)

Yes... I was able to reproduce the crash on 8.1 and it looks like an access violation, possibly caused by a double-free. I'll have a closer look at why this is happening.

 # Child-SP          RetAddr               Call Site
00 00000034`1b04c228 00007fff`b9b2b02e     ntdll!NtWaitForMultipleObjects+0xa
01 00000034`1b04c230 00007fff`b9b2ab8c     ntdll!RtlReportExceptionEx+0x452
02 00000034`1b04c800 00007fff`a3552cec     ntdll!RtlReportException+0xbc
03 00000034`1b04c890 00007fff`b9ab5bc2     vfbasics!AVrfpVectoredExceptionHandler+0x10c
04 00000034`1b04c8e0 00007fff`b9ab4413     ntdll!RtlpCallVectoredHandlers+0xe6
05 00000034`1b04c970 00007fff`b9af20ba     ntdll!RtlDispatchException+0x63
06 00000034`1b04d040 00007fff`a3f726ee     ntdll!KiUserExceptionDispatch+0x3a
07 00000034`1b04d770 00007fff`a3f7868d     vrfcore!VerifierStopMessageEx+0x81e
08 00000034`1b04dad0 00007fff`a2d3a9d0     vrfcore!VfCoreRedirectedStopMessage+0x8d
09 00000034`1b04db60 00007fff`b9b2a4d3     verifier!VerifierStopMessage+0xa0
0a 00000034`1b04dc00 00007fff`a3552853     ntdll!RtlApplicationVerifierStop+0x103
0b 00000034`1b04dc60 00007fff`a35536a0     vfbasics!VerifierStopMessage+0x223
0c 00000034`1b04dcc0 00007fff`a3552bfa     vfbasics!AVrfpCheckFirstChanceException+0x148
0d 00000034`1b04dd50 00007fff`b9ab5bc2     vfbasics!AVrfpVectoredExceptionHandler+0x1a
0e 00000034`1b04dda0 00007fff`b9ab4413     ntdll!RtlpCallVectoredHandlers+0xe6
0f 00000034`1b04de30 00007fff`b9af20ba     ntdll!RtlDispatchException+0x63
10 00000034`1b04e500 00007fff`ab090027     ntdll!KiUserExceptionDispatch+0x3a
11 00000034`1b04ec00 00007fff`ab0a6390     libssh2!_libssh2_wincng_bignum_free+0x17 [D:\dev\fork-libssh2\src\wincng.c @ 3765] 
12 00000034`1b04ec30 00007fff`ab0a6819     libssh2!ecdh_sha2_nistp+0x58c0 [D:\dev\fork-libssh2\src\kex.c @ 2235] 
13 00000034`1b04f3e0 00007fff`ab09c5cb     libssh2!kex_method_ecdh_key_exchange+0x3e9 [D:\dev\fork-libssh2\src\kex.c @ 2340] 
14 00000034`1b04f480 00007fff`ab0c2caf     libssh2!_libssh2_kex_exchange+0x4db [D:\dev\fork-libssh2\src\kex.c @ 3987] 
15 00000034`1b04f4f0 00007fff`ab0c0a9a     libssh2!session_startup+0x1ff [D:\dev\fork-libssh2\src\session.c @ 772] 
16 00000034`1b04f550 00007ff7`45776406     libssh2!libssh2_session_handshake+0x2a [D:\dev\fork-libssh2\src\session.c @ 863] 
17 00000034`1b04f590 00007ff7`45775b3c     test_aa_warmup!connect_to_server+0x36 [D:\dev\fork-libssh2\tests\session_fixture.c @ 68] 
18 00000034`1b04f5d0 00007ff7`457756d4     test_aa_warmup!start_session_fixture+0x31c [D:\dev\fork-libssh2\tests\session_fixture.c @ 202] 
19 00000034`1b04f630 00007ff7`45771dd9     test_aa_warmup!main+0x44 [D:\dev\fork-libssh2\tests\runner.c @ 55] 
1a 00000034`1b04f6d0 00007ff7`45771c7e     test_aa_warmup!invoke_main+0x39 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 79] 
1b 00000034`1b04f720 00007ff7`45771b3e     test_aa_warmup!__scrt_common_main_seh+0x12e [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 288] 
1c 00000034`1b04f790 00007ff7`45771e6e     test_aa_warmup!__scrt_common_main+0xe [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 331] 
1d 00000034`1b04f7c0 00007fff`b98713d2     test_aa_warmup!mainCRTStartup+0xe [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp @ 17] 
1e 00000034`1b04f7f0 00007fff`b9a754f4     kernel32!BaseThreadInitThunk+0x22
1f 00000034`1b04f820 00000000`00000000     ntdll!RtlUserThreadStart+0x34

The last commit fixed the crash and tests on Windows Server 2012 are now failing with

libssh2_session_handshake failed (-8): Unable to exchange encryption keys

It's quite possible that CNG on Windows Server 2012 doesn't support all required feature yet, but I'll have to verify that.

It's quite possible that CNG on Windows Server 2012 doesn't support all required feature yet, but I'll have to verify that.

The problematic line of code is the call to BCryptDeriveKey(..., BCRYPT_KDF_RAW_SECRET, ... ). This call works reliable on Windows 10+, but consistently returns STATUS_NOT_SUPPORTED on Windows 8.1 and Server 2012 R2.

bcrypt.h suggests that BCRYPT_KDF_RAW_SECRET should be available on WinBlue (=8.1) and later:

#if (NTDDI_VERSION >= NTDDI_WINBLUE)
#define BCRYPT_KDF_RAW_SECRET               L"TRUNCATE"
#endif

The documentation for BCRYPT_KDF_RAW_SECRET also indicates that it should work on 2012R2/8.1:

Windows 8, Windows Server 2008, Windows Vista, Windows Server 2003 and Windows XP: This value is not supported.

But the documentation doesn't distinguish between RSA and ECC, and it's possible that ECC support was added later. Interestingly, the .NET runtime guards calls with a IsWindowsVersionAtLeast(10) version check, which supports this thesis.

Windows Server 2012 R2 and Windows 8.1 are both out of support, so I figure it might be okay if WinCNG/ECDSA doesn't work on these platforms? If so, is there any established pattern that I could use to ensure we skip respective tests when executed on these Windows versions?

The documentation for BCRYPT_KDF_RAW_SECRET also indicates that it should work on 2012R2/8.1:

Windows 8, Windows Server 2008, Windows Vista, Windows Server 2003 and Windows XP: This value is not supported.

But the documentation doesn't distinguish between RSA and ECC, and it's possible that ECC support was added later. Interestingly, the .NET runtime guards calls with a IsWindowsVersionAtLeast(10) version check, which supports this thesis.

Windows Server 2012 R2 and Windows 8.1 are both out of support, so I figure it might be okay if WinCNG/ECDSA doesn't work on these platforms? If so, is there any established pattern that I could use to ensure we skip respective tests when executed on these Windows versions?

It's fine to not support it on these versions (do we have any other choice?). But, we don't have a pattern to skip tests based on runtime availability of algos yet. Also, we don't have plumbing for Windows version detection — this can lead into a rabbit whole, also with manifests, so I'd be happy to avoid it.

Do I understand correctly that BCryptOpenAlgorithmProvider( ... BCRYPT_ECDSA_* ) succeeds, but then BCryptDeriveKey(..., BCRYPT_KDF_RAW_SECRET, ... ) fails? Can we use this to detect the feature?

This reminds of these earlier commit dealing with a similar scenario (with version detection briefly introduced, then solved differently):
7a26697
3baa367

Do I understand correctly that:

• We need a way to runtime detect if ECDSA is truly supported by Windows?
• We need a way to inform the tests / caller if ECDSA support is missing, to skip the test / not rely on this feature?

/cc @mback2k

Do I understand correctly that BCryptOpenAlgorithmProvider( ... BCRYPT_ECDSA_* ) succeeds, but then BCryptDeriveKey(..., BCRYPT_KDF_RAW_SECRET, ... ) fails? Can we use this to detect the feature?

Yes, exactly. So it's not that ECDSA isn't supported at all on older Windows versions, it's just that this particular function/flag combination doesn't seem to be implemented.

This reminds of these earlier commit dealing with a similar scenario (with version detection briefly introduced, then solved differently): 7a26697 3baa367

I think it's the same issue: IIUC, with RSA, the solution was to attempt to use BCryptDeriveKey(..., BCRYPT_KDF_RAW_SECRET, ... ), and if that fails, to fall back to the old method (which stopped working on newer Windows 10 versions).

The issue with ECDSA is that we don't have an alternate method to fall back to... or at least I'm not aware of one.

I agree that checking Windows versions tends to be complex and/or unreliable, so I'd rather avoid that -- at least within the library. Testing in advance if BCryptDeriveKey(..., BCRYPT_KDF_RAW_SECRET, ... ) works is theoretically possible, but it requires a fair amount of set up and might be quite slow -- so I'm not convinced this is a great approach either.

What might work is the following:

1. Introduce an error code LIBSSH2_ERROR_UNSUPPORTED_PLATFORM
2. Return LIBSSH2_ERROR_UNSUPPORTED_PLATFORM when BCryptDeriveKey(..., BCRYPT_KDF_RAW_SECRET, ... ) fails with STATUS_NOT_SUPPORTED
3. Modify the unit tests in one of two ways:
   • Skip ECDSA tests when run on old Windows versions (requires a Windows version check, but only in the unit test, not in the library)
   • Accept LIBSSH2_ERROR_UNSUPPORTED_PLATFORM as "inconclusive" outcome (i.e., don't fail the test when encountering this error code)

Wdyt?

For tests the LIBSSH2_ERROR_UNSUPPORTED_PLATFORM = 'inconclusive' path is looking good.

Thinking of downstream code trying to use libssh2 though, I'm not sure this would be workable without breaking those when run in an unfit (pre-10) Windows env.

Those apps either need to keep ECDSA disabled from the get go or fallback to something else automatically.

This brings back to a reliable / fast / manifest-less (end-user apps may not have manifests, nor do our tests), feature or Windows 10 detection on init.

Could RtlVerifyVersionInfo from ntdll fit this? curl has this: https://github.com/curl/curl/blob/3d569aaaf8714e18db526c6a6f5de9b6c6d9af5a/lib/version_win32.c, but it's extremely complicated, I believe it may work without manifests but I'm not sure. This is also heavy and recursive code and aimed to check for precise OS revisions, which we don't need here. But perhaps we'd only need a single call for the purpose of this PR.

Or perhaps RtlGetVersion?: https://stackoverflow.com/questions/36543301/detecting-windows-10-version/36545162#36545162

There is Is­Windows­10­Or­Greater() but it requires Win10 SDK, needing build-time feature detection and it won't always be available. Also cannot be loaded dynamically: https://devblogs.microsoft.com/oldnewthing/20170112-00/?p=95175

Another option is to enable this only when building for Win10 or higher based on _WIN32_WINNT. This limits usability, but solves the problem at build time.

Thinking of downstream code trying to use libssh2 though, I'm not sure this would be workable without breaking those when run in an unfit (pre-10) Windows env.

That's a good point.

I'm sure we could do a Windows version check somehow, but I worry that it would be a high-impact change: All places that currently use #if LIBSSH2_ECDSA (of which there are quite a few, especially in kex.c) would need to be changed to invoke some helper function that checks if ECDSA is enabled and functional on this platform.

Another option is to enable this only when building for Win10 or higher based on _WIN32_WINNT. This limits usability, but solves the problem at build time.

Guarding the feature by a _WIN32_WINNT check might still be risky as _WIN32_WINNT reflects the SDK version at compile time, it doesn't have any effect at runtime. However, I suppose we could introduce a build option ENABLE_CNG_ECDSA to let users "opt in" to WinCNG ECDSA support if they're ok with it only working on Win10+.

If ENABLE_CNG_ECDSH is set, we should probably also set the DLL subsystem version to 10 using something like

set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /SUBSYSTEM:WINDOWS,10")

That would also block libssh2.dll from being loaded on older Windows versions.

I'm sure we could do a Windows version check somehow, but I worry that it would be a high-impact change: All places that currently use #if LIBSSH2_ECDSA (of which there are quite a few, especially in kex.c) would need to be changed to invoke some helper function that checks if ECDSA is enabled and functional on this platform.

That'd be harsh indeed. It means it's not enough to do this once in _libssh2_wincng_init() and leaving _libssh2_wincng.hAlgECDSA[] elements NULL.

If ENABLE_CNG_ECDSH is set, we should probably also set the DLL subsystem version to 10 using something like

set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /SUBSYSTEM:WINDOWS,10")

That would also block libssh2.dll from being loaded on older Windows versions.

Good idea, we can go with it.

The mingw-w64 equivalent for the MSVC linker option above seems to be: -Wl,--subsystem,windows:10, I'm not sure of the version number format though.

I propose using the names LIBSSH_ECDSA_WINCNG in C and ENABLE_ECDSA_WINCNG in CMake. They contain wincng for easy grepping and aligns with naming we used so far. Autotools also needs an option, perhaps --enable/disable-ecdsa-wincng?

Thanks @jpassing for the great work. Merged now.

Thanks a lot for your help and fixing the build scripts, @vszakats!