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.

Sign up for GitHub

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

Jump to bottom

win: confirm that KB2921916 installer workaround works #1051

Closed
crawshaw opened this issue Dec 21, 2020 · 28 comments
Closed

win: confirm that KB2921916 installer workaround works #1051

crawshaw opened this issue Dec 21, 2020 · 28 comments
Labels
L2 Few Likelihood OS-windows P3 Can't get started Priority level T2 Visual Polish Issue type

Comments

@crawshaw
Copy link
Member

The latest wintun requires installing: https://download.wireguard.com/windows-toolchain/distfiles/Windows6.1-KB2921916-x64.msu. Without it we see the error:

[Wintun] SelectDriver: Extracting driver
[Wintun] SelectDriver: Installing driver
[Wintun] SelectDriver: Could not install driver to store: The publisher of an Authenticode(tm) signed catalog has not yet been established as trusted. (Code 0xE0000242)
[Wintun] CreateAdapter: Failed to select driver

Windows Update does not automatically install this update as Win7 is EOL.
@bradfitz
Copy link
Member

we don't use an MSI yet so we can't really bundle an MSU from what I understand?

Should we also host that file and detect at runtime if it's necessary? We can try to fetch+install it before loading wintun.

@crawshaw
Copy link
Member Author

WireGuard does something similar: https://git.zx2c4.com/wireguard-windows/tree/installer/customactions.c#n145

@zx2c4
Copy link

zx2c4 commented Dec 21, 2020

https://download.wireguard.com/windows-toolchain/distfiles/Windows6.1-KB2921916-x64.msu for x64 and
https://download.wireguard.com/windows-toolchain/distfiles/Windows6.1-KB2921916-x86.msu for x86.

If you do detection, I'd recommend the hacky memmem trick that @crawshaw linked to, looking for "Signature Hash". I reversed all the binaries and identified that as a very reliable distingusher. In practice I think it's more reliable than querying the hotfix database on the system, which sometimes lacks the KB entry despite the dll being updated, and the version numbers and times in the dll resource are also not as reliable as they ought to be. So https://git.zx2c4.com/wireguard-windows/tree/installer/customactions.c#n145 is your best bet.

As far as bundling the MSU goes - you can do this from nsis. You probably want to do something like:

wusa.exe /quiet /warnrestart path\to\temporary\nsis\directory\blahblah.msu

See https://ss64.com/nt/wusa.html for other options. There's no need to do this with MSI, and probably MSI would take the same approach as invoking wusa.exe anyway.

Alternatively, if you do this at runtime in the app instead of at install time from nsis, and you want to auto-fetch, please mirror those files on your own servers.

@apenwarr
Copy link
Member

I think #1051 perhaps eliminated the need for this?

@bradfitz
Copy link
Member

Did you mean https://github.com/tailscale/corp/issues/1101 ?

@apenwarr
Copy link
Member

apenwarr commented Jan 14, 2021 via email

@zx2c4
Copy link

zx2c4 commented Jan 14, 2021

Care to share details? That link is private.

@bradfitz
Copy link
Member

@zx2c4, the core of that change is that at the end of the install (where it's running with elevated privileges), it calls:

// InstallWintunDriver loads wintun.dll to force it to install its
// kernel driver, which can take a long time (especially on Windows
// 7). We want to do it early (at install time), so the service starts
// up quicker later, to avoid the GUI timing out trying to connect to
// it.
func InstallWintunDriver() error {
	pool, err := wintunPool()
	if err != nil {
		return fmt.Errorf("MakePool: %w", err)
	}
	adapter, _, err := pool.CreateAdapter("Tailscale Installer", nil)
	if err != nil {
		return fmt.Errorf("CreateAdapter: %w", err)
	}
	_, err = adapter.Delete(false)
	if err != nil {
		return fmt.Errorf("Adapter.Delete: %w", err)
	}
	return nil
}

The change was primarily about eliminating the race between the driver install from the service and the GUI waiting to connect to the service.

By doing it at install time, by the time the server + GUI start later, the driver's already been installed and the service starts up much more quickly.

But I'm not sure how that helps this issue? Isn't KB2921916 really just some hacky fix (that Microsoft later abandoned as not a good idea?) that disables some verification? The fact that Microsoft never included KB2921916 in subsequent roll-up patch releases seems like a red flag. Or is @apenwarr saying that after the UAC elevation, the driver install works without KB2921916 somehow?

@zx2c4
Copy link

zx2c4 commented Jan 14, 2021 •
edited

That's not going to be robust when the driver gets removed from the store and readded by other things later.

KB2921916 isn't hacky at all. It changes the code from:

char hash[20];
assert(GetCertHash(thing, hash, sizeof(hash)));

to

size_t hash_size = GetSizeOfHash(thing);
char *hash = malloc(hash_size);
assert(GetCertHash(thing, hash, hash_size));

The old code failed when sha2 was used, because 20 bytes is too small. The new code is the proper fix. The patch isn't hacky at all and works extremely well. I reverse engineered every byte of the change of the binary and I can't find any bugs with the new code or any idea why it'd be a problem.

KB2921916 is gone from Microsoft's site because they pulled all the KBs when they sunsetted Windows 7.

@bradfitz
Copy link
Member

KB2921916 isn't hacky at all. It changes the code from:

Ah, thanks for the explanation!

KB2921916 is gone from Microsoft's site because they pulled all the KBs when they sunsetted Windows 7.

Weird.

That's not going to be robust when the driver gets removed from the store and readded by other things later.

We still do the pool.CreateAdapter on service start-up regardless (so if it disappears, it'll still come back). Doing it in the installer additionally just makes the service start-up's call to pool.CreateAdapter much faster.

Empirically it makes the Windows 7 install experience much better.

@zx2c4
Copy link

zx2c4 commented Jan 14, 2021

We still do the pool.CreateAdapter on service start-up regardless (so if it disappears, it'll still come back). Doing it in the installer additionally just makes the service start-up's call to pool.CreateAdapter much faster.

Except if the driver gets removed, pool.CreateAdapter will reinstall it, and then you'll run into the sha2 issue again and installation will fail.

@bradfitz
Copy link
Member

Sorry, I still don't understand how the func InstallWintunDriver code I pasted above has anything to do with KB2921916.

It's possible you and @apenwarr are having a separate conversation over my head.

@zx2c4
Copy link

zx2c4 commented Jan 14, 2021

Alright, here's what's up:

On Windows 10, the driver is signed by Microsoft, and the certificate is already included in Windows, so everything works well and there's nothing to do.

On Windows 7 and Windows 8, the driver is signed with an authenticode certificate. In order to install a driver with an authenticode signature, one must first add the certificate to the system certificate store. After that, installing a driver into the driver store works well, and silently without any popups or UI required.

Except on Windows 7, due to that 20-byte sha1-hardcoding bug I mentioned above, it can't actually deal with the sha2 signature. So it falls back to assuming that the driver is signed with an unknown certificate, and prompts the user, "hey are you sure you want to install this?" And, since Windows services don't have UI access, this translates into a failure.

Calling that function in the installer appears to work because it moves that UI prompt to install time, rather than service time. But if the driver is ever removed, then pool.CreateAdapter being called by the service will install it again, in which case, the service won't have UI access and the operation will fail.

The correct solution is to install that KB, which fixes the root cause of the issue and allows the installation of the certificate into the trusted certificate store to work as intended.

@apenwarr
Copy link
Member

apenwarr commented Jan 20, 2021 via email

@zx2c4
Copy link

zx2c4 commented Jan 20, 2021

Note that there is a difference between Windows 7 not understanding authenticode (which is a pain) and it thinking your driver is completely unsigned. There's an old-style signing method that works with Windows 7. Before wintun.dll, we bundled two copies of wintun.sys, one signed with the new-style Windows 10 thing and one signed with the old-style Windows 7/8 thing, and installed the right one. It still popped up a prompt on Windows 7, but the prompt was: "This driver is signed by Tailscale, do you want to use it anyway?" It's kind of a silly question, but it's how the OS was intended to work, so I'm not sure we should be interfering with it. When we tried to include both certs on the same driver, Windows 7 would claim it was entirely unsigned, which was scary for users (and I think prevented installing on Windows 7-64bit or something) so we couldn't do that. ᐧ

Yes, and this is what wintun.dll does. The issue is simply that the dialog box cannot be shown from a non-interactive service.

@zx2c4
Copy link

zx2c4 commented Jan 20, 2021

You are correct that my suggested solution would stop working if someone ever uninstalled the driver. But if they do that, it seems like they're trying to break their system, so perhaps we should let it be broken.

I'm pretty sure you're underestimating how common of a flow that winds up being. Maybe I'm wrong, but keep this in mind should you get unusual bug reports down the line. On the other hand, perhaps win7 usage won't be high enough to warrant a real solution.

@zx2c4
Copy link

zx2c4 commented Jan 20, 2021

The problem with Microsoft hotfixes is that they are explicitly intended to be used only as an interim solution until the "real" fix gets properly tested and bundled into an actual OS patch.

Whatever your impression of Microsoft's policies is, all I can say is that I've reverse engineered every byte of that hotfix and cannot think of a more straight forward way of fixing the bug.

@apenwarr
Copy link
Member

apenwarr commented Jan 20, 2021 via email

@zx2c4
Copy link

zx2c4 commented Jan 20, 2021

#1055 (comment)

@apenwarr
Copy link
Member

apenwarr commented Jan 20, 2021 via email

@zx2c4
Copy link

zx2c4 commented Jan 20, 2021

It is designed to intentionally do that, actually. You can poke around in the code and look if you care. Or just take my word for it that that trick you're using will break under certain realistic circumstances. Given the small userbase of win7, maybe it doesn't matter. But I think I've spent enough time here assisting with closed-source proprietary software to add more, so I'd encourage you to just be attentive to bug reports if you still want to go this route, and hopefully the win7 market share keeps dwindling.

@xjasonlyu xjasonlyu mentioned this issue Mar 3, 2021
Closed
@imgk imgk mentioned this issue Mar 26, 2021
Closed
@DentonGentry DentonGentry added L2 Few Likelihood P3 Can't get started Priority level T2 Visual Polish Issue type labels May 20, 2021
@apenwarr apenwarr self-assigned this Jun 1, 2021
@apenwarr apenwarr changed the title win: need to ship KB2921916 in nsis installer for windows 7 win: confirm that KB2921916 installer workaround works Jun 8, 2021
@crawshaw
Copy link
Member Author

We tested this when we launched the new version of wintun. On win7 a dialog appears telling users where to go to get the KB.

@apenwarr
Copy link
Member

That's not what this bug is about. There's a workaround in NSIS that should prevent the need for the kb altogether. It worked, once, but we have to recheck it because it might have been broken.

@apenwarr apenwarr reopened this Jun 16, 2021
@crawshaw
Copy link
Member Author

Can you point me at the code? I'm not aware of any workaround, and it certainly didn't work when we released 1.4. That's why we added the dialog that points users at https://github.com/tailscale/tailscale/wiki/Win7.

@apenwarr
Copy link
Member

NSIS calls tailscale-ipn.exe /install, which calls doInstall():
https://github.com/tailscale/corp/blob/main/win/ui/main_windows.go#L104
which calls InstallWintunDriver(), which calls wintunPool().CreateAdapter().

See very long boring thread above for various disagreements about whether this is the right or the wrong solution. But nevertheless, it's there, and I'd like to confirm that it works as "expected" (ie. it's okay directly after installing Tailscale on Windows 7, but bets are off if some other app fiddles with wintun after that.)

@DentonGentry
Copy link
Contributor

I just installed a Win7 32bit VM, the IE8 testing VM from Microsoft. I had to install Chrome to be able to authenticate, as IE8 wouldn't let me click the button, but Tailscale 1.9.177 came up without an error and no dialog box to install KB2921916.

I can connect to other nodes on my Tailnet from the Win7 VM.

@apenwarr
Copy link
Member

apenwarr commented Jun 16, 2021 via email

@apenwarr
Copy link
Member

Finally had a chance to methodically retest this with tailscale v1.9.205. Looks like all is well, and I didn't need to install the hotfix. Phew.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
L2 Few Likelihood OS-windows P3 Can't get started Priority level T2 Visual Polish Issue type
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@bradfitz @zx2c4 @apenwarr @crawshaw @DentonGentry