12

I have a web site where users can upload files. I do not want those files to be accessible by anyone.

I have seen that some people create a folder (say my_secret_folder) at the same level of the www directory. Then, they upload files (with PHP script) using:

$destination = $_SERVER['DOCUMENT_ROOT'] . "/../my_secret_folder/" . $filename;

Where $destination is the full path of the uploaded file.

Then, only the PHP script (from within the www folder) is allowed to access the file. Is this a good practice to upload files to a folder outside the public www folder?

Bob Ortiz
  • 444
  • 4
  • 22
2WFR
  • 135
  • 5
    Just remember when doing this you have to be very, very careful about filename - e.g. ..\look_where_i_am has been a security vulnerability for decades. – Voo Jan 04 '23 at 14:09

4 Answers4

30

Yes, it is a good practice. Placing it outside the webroot means that the files will not be publicly exposed by a simple configuration mistake in the web server, thus adding another barrier to exposure - and it has essentially zero cost to implement.

vidarlo
  • 9,165
  • 2
    And even if they are intended to be publicly exposed, this is a reasonably good practice, because it lets the server control when and how the file gets exposed in a much simpler way than would be the case with uploading directly to the final location. This is especially important if the files need to be processed in some way before public exposure. – Austin Hemmelgarn Jan 04 '23 at 02:39
3

Assuming that this is an apache server, have your considered placing restrictions on the directory?

<Directory '/var/www/html/uploads'>
    Options -Indexes
    <Limit GET>
        Require user admin
    </Limit>
</Directory>

Don't show a list of files if somebody tries to browse the directory and limit an HTML GET to somebody logged in as admin.

doneal24
  • 881
  • 11
    I would have to say hard disagree. Even if you have this, there's no guarantees that configuration won't be lost or deployed. It's much safer to keep it out of the webroot. – HSchmale Jan 04 '23 at 00:28
  • 4
    @HSchmale If having the configuration changed is a concern then can you be certain that DocumentRoot won’t be changed? Or possibly a new admin will not know why the directory is where it is and move it? Put the directives in the config file and document it. – doneal24 Jan 04 '23 at 03:24
  • 6
    @doneal24 Yes, but I'd argue that it's probably an easier mistake to make to modify any point in the configuration you provided and break it (thus failing to apply the restrictions) than to have someone 1) accidentally modify the webroot 2) have the website still working somehow and 3) have the new webroot include also the other directory. That would require an intentional change. Obviously these kind of decisions, like everything security related, should be documented somewhere. – GACy20 Jan 04 '23 at 08:53
2

This has already been hinted at in a comment by @Voo, but this point is so important that I think it should be moved to an answer box.

$destination = $_SERVER['DOCUMENT_ROOT']."/../my_secret_folder/".$filename

This is good practice, if, and only if, the web user cannot control $filename. Otherwise, it's a path traversal vulnerability.

Consider what happens if the client crafts a special request where $filename is ../www/index.php. You just allowed your uploader to overwrite your web application's code.

Resist the temptation to solve this issue by blacklisting values such as / and ..: there are ways around that, the link above lists a few. The canonical solution to this problem is to either (a) choose the file names yourself or (b) have an explicit whitelist of allowed characters.

Heinzi
  • 2,249
  • In my case, the user uploads a file with an Ajax request that calls a PHP script. This scripts then changes the filename to 'toto_id' where id is the user id and 'toto' is a string I have chosen. Finally it moves it to the 'secret' folder. Is the attack still possible ? – 2WFR Jan 05 '23 at 18:51
  • ... and the extension of 'toto_id' is set to .png only. – 2WFR Jan 05 '23 at 18:58
  • @2WFR: Sounds good to me. – Heinzi Jan 05 '23 at 19:49
  • Obviously the user can check if his uploaded file is ok. Here the php script that reads the file has the user id as an input (not the file name as it is always ´toto’). But then it is easy to check that the id corresponds to the user with a valid php session. – 2WFR Jan 05 '23 at 20:29
0

I would argue that this is not a good practice. The security risks that @Heinzi pointed out are very real, but even if you do everything correctly security-wise, you still have end up having a bunch of [presumably important] user files stored on your server. Servers should ideally be ephemeral. There are much better solutions for storing files than just having them lie around in your server. Even if you are taking automated backups (are you?), your setup for them is probably nowhere near as robust as it ideally could be.

I'd advice you to for example look into AWS S3 or Google Cloud Storage, these kind of managed file storage solutions are extremely cheap nowadays and take care of all the hazzle for you.

ruohola
  • 109