Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-sysperms=false (default) doesn't appear to do anything #13371

Closed
TheBlueMatt opened this issue Jun 1, 2018 · 5 comments · Fixed by #17127
Closed

-sysperms=false (default) doesn't appear to do anything #13371

TheBlueMatt opened this issue Jun 1, 2018 · 5 comments · Fixed by #17127

Comments

@TheBlueMatt
Copy link
Contributor

Despite -sysperms=false (default) docs saying files will be created with perms 077, none of my default-created datadirs appear to have any permissions aside from 0600. Maybe I'm misreading the docs but it appears umask(077) isnt doing anything.

@n2yen
Copy link

n2yen commented Jun 3, 2018

can work on this.

n2yen added a commit to n2yen/bitcoin that referenced this issue Sep 30, 2018
…cSetup() as it is too late there.

When the option -sysperms=false (default) is enabled, the created datadirs does not take into account the default umask setting of '077'.
Fix moves the umask operation as early as possible inside of bitcoind, and bitcoin-qt. This was needed because by the time AppInitBasicSetup()
is called, datadir, has typically already been created. Calling umask earlier ensures any filesystem writes that follows has the correct umask set.

Tests: feature_nosysperms.py - validates datadir permissions where -sysperms=false (default)
n2yen added a commit to n2yen/bitcoin that referenced this issue Sep 30, 2018
…ticalswift'.

  remove commented out code and styling as per PEP.
n2yen added a commit to n2yen/bitcoin that referenced this issue Sep 30, 2018
…1221'.

    - add check for windows platform, feature_nosysperms.py will explicitly skip the test if
    'Windows' platform is detected.
n2yen added a commit to n2yen/bitcoin that referenced this issue Sep 30, 2018
… to be invalid

if bitcoind is built with NO_WALLET=1. That is -sysperms will fail if
not accompanied with -disablewallet option
n2yen added a commit to n2yen/bitcoin that referenced this issue Sep 30, 2018
@hebasto
Copy link
Member

hebasto commented Apr 26, 2019

@TheBlueMatt

Despite -sysperms=false (default) docs saying files will be created with perms 077, none of my default-created datadirs appear to have any permissions aside from 0600. Maybe I'm misreading the docs but it appears umask(077) isnt doing anything.

-sysperms=false makes Bitcoin Core create files with umask = 077 rather permissions 077.
And this implies 0600 permissions for regular files and 0700 for directories.
See: Debian Reference

So, Bitcoin Core creates files as expected ;)

n2yen added a commit to n2yen/bitcoin that referenced this issue May 21, 2019
…cSetup() as it is too late there.

When the option -sysperms=false (default) is enabled, the created datadirs does not take into account the default umask setting of '077'.
Fix moves the umask operation as early as possible inside of bitcoind, and bitcoin-qt. This was needed because by the time AppInitBasicSetup()
is called, datadir, has typically already been created. Calling umask earlier ensures any filesystem writes that follows has the correct umask set.

Tests: feature_sysperms.py - validates datadir permissions where -sysperms=false (default)
Notes:
'-sysperms' option requires '-disablewallet'
- add check for windows platform, feature_nosysperms.py will explicitly skip the test if
  'Windows' platform is detected.

if bitcoind is built with NO_WALLET=1. That is -sysperms will fail if
not accompanied with -disablewallet option
n2yen added a commit to n2yen/bitcoin that referenced this issue Jul 27, 2019
…cSetup() as it is too late there.

When the option -sysperms=false (default) is enabled, the created datadirs does not take into account the default umask setting of '077'.
Fix moves the umask operation as early as possible inside of bitcoind, and bitcoin-qt. This was needed because by the time AppInitBasicSetup()
is called, datadir, has typically already been created. Calling umask earlier ensures any filesystem writes that follows has the correct umask set.

Tests: feature_sysperms.py - validates datadir permissions where -sysperms=false (default)
Notes:
'-sysperms' option requires '-disablewallet'
- add check for windows platform, feature_nosysperms.py will explicitly skip the test if
  'Windows' platform is detected.

if bitcoind is built with NO_WALLET=1. That is -sysperms will fail if
not accompanied with -disablewallet option
@niVelion
Copy link

niVelion commented Jan 12, 2022

Despite -sysperms=false (default) docs saying files will be created with perms 077

I assume you're referring to these docs:

$ bitcoind --help | grep --after-context 3 sysperms
  -sysperms
       Create new files with system default permissions, instead of umask 077
       (only effective with disabled wallet functionality)

I don't know where to find bitcoind's default values, but you mention this setting is by default false, which makes sense given what it does.

Running umask on my Debian-based distro returns my system default umask 002, meaning the system (that is, not my user) will create new files with permissions u=rwx g=rwx o=rx.

Perhaps the help text would be more readable if it said:

Create new files with system default umask, instead of umask 077.

In fact, this is what the commit message that introduced this change said!

  • bdd5b58 Add option -sysperms to disable 077 umask (create new files with system default umask)

Link to help text: https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L430

Shall I make that change @TheBlueMatt ?

@susanka068
Copy link

Hello, I'd like to tackle this issue. Is this issue still open ?

willcl-ark added a commit to willcl-ark/bitcoin that referenced this issue Nov 16, 2022
Fixes bitcoin#13371

Currently we check the config file before applying the effect of the
`-sysperms` program option. On first boot, this can result in
directories (and settings.json file) being created with incorrect
permissions.
willcl-ark added a commit to willcl-ark/bitcoin that referenced this issue Nov 16, 2022
Fixes bitcoin#13371

Currently we check the config file before applying the effect of the
`-sysperms` program option. On first boot, this can result in
directories (and settings.json file) being created with incorrect
permissions.
willcl-ark added a commit to willcl-ark/bitcoin that referenced this issue Nov 16, 2022
Fixes bitcoin#13371

Currently we check the config file before applying the effect of the
`-sysperms` program option. On first boot, this can result in
directories (and settings.json file) being created with incorrect
permissions.
@willcl-ark
Copy link
Member

willcl-ark commented Nov 16, 2022

@TheBlueMatt

Despite -sysperms=false (default) docs saying files will be created with perms 077, none of my default-created datadirs appear to have any permissions aside from 0600. Maybe I'm misreading the docs but it appears umask(077) isnt doing anything.

-sysperms=false makes Bitcoin Core create files with umask = 077 rather permissions 077. And this implies 0600 permissions for regular files and 0700 for directories. See: Debian Reference

So, Bitcoin Core creates files as expected ;)

Just to check:

System: Ubuntu 22.04
umask: 002 (OS default)

I've tested creating a new datadir on master with default bitcoind settings (i.e. without setting -sysperms and with wallet enabled): bitcoind -signet. We expect bitcoin to use umask 077 by default.

For directories expect: 777 - 077 = 700 = rwx------
For files expect:      666 - 077 = 600 = rw-------

It created the following:

will@ubuntu in /tmp
❯ exa -alR .bitcoin
drwxrwxr-x - will 15 Nov 14:22 signet
drwxrwxr-x - will 15 Nov 14:22 wallets

.bitcoin/signet:
.rw-------    0 will 15 Nov 14:22 .lock
.rw-------   37 will 15 Nov 14:22 anchors.dat
.rw-------   31 will 15 Nov 14:22 banlist.json
drwx------    - will 15 Nov 14:22 blocks
drwx------    - will 15 Nov 14:22 chainstate
.rw------- 7.0k will 15 Nov 14:22 debug.log
.rw------- 248k will 15 Nov 14:22 fee_estimates.dat
.rw-------   18 will 15 Nov 14:22 mempool.dat
.rw-------   99 will 15 Nov 14:22 onion_v3_private_key
.rw-------  11k will 15 Nov 14:22 peers.dat
.rw-rw-r--    4 will 15 Nov 14:22 settings.json
drwxrwxr-x    - will 15 Nov 14:22 wallets

It seems to have used the system umask to create .bitcoin/, signet/, signet/wallets/ and top level wallets/ directories (777-002=775), but not blocks/ and chainstate/ directories. It has also used system umask to create the settings.json file.

This happens because ReadConfigFiles() is called before AppInitBasicSetup() (which sets the umask) and ReadConfigFiles will internally call GetConfigFile() which in turn calls AbsPathForConfigVal(), where calling either of GetDataDirNet() or GetDataDirBase() will init the base file structure before the -sysperms flag has been parsed.


Starting with bitcoind -signet -sysperms -disablewallet:

For directories expect: 777 - 002 = 775 = rwxrwxr-x
For files expect:      666 - 002 = 664 = rw-rw-r--

Which creates the following:

will@ubuntu in /tmp
❯ exa -al -R .bitcoin_sysperms/
drwxrwxr-x - will 15 Nov 16:18 signet
drwxrwxr-x - will 15 Nov 16:17 wallets

.bitcoin_sysperms/signet:
.rw-rw-r--    0 will 15 Nov 16:18 .lock
.rw-rw-r--   37 will 15 Nov 16:18 anchors.dat
.rw-rw-r--   31 will 15 Nov 16:18 banlist.json
drwxrwxr-x    - will 15 Nov 16:18 blocks
drwxrwxr-x    - will 15 Nov 16:18 chainstate
.rw-rw-r-- 6.5k will 15 Nov 16:18 debug.log
.rw-rw-r-- 248k will 15 Nov 16:18 fee_estimates.dat
.rw-rw-r--   18 will 15 Nov 16:18 mempool.dat
.rw-rw-r--   99 will 15 Nov 16:18 onion_v3_private_key
.rw-rw-r--  12k will 15 Nov 16:18 peers.dat
.rw-rw-r--    4 will 15 Nov 16:18 settings.json
drwxrwxr-x    - will 15 Nov 16:17 wallets

I have a PR which results in the following (correct) filestructure when started without the -sysperms flag:

will@ubuntu in /tmp/13371
❯ exa -al -R
drwx------ - will 16 Nov 13:47 .bitcoin

./.bitcoin:
drwx------ - will 16 Nov 13:47 signet
drwx------ - will 16 Nov 13:47 wallets

./.bitcoin/signet:
.rw-------    0 will 16 Nov 13:47 .lock
.rw-------   37 will 16 Nov 13:47 anchors.dat
.rw-------   31 will 16 Nov 13:47 banlist.json
drwx------    - will 16 Nov 13:47 blocks
drwx------    - will 16 Nov 13:47 chainstate
.rw------- 7.1k will 16 Nov 13:47 debug.log
.rw------- 248k will 16 Nov 13:47 fee_estimates.dat
.rw-------   18 will 16 Nov 13:47 mempool.dat
.rw-------   99 will 16 Nov 13:47 onion_v3_private_key
.rw-------  16k will 16 Nov 13:47 peers.dat
.rw-------    4 will 16 Nov 13:47 settings.json
drwx------    - will 16 Nov 13:47 wallets

And with -sysperms -disablewallet:

will@ubuntu in /tmp/13371
❯ exa -al -R
drwxrwxr-x - will 16 Nov 13:54 .bitcoin_sysperms

./.bitcoin_sysperms:
drwxrwxr-x - will 16 Nov 13:55 signet
drwxrwxr-x - will 16 Nov 13:54 wallets

./.bitcoin_sysperms/signet:
.rw-rw-r--    0 will 16 Nov 13:55 .lock
.rw-rw-r--   37 will 16 Nov 13:55 anchors.dat
.rw-rw-r--   31 will 16 Nov 13:55 banlist.json
drwxrwxr-x    - will 16 Nov 13:55 blocks
drwxrwxr-x    - will 16 Nov 13:55 chainstate
.rw-rw-r-- 7.0k will 16 Nov 13:55 debug.log
.rw-rw-r-- 248k will 16 Nov 13:55 fee_estimates.dat
.rw-rw-r--   18 will 16 Nov 13:55 mempool.dat
.rw-rw-r--   99 will 16 Nov 13:55 onion_v3_private_key
.rw-rw-r--  14k will 16 Nov 13:55 peers.dat
.rw-rw-r--    4 will 16 Nov 13:55 settings.json
drwxrwxr-x    - will 16 Nov 13:54 wallets

I would be interested in more feedback on whether such a change is desirable because changing the default file permissions could possibly break things upstream for other projects?

@bitcoin bitcoin locked and limited conversation to collaborators Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants