Sanitizing string to prevent relative URI paths

Question

I have this HTTP handler I created to update information in a local SQL Express database.

I realised that it was possible for a user to use relative URI paths "/../../file.zip" as the query string, and would be able to download files outside of the restricted area.

The site is not live yet so it's not a security issue right now, but I would really like to prevent things like this.

I have added a simple string.replace line that removes any ".." from the input query.

Is there anything else that I should do here to secure this?

public void ProcessRequest(HttpContext context)
{
    string filesPath = "C:/Downloads/";
    string fileName = context.Request.QueryString["filename"];
    fileName = fileName.Replace("'", "''").Replace("..", "").Replace("/", "").Replace("\\", "");

    if (!string.IsNullOrEmpty(fileName) && File.Exists(filesPath + fileName))
    {
        context.Response.ContentType = "application/octet-stream";
        context.Response.AddHeader("Content-Disposition", string.Format("attachment; filename=\"{0}\"", fileName));
        context.Response.WriteFile(filesPath + fileName);
        //Do work to update SQL database here
    }
    else
    {
        context.Response.ContentType = "text/plain";
        context.Response.Write(filesPath + fileName + " Invalid filename");
    }
}

In general you should reject invalid input rather than trying to remove harmful sequences - consider what your replacement does to `./.`. There are also a bunch of patterns that cause weirdness, like empty filenames, leading and trailing dots and spaces, control characters, `SHORTN~1.AME`s and potentially the reserved filenames (`com1` et al). Using input in filenames is hard to get right, [especially in Windows](http://msdn.microsoft.com/en-gb/library/windows/desktop/aa365247(v=vs.85).aspx) - much better if you can to (as Jason suggests) use a generated ID for the filename on the local disc. — bobince, Apr 10 '13 at 16:33
@bobince Great tips there. This is why I asked the question, as I knew there would be a better way to approach this, I was just looking for a bit of guidence on an important issue like this. — Rikki B, Apr 10 '13 at 20:42

Fabske · Accepted Answer · 2013-04-10T22:48:15.050

12

I usually use this simple code to check this problem:

(I type it directly so it may not compile, it's just to give you the idea)

private string getPath(string basePath, string fileName)
{
    var fullPath = System.IO.Path.GetFullPath(System.IO.Path.Combine(basePath, fileName));
    if (fullPath.StartsWith(basePath))
        return fullPath;
    return null;
}

The goal is to use Path.GetFullPath. This method will translate any /../ etc to a complete path. Then check that the returned path is in the allowed directory.
Be carefull that this method may returns slighty different path than expected, read MSDN for detailed explanations

edited Apr 10 '13 at 22:48

answered Apr 10 '13 at 14:59

Fabske

2,106
18
33

1

I like the idea behind this approach -- let the system tell you what file it's about to open, and make sure it's valid. If you try to blacklist characters or strings in the filename, you will undoubtedly miss something. – Ryan M Apr 10 '13 at 15:31
Great idea here also. This was what I was trying to do originally, but I failed to find the correct method to use. GetFullPath is the method I failed to find. – Rikki B Apr 10 '13 at 20:48

score 2 · Answer 2 · answered Apr 10 '13 at 15:18

2

You could have Request.QueryString["filename"] actually be a key that represents a file. The key could be a number or a random string if you don't want users to be able to easily guess file keys. You could store the mapping in a database, and use the key to retrieve the local filename (and maybe a display filename if you want to make the two differ and really hide your implementation details).

answered Apr 10 '13 at 15:18

Jason P

26,984
3
31
45

This is the best way to do this I think, but I marked up @Fabske answer because it answered my original question. I'm not sure how to do this, but I will look into using this approach. – Rikki B Apr 10 '13 at 20:54

Sanitizing string to prevent relative URI paths

2 Answers2