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

hebasto commented at 8:07 pm on October 13, 2019: member

On master (1e7564eca8a688f39c75540877ec3bdfdde766b1) docs say:

0$ ./src/bitcoind -help | grep -A 3 sysperms
1  -sysperms
2       Create new files with system default permissions, instead of umask 077
3       (only effective with disabled wallet functionality)

Basing on that, one could expect that running bitcoind first time will create data directory and wallets/ subdirectory with safe 0700 permissions.

But that is not the case:

0$ stat .bitcoin | grep id
1Access: (0775/drwxrwxr-x)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)
2$ stat .bitcoin/wallets | grep id
3Access: (0775/drwxrwxr-x)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)

Both directories, in fact, are created with system default permissions.

With this PR:

0$ stat .bitcoin/wallets | grep id
1Access: (0700/drwx------)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)
2$ stat .bitcoin/wallets | grep id
3Access: (0700/drwx------)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)

This PR:

is alternative to bitcoin/bitcoin#13389
fixes bitcoin/bitcoin#15902
fixes bitcoin/bitcoin#22595
closes bitcoin/bitcoin#13371
reverts bitcoin/bitcoin#4286

Changes in behavior: removed -sysperms command-line argument / configure option. The related discussions are here:

If users rely on non-default access permissions, they could use chmod.

fanquake added the label Utils/log/libs on Oct 13, 2019

DrahtBot commented at 8:43 pm on October 13, 2019: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	john-moffett, willcl-ark
Concept ACK	laanwj, practicalswift
Stale ACK	jonasschnelli

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/util/system.cpp:753 in ddd3cd30cb outdated

749@@ -741,6 +750,10 @@ const fs::path &GetBlocksDir()
750 
751 const fs::path &GetDataDir(bool fNetSpecific)
752 {
753+#ifndef WIN32

laanwj commented at 9:31 am on October 14, 2019:

GetDataDir, a getter, is not the place to do things such as set the umask. Please don’t add these kind of side-effects.

hebasto commented at 9:40 am on October 14, 2019:

Maybe GetDataDir is not a proper name for this function as it already has an intended side-effect: https://github.com/bitcoin/bitcoin/blob/ddd3cd30cb050fe9b8738778beecba1d8bd50d52/src/util/system.cpp#L777-L780

laanwj commented at 9:41 am on October 14, 2019:

I know. I kind of want to get rid of that too. But don’t add any more

(edit: also I think setting umask is the worst kind of side effect possible, it’s global state of the program over all threads)

hebasto commented at 11:31 am on October 14, 2019:

Done.

in src/util/system.cpp:69 in ddd3cd30cb outdated

64@@ -65,6 +65,15 @@
65 
66 #include <thread>
67 
68+#ifndef WIN32
69+static const int private_umask = 077;

laanwj commented at 9:32 am on October 14, 2019:

please use mode_t instead of int here, that’s the argument type for umasks

hebasto commented at 11:31 am on October 14, 2019:

Done.

laanwj commented at 9:32 am on October 14, 2019: member

Concept ACK on removing -sysperms.

hebasto force-pushed on Oct 14, 2019

hebasto commented at 11:31 am on October 14, 2019: member

@laanwj Thank you for your review. All your comments have been addressed.

DrahtBot added the label Needs rebase on Oct 15, 2019

hebasto force-pushed on Oct 16, 2019

hebasto commented at 4:08 pm on October 16, 2019: member

Rebased after #17138 has been merged.

DrahtBot removed the label Needs rebase on Oct 16, 2019

in src/util/system.h:39 in 005649fcaf outdated

34@@ -35,6 +35,10 @@
35 
36 #include <boost/thread/condition_variable.hpp> // for boost::thread_interrupted
37 
38+#ifndef WIN32
39+void SetDefaultUmask();

laanwj commented at 3:13 pm on October 21, 2019:

Wouldn’t SetupEnvironment be the right place for this? Or is that too early?

hebasto commented at 4:03 pm on October 21, 2019:

Currently, SetDefaultUmask() is called just before the first possible disk write.

SetupEnvironment() seems too broad, as it is called in utilities too. And, yes, I think it is too early. But I could be wrong ;)

laanwj commented at 10:55 am on October 23, 2019:

But does that matter? What would be the consequences of setting it too early? Why wouldn’t we want to set the umask for the utilties?

hebasto commented at 6:39 pm on October 26, 2019:

Wouldn’t SetupEnvironment be the right place for this?

Agree.

hebasto force-pushed on Oct 26, 2019

hebasto commented at 6:40 pm on October 26, 2019: member

@laanwj Thank you for review. Your comment has been addressed.

hebasto commented at 8:04 pm on November 25, 2019: member

@promag Would you mind reviewing this PR?

DrahtBot added the label Needs rebase on Apr 19, 2020

hebasto force-pushed on Apr 20, 2020

hebasto commented at 7:31 pm on April 20, 2020: member

Rebased 7213e8cbe685556365021176d686f45bdd036dae -> 67b12091c78b0183aa9dc451d176b16f832d93f7 (pr17127.04 -> pr17127.05) due to the conflict with #15761.

DrahtBot removed the label Needs rebase on Apr 20, 2020

DrahtBot added the label Needs rebase on May 8, 2020

hebasto force-pushed on May 9, 2020

hebasto commented at 3:52 am on May 9, 2020: member

Rebased 67b12091c78b0183aa9dc451d176b16f832d93f7 -> 297e61cdb8293c0bfd1d43f22eefbece3f541802 (pr17127.05 -> pr17127.06) due to the conflict with #16224.

DrahtBot removed the label Needs rebase on May 9, 2020

jonasschnelli commented at 9:16 am on May 29, 2020: contributor

utACK 297e61cdb8293c0bfd1d43f22eefbece3f541802

DrahtBot added the label Needs rebase on Jul 23, 2020

hebasto force-pushed on Jul 24, 2020

hebasto commented at 7:25 am on July 24, 2020: member

Rebased 297e61cdb8293c0bfd1d43f22eefbece3f541802 -> 337a7709502f562b764cce78f6de1d1a744cebd8 (pr17127.06 -> pr17127.07) due to the conflict with #15935.

DrahtBot removed the label Needs rebase on Jul 24, 2020

DrahtBot added the label Needs rebase on Jul 30, 2020

hebasto force-pushed on Jul 31, 2020

hebasto commented at 1:42 pm on July 31, 2020: member

Rebased 337a7709502f562b764cce78f6de1d1a744cebd8 -> 788143749f7a71d0b3af12275e2fde7dc74ff03d (pr17127.07 -> pr17127.08) due to the conflict with #19561.

DrahtBot removed the label Needs rebase on Jul 31, 2020

DrahtBot added the label Needs rebase on Aug 26, 2020

hebasto force-pushed on Aug 26, 2020

hebasto commented at 9:33 am on August 26, 2020: member

Rebased 788143749f7a71d0b3af12275e2fde7dc74ff03d -> 677617651e5b831da8d12f769a47d2957400d987 (pr17127.08 -> pr17127.09) due to the conflict with #19779.

DrahtBot removed the label Needs rebase on Aug 26, 2020

DrahtBot added the label Needs rebase on Sep 28, 2020

hebasto force-pushed on Sep 29, 2020

hebasto commented at 10:34 am on September 29, 2020: member

Rebased 677617651e5b831da8d12f769a47d2957400d987 -> ef8ed07715f90bc156a4d4015ea1db9ee975963d (pr17127.09 -> pr17127.10) due to the conflict with #15367.

DrahtBot removed the label Needs rebase on Sep 29, 2020

practicalswift commented at 7:48 pm on June 23, 2021: contributor

Concept ACK

DrahtBot added the label Needs rebase on Feb 3, 2022

hebasto force-pushed on Feb 5, 2022

hebasto commented at 8:37 pm on February 5, 2022: member

Rebased ef8ed07715f90bc156a4d4015ea1db9ee975963d -> 9b105d7289a793eaa30ae8fe7937b1356969758b (pr17127.10 -> pr17127.11) due to the conflict with #20744.

DrahtBot removed the label Needs rebase on Feb 5, 2022

hebasto renamed this:
~~util: Correct permissions for datadir and wallets subdir~~
util: Set safe permissions for data directory and `wallets/` subdir
on Feb 5, 2022

hebasto commented at 9:20 pm on February 5, 2022: member

PR description has been updated.

willcl-ark approved

willcl-ark commented at 8:51 am on November 17, 2022: contributor

ACK 9b105d7289a793eaa30ae8fe7937b1356969758b

The test datadir will have the following files created in it after node shutdown:

banlist.json, debug.log, fee_estimates.dat, mempool.dat, peers.dat, settings.json

Would it be worth adding a permissions test for e.g debug.log to check that the correct umask is also being applied to files?

hebasto commented at 3:19 pm on November 30, 2022: member

@willcl-ark

The test datadir will have the following files created in it after node shutdown:

banlist.json, debug.log, fee_estimates.dat, mempool.dat, peers.dat, settings.json

Would it be worth adding a permissions test for e.g debug.log to check that the correct umask is also being applied to files?

Sorry, but I didn’t quite get your suggestion. Mind elaborating it?

willcl-ark commented at 4:11 pm on November 30, 2022: contributor

Sorry, but I didn’t quite get your suggestion. Mind elaborating it?

My fault for not being clearer!

You added a test for directory permissions, but not not one for files. As files and folders will be created with different permissions, it might make sense to add this at the same time?

For directories the test covers datadir and walletsdir. For files I was suggesting we could test debug.log.

willcl-ark commented at 9:45 am on December 9, 2022: contributor

This also fixes https://github.com/bitcoin/bitcoin/issues/22595

hebasto force-pushed on Dec 9, 2022

hebasto commented at 1:18 pm on December 9, 2022: member

Updated 9b105d7289a793eaa30ae8fe7937b1356969758b -> 07c496d01b7ab03451b1d8278cef2652540ff191 (pr17127.11 -> pr17127.12):

rebased
addressed @willcl-ark’s comment

fanquake requested review from willcl-ark on Dec 20, 2022

willcl-ark approved

willcl-ark commented at 11:09 am on January 6, 2023: contributor

ACK 07c496d01

Might want to add this to the release notes too?

Remove `-sysperms` option

This change effectively reverts commits from
https://github.com/bitcoin/bitcoin/pull/4286.

Users, who rely on non-default access permissions, should use `chmod`
command.

8a6219e543

in src/util/system.cpp:69 in 07c496d01b outdated

63@@ -64,6 +64,16 @@
64 #include <thread>
65 #include <typeinfo>
66 
67+#ifndef WIN32
68+namespace {
69+constexpr mode_t private_umask = 0077;

fanquake commented at 11:23 am on January 6, 2023:

Is there a reason to add 10 lines here, and another #ifndef, instead of putting the 2 lines of code where they are actually used? i.e why not just:

0#ifndef WIN32
1    constexpr mode_t private_umask = 0077;
2    umask(private_umask);
3#endif

in SetupEnvironment()?

hebasto commented at 1:46 pm on February 5, 2023:

Thanks! Updated.

hebasto force-pushed on Feb 5, 2023

hebasto commented at 1:46 pm on February 5, 2023: member

Updated 07c496d01b7ab03451b1d8278cef2652540ff191 -> 15c7105a2ddc720ed0cac0fe7995755cad437880 (pr17127.12 -> pr17127.13):

rebased
addressed @fanquake’s comment

fanquake requested review from willcl-ark on Feb 5, 2023

fanquake requested review from john-moffett on Feb 5, 2023

in src/i2p.cpp:339 in 15c7105a2d outdated

335@@ -336,7 +336,7 @@ void Session::GenerateAndSavePrivateKey(const Sock& sock)
336 {
337     DestGenerate(sock);
338 
339-    // umask is set to 077 in init.cpp, which is ok (unless -sysperms is given)
340+    // umask is set to 077 in init.cpp, which is ok.

john-moffett commented at 3:48 pm on February 5, 2023:

In system.cpp now, right?

hebasto commented at 11:12 am on February 6, 2023:

Thanks! Updated.

in src/rpc/request.cpp:89 in 15c7105a2d outdated

85@@ -86,7 +86,7 @@ bool GenerateAuthCookie(std::string *cookie_out)
86     std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);
87 
88     /** the umask determines what permissions are used to create this file -
89-     * these are set to 077 in init.cpp unless overridden with -sysperms.
90+     * these are set to 077 in init.cpp.

john-moffett commented at 3:55 pm on February 5, 2023:

system.cpp

hebasto commented at 11:12 am on February 6, 2023:

Thanks! Updated.

john-moffett commented at 5:54 pm on February 5, 2023: contributor

Approach ACK 15c7105a2ddc720ed0cac0fe7995755cad437880

I agree that this may warrant an explicit release note considering a command line option is being dropped.

Apply default umask in `SetupEnvironment()`

This change makes all filesystem artifacts--files and directories--being
created with the default umask.

581f16ef34

test: Add test for file system permissions c9ba4f9ecb

hebasto force-pushed on Feb 6, 2023

hebasto commented at 11:11 am on February 6, 2023: member

Updated 15c7105a2ddc720ed0cac0fe7995755cad437880 -> c9ba4f9ecb1a282d98e7456a84ca84362b161757 (pr17127.13 -> pr17127.14, diff):

addressed @john-moffett’s comments

john-moffett commented at 4:49 pm on February 6, 2023: contributor

ACK c9ba4f9ecb1a282d98e7456a84ca84362b161757

willcl-ark approved

willcl-ark commented at 7:46 pm on February 6, 2023: contributor

ACK c9ba4f9ecb1a282d98e7456a84ca84362b161757

fanquake merged this on Feb 7, 2023

fanquake closed this on Feb 7, 2023

hebasto deleted the branch on Feb 7, 2023

sidhujag referenced this in commit 9c8d89e2e9 on Feb 7, 2023

bitcoin locked this on Feb 7, 2024

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

Reviews

Conflicts

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