php / php-src

The PHP Interpreter
https://www.php.net
Other
38.16k stars 7.75k forks source link

Can open files using invalid paths #16622

Open cod3beat opened 4 days ago

cod3beat commented 4 days ago

Description

The path / /../ /../ /../ /../path/to/file is considered invalid by some file check functions:

<?php

is_file("/ /../ /../ /../ /../path/to/file"); // -> false
realpath("/ /../ /../ /../ /../path/to/file"); // -> false
filesize("/ /../ /../ /../ /../path/to/file"); // -> false

However, the path can actually be used to open /path/to/file :

<?php
echo file_get_contents("/ /../ /../ /../ /../path/to/file");

$res = fopen("/ /../ /../ /../ /../path/to/file", "r");
echo fread($res, 1024);

PHP Version

< 8.3

Operating System

No response

bukka commented 4 days ago

Just for the reference this was initially created as a security issue so I will extract some of my comments from it here as well:

I have to agree that this is a bit confusing and the spaces confused me as well so I actually needed to do a bit of debugging. What's happening is that the file_get_contents operates on streams which uses _php_stream_open_wrapper_ex that will use plain_wrapper's _php_stream_fopen that calls expand_filepath and uses real expanded file path. This path operates on the virtual path removes parent directory when .. present from the path without checking the parent existence. So something like/a/../b/cwill become/b/c. That's what happens here (in this case instead ofa`, space is used).

I don't think we can change the current behaviour but we should maybe document it better. I also think the issue here is inconsistency how this operates and maybe apply the same logic to is_file and friends. That would be more a feature request though. I actually remember that I already tried to make it consistent due to another bug: see 766cac0 . The problem with that is that expand_filepath is expansive and it resulted in 8% perf degradation for Symfony demo. Possibly we could try to use expand_file only if .. is in the path which might reduce the perf impact. It will need some benchmarking though.