Issue with realpath() and open_basedir() altogether

[pounard]

This issue is a reddit of #2387 which is probably experiencing a side effect of the same bug.

Context is, I am developping against Symfony 3.4 with some application. Our admins, which are sometime (maybe too much) paranoid always do set open_basedir PHP directive, on a per-project basis, in order to avoid PHP breaking out its rightful scope.

Ou application uses Symfony's 3 style (not Symfony 4-flex style) so we have the following folders in our directory structure:

• /srv/http/someproject/app/Resources/views
• /srv/http/someproject/composer.json
• /srv/http/someproject/var/cache
• ...

Problem here lies in the \Twig_Loader_Filesystem::__construct(), which does the following:

    public function __construct($paths = array(), $rootPath = null)
    {
        $this->rootPath = (null === $rootPath ? getcwd() : $rootPath).DIRECTORY_SEPARATOR;
        if (false !== $realPath = realpath($rootPath)) {
            $this->rootPath = $realPath.DIRECTORY_SEPARATOR;
        }

Since we have open_basedir restriction over the project, which basically is set to this (and a few more folders that show no interest in being here): /srv/http/someproject/app:/srv/http/someproject/var/cache, this means that when \Twig_Loader_Filesystem::__construct() is called with the $rootPath parameter set to /srv/http/someproject/, it tries a realpath() call outside of the allowed open_basedir folders.

So I have here a major problem, because it's hardcoded, hence our sysadmins can't enforce a proper security sandbox for the projets.

I also a real question, the \Twig_Loader_Filesystem::$rootPath property behing private, and having no public accessor, this means that it has never been meant to be used outside of a few very specific use cases I spotted:

• in addPath() and prependPath() it serves the purpose of ensuring that all paths are absolute (heck, why not) - but in real life, in Symfony's use case, we do always end up with path already being absolute (so there is no purpose in that),

• in getCacheKey() having an absolute path seems to make the cache key even more unique, so okay, why not, it makes sense,

• in findTemplate() it serves the purpose of absoluting each registered template path, which seems a duplicate of doing it within addPäth() and therefore is always a no-op (I think it could be removed, also it would potentially be a minor BC break under very peculiar conditions, but not in Symfony as I recall).

Are those realpath() really necessary ?

In my situation, I can see two solutions to fix this:

• a quick fix is to simple remove the realpath() call in the constructor, and from this, I can a see a single an only one potential side effect: if your CLI and your HTTPd don't see the same path (for example, if the HTTPd context is chrooted and the CLI context is not) it will create different cache keys for both. This by extension would mean that warming template caches from CLI would not be possible anymore.

• or a better one, add an optional $noRealpath constructor argument, which would turn to be a micro optimization for Symfony's framework, that in our case seems to feed the loader with only absolute paths from the begining (making those realpath() calls always being obsolete).

In all cases, in the current state of things, twig prevents sysadmins from correctly securing/sandboxing their PHP applications, and that's bad.

[pounard]

FYI I also thought of just passing null to the $rootDir argument, but in the end PHP still emit red warnings because Twig attempts to determine a root directory on its own (seriously, I think this is a very, very bad behaviour, you never know what will really be the current working directory, it will depend on how is configured the environment, or from where a CLI command was run).

[stof]

If the root of your project is /srv/http/someproject/, why is it not the folder being added to è open_basedir` ?

[regilero]

@stof I think you do not clearly understand open_basedir. The goal of open_basedir is to restrict PHP access. Say you have a project, with documentation directories, and other sensitive stuff, like the composer.json that could be considered sensitive stuff in an information disclosure issue. In case of any include security issue, or a php:filter issue, any attack where the application goal is hijacked to try to extract informations, the open_basedir is the last security. It prevents PHP access on stuff that should not be readable by PHP (the only other security on that is php user read access on OS level).

So clearly the way of fixing issue related to open_basedir issues is not to allow all files to be read by PHP. That's the goal of open_basedir, you do not want PHP to be able to read these directories.

If we take @pounard example you have the composer file, a subdirectory /var/cache, you may have some other directories under this var/, like var/log/nginx, or a /docs, a /secret, etc.

[pounard]

@stof I completely agree with what @regilero said.

[stof]

@regilero if the doc is sensible, the best solution would be to not deploy it at all IMO. This way, nothing can access it in case of an attack.

And btw, restricting access to composer.json while allowing access to vendor/ will not help preventing disclosure of installed packages, as the internal composer metadata about installed packages ends up in vendor/composer (and contains this info).

The realpath call is there to ensure that we don't end up with paths like /srv/http/someproject/app/.. for the root dir. While this would still give the right folder, it would not work properly when trying to replace the root dir in the template absolute path to build the cache key (ensuring that the cache key is based only on the location inside the project, not on the location of the project on disk, which might be important when warming up the cache from a different location) and could generate different cache keys for the same template (creating really weird issues in some cases)

[pounard]

@stof I don't want to be rude, but it's been many times that realpath() over-use in both Twig and Symfony caused us such trouble. Problem is that you force symbolic links resolution, which in certain (sometime many) conditions breaks the PHP open_basedir behavior (even when the looked-up folder being in open_basedir). In the last 2 or 3 years, got the exact same problem tens of times.

The realpath call is there to ensure that we don't end up with paths like /srv/http/someproject/app/.. for the root dir.

If you end up with ".." in your paths, there's probably another problem behind, the app using Twig should probably not use ".." but use correctly dirname() instead when necessary. That said, we cannot fix all applications in the world, but at least the realpath() usage could be changed to either:

• something conditional (calling realpath() only when ".." is matched in the string),
• making it optional (using another constructor parameter) that would allow people like us to work around our bug without being invasive on Twig itself,
• a smarter path fixing algorithm that replaces '@/[^/]+/..@' by '' (ugly very quickly written pseudo-code, but I guess you get the point here).

In all cases, using realpath() so lightly is dangerous: it's not only about fixing the path, but has many many other implications:

• returns null if the path could not be looked up by the OS for many reasons (permission denied, symbolic link not resolved due to temporarily unmounted mointpoint, linked path outside the open_basedir, and probably a lot of other use cases),
• on some OSes, it probably also alters the path by normalizing it (which may disturb the debugging process with many side effects),
• it tries to resolve symlinks: in most case, we don't want that: I am a developer, and my application is meant to be sandboxed somewhere, maybe that in my dev box, or dev vm, I don't use symlinks, but maybe that on the future production env, it will,
• maybe that, I know at some point in time, my path is invalid but I still want to give it to twig somehow, maybe for some reason it gets caught later by a stream wrapper that the underlying OS don't know of, maybe my stream wrapper implementation does not implement realpath() correctly (sorry that was a Drupalism, streamwrappers don't have a realpath resolution method, this makes no sense).

I think in this issue case, using realpath() is a misuse of it.

[fabpot]

Let's calm down :) We are all trying to do our best trying to understands problems and finding the best appropriate solutions.

Thank to @pounard for the great explanations and the thorough investigation. I really appreciate it as it makes my life much easier :)

I think you are right when you say that realpath caused us quite a few problems in the past. As of PHP 5.3, the value of open_basedir can be mutated at runtime, so it's probably time to add some tests around it.

In any case, I will try to work on this topic in the next few weeks to see what we can do to be more "compatible" with projects using open_basedir (added tests will also help us avoiding regressions in the future).

[pounard]

@fabpot I'd be glad to help, since it's recurrent problem for us, I can easily test patches in affected projects, but I can also help patching, since I start to have quite some experience with realpath'ing and it's problems.

[pounard]

I created a PR #2709 that illustrate a way to solve this, by avoiding all calls to realpath() replacing it by a normalizePath() method that does not check for file existence.

This method carries also the benefit of having a stronger path normalisation and matching for schemed URI such as phar://...

Please be nice, I didn't attempt to make the code fast, rather illustrate that it's easily possible to work around and prevent realpath() usage.

Python os.path package carries two methods: abspath() and realpath() - the method I propose here would be like replacing realpath() by abspath(): does the path normalization, but does not enforce symlink resolution or file existence checks.

EDIT: I made the PR for the 1.x branch, since at this time, I experience the problem with 1.x version, but it should be rather easy to port in 2.x.

[pounard]

Please PR #2709 is ready for review. My \Twig_Loader_Filesystem::normalizePath() may seem a bit complex, but it's actually working well. I did attempt a few other alternative implementations see https://gist.github.com/pounard/4c242bf94e36d4e8f8f1ae68021ec45c in which I implemented an additional one that implements it with Python's os.path.normpath() algorithm (it's much slower than using the regexes).

[pounard]

Still no reviews or discussion about this ?

[fabpot]

Some quick comments (I still haven't had time to work on this a lot):

• We cannot take a decision by saying "this does not happen when using Twig with Symfony" as Twig is used by way more people than just the Symfony community

• I don't want to have two branches, one that uses realpath and a new one

• BC is important

Reading some material, and from my own experience, we should keep in mind the following:

• Normalizing a path might mean breaking a path that contains symbolic links (as the resolution might be different)

• Support for Windows is important (\\server\... is a path as well)

• Speed is important as well

[fabpot]

Reading the code, I'm wondering if we cannot "just" remove the realpath call in the constructor. As we are calling realpath on the full template filename later on, it seems redundant. That way, realpath is only ever used on a path that must be within the open_basedir directory.

[pounard]

We cannot take a decision by saying "this does not happen when using Twig with Symfony" as Twig is used by way more people than just the Symfony community

Of course, I agree and understand this.

I don't want to have two branches, one that uses realpath and a new one

Having a realpath()'ed branch would keep backward compatiblity (and should probably be used by default until next major).

BC is important

Indeed.

Normalizing a path might mean breaking a path that contains symbolic links (as the resolution might be different)

Actually the OS does the real resolution, so normalizing the path as I do should not break anything. Calling realpath() on the normalized path should give the exact same result than on the non-normalized one, that's the beauty of it: normalization canonicalises the aliased path without resolving symlinks, but once you will really load the file, later during rendering or compiling, the OS will transparently resolve the symlinks without having us to care about. In two words: if it succeeds in realpath'ing the file, it will also resolve it transparently on file load the exact same way.

Support for Windows is important (\server... is a path as well)

That needs testing, I should add Windows paths in the unit tests.

Speed is important as well

Actual implementation should be fast, I think. Good thing is the time it looses for normalization, in theory, should be compensated by the fact it does less I/O's, and does not ask the OS and FS to do a symlink resolution. Once everything is in place and cached properly, it will not resolve anything anymore (at least it should not).

[pounard]

Reading the code, I'm wondering if we cannot "just" remove the realpath call in the constructor. As we are calling realpath on the full template filename later on, it seems redundant. That way, realpath is only ever used on a path that must be within the open_basedir directory.

That would be a very nice first step! It still can possibly have side effects if I recall from when I was working on this subject. Need help to do a PR?

[fabpot]

A PR for this very first step would be really nice. Bonus if you can add some tests proving that it works well with open_basedir.

[pounard]

And there is only thing we say to PR ! Not today.

I always love to place a good quote. Joke aside, I'll try to manage some spare time next week to do this. Thanks again for answering.

[stof]

Reading the code, I'm wondering if we cannot "just" remove the realpath call in the constructor. As we are calling realpath on the full template filename later on, it seems redundant. That way, realpath is only ever used on a path that must be within the open_basedir directory.

wouldn't that break things in places computing the path relative to the root (to create cache keys) if the root was involving a symlink ?