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

c9ba4f9ecb1a282d98e7456a84ca84362b161757 test: Add test for file system permissions (Hennadii Stepanov)
581f16ef3404274cb5c1a79dd3d6ee7b584f9844 Apply default umask in `SetupEnvironment()` (Hennadii Stepanov)
8a6219e54379911605aed860519e0194f1433b72 Remove `-sysperms` option (Hennadii Stepanov)

Pull request description:

  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:
  - https://github.com/bitcoin/bitcoin/pull/13389#issuecomment-395306690
  - https://github.com/bitcoin/bitcoin/pull/13389#issuecomment-539906114
  - https://github.com/bitcoin/bitcoin/pull/13389#discussion_r279160472

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

ACKs for top commit:
  john-moffett:
    ACK c9ba4f9ecb1a282d98e7456a84ca84362b161757
  willcl-ark:
    ACK c9ba4f9ecb1a282d98e7456a84ca84362b161757

Tree-SHA512: 96c745339e6bd0e4d7bf65daf9a721e2e1945b2b0ab74ca0f66576d0dc358b5de8eb8cdb89fe2160f3b19c39d2798bb8b291784316085dc73a27102d3415bd57

9 files changed, 57 insertions, 15 deletions

diff --git a/doc/init.md b/doc/init.md
index 399b819bf4..7f79027718 100644
--- a/doc/init.md
+++ b/doc/init.md
@@ -70,7 +70,7 @@ NOTE: When using the systemd .service file, the creation of the aforementioned
 directories and the setting of their permissions is automatically handled by
 systemd. Directories are given a permission of 710, giving the bitcoin group
 access to files under it _if_ the files themselves give permission to the
-bitcoin group to do so (e.g. when `-sysperms` is specified). This does not allow
+bitcoin group to do so. This does not allow
 for the listing of files under the directory.
 
 NOTE: It is not currently possible to override `datadir` in
diff --git a/src/i2p.cpp b/src/i2p.cpp
index 586ee649a7..8e98440747 100644
--- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -336,7 +336,7 @@ void Session::GenerateAndSavePrivateKey(const Sock& sock)
 {
     DestGenerate(sock);
 
-    // umask is set to 077 in init.cpp, which is ok (unless -sysperms is given)
+    // umask is set to 0077 in util/system.cpp, which is ok.
     if (!WriteBinaryFile(m_private_key_file,
                          std::string(m_private_key.begin(), m_private_key.end()))) {
         throw std::runtime_error(
diff --git a/src/init.cpp b/src/init.cpp
index 5b486854e0..73ae36e4f7 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -459,11 +459,6 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-startupnotify=<cmd>", "Execute command on startup.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-shutdownnotify=<cmd>", "Execute command immediately before beginning shutdown. The need for shutdown may be urgent, so be careful not to delay it long (if the command doesn't require interaction with the server, consider having it fork into the background).", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 #endif
-#ifndef WIN32
-    argsman.AddArg("-sysperms", "Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-#else
-    hidden_args.emplace_back("-sysperms");
-#endif
     argsman.AddArg("-txindex", strprintf("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)", DEFAULT_TXINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-blockfilterindex=<type>",
                  strprintf("Maintain an index of compact filters by block (default: %s, values: %s).", DEFAULT_BLOCKFILTERINDEX, ListBlockFilterTypes()) +
@@ -821,10 +816,6 @@ bool AppInitBasicSetup(const ArgsManager& args)
     }
 
 #ifndef WIN32
-    if (!args.GetBoolArg("-sysperms", false)) {
-        umask(077);
-    }
-
     // Clean shutdown on SIGTERM
     registerSignalHandler(SIGTERM, HandleSIGTERM);
     registerSignalHandler(SIGINT, HandleSIGTERM);
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index 0bb5533d71..6f37fe2a99 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -86,7 +86,7 @@ bool GenerateAuthCookie(std::string *cookie_out)
     std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);
 
     /** the umask determines what permissions are used to create this file -
-     * these are set to 077 in init.cpp unless overridden with -sysperms.
+     * these are set to 0077 in util/system.cpp.
      */
     std::ofstream file;
     fs::path filepath_tmp = GetAuthCookieFile(true);
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 0cea283ece..e72c970157 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -1360,6 +1360,11 @@ void SetupEnvironment()
     SetConsoleCP(CP_UTF8);
     SetConsoleOutputCP(CP_UTF8);
 #endif
+
+#ifndef WIN32
+    constexpr mode_t private_umask = 0077;
+    umask(private_umask);
+#endif
 }
 
 bool SetupNetworking()
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 773f094274..5403e38950 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -122,9 +122,6 @@ bool WalletInit::ParameterInteraction() const
         return InitError(Untranslated("-zapwallettxes has been removed. If you are attempting to remove a stuck transaction from your wallet, please use abandontransaction instead."));
     }
 
-    if (gArgs.GetBoolArg("-sysperms", false))
-        return InitError(Untranslated("-sysperms is not allowed in combination with enabled wallet functionality"));
-
     return true;
 }
 
diff --git a/test/functional/feature_posix_fs_permissions.py b/test/functional/feature_posix_fs_permissions.py
new file mode 100755
index 0000000000..c5a543e97a
--- /dev/null
+++ b/test/functional/feature_posix_fs_permissions.py
@@ -0,0 +1,43 @@
+#!/usr/bin/env python3
+# Copyright (c) 2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test file system permissions for POSIX platforms.
+"""
+
+import os
+import stat
+
+from test_framework.test_framework import BitcoinTestFramework
+
+
+class PosixFsPermissionsTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_platform_not_posix()
+
+    def check_directory_permissions(self, dir):
+        mode = os.lstat(dir).st_mode
+        self.log.info(f"{stat.filemode(mode)} {dir}")
+        assert mode == (stat.S_IFDIR | stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
+
+    def check_file_permissions(self, file):
+        mode = os.lstat(file).st_mode
+        self.log.info(f"{stat.filemode(mode)} {file}")
+        assert mode == (stat.S_IFREG | stat.S_IRUSR | stat.S_IWUSR)
+
+    def run_test(self):
+        self.stop_node(0)
+        datadir = os.path.join(self.nodes[0].datadir, self.chain)
+        self.check_directory_permissions(datadir)
+        walletsdir = os.path.join(datadir, "wallets")
+        self.check_directory_permissions(walletsdir)
+        debuglog = os.path.join(datadir, "debug.log")
+        self.check_file_permissions(debuglog)
+
+
+if __name__ == '__main__':
+    PosixFsPermissionsTest().main()
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 6ff4e4ee54..513b795478 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -880,6 +880,11 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         if platform.system() != "Linux":
             raise SkipTest("not on a Linux system")
 
+    def skip_if_platform_not_posix(self):
+        """Skip the running test if we are not on a POSIX platform"""
+        if os.name != 'posix':
+            raise SkipTest("not on a POSIX system")
+
     def skip_if_no_bitcoind_zmq(self):
         """Skip the running test if bitcoind has not been compiled with zmq support."""
         if not self.is_zmq_compiled():
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index a301cf9184..0fe28b3855 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -211,6 +211,7 @@ BASE_SCRIPTS = [
     'p2p_addrv2_relay.py',
     'p2p_compactblocks_hb.py',
     'p2p_disconnect_ban.py',
+    'feature_posix_fs_permissions.py',
     'rpc_decodescript.py',
     'rpc_blockchain.py',
     'rpc_deprecated.py',