Add option to disable 077 umask (create new files with system default umask) #4286

runeksvendsen · 2014-06-04T11:21:20Z

The option is only effective for either wallet-less builds or if
-disablewallet is specified as well.

This is useful when the wallet is handled by an application other than bitcoind (for example Armory). If bitcoind runs as a user different from the user that runs Armory, Armory is not able to read the blockchain files that it needs to. This allows bitcoind to be run as a separate user and function with Armory (and other applications that need to read bitcoind data files) at the same time.

… umask) The option is only effective for either wallet-less builds or if -disablewallet is specified as well.

jgarzik · 2014-06-04T18:51:51Z

would prefer umask call not get moved
seems overly complex for what it does
poorly named. should be "system default perms" not necessarily "lax" perms... they might not be lax, and the point of the feature is not to be lax, but to pass through the system default.

runeksvendsen · 2014-06-05T10:32:30Z

Hey Jeff

I agree that it's a bit messy. Here's another version. It keeps the umask call in the same place, only adds 11 lines of code, and I've changed the option name to "-sysperms". But you can name it anything you want, I'm not good with names.
The only downside is that nothing is logged to debug.log when this option is in effect. But this needed to change anyway, as the first version of this patch wrote to debug.log before anything else (and before all the newlines are added, so it appears to be part of the log from the previous run of bitcoind, except from the timestamp).

Is that better?

luke-jr · 2014-06-05T15:54:55Z

IMO we really shouldn't be changing umask at all, just opening sensitive files with the correct mode...

laanwj · 2014-06-06T18:35:32Z

@luke-jr I agree that would be much more elegant. The wallet (and associated environment files) should have the minimum possible permissions, whereas the block files and leveldb indices should be readable by anyone (or at least - as defined by the inherited umask).
A possible obstacle would be the degree of configurability that leveldb and berkeleydb give us over created files. I'm not sure for example bdb allows setting the permissions for its database env.

runeksvendsen · 2014-06-08T10:45:38Z

I agree that the best solution is to not change the umask at all, but selective create sensitive files with the appropriate permissions. And when that is implemented, the code that this patch changes should be removed as a part of it. But until it's done the proper way, this patch offers a way to get around the problems associated with ignoring system default permissions.

laanwj · 2014-06-08T10:58:25Z

src/init.cpp

-    umask(077);
+
+#ifdef ENABLE_WALLET
+    if (!GetBoolArg("-sysperms", false) || !GetBoolArg("-disablewallet", false))


For sake of explicitness you could make it a fatal error if -sysperms is specified but the wallet is enabled. I like that slightly more than ignoring the conflict.

laanwj · 2014-06-08T10:59:21Z

@runeksvendsen Agreed. This is the simplest solution for now, and offers blanket protection. It's easy to forget setting the umask for some wallet-related thing or another.

…uilds

cozz · 2014-06-08T16:07:34Z

So why not just skip umask in the disablewallet case? Why do we need -sysperms?

laanwj · 2014-06-08T16:12:51Z

src/init.cpp

+#else
+    if (!GetBoolArg("-sysperms", false))
+        umask(077);
+#endif


What about (same logic, reduces the number of GetBoolArg calls to one for each option):

if (GetBoolArg("-sysperms", false)) { #ifdef ENABLE_WALLET if (!GetBoolArg("-disablewallet", false)) return InitError("E....") #endif } else { umask(077); }

runeksvendsen · 2014-06-08T17:47:54Z

@laanwj I can do that. I thought about that as well, but I didn't do it because I thought the other approach would be clearer, and I was unsure about how an empty if-clause is handled (in case ENABLE_WALLET isn't set).

With the wallet functionality enabled, if debug.log does not exist and only -sysperms is passed (and not -disablewallet), debug.log will be created with system default permissions (ie. not 600). Not sure if this could pose a problem, but I just thought I would mention it anyway.

laanwj · 2014-06-08T19:02:10Z

@runeksvendsen empty if clauses are perfectly safe, if you surround them with braces like done here.

@cozz well this is more explicit; having the umask setting be a property of enable-wallet mode doesn't pass the principle of least surprise for me.

laanwj · 2014-06-25T10:35:25Z

src/init.cpp

@@ -210,6 +210,9 @@ std::string HelpMessage(HelpMessageMode hmm)
    strUsage += "  -pid=<file>            " + _("Specify pid file (default: bitcoind.pid)") + "\n";
    strUsage += "  -reindex               " + _("Rebuild block chain index from current blk000??.dat files") + " " + _("on startup") + "\n";
    strUsage += "  -txindex               " + _("Maintain a full transaction index (default: 0)") + "\n";
+#if !defined(WIN32)
+    strUsage += "  -sysperms              " + _("Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)") + "\n";


NIT: leave the command-line options sorted within their category

BitcoinPullTester · 2014-06-25T23:36:42Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4286_7e09b36ea5316671dfc86df62d90f78597b22fe9/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

… umask) The option is only effective for either wallet-less builds or if -disablewallet is specified as well. Rebased-By: Wladimir J. van der Laan <laanwj@gmail.com> Rebased-From: 34d5fc0 4e1a196 bd4307b d53a33b 7e09b36 Github-Pull: #4286

gmaxwell · 2014-07-14T13:05:35Z

In the future please try to get the "this is useful" text describing the motivation into the commit message. I was scratching my head at this. :)

runeksvendsen · 2014-07-17T20:31:32Z

@gmaxwell Good to know. I will do that in the future.

… umask) The option is only effective for either wallet-less builds or if -disablewallet is specified as well. Rebased-By: Wladimir J. van der Laan <laanwj@gmail.com> Rebased-From: 34d5fc0 4e1a196 bd4307b d53a33b 7e09b36 Github-Pull: bitcoin#4286 Conflicts: src/init.cpp

… umask) The option is only effective for either wallet-less builds or if -disablewallet is specified as well. Rebased-By: Wladimir J. van der Laan <laanwj@gmail.com> Rebased-From: 34d5fc0 4e1a196 bd4307b d53a33b 7e09b36 Github-Pull: bitcoin#4286 (cherry picked from commit bdd5b58) # Conflicts: # src/init.cpp

Add option to disable 077 umask (create new files with system default…

34d5fc0

… umask) The option is only effective for either wallet-less builds or if -disablewallet is specified as well.

Correcting to account for jgarzik's feedback

4e1a196

laanwj reviewed Jun 8, 2014
View reviewed changes

Make -sysperms without -disabledwallet a fatal error wallet-enabled b…

bd4307b

…uilds

laanwj reviewed Jun 8, 2014
View reviewed changes

Simplify code, as per laanwj's suggestion

d53a33b

laanwj reviewed Jun 25, 2014
View reviewed changes

leave the command-line options sorted within their category

7e09b36

laanwj merged commit 7e09b36 into bitcoin:master Jul 14, 2014

hebasto mentioned this pull request Apr 26, 2019

Wrong permissions for datadir and walletdir #15902

Closed

hebasto mentioned this pull request Oct 13, 2019

util: Set safe permissions for data directory and wallets/ subdir #17127

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to disable 077 umask (create new files with system default umask) #4286

Add option to disable 077 umask (create new files with system default umask) #4286

runeksvendsen commented Jun 4, 2014

jgarzik commented Jun 4, 2014

runeksvendsen commented Jun 5, 2014

luke-jr commented Jun 5, 2014

laanwj commented Jun 6, 2014

runeksvendsen commented Jun 8, 2014

laanwj Jun 8, 2014

laanwj commented Jun 8, 2014

cozz commented Jun 8, 2014

laanwj Jun 8, 2014

runeksvendsen commented Jun 8, 2014

laanwj commented Jun 8, 2014

laanwj Jun 25, 2014

BitcoinPullTester commented Jun 25, 2014

gmaxwell commented Jul 14, 2014

runeksvendsen commented Jul 17, 2014

Add option to disable 077 umask (create new files with system default umask) #4286

Add option to disable 077 umask (create new files with system default umask) #4286

Conversation

runeksvendsen commented Jun 4, 2014

jgarzik commented Jun 4, 2014

runeksvendsen commented Jun 5, 2014

luke-jr commented Jun 5, 2014

laanwj commented Jun 6, 2014

runeksvendsen commented Jun 8, 2014

laanwj Jun 8, 2014

Choose a reason for hiding this comment

laanwj commented Jun 8, 2014

cozz commented Jun 8, 2014

laanwj Jun 8, 2014

Choose a reason for hiding this comment

runeksvendsen commented Jun 8, 2014

laanwj commented Jun 8, 2014

laanwj Jun 25, 2014

Choose a reason for hiding this comment

BitcoinPullTester commented Jun 25, 2014

gmaxwell commented Jul 14, 2014

runeksvendsen commented Jul 17, 2014