-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
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]>
First, this won't pass the core check if the file is added. If this should be available, why just don't call |
See also nextcloud/serverinfo#107 |
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:
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. |
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. |
Yes
One more reason to not add it to core? 😝
Neither the fact that a sysadmin need a phpinfo 😉 The voice to core maintainers 📣 |
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: 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. |
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.
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 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. (The same with phpMyAdmin and awstat in /etc/apache2/sites-available and a2ensite resp. a2dissite) |
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:
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! |
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
The 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 And even if mod_rewrite is considered a de facto requirement, the Rewrite rules don't get installed unless 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. |
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... |
why not capture phpinfo() and show it instead of messing with new files?
|
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:
Here's my current thinking (drawn from this thread and some others):
@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! |
This is possibke via the serverinfo app now: https://github.com/nextcloud/serverinfo?tab=readme-ov-file#show-phpinfo--nextcloud-28 |
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. :-) |
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
Checklist