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

Windows: Avoid launching as admin when NSIS installer ends. #12985

Merged
merged 1 commit into from Apr 18, 2018

Conversation

Projects
None yet
6 participants
@JeremyRand
Copy link
Contributor

JeremyRand commented Apr 14, 2018

The Bitcoin Core NSIS script runs with elevated privileges. Unfortunately, this means that it launches Bitcoin Core itself with elevated privileges when the user chooses to launch Bitcoin Core at the end of the installation procedure. This PR works around the issue by having explorer.exe launch Bitcoin Core. Seems to be a similar approach to what http://nsis.sourceforge.net/ShellExecAsUser_plug-in does, but without a plugin.

I've tested this with Sysinternals Process Explorer on Windows 10 32-bit. I wouldn't expect any differences in behavior on other Windows releases, but if anyone would like to test on other Windows releases, feel free.

h/t to "UK" at https://mdb-blog.blogspot.se/2013/01/nsis-lunch-program-as-user-from-uac.html?showComment=1410158039989#c2463780017054126736 for the sample code.

Fixes #7990.

Avoid launching as admin when NSIS installer ends.
The Bitcoin Core NSIS script runs with elevated privileges.  Unfortunately, this means that it launches Bitcoin Core itself with elevated privileges when the user chooses to launch Bitcoin Core at the end of the installation procedure.  This commit works around the issue by having explorer.exe launch Bitcoin Core.  Seems to be a similar approach to what http://nsis.sourceforge.net/ShellExecAsUser_plug-in does, but without a plugin.

h/t to "UK" at https://mdb-blog.blogspot.se/2013/01/nsis-lunch-program-as-user-from-uac.html?showComment=1410158039989#c2463780017054126736 for the sample code.

Fixes #7990.

@fanquake fanquake added the Windows label Apr 14, 2018

@ken2812221
Copy link
Member

ken2812221 left a comment

Tested ACK 7d8a8cc

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Apr 16, 2018

@ken2812221 Could you provide the basic steps you used to test this, so anyone else can recreate on a Windows VM?

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Apr 16, 2018

@fanquake I just add wallet=C:\Windows\xxxx to my config file.

if bitcoin core has higher privilege, it can successfully create the directory, otherwise it crash

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 16, 2018

Concept ACK - would like to see some more tested ACKs.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 17, 2018

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Apr 18, 2018

I've tested this using the method @ken2812221 suggested. Basically create a bitcoin.conf with

wallet=C:\Windows\xxxx

If you then install 0.16.0, and choose to run at the end of the installation, Core will error on startup, because we can't specify a path as a wallet name.
error

2018-04-18 04:48:31 Using 4 threads for script verification
2018-04-18 04:48:31 scheduler thread start
2018-04-18 04:48:31 Using wallet directory C:\Users\User\AppData\Roaming\Bitcoin\wallets
2018-04-18 04:48:31 init message: Verifying wallet(s)...
2018-04-18 04:48:41 Shutdown: In progress...
2018-04-18 04:48:41 scheduler thread interrupt
2018-04-18 04:48:41 Shutdown: done

However if you then install the gitian build provided by @jonasschnelli, and launch Core at the end of the installation, we get an exception. Looking at the debug.log, we no longer have access to C:\Windows\.
fatal exception

2018-04-18T04:40:39Z init message: Verifying wallet(s)...
2018-04-18T04:40:39Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2018-04-18T04:40:39Z Using wallet wallet.dat
2018-04-18T04:40:39Z 

************************
EXCEPTION: N5boost10filesystem16filesystem_errorE       
boost::filesystem::create_directory: Access is denied: "C:\Windows\xxxx"       
C:\Program Files\Bitcoin\bitcoin-qt.exe in Runaway exception   
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 18, 2018

@fanquake Thanks for testing. To be clear: All in all that seems a positive result? I suppose you used C:\windows as a path that bitcoin core is supposed to not have access to?

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Apr 18, 2018

@laanwj Yes, I think it's a positive result. Core should not have access to write files into C:\Windows\.

@laanwj laanwj added this to the 0.16.1 milestone Apr 18, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 18, 2018

Right - it's similar to /usr/. I've kind of forgot about windows things somewhat.
Looks good to me, then. Also added "to backport" as sane installer behavior is very important.

@laanwj laanwj merged commit 7d8a8cc into bitcoin:master Apr 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 18, 2018

Merge #12985: Windows: Avoid launching as admin when NSIS installer e…
…nds.

7d8a8cc Avoid launching as admin when NSIS installer ends. (JeremyRand)

Pull request description:

  The Bitcoin Core NSIS script runs with elevated privileges.  Unfortunately, this means that it launches Bitcoin Core itself with elevated privileges when the user chooses to launch Bitcoin Core at the end of the installation procedure.  This PR works around the issue by having `explorer.exe` launch Bitcoin Core.  Seems to be a similar approach to what http://nsis.sourceforge.net/ShellExecAsUser_plug-in does, but without a plugin.

  I've tested this with Sysinternals Process Explorer on Windows 10 32-bit.  I wouldn't expect any differences in behavior on other Windows releases, but if anyone would like to test on other Windows releases, feel free.

  h/t to "UK" at https://mdb-blog.blogspot.se/2013/01/nsis-lunch-program-as-user-from-uac.html?showComment=1410158039989#c2463780017054126736 for the sample code.

  Fixes #7990.

Tree-SHA512: f40d6b6e5bb72952dcfbf223b68bfeb9a03bd5638f41b1700f4651f6452ce3fe7468129f6652c4f546210a5fd2521b2574c4b6068c5aea01ed2d719a8a838cd8

@fanquake fanquake referenced this pull request Apr 18, 2018

Merged

[0.16] Backports #12967

@Michagogo

This comment has been minimized.

@JeremyRand

This comment has been minimized.

Copy link
Contributor Author

JeremyRand commented Apr 18, 2018

@Michagogo I'm not sure whom at Microsoft Ericlaw talked to (he doesn't give any citation), but Microsoft has recommended the approach of using explorer.exe for this.

@Michagogo

This comment has been minimized.

Copy link
Contributor

Michagogo commented Apr 19, 2018

@JeremyRand

This comment has been minimized.

Copy link
Contributor Author

JeremyRand commented Apr 19, 2018

@Michagogo the Microsoft page I linked says:

The solution here is to go back to Explorer and ask Explorer to launch the program for you. Since Explorer is running as the original unelevated user, the program (in this case, the Web browser) will run as Bob.

So while I haven't audited the Microsoft sample code to verify that it does that (Windows C API's aren't my specialty), I think it's reasonable to infer that the Microsoft sample code matches the Microsoft description (which matches what this PR does).

@Michagogo

This comment has been minimized.

Copy link
Contributor

Michagogo commented Apr 19, 2018

fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 26, 2018

Avoid launching as admin when NSIS installer ends.
The Bitcoin Core NSIS script runs with elevated privileges.  Unfortunately, this means that it launches Bitcoin Core itself with elevated privileges when the user chooses to launch Bitcoin Core at the end of the installation procedure.  This commit works around the issue by having explorer.exe launch Bitcoin Core.  Seems to be a similar approach to what http://nsis.sourceforge.net/ShellExecAsUser_plug-in does, but without a plugin.

h/t to "UK" at https://mdb-blog.blogspot.se/2013/01/nsis-lunch-program-as-user-from-uac.html?showComment=1410158039989#c2463780017054126736 for the sample code.

Fixes bitcoin#7990.

GitHub-Pull: bitcoin#12985
Rebased-From: 7d8a8cc

laanwj added a commit that referenced this pull request May 16, 2018

Merge #12967: [0.16] Backports
8fca086 List support for BIP173 in bips.md (Pieter Wuille)
9645aa6 Remove blockmaxsize option from init.cpp (fanquake)
7847b92 Default to defining endian-conversion DECLs in compat w/o config (Matt Corallo)
1720eb3 qt:Show the entire Window when double clicking on taskbar (Chun Kuan Lee)
e055bc0 depends: Fix Qt build with XCode 9.3 (fanquake)
0684cf9 Avoid launching as admin when NSIS installer ends. (JeremyRand)
e802c22 [config] Remove blockmaxsize option (John Newbery)
f118a7a Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)
f60e84d Limit the number of IPs we use from each DNS seeder (e0)

Pull request description:

  Backports:
  - #12626 Limit the number of IPs addrman learns from each DNS seeder
  - #12650 gui: Fix issue: "default port not shown correctly in settings dialog"
  - #12756 [config] Remove blockmaxsize option
  - #12985 Windows: Avoid launching as admin when NSIS installer ends.
  - #12946 depends: Fix Qt build with XCode 9.3
  - #12998 Default to defining endian-conversion DECLs in compat w/o config
  - #12999 qt: Show the Window when double clicking the taskbar icon
  - #13064 List support for BIP173 in bips.md

  to the 0.16 branch.

Tree-SHA512: 3e6b47c54b2cd2bdd81fbc6176cb31e46423f6e05988984d3a09b3535e3cee101ffb071cf753a4beff3c9f0521eb5de4b7c0424a3e97da801d56b4015847ac0f

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jun 29, 2018

Avoid launching as admin when NSIS installer ends.
The Bitcoin Core NSIS script runs with elevated privileges.  Unfortunately, this means that it launches Bitcoin Core itself with elevated privileges when the user chooses to launch Bitcoin Core at the end of the installation procedure.  This commit works around the issue by having explorer.exe launch Bitcoin Core.  Seems to be a similar approach to what http://nsis.sourceforge.net/ShellExecAsUser_plug-in does, but without a plugin.

h/t to "UK" at https://mdb-blog.blogspot.se/2013/01/nsis-lunch-program-as-user-from-uac.html?showComment=1410158039989#c2463780017054126736 for the sample code.

Fixes bitcoin#7990.

GitHub-Pull: bitcoin#12985
Rebased-From: 7d8a8cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.