Make logfile's mode configurable.

[Cybso]

The file logger currently resets the mode of the logfile to 0640.

When the webserver is running as a different user than the cron job
(but both are in the same group) the files mode has to be 0660. The
current implementation breaks logging for the user that is not the
owner of the logfile.

This patch introduces a new config option 'logfilemode' that expects
an octal value (defaults to 0640). Unless the value is lower or equal
than 0 the logfile's mode will be changed to this value.

Signed-off-by: Roland Tapken roland@bitarbeiter.net

[juliusknorr]

Thanks for your contribution @Cybso. Makes sense to me to have an option for this. 👍

[MorrisJobke]

Technically we don’t support that Cron and Webserver is run by different users. Our documentation states this already because it could then cause the files in the data directory having the wrong permission. The same as what happened here. And also adding just more settings to „fix“ this flaw is also something we should avoid to not fragment the different setups and make it even harder why something happened.

[Cybso]

@MorrisJobke I understand your point here. However, in that case the detection of the webserver user is defective, because I simply cannot execute cron.php as www-data if the nextcloud installation (precisly: config.php) is owned by another user than www-data:

# sudo -u www-data php cron.php
Console has to be executed with the same user as the web server is operated
Current user: www-data
Web server user: nextcloud
# ps aux |grep apache2
root      2752  0.0  1.3 706732 53884 ?        Ss   Feb09   0:03 /usr/sbin/apache2 -k start
www-data  3205  0.0  1.2 729496 50124 ?        S    00:05   0:18 /usr/sbin/apache2 -k start
www-data  3206  0.0  1.3 729416 52700 ?        S    00:05   0:19 /usr/sbin/apache2 -k start
www-data  3207  0.0  1.2 729496 50128 ?        S    00:05   0:18 /usr/sbin/apache2 -k start
www-data  3208  0.0  1.2 729496 50124 ?        S    00:05   0:18 /usr/sbin/apache2 -k start
www-data  3209  0.0  1.2 729496 50132 ?        S    00:05   0:18 /usr/sbin/apache2 -k start
www-data  3262  0.0  1.2 729496 50128 ?        S    00:06   0:18 /usr/sbin/apache2 -k start
root      3556  0.0  0.0  12784  1020 pts/0    S+   15:39   0:00 grep apache2
# ls -l config/config.php
-rw-r--r-- 1 nextcloud www-data 1471 Feb  9 17:50 config/config.php

config.php is not owned (and not even writable) by the webserver user in my setup for security reasons(*). So maybe a better fix would be an option to suppress this check when being executed via CLI. Nextcloud supports non-writable config files via config_is_read_only = true, so I think for completeness it should also support that this file is not owned by www-data and that cron.php might not be able to detect the webserver user for sure.

I'll workaround this by executing the cron job via an HTTP request in future. But I think this problem should be discussed further.

(*): The security policy of my company does not allow any executable or includable files within the document root to be writable by the webserver when not absolutly needed (e.g. while doing updates). This could open an attack vector where a code injection exploit could be used to append PHP code to, lets say, config.php that sends login data back to the attackers server - and thus receive admin credentials in clear text.

[MorrisJobke]

config.php is not owned (and not even writable) by the webserver user in my setup for security reasons(*).

Sadly we do not allow this setup. Because we need to write the version number to this and when you want to upgrade via the web the config.php needs to be writable.

Nextcloud supports non-writable config files via config_is_read_only = true, so I think for completeness it should also support that this file is not owned by www-data and that cron.php might not be able to detect the webserver user for sure.

Have you enabled this setting? I thought we then don't check the config.php permissions.

(*): The security policy of my company does not allow any executable or includable files within the document root to be writable by the webserver when not absolutly needed (e.g. while doing updates). This could open an attack vector where a code injection exploit could be used to append PHP code to, lets say, config.php that sends login data back to the attackers server - and thus receive admin credentials in clear text.

We are aware of this and plan to fix this by putting the config into another file format and just parse this then But it is sadly a long road to go.

[Cybso]

Sadly we do not allow this setup. Because we need to write the version number to this and when you want to upgrade via the web the config.php needs to be writable.

I know, the ownership of the installation is changed to 'www-data' before updates or app installations and secured again when the changes are finished. No problem so far (besides cron.php)...

Have you enabled this setting? I thought we then don't check the config.php permissions.

The setting is ignored in cron.php. But maybe the smartest solution to this problems would be to check for this setting and skip the process owner check if it has been enabled. Should I commit it into this PR or create a new branch for that change?

if (!$config->getValue('config_is_read_only', false)) {
   // current behaviour
} else {
    \OCP\Util::writeLog('cron', 'The process owner check is skipped (config_is_read_only=true)', \OCP\Util::INFO);
}

[ChristophWurst]

This branch has conflicts that must be resolved

Besides this, @Cybso, is this change still relevant? From the comments it sounds like a fix for a very specific setup that we actually don't support.

[Cybso]

@ChristophWurst Although this particular case is likely to be quite an exception, I still think the patch should be merged. Because if the configuration is read-only, it does not matter to cron.php who owns the config.php.

[Cybso]

Oh wait, I spoke of the patch in ded52ab

The original patch is obsolete.

[ChristophWurst]

cron.php who owns the config.php.

Yes, it does. Because cron jobs might want to change system configuration and thus they have to be able to write to that file.

[Cybso]

@ChristophWurst cron.php should not try to change the system configuration if 'config_is_read_only' is set

'config_is_read_only' => false,
In certain environments it is desired to have a read-only configuration file.

When this switch is set to true Nextcloud will not verify whether the configuration is writable. However, it will not be possible to configure all options via the Web interface. Furthermore, when updating Nextcloud it is required to make the configuration file writable again for the update process.

[ChristophWurst]

@ChristophWurst cron.php should not try to change the system configuration if 'config_is_read_only' is set

Oh, I'm sorry, I totally missed that part.

The setting is ignored in cron.php. But maybe the smartest solution to this problems would be to check for this setting and skip the process owner check if it has been enabled. Should I commit it into this PR or create a new branch for that change?

Then this sounds like the more sane way to go where we just fix the behavior of our existing configuration options instead of adding a new one 👍

[MorrisJobke]

Oh wait, I spoke of the patch in ded52ab

The problem with this is, that the cron job could now be run by a user who cannot read the config file as well. Also this would allow to run cron with a user that has more permissions than the web server user and if this cron jobs writes a file to the file system it will be unwritable by the web server. :/ So there needs to be then another way to verify the owner. Maybe the nextcloud log file? But what if logging to the systemd journal is configured. 🙈

[Cybso]

@MorrisJobke I would only check if the config is readable and if the cron job isn't run by root.

If the admin decides to run the job by a non-root user that is not www-data but is able to write into the data directory I think one can safely assume the user knows what he's doing.

[MorrisJobke]

If the admin decides to run the job by a non-root user that is not www-data but is able to write into the data directory I think one can safely assume the user knows what he's doing.

We thought so in the past as well, but we got teached that some assumptions can't be made. On the other side it should still be safe as it is "read-only config + non-root user", which is already quite good failsafe.

[kesselb]

When the default is 640 i would prefer to use this here as well.

[MorrisJobke]

I updated this ;)

[MorrisJobke]

I rebased and fixed the merge conflict.

[codecov]

Codecov Report

Merging #8282 into master will decrease coverage by 1.76%.
The diff coverage is 75%.

@@             Coverage Diff              @@
##             master    #8282      +/-   ##
============================================
- Coverage     53.45%   51.69%   -1.77%     
+ Complexity    25555    25386     -169     
============================================
  Files          1594     1599       +5     
  Lines         96259    95104    -1155     
  Branches       1343     1376      +33     
============================================
- Hits          51459    49164    -2295     
- Misses        44800    45940    +1140

Impacted Files	Coverage Δ	Complexity Δ
config/config.sample.php	0% <0%> (ø)	0 <0> (ø)	⬇️
lib/private/Log/File.php	56.52% <100%> (-2.37%)	31 <0> (-1)
...are/Security/Exceptions/AppNotEnabledException.php	0% <0%> (-100%)	1% <0%> (ø)
apps/workflowengine/lib/Check/RequestUserAgent.php	0% <0%> (-96.43%)	10% <0%> (-4%)
.../twofactor_backupcodes/lib/Db/BackupCodeMapper.php	10% <0%> (-90%)	5% <0%> (ø)
core/Command/Maintenance/Mode.php	0% <0%> (-83.79%)	6% <0%> (-2%)
lib/private/Preview/GeneratorHelper.php	0% <0%> (-80%)	5% <0%> (ø)
apps/dav/lib/CalDAV/CalendarRoot.php	0% <0%> (-75%)	1% <0%> (-3%)
apps/comments/lib/Activity/Listener.php	23.52% <0%> (-56.87%)	10% <0%> (ø)
lib/private/PreviewManager.php	3.79% <0%> (-49.96%)	49% <0%> (ø)
... and 870 more

[MorrisJobke]

Let me also fix the tests and then this can go in.