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

(htaccess) Permit admin-deployed phpinfo.php access #38151

Closed

Conversation

joshtrichards
Copy link
Member

Summary

Even though some PHP info is exposed in NC under Admin->System, accessing phpinfo() via the web server is still routinely needed by admins to troubleshoot an installation.

One common case would be if the NC installation is completely broken - in which case Admin->System would also be inaccessible.

Executing PHP from the command-line is not a useful substitute - even though it's often mistakenly used instead while troubleshooting - since it may or may not have the same settings or even be the same version of PHP active in the web server.

Unfortunately our current .htaccess mod_rewrite rules prevent admins from accessing their own self-created phpinfo.php. This fixes that.

While the admin will still needs to provide their own phpinfo.php file, this enables it to be accessible as expected (and as I believe was possible several NC versions ago IIRC anyhow).

If the phpinfo.php file does not exist (the default state), things still fall through to the NC 404 handler so there's no change in behavior under normal operation.

(Thanks to the poster PSN for the idea: https://help.nextcloud.com/t/how-to-get-phpinfo-to-display-info/133180/8)

TODO

  • A separate PR for adding similar functionality to the NGINX example configurations in the Documentation repo

Checklist

Even though some PHP info is exposed in NC under Admin->System, accessing phpinfo() via the web server is still routinely needed to troubleshoot an installation. One common case would be if the NC installation is completely broken - in which case Admin->System would also be inaccessible to the admin. 

While the admin will still needs to provide their own phpinfo.php file, this enables it to be accessible as expected (and as I believe was once possible several NC versions ago IIRC anyhow).

(Thanks to PSN for the idea: https://help.nextcloud.com/t/how-to-get-phpinfo-to-display-info/133180/8)

Signed-off-by: Josh Richards <[email protected]>
@solracsf
Copy link
Member

solracsf commented May 9, 2023

First, this won't pass the core check if the file is added.
Second, i'm personally not a fan of bringing external files to the Nextcloud installation (moreover in core), even if I get the mentioned points. Also because the phpinfo.php file could contain, well, anything !

If this should be available, why just don't call phpinfo() somewhere in the serverinfo app and make this availlable on that app?

@solracsf
Copy link
Member

solracsf commented May 9, 2023

See also nextcloud/serverinfo#107

@joshtrichards
Copy link
Member Author

You mean it won't pass if an admin adds their own phpinfo.php file in the root? (Because just adding the rule itself that is in this PR doesn't seem to trigger any checks as far as I can tell?).

Admins install phpinfo.php files themselves routinely. It's on them to remove it/manage it. This just decreases the burden during what is generally a frustrated troubleshooting moment where extra complexity is generally not desired by the admin.

I actually agree on the serverinfo front but two things there:

  • I've seen those issues and so far adding phpinfo in NC's serverinfo has been denied
  • It doesn't address common situations where the NC installation is completely non-working (such as during a botched PHP upgrade or config change) because in that situation NC serverinfo itself won't be accessible anyhow (and since NC's .htaccess is in-place an admin can't easily isolate what's going on themselves until they unravel why they can't deploy a common PHP troubleshooting technique).

Unless I'm overlooking something this doesn't create a new attack vector per se, since NC isn't providing or enabling random php files to be added or executed. It's still 100% on the admin to activate by deploying their own file (and, for that matter, deciding what's in it).

Don't get me wrong - I'm not in love with the whole idea. I know folks will troubleshoot and leave the file around. But that isn't new nor NC's fault.

@joshtrichards
Copy link
Member Author

I just had a thought that might address our mutual concern about a random phpinfo.php file being left around (when it should only exist briefly but is accidentally left in place): I could amend this PR and add a check so that we warn the admin if we see that they've left a random phpinfo.php file laying around in their NC webroot.

@solracsf
Copy link
Member

solracsf commented May 9, 2023

You mean it won't pass if an admin adds their own phpinfo.php file in the root?

Yes

I've seen those issues and so far adding phpinfo in NC's serverinfo has been denied

One more reason to not add it to core? 😝

But that isn't new nor NC's fault.

Neither the fact that a sysadmin need a phpinfo 😉

The voice to core maintainers 📣

@joshtrichards
Copy link
Member Author

joshtrichards commented May 9, 2023

Having the check fail seems fine. I mean that's a good thing right? That'll help the admin not leave the file around. 😉

Without this PR, the documentation will need a PR then because the process suggested for troubleshooting PHP using phpinfo() does not work:

https://docs.nextcloud.com/server/latest/admin_manual/issues/general_troubleshooting.html?highlight=phpinfo#php-version-and-information

At a minimum, the process will have to include the user disabling the NC .htaccess file (renaming it temporarily presumably) or editing it directly (manually) to add a similar adjustment as in my PR.

This is not only extra work for the user, but creates other opportunities for things to get messed up, no?

Also seems odd to have the user disable the NC .htaccess in order to troubleshoot their NC installation.

Right now the only way to troubleshoot PHP (in a relatively universally reliable manner) when things are messed up is for the user to also tweak NC's htaccess in their environment (or otherwise disable AllowOverride in the Apache config).

The only exception is if one has NC installed in a subdirectory (because the user has the webroot to themselves and NC isn't involved).

This also creates an additional burden when users are trying to get assistance in the forum or send in issues.

I'm certainly open to alternatives or adjustments.

@ernolf
Copy link
Contributor

ernolf commented May 9, 2023

You mean it won't pass if an admin adds their own phpinfo.php file in the root? (Because just adding the rule itself that is in this PR doesn't seem to trigger any checks as far as I can tell?).

With all due respect, I find this solution suboptimal, as it expands attack surface if it is publicly known in the code that the rewrite rules for phpinfo.php do not apply.

Admins install phpinfo.php files themselves routinely. It's on them to remove it/manage it. This just decreases the burden during what is generally a frustrated troubleshooting moment where extra complexity is generally not desired by the admin.

While the idea is not to always leave the phpinfo.php file in place, I suspect many will not remove it out of laziness or carelessness. So it's just a matter of patience until the phpinfo() information is exposed somewhere.

We all know the trail of the scan-scripts scanning for PHPMyAdmin and phpinfo.php in our server log files on a daily basis.

Detailed information revealed by phpinfo() can assist attackers in crafting convincing social engineering attacks. Armed with specific details about your PHP environment, attackers can impersonate legitimate entities or use technical jargon to deceive users into providing sensitive information or performing actions that compromise security.

Nextcloud, like any secure web application, relies on various security measures, including rewrite rules, to protect against common vulnerabilities and enhance security. By intentionally bypassing rewrite rules for the phpinfo.php file, you undermine the security mechanisms put in place and weaken the overall security posture of your Nextcloud installation.

Permanently exposing the phpinfo.php file goes against widely accepted best practices and industry standards for securing web applications. It is generally recommended to remove or restrict access to the phpinfo() function in production environments to minimize the risk of information exposure and potential security breaches.

This change wouldn't affect me personally anyhow, as I disabled the .htaccess and packed the rules (for performance reasons) in the *.conf files, so it's easy for me to omit that.


I use a complete different approach: directories completely outside of the webroot and the server tree e.g. /home/www-data/server-status, mounted as "alias" inside the config directory, restricted by password and required local IP. There I have all tools for extern analization such as phpMyAdmin, mod_server-status, phpinfo.php, awstat etc.:

So my phpinfo.php is inside of that directory and an index.html that redirects to server-status
just as an example for apache2:

Alias /config/status /home/www-data/server-status

<Directory /home/www-data/server-config>
    Options Indexes FollowSymLinks
    AllowOverride None
    Require all granted
    AddHandler php-script .php
    AddType text/html .php
    DirectoryIndex index.php
</Directory>

<Location "/config/status">
    AuthType Basic 
    AuthUserFile /home/www-data/.passwd 
    AuthName "Server status of **SERVER_NAME**" 
    Order Allow,Deny
    Allow from 127.0.0.1 ::1
    Allow from 192.168.188.1/255.255.254.0
    Require user **USER**
</Location>

<Location "/config/status/server-status">
    SetHandler server-status
</Location>

As you can see, that whole branch is password-protected and only accessible from local addresses.
And when I put this config file inside of my /etc/apache2/conf-available, I can activate it with
sudo a2enconf server-status && sudo systemctl reload apache2
and deactivate with
sudo a2disconf server-status && sudo systemctl reload apache2

(The same with phpMyAdmin and awstat in /etc/apache2/sites-available and a2ensite resp. a2dissite)

@joshtrichards
Copy link
Member Author

joshtrichards commented May 9, 2023

@ernolf:

With all due respect, I find this solution suboptimal, as it expands attack surface if it is publicly known in the code that the rewrite rules for phpinfo.php do not apply.

This is already the case since the NC's .htaccess doesn't even apply to subdirectory installations.

I'm aware of the issues with exposing phpinfo() publicly. I'm not proposing including a phpinfo.php file.

The issue here is that admins are prevented from using phpinfo() in any arbitrary named php file at all if NC is installed unless they wish to disable their NC .htaccess rules (including all other rewrite checks) in their NC install in order to troubleshoot PHP issues. (Well, other than the subdirectory installation exception noted above).

I'm trying to:

  • reduce admin friction
  • minimize the chance that troubleshooting steps lead to breaking NC in additional ways (i.e. by an admin trying to figure out why they can't access their own phpinfo.php file)
  • keep people from falling back on using php from the CLI which isn't an effective approach for reasons I know you're aware of from your own forum posts while trying to support other admins :-)
  • establish a clear, relatively narrowly defined process (to support something admins are already doing in wacky undocumented ways), that can be used and documented within the NC docs properly (because the current process in the NC Admin Manual already tells admins to create a phpinfo.php file in their webroot even though that doesn't actually work as things are currently implemented in NC hah)

If the supported process is narrow enough we can even include checks for a stray phpinfo.php being left around and give a warning with all the other security checks we also do.

@solracsf - Worrying about what an admin opts to put in their phpinfo.php file seems a stretch - they can already put whatever they want in literally any file in the webroot. I'm trying to discourage admins (particularly less experienced ones) from rummaging around or tweaking random config files recklessly and creating a bigger mess - whether from a functionality or from a security standpoint.

Thank you to you both for your thoughts so far I'm appreciating the discussion and feedback!

@joshtrichards joshtrichards changed the title (htaccess) Permit user-provided phpinfo.php access (htaccess) Permit admin-deployed phpinfo.php access May 9, 2023
@joshtrichards
Copy link
Member Author

Just discovered something interesting (well depending your definition of the word hah).

The Rewrite rules this PR is targeting aren't even added in a default installation. But it just so happens that due to circumstances there are a lot of installations that end up with them in the end (and most Docker ones). 😄

Mostly documenting this here for now so it's somewhere until I decide on any other next steps.

Here's the deal:

The RewriteCond associated bits only get created if htaccess.RewriteBase is set to something non-empty in config.php (and it's empty by default):

The htaccess.RewriteBase parameter is only talked about in the manual to enable Pretty URLs:

Here's which install methods have it enabled by default and which don't:

It's just many (most but not all) Docker-based users and basically all (that follow the docs) nginx users that are likely to have it.

So: The docs are a little ambiguous about mod_rewrite being required. Basically it isn't (in the sense that we don't technically check for it and NC happily works without pretty URLs - a cosmetic thing mostly - only warning about the loss of some /.well-known/ URL handling).

And even if mod_rewrite is considered a de facto requirement, the Rewrite rules don't get installed unless htaccess.RewriteBase is set to something other than the default (which is an empty string).

Just happens that the main Docker containers turn on Pretty URLs for people as a convenience.

Now I'm wondering how many manual+snap+miscellaneous installs there are out there and don't even have any of these rules in place. 🤔 😆

Whew. And I had no idea I would be going down this rabbit hole today. hah.

@joshtrichards joshtrichards marked this pull request as draft May 12, 2023 15:58
@szaimen
Copy link
Contributor

szaimen commented May 12, 2023

So what is the question with this now? I dont think this is needed for AIO for example as all necessary php extensions are shipped...

@purefan
Copy link

purefan commented May 16, 2023

why not capture phpinfo() and show it instead of messing with new files?

function pinfo() {
    ob_start();
    phpinfo();
    $data = ob_get_contents();
    ob_clean();
    return $data;
  }

credit

@joshtrichards
Copy link
Member Author

joshtrichards commented May 19, 2023

Okay I've been reflecting on this and taking in a lot of the feedback. Thank you everyone.

I'm thinking of tackling this on a couple fronts in an alternative manner for now. There are basically two use cases:

  • "Nextcloud isn't functioning well (either recurring problems or less than desirable performance) and I want to dig into my PHP installation/config"
  • "Nextcloud isn't functioning at all and I want to check my PHP installation/config"

Here's my current thinking (drawn from this thread and some others):

  • Proposing a couple of updates to the docs in the Troubleshooting section where we already suggest people use phpinfo() (which won't work in the majority of installations as described due to Pretty URLs - AKA htaccess.RewriteBase in config.php - being fairly ubiquitous): https://docs.nextcloud.com/server/latest/admin_manual/issues/general_troubleshooting.html?highlight=phpinfo#php-version-and-information
    • Encouraging admins to use /settings/admin/serverinfo for accessing most of the essential PHP details within their active NC installation if their NC installation is functioning enough to permit them to do so (this will provide more accurate info too since some parameters exposed via an external phpinfo() call won't match what's actually being used by NC since NC has overrides in various places)
    • Encouraging admins to utilize their OS/distributions package management to attempt to ascertain their PHP version and probable config locations if NC isn't functioning enough to access serverinfo
    • Clarifying that a manually created phpinfo.php (or any other file name of their choosing for that matter) won't work if Pretty URLs is on (usually is). Then - I guess - telling them to either: (a) disable Pretty URLs temporarily (and the ramifications) or; (b) temporarily adjust the Nextcloud .htaccess or Apache config while they're accessing their own phpinfo.php. We already remind people to delete their phpinfo.php file when they're done (due to the security risk) <-- btw, this is what I was trying to avoid by submitting this PR: the creation of yet another way for admins to screw up their configuration by messing with their .htaccess or Apache config while troubleshooting
  • Working with the https://github.com/nextcloud/serverinfo devs to see if it makes any sense to make some additions to the PHP info provided there - e.g. [Enhancement] System Info serverinfo#362 (comment)

@szaimen Agreed as AIO is more fully self-contained, but the htaccess is generated by server so my not-so-great PR would come along for the ride for any of the downstream packagers (AIO, community Docker, whatever) regardless

@purefan The idea is fine, but it doesn't help with the use case where NC's isn't functioning due to some whacky PHP configuration or a botched PHP version upgrade/downgrade/default version mismatch (the forums are full of these).

@solracsf Thank you for weighing in and pushing back on me a bit. I don't love the idea of encouraging messing with the files in the installation directory either. I'm coming from the perspective that we already tell people in the Manual to stick a phpinfo.php file in their install root. When I discovered that no longer works as documented (for the most common configuration where people have Pretty URLs enabled), I sought a PR to minimize telling people to mess even more with their installation. I don't like this PR either - I view(ed) it as the "lesser of two evils". 🤣

@ernolf I'd agree, but the challenge is we already tell people to deploy a phpinfo.php file (which I hate because, yeah, it'll get left in place). The thing is this documented troubleshooting approach of using phpinfo.php no longer works for the most commonly used configuration. And, as best as I can tell, the Rewrite* rules aren't really used for security because they aren't even enabled if Pretty URLs aren't on. (P.S. If we're starting to rely on them for security then the docs should also be updated for that because in that case the htaccess.rewritebase parameter in config.php is no longer just about Pretty URLs).

On the upside, I'm glad serverinfo has started exposing a lot of basic PHP info - but it's only useful as long as Nextcloud is still functioning!

@solracsf solracsf added this to the Nextcloud 28 milestone Nov 21, 2023
@blizzz blizzz mentioned this pull request Nov 22, 2023
5 tasks
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@szaimen
Copy link
Contributor

szaimen commented Jan 10, 2024

This is possibke via the serverinfo app now: https://github.com/nextcloud/serverinfo?tab=readme-ov-file#show-phpinfo--nextcloud-28

@joshtrichards
Copy link
Member Author

That helps when the PHP installation is functioning enough to load that. Still a challenge when PHP installation/setup is too broken for Nextcloud to be functioning, but only so much we can overcome I guess. :-)

@skjnldsv skjnldsv added the 2. developing Work in progress label Feb 21, 2024
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants