Checking File Path When Deleting A File

Question

If I have a web method that deletes a file when called and it accepts three parameters (cNum, year, and fileName). Do I need to be worried about exploits of this method. The only thing I could think of would be using ..\..\..\ to drive the delete further up the folder structure. that should be pretty easy to remove that. But is there anything else that I should be worried about?

[WebMethod(EnableSession = true, 
           Description = "Method for deleting files uploaded by customers")]
[ScriptMethod(ResponseFormat = ResponseFormat.Xml)]
public Boolean deleteCustFiles(string cNum, string year, string fileName)
{
    try
    {
        if (String.IsNullOrEmpty(cNum) 
            || String.IsNullOrEmpty(year) 
            || String.IsNullOrEmpty(fileName))
                throw new Exception();

        string path = Server.MapPath(@"~\docs\custFiles\" 
                                        + year + @"\" 
                                        + cNum + @"\" + fileName);
        File.Delete(path);
    }
    catch
    {
        throw new Exception("Unable to delete file");
    }
    return true;
}

Answer 1

I would recommend using the GetFileName method on the Path class to cleanse the filename parameter, like so:

public Boolean deleteCustFiles(string cNum, string year, string fileName)
{
    // Cleanse fileName.
    fileName = Path.GetFileName(fileName);

The GetFileName method strips all directory information from a path, which is exactly what you want to do here.

With input like:

..\..\..\filename.ext

You would get:

filename.ext

In return, you don't have to worry about someone injecting a path which would escape the directory that you are targeting (assuming that this filename is user-input or from an open endpoint where someone could enter any input they want).

This then allows you to then append your custom path to fileName.

This only works of course if all of your files are in a pre-defined directory, which it seems it is.

This does not however, do anything to handle deleting files that a user doesn't have access to. If the files belong to another user in that directory, then there's no check here to see if that's the case (but if all users have rights to delete these files, then it's ok).

Also, you might want to use the Combine method on the Path class to combine your paths, like so:

string path = Server.MapPath(@"~\docs\custFiles\")
path = Path.Combine(path, year);
path = Path.Combine(path, cNum);
path = Path.Combine(path, fileName);

If you're using .NET 4.0 or above, you can use the overload of the Combine method that takes the parts of the path as a parameter array:

string path = Path.Combine(
    Server.MapPath(@"~\docs\custFiles\"),
    year, cNum, fileName);

Finally, as Shai points out, if possible (for a complete solution), to make this even more secure you should be enabling permissions on the file-system level.

If you are impersonating the user or using a constrained user account to handle all of the requests, then you should grant that user access to just the ~\docs\custFiles\ directory (and any sub directories).

Anything above that directory the user account should have no access to.

Answer 2

It is a good idea to check the file names and directory names if they are valid file names or not, check them against this char array:

Path.GetInvalidFileNameChars

EDIT:

And you should probably also validate the year and number like this:

bool valid = int.TryParse(num, out temp);