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

runeksvendsen commented at 11:21 am on June 4, 2014: contributor

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.

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

34d5fc050d

jgarzik commented at 6:51 pm on June 4, 2014: contributor

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 commented at 10:32 am on June 5, 2014: contributor

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?

Correcting to account for jgarzik's feedback 4e1a196e91

luke-jr commented at 3:54 pm on June 5, 2014: member

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

laanwj commented at 6:35 pm on June 6, 2014: member

@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 commented at 10:45 am on June 8, 2014: contributor

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.

in src/init.cpp: in 4e1a196e91 outdated

424@@ -422,7 +425,14 @@ bool AppInit2(boost::thread_group& threadGroup)
425     }
426 #endif
427 #ifndef WIN32
428-    umask(077);
429+
430+#ifdef ENABLE_WALLET
431+    if (!GetBoolArg("-sysperms", false) || !GetBoolArg("-disablewallet", false))

laanwj commented at 10:58 am on June 8, 2014:

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 commented at 10:59 am on June 8, 2014: member

@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.

Make -sysperms without -disabledwallet a fatal error wallet-enabled builds bd4307b84b

cozz commented at 4:07 pm on June 8, 2014: contributor

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

in src/init.cpp: in bd4307b84b outdated

434+            return InitError("Error: -sysperms is not allowed in combination with enabled wallet functionality");
435+    }
436+#else
437+    if (!GetBoolArg("-sysperms", false))
438+        umask(077);
439+#endif

laanwj commented at 4:12 pm on June 8, 2014:

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

0    if (GetBoolArg("-sysperms", false)) {
1#ifdef ENABLE_WALLET
2        if (!GetBoolArg("-disablewallet", false))
3            return InitError("E....")
4#endif
5    } else {
6        umask(077);
7    }

runeksvendsen commented at 5:47 pm on June 8, 2014: contributor

@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.

Simplify code, as per laanwj's suggestion d53a33b828

laanwj commented at 7:02 pm on June 8, 2014: member

@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.

leave the command-line options sorted within their category 7e09b36ea5

in src/init.cpp: in d53a33b828 outdated

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

laanwj commented at 10:35 am on June 25, 2014:

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

BitcoinPullTester commented at 11:36 pm on June 25, 2014: none

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.

laanwj merged this on Jul 14, 2014

laanwj closed this on Jul 14, 2014

laanwj referenced this in commit 0ca7902074 on Jul 14, 2014

laanwj referenced this in commit bdd5b587fc on Jul 14, 2014

gmaxwell commented at 1:05 pm on July 14, 2014: contributor

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 commented at 8:31 pm on July 17, 2014: contributor

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

MathyV referenced this in commit d14bf45d15 on Nov 24, 2014

reddink referenced this in commit 1a093ba5d5 on May 27, 2020

MarcoFalke locked this on Sep 8, 2021

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