The scenario

Let’s say that you have system where customers can logon to download their invoices. We’re using ASP.NET MVC and the action that retrieves our list of invoices is quite simple, it looks like this:

public ActionResult MyInvoices()
{
    // Get the file names from the persistent store
    var documents = new[] { "Filip_20130712.pdf", "Filip_20130713.pdf", "Filip_20130714.pdf" };
 
    return View(documents);
}

In reality we would’ve fetched the document names from the database, or maybe even just fetching the invoice dates and concatenating the file names ourselves. The view that shows the user the list of invoices and lets the user download them is just as simple as the code fetching the documents:

<h2>MyInvoices</h2>
<ul>
    @foreach (var document in Model)
    {
        <li><a href="/Home/DownloadDocument?document=@document">Download @document</a></li>
    }
</ul>

This gives us the following beautiful website that shows a list of our invoices:



Now let’s take a look at the implementation of the method that lets us download the invoice. Here’s how I want it to work:
  • Verify that the document requested exists on the server
  • Feed the document to the client
Simple enough, right?

Simply enough we can use Server.MapPath to get the current directory of our application and then use Path.Combine to ask for the document in our Invoices folder.

public ActionResult DownloadDocument(string document)
{
    var documentPath = Server.MapPath(Path.Combine("Invoices", document));
 
    if (!System.IO.File.Exists(documentPath))
    {
        return null;
    }
 
    return File(documentPath, "application/pdf", document);
}

Now, if we run this application and navigate to /Home/MyInvoices we’re going to see the list that we saw in the screenshot above. If we click the first document and request it and at the same time debug the application, we’re going to see that it requests a file from \Home\Invoices\. So far so good, right?

Security?


You might have already figured out that there’s a huge security risk with the above demo, let’s try something crazy. We know that it combines the text that we pass to the action with the current path, what happens if we append some dots and slashes?

Navigate to the following address:

/Home/DownloadDocument?document=..\..\web.config

Do you see that this is a security risk now? Here’s what just happened:


How do you make it more secure?


Most important of all: don’t use the data that the user requests directly to find the files on disk. Rather have an Id (Guid) passed to a method that looks up the file name in the database and the fetches that from your disk, this way the user never, ever, gets to decide where to download the data from

So what did we learn from this?


Don’t trust the data passed to your actions. This might be truly obvious to a lot of you, but I’ve seen this code in production code so think twice before you introduce insecurity in your applications.

This article was originaly posted on blog.filipekberg.se.