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

Move webroot to a subdirectory #946

Closed
mlueck88 opened this Issue Jul 19, 2016 · 18 comments

Comments

Projects
None yet
@mlueck88
Copy link

commented Jul 19, 2016

Currently all files of Grav are inside the root directory which is also the web root. The access to sensitive files, like the configurations, is only prevented by the webserver configuration file like .htaccess. It just does not feel right to rely on this, especially since people unexperienced with server config files (like me) can easily make mistakes and make all the files public.

Also, it is currently possible to "see" the structure of all folders and files from outside, including filenames, even from the data folder with (possibly sensitive) information. This is because the server sends a 403 if the path exists and 404 if not.

Even worse, all files, for example under the /user/data folder, can be downloaded in plain text if the file ending is not in the list of the .htaccess file, or if it has no ending. I just tested it with a csv file. If access to this folder is required by, say, plugin assets, then a whitelist would be more appropriate.

These problems were solved if the webroot is a (configurable) subfolder of the grav installation (of course index.php, compiled assets and images would have to be moved there).

@rhukster

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

The main reason why Grav is not this way 'by default' is that Grav is intended to be a simple to install and get running solution. Having to manually move things around and set things up is all well and good, but is only for the more advanced user.

That said, i would like to see Grav with this functionality as it makes 100% sense from a security perspective. It probably can mostly be done via the setup.php functionality that lets you configure where things in Grav are located, but i've not tested it and probably something is missing. This is something that we will look to do in the future.

@rhukster

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

BTW: https://learn.getgrav.org/advanced/multisite-setup

These are the docs for multisite which is basically outlining setup.php. This is something that needs better documentation too. Any help with that effort is appreciated!

@mlueck88

This comment has been minimized.

Copy link
Author

commented Jul 19, 2016

That looks indeed promising! The possibility to code this in the php files is definitely something. I will try it.

@mlueck88

This comment has been minimized.

Copy link
Author

commented Jul 20, 2016

After some short research I think I identified the most important points that fail with a dynamic webroot. I will list them here.

  1. Plugin templates can access plugin files (assets, images) directly.

    I grepped through the admin plugin as an example and found paths like "{{ base_url_simple }}/{{ theme_url }}/images/favicon.png". Here, these twig variables could not be used just like that anymore, because the theme sources would not be visible anymore. Luckily, at least for the admin plugin, the favicon is the only resource that the plugin requests directly, everything else is CSS/JS which is managed by Assets.

    • It would be nice to be able to request files like "/user/plugin/admin/.../favicon.png" by their path ... but just from the template and not from the browser. A small twig wrapper would be ideal.
      Perhaps the "url" twig function can be extended with some parameter named "internal", to prepare requested resources in some publically available folder, and return the URL?
      It could work similar to the way the Asset and Medium classes work: Check if the file exists and provide it publically under some generated (or user-provided) asset name.
      But it cannot be done with the other implementations currently, because, unfortunately, Assets is for css/js only and Medium is tied to pages.
  2. If pipelining and minifying are both disabled, then css and js assets are "hotlinked" and not copied into the (compiled) assets folder. (Also minifying without pipelining seems to do nothing.)

    • This could be fixed easily as the functionality is already there, but is skipped if pipelining is disabled.
  3. Media.

    Images requested in pages seem to be always prepared in the public /images folder, even when the cache is disabled and even when no image-manipulation operations are executed on the medium. So this works well.
    But other media, like .txt files, are linked directly when occurring in markdown, like Alt resolves to /user/pages/01.page/file.txt, and also whether the thumbnail is prepared or hotlinked depends on the system config.

    This is due to the different implementations of ImageMedium and the general Medium classes, in particular the method "cacheFile" that exists in ImageFile has no equivalent in the other media.

Of course nothing of the above is required if the corresponding tree of statically accessible files configured in each plugin is just reproduced inside the webroot.
This would however require somethink like a console command for the users who choose to use such an external webroot, and this command would have to be called everytime a plugin is changed.
(Technically also every time when page media change.)

So maybe wrapping all URLs could still be preferrable (almost all URLs are already "processed" in some form anyway).

@viion

This comment has been minimized.

Copy link

commented Nov 17, 2016

Will there be any movement to this or would PR's be accepted that move it? (seems like it would require major work to the whole system).

Having the root accessible unless restricted to me is screaming security holes all over and in no way can I describe this as a viable CMS to a company. The main issue is you have to restrict things, or they're immediately public, and the average user is not going to know how to do this and will rely on what you give them.

If it was restricted to /web like Symfony (and many others), it would be a vast improvement...

Since someone posted then deleted their comment, here are the security concerns:

  • By default the folder / is web accessible, folders within it have been added to nginx config and there are htaccess acesss, what if I make another folder? With "company_profits.pdf" inside (bad example but not far off users being silly), now that file is immediately web accessible unless I go modify the configuration, this should not be the case, everything should be restricted by default, with only the sub directory accessible.
  • What if someone doesn't use the nginx profile provided? Everything is web accessible.

Mainly talking about nginx because it's configuration is baked into a file, not per folder.

Edit: Further notes, these are all web accessible:

  • site/README.md + CONTRIBUTING.md + CHANGELOG.md (no harm)
  • Anything else in site/ downloads (eg Vagrant files, uploaded files.... etc)
  • Everything in site/webserver-configs/ downloads (bad, shows all possible server configurations)
  • Anything in site/tmp/test.txt (bad, can run anything in here, including PHP)
  • Can run various files in system, eg: system/src/Grav/Common/Config/Setup.php (bad, why is this possible??)
  • Can run random code in cache? eg: site/cache/compiled/files/2ec7127e23f0eae025be8bc4c4ae2f42.yaml.php
  • Anything in site/backup downloads (bad)
@flaviocopes

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

Some of the things you highlighted are fixed in the default configuration. The default .htaccess for example limits access to webserver-configs. But not the default nginx config file (we need to fix this). tmp folder is not denied anywhere, something we should add as a default.

What if someone doesn't use the nginx profile provided? Everything is web accessible.

We provide the default configuration which has sensible defaults, but we can't make sure people will use it or won't modify it. But if you modify it, you're kind of supposed to know what you are doing.

what if I make another folder?

Root folders can't be added via the Admin plugin, which is what most people will use. Admin is just going to act under the user/ folder. If you're using the filesystem directly, or via FTP you add a folder the the root of the website, you're supposed to know that is going to be available to anyone unless you deny it.

All the files you list as web accessible are denied in the default htaccess configuration, (except tmp as I noted above). If you remove the default config, then of course they are accessible, we can't do much about it because Grav has not been thought with this kind of defensive measures in mind. If you add a vagrant file to the project root, then you add it to .htaccess or your nginx config too. User files should generally all go under user/, also because it's easier to move the site around.

So probably food for thought for Grav 2.0, but if you have ideas to make this viable, we're open for discussion. Maybe we should do the opposite in our default config files, only allow the files we want to allow, and deny everything else.

My 2c :)

@viion

This comment has been minimized.

Copy link

commented Nov 18, 2016

The files I listed as accessible are using the provided Nginx configuration, default settings, using the current version found on the getgrav website. .htaccess doesn't fit the issue, Nginx doesn't read those.

  • Using the default Nginx configuration, downloading Grav from the site I can directly access web configurations and Grav source code.

Eg of execution:

  • website.com/system/src/Grav/Common/Config/Setup.php

    Fatal error: Class 'Grav\Common\Data\Data' not found in /vagrant/system/src/Grav/Common/Config/Setup.php on line 18

  • website.com/system/src/Grav/Console/ConsoleCommand.php

    Fatal error: Class 'Symfony\Component\Console\Command\Command' not found in /vagrant/system/src/Grav/Console/ConsoleCommand.php on line 16

etc, I know opening these alone wouldn't do much, especially when most are encased in classes. If there happened to be an exploitable file, all installations are vulnerable through a simple request. (Errors turned on just to show output, in reality it would be a 200 blank file.)

For dev and setup, this is great, it's quick and easy (changing index to a subdir wouldn't slow this down though?). But because of it's nature, it's failing best practices, which make it hard to persuade to Leads or Managers that it should be used. Grav is very powerful and well built (with a gorgeous UI), so I push this issue with the intent to support it.

@flaviocopes

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

That's part of what I said, the default nginx.conf has

    # deny all direct access for these folders
    location ~* /(.git|cache|bin|logs|backups|tests)/.*$ { return 403; }
    # deny running scripts inside core system folders
    location ~* /(system|vendor)/.*\.(txt|xml|md|html|yaml|php|pl|py|cgi|twig|sh|bat)$ { return 403; }

So it should block backups and cache, and any php script under system.

But misses webserver-configs and misses denying .md files in the root, something .htaccess does. Also misses tmp like .htaccess

@viion

This comment has been minimized.

Copy link

commented Nov 18, 2016

# deny all direct access for these folders
location ~* /(.git|cache|bin|logs|backups|tests)/.*$ { return 403; }
# deny running scripts inside core system folders
location ~* /(system|vendor)/.*\.(txt|xml|md|html|yaml|php|pl|py|cgi|twig|sh|bat)$ { return 403; }

These do not seem to be working.

I cloned the current repository, and setup a vagrant with:

  • PHP 7.0.1
  • Nginx 1.8.1

Here is my config:

server {
    listen 80;
    index index.html index.php;

    ## Begin - Server Info
    root /vagrant/;
    server_name viion.dev;
    ## End - Server Info

    ## Begin - Index
    # for subfolders, simply adjust:
    # `location /subfolder {`
    # and the rewrite to use `/subfolder/index.php`
    location / {
        try_files $uri $uri/ /index.php?_url=$uri;
    }
    ## End - Index

    ## Begin - PHP
    location ~ \.php$ {
        # Choose either a socket or TCP/IP address
        fastcgi_pass unix:/var/run/php/php7.0-fpm.sock;
        # fastcgi_pass 127.0.0.1:9000;

        fastcgi_split_path_info ^(.+\.php)(/.+)$;
        fastcgi_index index.php;
        include fastcgi_params;
        fastcgi_param SCRIPT_FILENAME $document_root/$fastcgi_script_name;
    }
    ## End - PHP

    ## Begin - Security
    # deny all direct access for these folders
    location ~* /(.git|cache|bin|logs|backups|tests)/.*$ { return 403; }
    # deny running scripts inside core system folders
    location ~* /(system|vendor)/.*\.(txt|xml|md|html|yaml|php|pl|py|cgi|twig|sh|bat)$ { return 403; }
    # deny running scripts inside user folder
    location ~* /user/.*\.(txt|md|yaml|php|pl|py|cgi|twig|sh|bat)$ { return 403; }
    # deny access to specific files in the root folder
    location ~ /(LICENSE.txt|composer.lock|composer.json|nginx.conf|web.config|htaccess.txt|\.htaccess) { return 403; }
    ## End - Security
}

Changes from default:

  • uncomment port
  • set root folder
  • set server name
  • change from php5 to 7

With this, I can access:

The following restricts everything:

location ~ /system/ {
    deny all;
    return 403;
}

To further pressure this, it took about 5 minutes of Googling, to find sites that use Grav and find 1 that was on Nginx and where I can access these same files... Obviously will not post it.

If I'm missing something completely, let me know :)

@flaviocopes

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

The default nginx.conf is indeed not doing the job it's supposed to do, going to take a look!

@flaviocopes

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2016

If you could test the newly committed nginx.conf it would be great. Moved the security section before the PHP handler (effectively blocking PHP scripts!) and the backup folder that was wrongly referenced as backups

@matthew-dean

This comment has been minimized.

Copy link

commented Feb 18, 2017

+1, either move all publicly-accessible files to a sub-folder, or allow easy configuration

@treb0r

This comment has been minimized.

Copy link

commented Nov 22, 2017

+1, I think that this is a very important enhancement. Security is key to the growth and success of GraV!

@frickenate

This comment has been minimized.

Copy link

commented Nov 30, 2017

Late comment, but just so you're aware - you've lost another user because of this. It's unacceptable in 2017 to have source code files in the public document root. The only permissible files in the public document root are a basic index.php front controller, and the static files (images, css, js, fonts). Any other files are inexcusable. Some people, myself included, are looking at options like Grav specifically because Wordpress's similar laissez-faire attitude about basic security principles means the product must be avoided.

I ran a script to iterate pulling every file in the document root, with the provided blacklist rules in place. Many vendor files are still accessible - including an exe file. Even if you were to have exhaustive blacklisting rules (or preferably, a whitelist), the fact that everything - including user information and password hashes - is accessible by default unless properly blocked at the web sever level is unforgivable.

You may want to support users who are stuck on hosting providers who only allow ftp upload to the webroot, as things were in the 90's. For those of us who have full access to properly configure a secure-as-possible setup, this directory structure simply means your product cannot be considered as a viable option.

@nephatrine

This comment has been minimized.

Copy link

commented Feb 7, 2018

I would also greatly appreciate being able to move certain folders out of the webroot. I've tested my nginx rules pretty thoroughly, but it would still be more peace of mind. Even something like how Dokuwiki is configured would work. They have everything is in the webroot by default but there's relatively straightforward directions on moving the important source and configuration folders outside.

@gaambo

This comment has been minimized.

Copy link

commented Oct 8, 2018

just starting with grav as a simple/small wordpress alternative for our web agency. the issue of having all files (inkl. vendor) in the root directory is a big one for us. have there been any changes to this (couldn't find anything in the docs) since the last activity in this issue? is there something we can help with?
i looked at the multisite setup.php but @mlueck88 seems to suggest a bigger rewrite needs to haben to enable this functionality.
would be glad to hear if there's anything new about this!

@magnus-eriksson

This comment has been minimized.

Copy link

commented Dec 21, 2018

I know it's up to you guys to decide how you want your application to be structured, but I totally agree with several of the previous commenters. Grav's file structure is a big security issue, which is the sole reason I won't use it for my customers (or my self). If that were fixed, I would definitely consider using it.

Just read this thread: "We need to fix/add that" when people pointing out what you've missed in the htaccess and nginx-conf. It doesn't matter how smart you are, you will most likely keep missing some things (most people would).

There's also the case of the web server acting up, making the mod_rewrite to be shut off (it has happened), then all the sensitive files will be accessible, no matter what you put in your htaccess.

Restricting the site using htaccess or location rules should only be done if there's no other way. You have another way, rebuilding the application to follow best practices.

It's not harder to deploy an application if you restructure it. And if someone have some hosting where they can't put code outside of the web root, they should rather change hosting. Don't let your application suffer because some people wants to use an inferior hosting.

@nithinkolekar

This comment has been minimized.

Copy link

commented Dec 26, 2018

One more thing I noticed about Grav is that, if you want to update core via web interface you must need to have whole web root writable by webserver.
IMO only assets and cache dir should be writable by webserver excluding any script files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.