winhttp: enable TLS 1.2 #4550

ethomson · 2018-02-25T23:40:08Z

Windows 7 does not enable TLS 1.2 by default, though with Software Updates it does support TLS 1.2 when requested. Explicitly enable TLS 1.0-1.2 support, in the case that we're on Windows 7.

This lets libgit2 users on Windows 7 talk to GitHub.com (the website), which now requires TLS 1.2 or better. This assumes that those Windows 7 users have installed the necessary software updates for said TLS 1.2 support. But those updates do not enable TLS 1.2 by default, we must explicitly enable it.

matklad · 2018-02-26T07:09:30Z

src/transports/winhttp.c

@@ -786,6 +790,14 @@ static int winhttp_connect(
 		goto on_error;
 	}

+	if (!WinHttpSetOption(t->session,


What happens if we are on the unpatched win7? Will this call return an error because the os does not know about TLS1_2? Should we perhaps ignore the return code then?

ethomson · 2018-02-26T16:32:53Z

So to follow up here after further investigation, here's some more data:

I believe that this function can fail if and only if the underlying operating system lacks TLS 1.2 support
When this call does fail because TLS 1.2 support is not available, the connection handle is still valid and the default protocol support is still enabled, so it's safe to continue on failure.
Windows 7 has support for TLS 1.2 out of the box, without needing service packs or any software updates. This is not terribly obvious, since there's a KB discussing TLS 1.2 support - that KB is actually for the registry key support for enabling TLS 1.2 as a default protocol. Even without that update, applications can opt-in to TLS 1.2 support with this patch.
Windows Vista was the first version of Windows to support TLS 1.2 at all.
Windows XP does not support TLS 1.2 at all, nor will it, since it's now out of support. That's true of consumer versions of Windows XP, anyway; note that there is a specialized Windows XP for Point of Sale devices, and it does actually have an update to enable TLS 1.2.

I've updated this patch to do a few things:

We now ignore errors when trying to set the protocol versions. (Thanks @matklad for asking about this.) I feel pretty confident that we should be able to set this on Windows 7 and we could validate the return code, but even though we don't technically support earlier versions of Windows, it seems like we might as well just ignore the errors. We should only fail when TLS 1.2 is not available and we should still be able to continue to talk to non-TLS 1.2 servers.
We hide this behind a version test and only set this version flag on Windows versions prior to version 8. This ensures that we don't accidentally forget to update this when TLS 1.3 is available. Instead, we will continue on with the OS defaults on newer platforms.

ethomson · 2018-02-26T16:36:23Z

This was implicit, but to be clear: this patch does now explicitly allow libgit2 to connect to github.com on Windows 7 (out of the box) and better. Windows XP and Vista can still connect to hosts that accept non-TLS 1.2, including Bitbucket and Visual Studio Team Services. (Though obviously both those sites will be disabling earlier versions as well and enforcing TLS 1.2.)

matklad · 2018-02-26T17:15:47Z

We hide this behind a version test and only set this version flag on Windows versions prior to version 8. This ensures that we don't accidentally forget to update this when TLS 1.3 is available.

I am afraid this is still better be updated for TLS 1.3. VerifyVersionInfo may return 6.2 even on newer version of windows, if manifests are not set-up properly:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms725492(v=vs.85).aspx

If the application has no manifest, VerifyVersionInfo behaves as if the operation system version is Windows 8 (6.2).

That is, with this exact patch (without updating to TLS 1.3) the potentially bad behavior is that an unmanifested application on windows 10 might get TLS 1.2 even if 1.3 is available.

ethomson · 2018-02-26T17:40:11Z

Yeah, that's not ideal. In a perfect world, we'd have a mechanism to query support and not look at version numbers. 🙄 I'll try to get that on somebody's backlog prior to TLS 1.3.

ethomson · 2018-02-26T17:42:40Z

Note, though, that non-manifested applications report themselves as Windows 8 (NT 6.2). This patch only flips the protocols on for versions prior to 6.2. So Windows 8, 8.1 and 10 are using the system defaults; if they add TLS 1.3 and turn that on as a default, then we should happily work with TLS 1.3.

matklad · 2018-02-26T17:50:06Z

This patch only flips the protocols on for versions prior to 6.2

Hm, according to the update description, Windows Server 2012 is affected, and it is 6.2. However, Windows 8 is not mentioned in the update...

ethomson · 2018-02-26T17:59:59Z

Hm, according to the update description, Windows Server 2012 is affected, and it is 6.2. However, Windows 8 is not mentioned in the update...

You're looking at the text on KB 3140245...?

It does indeed say that. I did not realize that until now, I assumed that since Server 2012 is Windows 8 that they would use the same policy here. Let me investigate.

Arnavion · 2018-02-26T19:19:10Z

Windows 8 went out of support a few months before that KB, which is probably why it isn't listed.

ethomson · 2018-02-26T23:34:24Z


I was totally mistaken, I was looking at a Windows 8.1 machine, not a Windows 8 machine.  I think that you're right @Arnavion.

I've updated this PR yet again, to _always_ try to enable TLS 1.2.  This means that we'll need to sort out support for TLS 1.3 when it becomes available, but this will suffice in the immediate term.

matklad · 2018-02-26T23:47:15Z

src/transports/winhttp.c

+
+#ifndef WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2
+#define WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2 0x00000800
+#endif


Hm, some similar constants are defined over here. Perhaps it makes sense to move these there as well?

Good eye, thanks @matklad, I had forgotten about that file entirely.

😢 In fact, those are for mingw/mingw32, which needs the whole lib infrastructure that's in deps/winhttp.

mingw64 does not use that deps/winhttp stuff, it has its own headers for winhttp... however, they're outdated. so we actually need to include the constants for mingw64 as well. (sigh.)

so we actually need to include the constants for mingw64 as well. (sigh.)

Hm, I wonder if the headers for msvc on old windows versions (XP and win7) could also be "outdated"? So perhaps the original code with ifndef/define was actually correct? :-)

Yep. Bringing those back.

ethomson · 2018-02-27T10:43:01Z

deps/winhttp/winhttp.h

+#define WINHTTP_FLAG_SECURE_PROTOCOL_TLS1    0x00000080
+#define WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1  0x00000200
+#define WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2  0x00000800
+#define WINHTTP_FLAG_SECURE_PROTOCOL_ALL     (WINHTTP_FLAG_SECURE_PROTOCOL_SSL2 | WINHTTP_FLAG_SECURE_PROTOCOL_SSL3 | WINHTTP_FLAG_SECURE_PROTOCOL_TLS1)


If you're looking at this and you're thinking "well, that's not all the bits anymore, you just added two!" then you'd be right, but this constant also doesn't actually include all the options! Ha ha. So I'm keeping this aligned with the actual value on Windows.

ethomson · 2018-02-27T10:59:35Z

Just to further pile on to this thread a bit - I was really hoping to avoid hardcoding these values on newer systems that already support TLS 1.2 out of the box by default. My thinking here was that if we did that then this instance of libgit2 would be prevented from automatically including TLS 1.3 support if that were ever added.

I looked at version checks, which fail for the reasons @matklad mentioned - Windows is actively trying to subvert using version checks as a means to determine whether things are supported, and so all of the APIs to get version information basically lie in order to make sure that you don't use them.

I also looked at WinHttpQueryOption, which unfortunately does not accept WINHTTP_OPTION_SECURE_PROTOCOLS. That value can only be set according to the documentation. (The usage backs this up, it fails trying to query.)

So, again, this is not ideal, but there's no option to do this, AFAICT. I've asked around and will revisit this is I've missed something clever here, because allowing the OS to increase the supported protocols would be ideal.

Include the constants for `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1` and `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2` so that they can be used by mingw. This updates both the `deps/winhttp` framework (for classic mingw) and adds the defines for mingw64, which does not use that framework.

For platforms that do not define `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1` and/or `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2`.

Versions of Windows prior to Windows 8 do not enable TLS 1.2 by default, though support may exist. Try to enable TLS 1.2 support explicitly on connections. This request may fail if the operating system does not have TLS 1.2 support - the initial release of Vista lacks TLS 1.2 support (though it is available as a software update) and XP completely lacks TLS 1.2 support. If this request does fail, the HTTP context is still valid, and still maintains the original protocol support. So we ignore the failure from this operation.

ethomson · 2018-02-27T12:51:29Z

Shipping this and cutting a new release candidate for 0.27.0. Thanks for your help @matklad!

The rc notably includes a fix for the tls error on windows 7: libgit2/libgit2#4550

matklad · 2018-02-27T13:04:31Z

Awesome, thanks a lot, @ethomson !

ethomson mentioned this pull request Feb 25, 2018

Configure WinHttp to use TLS 1.2 on windows 7 #4546

Closed

matklad reviewed Feb 26, 2018

View reviewed changes

matklad mentioned this pull request Feb 26, 2018

Windows 7 users broken by recent Github change rust-lang/cargo#5066

Closed

ethomson force-pushed the ethomson/winhttp branch from ed3d02d to 62c462c Compare February 26, 2018 15:53

alexcrichton mentioned this pull request Feb 26, 2018

Warn Windows 7 users about old TLS rust-lang/cargo#5069

Merged

matklad approved these changes Feb 26, 2018

View reviewed changes

aviks mentioned this pull request Feb 26, 2018

Error when adding packaging on julia JuliaLang/julia#26167

Closed

ethomson force-pushed the ethomson/winhttp branch from 070822d to c5c8283 Compare February 26, 2018 23:31

matklad reviewed Feb 26, 2018

View reviewed changes

ethomson force-pushed the ethomson/winhttp branch from c5c8283 to 0c1ff2b Compare February 27, 2018 10:33

ethomson commented Feb 27, 2018

View reviewed changes

matklad approved these changes Feb 27, 2018

View reviewed changes

ethomson added 3 commits February 27, 2018 11:22

winhttp: include constants for TLS 1.1/1.2 support

934e6a3

For platforms that do not define `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1` and/or `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2`.

ethomson force-pushed the ethomson/winhttp branch from 0c1ff2b to 5ecb622 Compare February 27, 2018 11:25

ethomson merged commit b4dde78 into master Feb 27, 2018

matklad added a commit to matklad/git2-rs that referenced this pull request Feb 27, 2018

Update libgit2 to the next rc

c97e119

The rc notably includes a fix for the tls error on windows 7: libgit2/libgit2#4550

matklad mentioned this pull request Feb 27, 2018

Update libgit2 to the next rc rust-lang/git2-rs#303

Merged

pks-t added the backport label Feb 28, 2018

pks-t mentioned this pull request Feb 28, 2018

v0.26.3 backports #4475

Merged

ethomson mentioned this pull request Mar 13, 2018

"unknown certificate check failure" when pulling changes from GitHub libgit2/libgit2sharp#1547

Closed

vitormcruz mentioned this pull request Sep 5, 2018

Iceberg fail to function behind a proxy on Pharo 7 32bits pharo-vcs/iceberg#885

Open

ethomson deleted the ethomson/winhttp branch October 26, 2018 13:37

ethomson mentioned this pull request Apr 7, 2019

WINHTTP: unknown certificate check failure (12) #5046

Closed

jakissel mentioned this pull request Jun 12, 2020

Intermittent "unknown certificate check failure" during Clone on machine with only TLS1.2 enabled, and all other protocols disabled #5557

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

winhttp: enable TLS 1.2 #4550

winhttp: enable TLS 1.2 #4550

ethomson commented Feb 25, 2018 •

edited

Loading

matklad Feb 26, 2018

ethomson commented Feb 26, 2018

ethomson commented Feb 26, 2018

matklad commented Feb 26, 2018

ethomson commented Feb 26, 2018

ethomson commented Feb 26, 2018

matklad commented Feb 26, 2018

ethomson commented Feb 26, 2018 •

edited

Loading

Arnavion commented Feb 26, 2018

ethomson commented Feb 26, 2018 •

edited

Loading

matklad Feb 26, 2018

ethomson Feb 27, 2018

ethomson Feb 27, 2018

matklad Feb 27, 2018

ethomson Feb 27, 2018

ethomson Feb 27, 2018

ethomson commented Feb 27, 2018

ethomson commented Feb 27, 2018

matklad commented Feb 27, 2018

winhttp: enable TLS 1.2 #4550

winhttp: enable TLS 1.2 #4550

Conversation

ethomson commented Feb 25, 2018 • edited Loading

matklad Feb 26, 2018

Choose a reason for hiding this comment

ethomson commented Feb 26, 2018

ethomson commented Feb 26, 2018

matklad commented Feb 26, 2018

ethomson commented Feb 26, 2018

ethomson commented Feb 26, 2018

matklad commented Feb 26, 2018

ethomson commented Feb 26, 2018 • edited Loading

Arnavion commented Feb 26, 2018

ethomson commented Feb 26, 2018 • edited Loading

matklad Feb 26, 2018

Choose a reason for hiding this comment

ethomson Feb 27, 2018

Choose a reason for hiding this comment

ethomson Feb 27, 2018

Choose a reason for hiding this comment

matklad Feb 27, 2018

Choose a reason for hiding this comment

ethomson Feb 27, 2018

Choose a reason for hiding this comment

ethomson Feb 27, 2018

Choose a reason for hiding this comment

ethomson commented Feb 27, 2018

ethomson commented Feb 27, 2018

matklad commented Feb 27, 2018

ethomson commented Feb 25, 2018 •

edited

Loading

ethomson commented Feb 26, 2018 •

edited

Loading

ethomson commented Feb 26, 2018 •

edited

Loading