Server: Exclude file name by patterns; required for Gentoo webapp integration

[LeCoyote]

Gentoo's webapp-config generates files in the nextcloud installation folder that causes the integrity check to fail. This issue was addressed in owncloud by allowing the exclusion of file name patterns. This is done in this commit: https://github.com/owncloud/core/pull/26782/commits/bf72f47c34b6e2579cfcbdcf4a582e42fd67c181
The patch applies nicely to nextcloud (changing the string "owncloud" to "nextcloud" obviously) and solves the problem.

[LeCoyote]

Pull request created: https://github.com/nextcloud/server/pull/3113

[stefangweichinger]

I hit this issue as well right now and successfully applied the mentioned patch (before even noticing this PR). "Voting" for and bumping the PR this way ;-)

[Anrock]

As a workaround, i've added 'integrity.check.disabled' => true to config/config.php

[okias]

I think it's "higher" priority enhancement, because it's really annoying + disabling integrity check doesn't sound like right thing to do.

Please guys, merge this commit :)

[okias]

Technical information
=====================
The following list covers which files have failed the integrity check. Please read
the previous linked documentation to learn more about the errors and how to fix
them.

Results
=======
- core
    - EXTRA_FILE
        - .webapp-nextcloud-12.0.2

Raw output
==========
Array
(
    [core] => Array
        (
            [EXTRA_FILE] => Array
                (
                    [.webapp-nextcloud-12.0.2] => Array
                        (
                            [expected] => 
                            [current] => 
                        )

                )

        )

)

This bug is still valid. Could be please assigned to fulfill integrity check under Gentoo servers?

Also this doesn't seems to be enhancement, but more like serious bug, since integrity check warning is completly useless on Gentoo systems (since it's triggered all the time).

[okias]

Could someone _please_ remove label enhancement and mark it as a _regular_ issue?

[okias]

small remainder, about broken Nextcloud integrity check on Gentoo systems.

[svoop]

NC13 is out – yet no fix. Please consider this:

The Gentoo Wiki suggests you disable the integrity check. You don't want people to do this.

However, if you don't disable it, when you're hacked, you won't realize it because you're used to always see the warning and don't check the corrupted files list never ever.

No matter what you do, the security check feature is broken on Gentoo. And worse, when NC servers are hacked, NC will take the blame, not Gentoo.

Please add the .webapp-nextcloud-* files to the manifest!

[okias]

@nickvergessen Hello Nick, I wonder why you consider this issue to be enhancement. This is clearly crippling security feature for Gentoo users. I'm aware that Gentoo is not so widely used by community as Ubuntu, but still it's used by many advanced users or system administrators and people focused on security.

Can you give us information, when someone looks at this issue?

[jdunn0]

I wonder why you consider this issue to be enhancement.

The reason that pull request #3113 is an enhancement is because it enhances the Integrity Check function with the ability to exclude file names that meet a specific pattern.
At the same time this pull request will enable users using webapp-config to use the Integrity Check feature which as you mentioned will enhance security for Gentoo users (and users of other Gentoo based systems that also use webapp-config).

As another user of Gentoo's webapp-config with the same issue, I decided to look at the Integrity Check function code and I don't think the enhancement in this pull request is needed as webapp-config will only create the two files .webapp and .webapp-nextcloud-[installedversion] and the existing code can be updated to check for just those 2 files without pattern matching.

If you check for the file by using a fixed string plus a variable value by uing '.webapp-nextcloud-'.$OC_VersionString instead of a pattern matching strong of '.webapp-nextcloud-*' in the $excludedFilenames array then existing code will work just fine.

The changes required in lib/private/IntegrityCheck/Iterator/ExcludeFileByNameFilterIterator.php to do this are:
1. Near the top of the file add require OC::$SERVERROOT . '/version.php'; so that the $OC_VersionString variable will be available.
2. Replace the line excluding .webapp in the $excludedFilenames array with these 2 lines:
'.webapp', // webapp-config uses this to identify an installed webapp
'.webapp-nextcloud-'.$OC_VersionString, // webapp-config uses this to identify a specific version of an installed webapp

Note: I replaced the .webapp line to update the the comment after it to be more descriptive of what webapp-config does and to remove the mention of any specific *nix distributions as it's not important to list the specific distributions that use webapp-config, similar to how the entry for '.directory' just says 'Dolphin (KDE)' but doesn't list every distribution that includes Dolphin and/or KDE.

I have not actually created a patch file yet or tested it on my Gentoo system but it should work.
The only potential problems I can see with the above code is that:
1. There might be an issue if the '.webapp-nextcloud-[oldversion]' file still exists while an upgrade install is in progress.
An upgrade install with webapp-config is usually quick enough that it would only matter if NextCloud was accessed during the short period of time (about a minute or so) the upgrade process is running.
2. This will fail to work if the version installed by webapp-config does not match the version contained in the version.php.
This should not happen with any of the ebuilds in the gentoo repository at the moment.
Those are nextcloud-13.0.0, nextcloud-12.0.5, nextcloud-12.0.4 and nextcloud-11.0.6.
I have only used nextcloud-13.0.0 but I assume the other versions install a version.php file with the same version number.
Unless a Gentoo specific version of NextCould was created with a name like nextcloud-13.0.0-r1 then the version number would always match.
I can't think of a reason that a Gentoo specific version would be created for NextCloud though as it's a self contained application that doesn't need any modifications to work in a specific distribution.

These issues can be handled though if the Integrity Check function is updated to read the .webapp to use the WEB_PVR value as the version when checking for the '.webapp-nextcloud-[installedversion]' file.

This could be done by adding a new function to check for a .webapp file in the application root directory before the $excludedFilenames array is checked and if the '.webapp' file exists, use a function like parse_ini_file to read the file and put it's values in a temporary array called $webappconfig (parse_ini_file won't work in PHP7 for this purpose so a different function would be needed though) then add two entries to $excludedFilenames array of .webapp and '.webapp-nextcloud-'.$webappconfig['WEB_PVR'] and then the function which checks the $excludedFilenames array will know to exclude those files.
If the new function doesn't see a .webapp file in the root directory then it just skips that part entirely and goes right to the regular behavior of skipping the files in the $excludedFilenames array.

That new function would be a bit more complicated but it would resolve the issue of ebuild versions not matching the exact ones in version.php and not require the including of the version.php file in the ExcludeFileByNameFilterIterator.php file.

It wouldn't that hard to write the code to do so, if the NextCloud developers would prefer not to add a dependency on version.php.
If so then I will create a patch that adds this function or I can create a patch based on the code listed earlier in my comment.
Either way is fine with me but I'm not really that familiar with the NextCloud code (Or even the OwnCloud code) so there could be issues with either method that I have not considered.
So I don't plan to create a patch and pull request until I know what the NextCloud developers would prefer.
The last thing I want to do, is create an unnecessary dependency on version.php if the version can be obtained a different way or accidentally create a potential security issue with a new function that has to parse a .webapp file if it exists to work (There may already be better function then parse_ini_file available that will handle comments with a Hash Symbol prefix, that read configuration data in the format KEY="VALUE" but I'm not sure what that function would be if it's available).
Either way, I think those 2 methods should be more secure then the pattern matching function in pull request #3113 as they only exclude the two files created instead of excluding all files that start with '.webapp-nextcloud-'.

Also, a comment in #3113 mentions NextCloud's built-in updater but that updater will not work when NextCloud is installed via webapp-config because it installs all files owned by root:root (as the default is to make the user running webapp-config the owner of the files, same with the group the files are set to and that user is usually root) which prevents any user without root access from tampering with the installed files.
This includes the user that runs the web server and PHP to protect against security holes in Apache, PHP or even the web app itself from overriding the web app's installed files.

The downside for web apps that have frequent updates like WordPress is that the process of updating it via the distributions package manager and then manually running webapp-config to update it (by default webapp-config wlll be automatically run via the package manager in the default virtual host but if you have multiple virtual host configured, then this is not helpful) can be less convenient then just installing the webapp manually and using it's built-in updater as will just automatically install the new versions without any manual updating required at all.
It's also less convenient if an update is available but an ebuild for it is not yet available but updated ebuilds are usually out within a day or so of a web apps release.

When to just let the web app update itself and when to use the package manager for updating it just like one does with apache, php, ssh, bash and other non-web apps seems to depend on how frequent the updates for a web app are, which method is more convenient and if you prefer security feature of not letting the web server and PHP change any of the web apps installed files.

Considering that the Integrity Check feature of NextCloud is primarily for the purpose of checking for files that have been tampered with and webapp-config prohibits this by installing the files as root:root, the Integrity Check feature may not actually be needed when webapp-config is used.

[svoop]

@jdunn0

Unless a Gentoo specific version of NextCould was created with a name like nextcloud-13.0.0-r1 then the version number would always match. I can't think of a reason that a Gentoo specific version would be created for NextCloud though as it's a self contained application that doesn't need any modifications to work in a specific distribution.

One very possible reason is a bug in the ebuild (aka: package) itself. Gentoo doesn't do silent updates, ebuild fixes will therefore always result in r-level updates. Same goes for ebuild updates made necessary due to changes in Portage (aka: package manager) unless a NC update happens at the same time.

However, I don't see any real security benefit from matching exact versions. Just be a little bit more restrictive when it comes to the pattern matching and you're all go:

/\.webapp-nextcloud-(\d+\.){3}(-r\d+)?/

[jdunn0]

@svoop: That is a good point, sometimes the revision is just the ebuild itself being updated.
The pattern you listed would be better then the pattern in pull request #3113 but the code changes in that pull request are still needed to add support for excluding files with pattern matching instead of exact string matches to the Integrity Checker function.

It appears that pull request #3113 was closed due to the pattern included with the pattern matching code of /^\.webapp-nextcloud-.*/ could match a lot more files then intended.

If that pull request can be reopened and the code changed to use the more restrictive pattern you listed of /\.webapp-nextcloud-(\d+\.){3}(-r\d+)?/ then the pull request might have a better chance of being accepted.

If the NextCloud developers just don't want the pattern matching code in the Integrity Checker function then one of the two methods I listed would need to be used instead.

I can just create a patch to fix this and put it in /etc/portage/patches/www-apps/nextcloud so portage automatically applies it or disable the Integrity Checker function altogether so it doesn't really matter to me if this issue is resolved or not.

I think a patch should be accepted that deals with the '.webapp-nextcloud-[installedversion]' file so all users using webapp-config to install NextCloud, no longer need to disable the Integrity Checker or see annoying notifications when using NextClould.

[svoop]

@jdunn0 This /^\.webapp-nextcloud-.*/ is indeed a bad idea since the .* is eager and will swallow anything (including /) if there's another / coming later. So it would match stuff like .webapp-nextcloud-10.0.0/../../hackerstuff.php/. To make it safe, either use my pattern from above or simply add a question mark like so: /^\.webapp-nextcloud-.*?/. It makes sure, the .* is not eager anymore and will stop at the first / coming up. Either way, I don't see any additional security risk by using a solid (non eager) pattern. Prove me wrong, but until so, I have to consider this argument invalid.

[jdunn0]

@svoop: That makes sense but because @MorrisJobke closed pull request #3113, I don't think the author of the pull request @LeCoyote can change the pull request to use the pattern you listed instead.
I think pull request #3113 needs to be opened again if possible so @LeCoyote can change the code to use the pattern you listed and then hopefully someone will merge the pull request and then this issue would be fixed.

[svoop]

@jdunn0

@LeCoyote can update the pattern on his fork and create a new PR. Or anybody else can fork his fork, update the pattern and create a new PR.

[LeCoyote]

@jdunn0 @svoop Created a new pull request because I must have done something wrong with my fork: https://github.com/nextcloud/server/pull/8647

[LeCoyote]

In spite of my efforts, it seems that the patch I grabbed from OwnCloud is not good enough. I'm sorry but I just cannot dive into NextCloud & PHPUnit or whatever else is needed just to fix this. I hope someone else can pick it up.

Nevermind that, it seems the pull request was approved by a reviewer, so there's still hope :)

[GreatEmerald]

I hope it gets merged soon. It certainly took a while to do something as trivial as excluding two files...

[kesselb]

/\.webapp-nextcloud-(\d+\.){3}(-r\d+)?/ does not work for .webapp-nextcloud-12.0.5?