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

Warn Windows 7 users about old TLS #5069

Merged
merged 2 commits into from Feb 26, 2018
Merged

Conversation

@matklad
Copy link
Member

matklad commented Feb 23, 2018

An eyepatch for #5066.

@retep998 what would the best way to check for windows 7 specifically? Currently I use just #[cfg(windows)], which might give false positives on 8 and 10.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Feb 23, 2018

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Feb 23, 2018

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 23, 2018

According to,
https://support.microsoft.com/en-us/help/3140245/update-to-enable-tls-1-1-and-tls-1-2-as-a-default-secure-protocols-in
Looks like the patch for windows requires or set a DefaultSecureProtocols registry entry. Maybe we can check for that?

And thanks for making this patch!

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Feb 23, 2018

Amusingly, trying to test out this fix on win7 with

cargo install --git https://github.com/matklad/cargo --branch win7-y-u-no-tls

fails with this very error =P

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Feb 23, 2018

If we get this landed and a commit somewhere (a 1.24.1 branch?) that targets stable that also does this by Monday morning, I feel fairly good about being able to include it in 1.24.1. Otherwise we'll just land it into beta I think.

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Feb 23, 2018

Ok, I've checked that the current patch works as expected on windows 7.

I've looked into detecting registry key/windows version a bit, but I've failed to find a smiple API in winapi-rs to do this, and I don't feel confident about using complex windows API, so it would be great if someone with more windows experience implements a version check =)

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Feb 23, 2018

  • The KB article declares that the issue also exists on Windows Server 2012 which is the server equivalent of Windows 8, but doesn't list Windows 8 itself as affected. But this is probably because Windows 8 became unsupported some time before that update was released, so it probably is affected and including it in the check should not be a problem.

  • The reg key doesn't exist on Windows 10 and Windows 10 works without it, so just testing the reg key's existence independent of the OS version will not be correct.

So IsWindows8Point1OrGreater returning FALSE is probably all you need. This is a header-only function so winapi or cargo would need to reimplement it in terms of VerifyVersionInfo(W)

The complication is that the version functions lie to applications without manifests that claim support for Windows 8.1 and 10, and I don't know if cargo has such a manifest. It certainly doesn't appear to.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 23, 2018

Unfortunately IsWindows8Point10OrGreater is not yet in winapi-rs. :-(

I tried to use GetVersionEx directly:

    use winapi::um::winnt::OSVERSIONINFOW;
    use winapi::um::sysinfoapi::GetVersionExW;
    let mut os = OSVERSIONINFOW {
        dwOSVersionInfoSize: std::mem::size_of::<OSVERSIONINFOW>() as u32,
        dwMajorVersion: 0,
        dwMinorVersion: 0,
        dwBuildNumber: 0,
        dwPlatformId: 0,
        szCSDVersion: [0; 128],
    };
    unsafe { GetVersionExW(&mut os) };
    println!("{:?}.{:?}", os.dwMajorVersion, os.dwMinorVersion);

and on my win10, I get 6.2. according to the docs at https://msdn.microsoft.com/en-us/library/windows/desktop/ms724834(v=vs.85).aspx that is win8, probably something about not having a manifest file. But if we get 6.1 on win 7 we may have a workaround. (I don't have a win7 box to test on.)

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Feb 23, 2018

@Eh2406 Yes, as I said in my comment, the function is header-only, and lies to applications without manifests.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Feb 23, 2018

Something like this will work, once you've figured out the manifest problem (not tested or compiled):

const WIN32_WINNT_WINBLUE_HIBYTE = 0x06;
const WIN32_WINNT_WINBLUE_LOBYTE = 0x03;

fn IsWindows8Point1OrGreater() -> bool {
    IsWindowsVersionOrGreater(WIN32_WINNT_WINBLUE_HIBYTE, WIN32_WINNT_WINBLUE_LOBYTE, 0)
}

fn IsWindowsVersionOrGreater(majorVersion: DWORD, minorVersion: DWORD, servicePackMajor: DWORD) -> bool {
    let conditionMask: DWORDLONG = VerSetConditionMask(VerSetConditionMask(VerSetConditionMask(0, VER_MAJORVERSION, VER_GREATER_EQUAL), VER_MINORVERSION, VER_GREATER_EQUAL), VER_SERVICEPACKMAJOR, VER_GREATER_EQUAL);

    let mut osvi: OSVERSIONINFOEXW = std::mem::zeroed();
    osvi.dwOSVersionInfoSize = std::mem::size_of::<OSVERSIONINFOEXW>();
    osvi.dwMajorVersion = majorVersion;
    osvi.dwMinorVersion = minorVersion;
    osvi.wServicePackMajor = servicePackMajor;

    VerifyVersionInfoW(&mut osvi, VER_MAJORVERSION | VER_MINORVERSION | VER_SERVICEPACKMAJOR, conditionMask) != FALSE
}

(Based on the implementation of those functions in VersionHelpers.h from the Win 10 SDK)

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 23, 2018

@Arnavion, I can't find your comment about manifests. I am sorry if I echoed info you had already provided.

once you've figured out the manifest problem

I don't think the manifest problem is trivial. I'd love to be proven wrong. I don't know of a way to add a manifest that doesn't involve a large build dependency. The win SDK for mt.exe or LLVM for its alternative. Do we want to require that for building cargo?

not tested or compiled

Yes that is a better solution, once it is converted to valid rust.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Feb 23, 2018

@retep998 should comment about that. It's not a problem for windows-msvc builds of course, but I don't know anything about windows-gnu (assuming the cargo.exe that ships with the windows-gnu toolchain is itself built with windows-gnu and not just the same binary as the one that ships with the windows-msvc toolchain).

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 24, 2018

If we need to deal with the manifest to lowerer the false positive rate, then perhaps we merge as is for now so it can get in 1.24.1. And add a FIXME to get dealt with wen manifests are fixed.


const WIN7_TLS_WARNING: &str = "\
Certificate check failure might be caused by outdated TLS on Windows 7.\n\
Please follow these instructions to enable more secure TLS:\n\n \

This comment has been minimized.

Copy link
@retep998

retep998 Feb 24, 2018

Member

Until we are able to reliably detect Windows 7, perhaps we should prefix this with If you are using Windows 7, please...

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Feb 24, 2018

Agreed. The function's comment and error message should also mention Server 2008R2 and Server 2012 in addition to 7, and perhaps word it like "If you're on ... then certificate check failure might be caused..."

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Feb 24, 2018

Having the necessary tooling from VC++/MinGW to link manifests is not an issue. The issue is having the support from Rust itself. It will involve non-trivial design work to come up with a good way to link manifests, and I doubt it will happen any time soon.

However there is a shorter term solution for manifests and that is having simple external manifests instead of embedded manifests. It would involve someone writing manifests such as cargo.exe.manifest and then getting the Rust packaging stuff to shove that in the same component and folder as cargo.exe.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 24, 2018

I think we can use rust-embed-resource or similar build script magic to include the manifest, without the design work from rustic. (If we have the tooling from VC++/MinGW installed.)

I have never had any luck getting side by side manifests to work, but it does sound like a simple solution.

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Feb 24, 2018

Hm, it looks like we won't be able to implement the manifest solution in any reasonable time frame :-(

Could we check for the presence of HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Internet Settings\WinHttp instead, as suggested by @Eh2406?

Specifically, is this key present on Windows 8 and Windows 10?

EDIT: hm, sorry, @Arnavion, I've missed the fact that you already checked that in #5069 (comment)! Thanks a lot ❤️ !

@matklad matklad force-pushed the matklad:win7-y-u-no-tls branch from da2c030 to abddedd Feb 24, 2018
@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Feb 24, 2018

Updated the error message to mention windows server and to be conditional on "if you are indeed on windows".

It seems unlikely to me that we will be able to implement the cargo.exe.manifest solution fast, and this I think means that this patch is the best we can hope for 1.24.1.

The failure mode here is that a user on Windows 10 might get this wrong worming if threre's genuinely something wrong with TLS. However, because we are checking for a specific code from libgit, this seams unlikely?

An eyepatch for #5066.
@matklad matklad force-pushed the matklad:win7-y-u-no-tls branch from abddedd to 9de29ff Feb 25, 2018
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 26, 2018

Thanks for the quick fix here @matklad! It sounds like this is hopefully going to be a temporary holdover until libgit2/libgit2#4550 is sorted out, after which we'll probably just update libgit2 pronto and get that into Cargo. In that sense it seems like we'll be using this for hopefully only a few cycles, but still a few cycles in the sense that it'll get backported and such.

This looks pretty conservative but I think one final thing we may wish to do is to only warn about connections to github.com perhaps? I believe that's the main instigator for this issue, right? It may also help reduce the noise of warnings in legitimate situations as that seems to come up relatively often (certificate errors on Windows).

I'd personally also be much more comfortable backporting a warning like this as well rather than libgit2 updates, so I think for sure we should pursue this.

@matklad would you be ok switching to only warning on github.com hostnames?

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Feb 26, 2018

@matklad would you be ok switching to only warning on github.com hostnames?

Sure will try to do this in a couple of hours!

@matklad

This comment has been minimized.

Copy link
Member Author

matklad commented Feb 26, 2018

Updated to check for GitHub specifically, and verified on the virtual machine that the message is indeed printed on an unpatched win7, and that a patched one works OK.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 26, 2018

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2018

📌 Commit 81e18de has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2018

⌛️ Testing commit 81e18de with merge ce87cfa...

bors added a commit that referenced this pull request Feb 26, 2018
Warn Windows 7 users about old TLS

An eyepatch for #5066.

@retep998 what would the best way to check for windows 7 specifically? Currently I use just `#[cfg(windows)]`, which might give false positives on 8 and 10.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ce87cfa to master...

@bors bors merged commit 81e18de into rust-lang:master Feb 26, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors added a commit that referenced this pull request Feb 26, 2018
[beta] warn about TLS for github specifically

Backport of #5069 to beta

r? @alexcrichton
@matklad matklad added the relnotes label Feb 26, 2018
bors added a commit that referenced this pull request Feb 26, 2018
[stable] warn about TLS for github specifically

Backport of #5069 to stable for 1.24.1 🔥 ⚠️

r? @alexcrichton
bors pushed a commit to rust-lang/rust that referenced this pull request Feb 27, 2018
bors added a commit to rust-lang/rust that referenced this pull request Feb 27, 2018
[beta] backport cargo TLS warning for win7

rust-lang/cargo#5069

r? @alexcrichton
@matklad matklad deleted the matklad:win7-y-u-no-tls branch Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.