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

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:20191013-permissions changing 9 files +57 −15
  1. hebasto commented at 8:07 PM on October 13, 2019: member

    On master (1e7564eca8a688f39c75540877ec3bdfdde766b1) docs say:

    $ ./src/bitcoind -help | grep -A 3 sysperms
  -sysperms
       Create new files with system default permissions, instead of umask 077
       (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:

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

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

    With this PR:

    $ stat .bitcoin/wallets | grep id
Access: (0700/drwx------)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)
$ stat .bitcoin/wallets | grep id
Access: (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.

  2. fanquake added the label Utils/log/libs on Oct 13, 2019
  3. DrahtBot commented at 8:43 PM on October 13, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

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

  6. laanwj commented at 9:32 AM on October 14, 2019: member

    Concept ACK on removing -sysperms.

  7. hebasto force-pushed on Oct 14, 2019
  8. hebasto commented at 11:31 AM on October 14, 2019: member

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

  9. DrahtBot added the label Needs rebase on Oct 15, 2019
  10. hebasto force-pushed on Oct 16, 2019
  11. hebasto commented at 4:08 PM on October 16, 2019: member

    Rebased after #17138 has been merged.

  12. DrahtBot removed the label Needs rebase on Oct 16, 2019
  13. 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.

  14. hebasto force-pushed on Oct 26, 2019
  15. hebasto commented at 6:40 PM on October 26, 2019: member

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

  16. hebasto commented at 8:04 PM on November 25, 2019: member

    @promag Would you mind reviewing this PR?

  17. DrahtBot cross-referenced this on Feb 11, 2020 from issue gui: Bilingual GUI error messages by hebasto
  18. DrahtBot cross-referenced this on Feb 11, 2020 from issue Replace -upgradewallet startup option with upgradewallet RPC by achow101
  19. DrahtBot added the label Needs rebase on Apr 19, 2020
  20. hebasto force-pushed on Apr 20, 2020
  21. hebasto commented at 7:31 PM on April 20, 2020: member

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

  22. DrahtBot removed the label Needs rebase on Apr 20, 2020
  23. DrahtBot cross-referenced this on May 6, 2020 from issue rpc: prevent circular deps by extracting into separate include by brakmic
  24. DrahtBot added the label Needs rebase on May 8, 2020
  25. hebasto force-pushed on May 9, 2020
  26. hebasto commented at 3:52 AM on May 9, 2020: member

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

  27. DrahtBot removed the label Needs rebase on May 9, 2020
  28. DrahtBot cross-referenced this on May 14, 2020 from issue doc: noban precludes maxuploadtarget disconnects by maflcko
  29. jonasschnelli commented at 9:16 AM on May 29, 2020: contributor

    utACK 297e61cdb8293c0bfd1d43f22eefbece3f541802

  30. jonasschnelli cross-referenced this on Jun 5, 2020 from issue Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() by n2yen
  31. DrahtBot cross-referenced this on Jun 5, 2020 from issue interfaces: Expose settings.json methods to GUI by ryanofsky
  32. DrahtBot cross-referenced this on Jun 5, 2020 from issue Add <datadir>/settings.json persistent settings storage by ryanofsky
  33. DrahtBot cross-referenced this on Jun 5, 2020 from issue Add loadwallet and createwallet load_on_startup options by ryanofsky
  34. DrahtBot cross-referenced this on Jun 16, 2020 from issue Replace boost::filesystem with std::filesystem by kiminuo
  35. DrahtBot cross-referenced this on Jun 25, 2020 from issue Remove the automatic creation and loading of the default wallet by achow101
  36. DrahtBot cross-referenced this on Jul 21, 2020 from issue refactor: Pass ArgsManager into functions that register args by S3RK
  37. DrahtBot added the label Needs rebase on Jul 23, 2020
  38. hebasto force-pushed on Jul 24, 2020
  39. hebasto commented at 7:25 AM on July 24, 2020: member

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

  40. DrahtBot removed the label Needs rebase on Jul 24, 2020
  41. DrahtBot added the label Needs rebase on Jul 30, 2020
  42. hebasto force-pushed on Jul 31, 2020
  43. hebasto commented at 1:42 PM on July 31, 2020: member

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

  44. DrahtBot removed the label Needs rebase on Jul 31, 2020
  45. DrahtBot cross-referenced this on Jul 31, 2020 from issue feature: Added ability for users to add a startup command by benthecarman
  46. DrahtBot cross-referenced this on Aug 4, 2020 from issue wallet: Replace -zapwallettxes with zapwallettxes RPC by achow101
  47. DrahtBot cross-referenced this on Aug 6, 2020 from issue wallet: Remove -zapwallettxes by achow101
  48. DrahtBot cross-referenced this on Aug 22, 2020 from issue Remove gArgs global from init by maflcko
  49. DrahtBot added the label Needs rebase on Aug 26, 2020
  50. hebasto force-pushed on Aug 26, 2020
  51. hebasto commented at 9:33 AM on August 26, 2020: member

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

  52. DrahtBot removed the label Needs rebase on Aug 26, 2020
  53. DrahtBot added the label Needs rebase on Sep 28, 2020
  54. hebasto force-pushed on Sep 29, 2020
  55. hebasto commented at 10:34 AM on September 29, 2020: member

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

  56. DrahtBot removed the label Needs rebase on Sep 29, 2020
  57. rom-burner cross-referenced this on Oct 13, 2020 from issue Specify the uid/gid of the bitcoin user with an environment variable by LordShadowen
  58. DrahtBot cross-referenced this on Dec 22, 2020 from issue Use std::filesystem. Remove Boost Filesystem & System by fanquake
  59. practicalswift commented at 7:48 PM on June 23, 2021: contributor

    Concept ACK

  60. DrahtBot cross-referenced this on Jun 24, 2021 from issue [TESTBED][NO-MERGE][POC] Use std::filesystem. Remove Boost Filesystem & System by kiminuo
  61. DrahtBot cross-referenced this on Oct 14, 2021 from issue [POC] build: static musl libc based bitcoind by fanquake
  62. DrahtBot added the label Needs rebase on Feb 3, 2022
  63. hebasto force-pushed on Feb 5, 2022
  64. hebasto commented at 8:37 PM on February 5, 2022: member

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

  65. DrahtBot removed the label Needs rebase on Feb 5, 2022
  66. 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
  67. hebasto commented at 9:20 PM on February 5, 2022: member

    PR description has been updated.

  68. dongcarl cross-referenced this on Nov 10, 2022 from issue init: Add option for rpccookie permissions by aureleoules
  69. DrahtBot cross-referenced this on Nov 17, 2022 from issue init: Evaluate sysperms before config file by willcl-ark
  70. willcl-ark approved
  71. 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?

  72. 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?

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

  74. willcl-ark commented at 9:45 AM on December 9, 2022: contributor

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

  75. hebasto force-pushed on Dec 9, 2022
  76. hebasto commented at 1:18 PM on December 9, 2022: member

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

  77. hebasto cross-referenced this on Dec 9, 2022 from issue `make install` creates files/folders with bad permissions if custom umask is used by xanoni
  78. fanquake requested review from willcl-ark on Dec 20, 2022
  79. willcl-ark approved
  80. willcl-ark commented at 11:09 AM on January 6, 2023: contributor

    ACK 07c496d01

    Might want to add this to the release notes too?

  81. 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:

    #ifndef WIN32
    constexpr mode_t private_umask = 0077;
    umask(private_umask);
#endif

    in SetupEnvironment()?

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

    Thanks! Updated.

  82. DrahtBot cross-referenced this on Jan 10, 2023 from issue test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg
  83. 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
  84. hebasto force-pushed on Feb 5, 2023
  85. hebasto commented at 1:46 PM on February 5, 2023: member

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

  86. fanquake requested review from willcl-ark on Feb 5, 2023
  87. fanquake requested review from john-moffett on Feb 5, 2023
  88. 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.

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

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

  91. Apply default umask in `SetupEnvironment()`
    This change makes all filesystem artifacts--files and directories--being
created with the default umask.
    581f16ef34
  92. test: Add test for file system permissions c9ba4f9ecb
  93. hebasto force-pushed on Feb 6, 2023
  94. hebasto commented at 11:11 AM on February 6, 2023: member

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

  95. john-moffett commented at 4:49 PM on February 6, 2023: contributor

    ACK c9ba4f9ecb1a282d98e7456a84ca84362b161757

  96. willcl-ark approved
  97. willcl-ark commented at 7:46 PM on February 6, 2023: contributor

    ACK c9ba4f9ecb1a282d98e7456a84ca84362b161757

  98. fanquake merged this on Feb 7, 2023
  99. fanquake closed this on Feb 7, 2023
  100. hebasto deleted the branch on Feb 7, 2023
  101. sidhujag referenced this in commit 9c8d89e2e9 on Feb 7, 2023
  102. willcl-ark cross-referenced this on Mar 15, 2023 from issue Allow groups of accounts to access the RPC cookie file by alevchuk
  103. willcl-ark cross-referenced this on Dec 7, 2023 from issue /home/bitcoin/.bitcoin permissions vs. /data/bitcoin by lewisphil
  104. bitcoin locked this on Feb 7, 2024
Contributors
hebastoDrahtBotlaanwjjonasschnellipracticalswiftwillcl-arkfanquakejohn-moffett
Labels
Utils/log/libs
Linked (view graph)
#4286 Add option to disable 077 umask (create new files with system default umask)#13371 -sysperms=false (default) doesn't appear to do anything#13389 Utils and libraries: Fix #13371 - move umask operation earlier in AppInit()#15902 Wrong permissions for datadir and walletdir#17138 Remove wallet access to some node arguments#22595 `make install` creates files/folders with bad permissions if custom umask is used#25270 Allow groups of accounts to access the RPC cookie file#26088 init: Add option for rpccookie permissions
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 11:54 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me