6

I'm using UNLINK with PHP and AJAX. I know that in this way is very dangerous, because everyone can delete any files. But I need to use AJAX because I can't reload the page when I delete the files.

So how should I do to allow to delete the file only for the user who owns it?

Please let me know other things too if you think I'm doing here something wrong or something else what you have in mind and you think that it will be useful : )

My PHP code:


<?php

    $photo_id       = $_GET['photo_id'];
    $thumbnail_id   = $_GET['thumbnail_id'];    

    function deletePhotos($id){
        return unlink($id);
    }

    if(isset($photo_id)){
        deletePhotos($photo_id);
    }
    if(isset($thumbnail_id)){
        deletePhotos($thumbnail_id);
    }


 ?>

My AJAX code:


function deletePhoto(photo, thumbnail){

        var photos = encodeURIComponent(photo);
        var thumbnails = encodeURIComponent(thumbnail);

        if (window.XMLHttpRequest) {// code for IE7+, Firefox, Chrome, Opera, Safari
          xmlhttp=new XMLHttpRequest();
        } else {// code for IE6, IE5
          xmlhttp=new ActiveXObject("Microsoft.XMLHTTP");
        }

        xmlhttp.onreadystatechange=function() {
            if (xmlhttp.readyState==4 && xmlhttp.status==200) {
                document.getElementById("media").innerHTML=xmlhttp.responseText;
            }
        }
        xmlhttp.open("GET", "http://192.168.2.104/images/users/delete_photo.php?photo_id="+photos+"&thumbnail_id="+thumbnails, true);
        xmlhttp.send();
    }
Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
Adam Halasz
  • 57,421
  • 66
  • 149
  • 213
  • 1
    AJAX has nothing to do with security. From the server point of view, AJAX call is no different from a regular one. Your problem not in AJAX but in lack of authorization. Sooner you understand it, sooner you solve your problem. – Your Common Sense Aug 05 '10 at 14:23
  • Hi @Col. Shrapnel, I don't think you are totally right, because without `AJAX` I don't need to make a file what anyone can access and can delete anything with a GET request. Otherwise I know that the problem here is with authorization that's why I've asked this question `So how should I do to allow to delete the file only for the user who owns it?` – Adam Halasz Aug 05 '10 at 14:31
  • 1
    how can you let user delete a file without such a script? – Your Common Sense Aug 05 '10 at 14:46
  • Not without such a script but without `GET`. – Adam Halasz Aug 05 '10 at 16:23
  • So what? think POST request shouldn't be secured? lol – Your Common Sense Aug 05 '10 at 17:53
  • you've said `I don't need to make a file`. Time to make your mind, dude :) – Your Common Sense Aug 05 '10 at 18:32

7 Answers7

9

You need to authenticate the user somehow.

Your user needs to be authenticated with a username and a password.

PHP session can be used to remember, and you should use a database table or a text file on the server to store file ownership information.

Then, before unlinking anything, your logic should make sure that the currently "authenticated" user is the owner of the file.

Wadih M.
  • 12,810
  • 7
  • 47
  • 57
  • If you mean logging in, then the user is logged in. If I don't allow users to access the file who's not logged in that's a bit better, but users who are logged in they still can delete each others files. – Adam Halasz Aug 05 '10 at 14:05
  • that's why you need to store file ownership in another table, and before unlinking anything you make sure that the "authenticated" user is the owner of the file. – Wadih M. Aug 05 '10 at 14:07
  • the only sensible answer here. @CIRK listen to that comment above. that's the only solution. You're going totally wrong way. AJAX is not your problem – Your Common Sense Aug 05 '10 at 14:24
  • Yes this is a logical answer, the problem with my overall code that in this stage I don't send any data to the database, but I think I can figure out so I will try it. – Adam Halasz Aug 05 '10 at 14:34
  • 1
    @CIRK yes, a perfect word - logical. without storing information if the file owner, you cannot verify a file owner. Very logical, isn't it? – Your Common Sense Aug 05 '10 at 14:39
  • Hi CIRK, user authentication and storing ownership meta data would be required. Otherwise, there would be no way really to determine the file's owner. – Wadih M. Aug 05 '10 at 15:10
  • Wadih, prepend user name with @ sign, like I do. this will make your comment personal and let them see your comment in " new responses" section. – Your Common Sense Aug 05 '10 at 15:21
4

you can simplify your task by using a very simple database substitution - a directory structure. keep user's files in user's directory. so, you can always check if particular user has rights to delete. Name a directory after user's name, or - much better - numeric user id

just something like

$photo_id = basename($_GET['photo_id'];)
$filename = $filebase.$_SESSION['user_id']."/".$photo_id;
if (file_exists($filename) unlink ($filename);
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
2

Limit the unlinking to the directory with the photos. That is, do not allow .. in the path, or check the full path after doing realpath(). Otherwise, the user can request delete_photo.php?photo_id=../../../../etc/passwd and break the system.

Sjoerd
  • 74,049
  • 16
  • 131
  • 175
  • If they encode '..' in a different character set, it could potentially pass through. I've read about it in the past. – Wadih M. Aug 05 '10 at 14:06
  • @Wadih yes, but that was a flaw (I think in Apache) that was fixed since. – Pekka Aug 05 '10 at 14:07
  • 1
    If php is running as root i think you have bigger problems on your hands. – rook Aug 05 '10 at 15:13
  • Indeed, because PHP is not root the example of /etc/passwd is exaggerated and does not really work. – Sjoerd Aug 06 '10 at 07:03
1

In your PHP:

  • Make sure $_GET['photo_id'] and $_GET['thumbnail_id'] don't contain "../"
  • Also make sure you prepend a basepath to the ID.

Otherwise users can delete any file.

As for the ownership, you have to store the information who owns which file somewhere on the server side (for example a MySql-DB). Then you should consult this location before deleting the file.

JochenJung
  • 7,183
  • 12
  • 64
  • 113
0

As Wadih M. has said. You need to authenticate your user. Then you can use that to compare the "Owner of the Image" to the "User currently log in". This will give you all the security you may want.

As I said before, name the varaibles so that they sound right. When I see "id" in a varaiable. I automatically assume as a programmer that it is a numeric var.

Anraiki
  • 786
  • 1
  • 10
  • 26
  • The ID is not a number :D it's the `file_name` with some unique stuff before them something like `efb03_orange.png`. The problem is that in this section I haven't sent anything yet to the database. So I don't know how to check if the logged in user is the user who owns the file. – Adam Halasz Aug 05 '10 at 14:22
  • I think that kind of breaking naming convention for the var. Should be like $imagePlaceholder. I will do further updates in answer to your comment. – Anraiki Aug 05 '10 at 14:38
  • I just notice that... making the person jump over more hoop may not be necessary once the "verification" of the owner of the file goes through. – Anraiki Aug 05 '10 at 15:03
  • 1
    *THE ABOVE IS A VERY DANGEROUS PIECE OF CODE.* because: people can put a %00 in $_GET['photo'], and since php doesn't care about the 0 byte **but all underlying libc functions do** you can unlink any existing file. Try this: `` so I repeat: just put it in a database *unless you are really sure you ruled out all possible loopholes, which you almost cannot do* – mvds Aug 05 '10 at 19:18
  • When you said "you", are you particularly referring to me or everyone? The code is dangerous if taken lightly. – Anraiki Aug 06 '10 at 14:48
0

have had the same problem and got around it using PHP's ftp_delete function

The iOSDev
  • 5,237
  • 7
  • 41
  • 78
WIGPIP
  • 1
-1

A different suggestion: don't store files on disk, but put them in a database. This keeps a very clear distinction between your site+scripts and "user data".

(someone once told me that files were files, and databases were for data, and those are different, but as I see it, files contain data anyway. mysql has a perfect LONGBLOB type to put anything in, and you can store meta-data, such as file-type and filename, in separate fields in the same data row, which keeps things clean and simple)

mvds
  • 45,755
  • 8
  • 102
  • 111
  • In the case of images I don't think it's a good idea to store them in the database (for performance reasons). Cause when displaying them you need a PHP script to read from the database. As Images are seperate HTTP requests this will result in multiple connections to the database, that need to be established. – JochenJung Aug 05 '10 at 14:16
  • Fyi, Microsoft SharePoint 3.0 does that (stores files in the DB). I don't necessarily agree with this decision, as I'm a believer that 'file system' should be used to store 'files', and 'databases' for tabular data. But this design pattern would make sense in some scenarios. – Wadih M. Aug 05 '10 at 15:15
  • Please explain to me how a filesystem is not a database as well? At non-exotic volumes, performance is certainly not a problem, judging from experience. The database connection can persist between requests. You have to weigh the downside (performance) to the upsides (ease of maintenance, backup, data consistency, security aspects as in the original question). Think about a mysql replication setup, the horror involved to also replicate the files! – mvds Aug 05 '10 at 15:25
  • @mvds I agree, maintaining file binary data and file meta data through the file system can be non-trivial especially when cross-platform communication is happening (linux, windows, etc). Both technologies have their own advantages/disadvantages. – Wadih M. Aug 05 '10 at 18:57
  • @Wadih: fortunately not all of us have facebook or youtube-like proportions, so the only upside of filesystem-files (performance) is not needed. Leaves a lot of tricky points - see my comment to @anraiki about 0 bytes in strings, in php vs the underlying libc functions. – mvds Aug 05 '10 at 19:22