Are You Serving Files Insecurely in ASP.NET

Posted by Filip Ekberg on July 12 2013 29 Comments

I recently came across something quite interesting when it comes to serving files from your application. In theory what will be discussed here applies to all programming languages that you use for web programming and not only ASP.NET. However the examples used below will be targeted at ASP.NET developers.

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:

MyInvoices

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:

MyInvoices3

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.

I really want to hear your thoughts on this and maybe if you have any insecurity stories of your own!

Vote on HN

29 Responses to Are You Serving Files Insecurely in ASP.NET

  1. BriceNo Gravatar says:

    And when a GUID is requested, re-check that the user has the rights to request this GUID. Because, once again, you can’t trust the GUID, it’s a user data.

  2. PhilNo Gravatar says:

    The classic phrase “never trust input data” is sometimes forgotten, this apply to all applications; just not web apps.

  3. Pingback: Dew Drop – July 12, 2013 (#1,584) | Alvin Ashcraft's Morning Dew

  4. AnonNo Gravatar says:

    Call `Path.GetFileName` to clean it up before using.

  5. lolnoNo Gravatar says:

    Seriously, even back in 2004 stuff like this was a beginners mistake, how is this on HN? Not exactly good publicity for you as a software engineer if you ‘came across’ this just now.

  6. Filip EkbergNo Gravatar says:

    I’d say this was even a beginner mistake long before that. However that doesn’t mean junior developers don’t tend to do the same mistakes that todays more senior developers did back in the day.

    There’s always going to be new junior developers out there and sometimes even the senior developers need to have their minds refreshed. I never stated that I came across this in a solution that I was a part of, I still found the information to be valuable to share.

  7. Niall MerriganNo Gravatar says:

    Its more common than you think. Directory traversal attacks are an unfortunate byproduct of people forgetting that you can never trust your users. I did part of this in my NDC session

  8. JohanNo Gravatar says:

    Nice post. But why spend hours implementing guid-system with db when you simple can add two-three lines code
    that make sure the only files in the list are allowed once? like all files must end with *.pdf
    Or use contentype to check it’s true nature :)

    As developer you know the req from the customer so what really was a miss in this action was a “pre- and post-condition check.”

    As you can se in your code: return File(documentPath, “application/pdf”, document);

    This is typical contract, you already know this must return pdf files so you can easily check the inputparam document if that one is *.pdf or also check the content-type of the file itself or more simple just let them access one folder only, your customer/download folder… not with Server.MapPath

    And before the return statement you simple add a post-condition to verify that the file that existed really was an pdf file. If you missed the check with the input-param. This is why Design by contract are so useful in many scenarios.

  9. Pingback: The Morning Brew - Chris Alcock » The Morning Brew #1398

  10. Filip EkbergNo Gravatar says:

    For security reasons it’s not a good idea to simply use the input to find a match on disk. Even if you check that the filename ends with pdf it’s not certain that the file was actually fetched from that folder. You could of course exacpe the filename, but I’d still argue that the more secure option is to never let the client ask for the files on disk directly.

    I don’t think it takes hours for an experienced developer to do this and it’s more secure than any name escaping/checks.

  11. JohanNo Gravatar says:

    In the real world customer don’t pay for overkill features. If not needed.
    That’s a fact not an issue. KISS is more important than complexity. When it comes
    to deadline, and money. But if the customer decide to put some extra hours work to implement the guid-solution and have no problems with deadlines than why not :)

    You show urls all the time in you html anyways, in this case IIS could not protect you because of the File type return. So it’s no big deal expose an url in thml. the big deal is the fault in the implementation it self.

    But it’s a more secure problem if the file-names lack important data and everyone can read the file-name, but that’s another issue here :) and probably this is a secure site for Intranet use. where the files should be added to a secure file-server for the customer rather than on the project path from the beginning.

    right?

  12. JohanNo Gravatar says:

    Fast fix if the problem exist today rather than spent mony and time to rebuild front-backend, uploadfeatures etc…
    Based on Design By Contract in code principle. (Defensive programming)

    public ActionResult DownloadDocument(string document)
    {
        if(!CheckFileIsCorrectFormat(document,FileType.PDF))
            return null;  //or throw exception if you want to log wrong usage can be useful for other devs reusing this method
     
         //skip Mappath and go to a fileserver path instead if possible
         var documentPath = Server.MapPath(Path.Combine("Invoices", document));

        if (!System.IO.File.Exists(documentPath))
            return null;  //or throw exception if you want to log wrong usage can be useful for other devs reusing this method

        //or check the that the file is ".pdf" application/pdf is the conract so this methid will never return .jpg enyway.
        var file = File(File(documentPath, "application/pdf", document);
        if(!CheckIfFileIsContentType(file,ContentType.PDF))
             return null; //or throw exception if you want to log wrong usage can be useful for other devs reusing this method

        return file;
    }
  13. Pingback: Dew Drop – July 15, 2013 (#1,585) | Alvin Ashcraft's Morning Dew

  14. FisherNo Gravatar says:

    Johan,
    i don’t think your solution is good. All you did is put some restrictions on file type and directory…

    But what about accessing different users invoices? Usually there is another process that creates invoices and if there is some file naming sequence and othere users can try to guess file names?

    It is always a good thing to make sure that user accesses just the content that really belongs to Him.

  15. JohanNo Gravatar says:

    Fisher,
    I see. The thing is that my solution was just a fast fix for an already open problem. Instead spend hours in rewrite code (if customer don’t want to pay for it) this one was a fast fix and also a way you always shall code anyway. ppl seams to miss defensive programming and handle code contracts. It solves lots of problems as is.

    If you change the MapPath and path.combine (I just added a comment over that line) to access a more personal folder on a fileserver (eg) Customer GUID it will suddenly be much more secure.

    I would also in MVC add an Authorize action-filter that will check if you are really you and have the correct permission to call that controller.

    /Home/DownloadDocument?document=@document <---- for this call

    Eg.
    If this is the user 2424-21312321-213123-21321 do I have access to file-server folder with guid 233F-32232-21323-3213-231

    But as I also said, if the filename lack information I think the guid solution is great. And the big picture and question here is… Are we telling ppl to fix the above problem or just do something else next time you want something similar?

    And how secure will we make things? Anv Action-filter with restriction can we reused in most of the calls.
    A DbC and defensive programming will also add better protection than many pages out there today. And what does the requirements from customer really say regarding the data you will let ppl access?

    Those are the questions that will make you as a designer/architect the idea of the best solution here. Maybe they want to show the file name why do the work around with guid then? When a reusable Action-filter will do the job for you and a restricted folder on a file-server? etc…

    This is a cool post by my Mate and colleague Filip, its open for many thoughts. I just wanted to add the value that to many developers miss, the customer values, the time, YAGNI issues, to many devs add to much complexity, ran out of time to fix something that do not need a big fix. And also create features the customer never wanted to pay for.
    And that’s the trickiest part as a developers to find that good enough solution for the problem that generate less time, less complexity, easy to maintain solution.

  16. TomaszNo Gravatar says:

    Johan,
    About Your solution I can agree it’s quick fix but I must say it’s dirty and You over estimate it’s value in production code. First of all every experienced developer knows that requirements for software change on daily basis and depends on client needs. In Your way You will end up by copying Your quick fix over and over again and changing file types maybe some paths to meet requirements. Second of all Your solution is good as long as You think about MVC, what about WebAPI or WCF etc. This time Your solution needs some major refactoring.
    So in my opinion it’s better to develop more generic solution which is based on GUID’s and user permissions put it in external library code and simple reference it in my projects without tying it up to Presentation layer. So down the road I will do my job once and benefit from it in many solutions save time and money with proper solution for my clients.

  17. JohanNo Gravatar says:

    Tomasz,

    Thanx for you comment, but I do not understand the part regarding copy the fix over and over again?
    I was referring to the lack of DbC and Defensive programming. And code by those principle is not a copy and fix issue it’s just ordinary principles.

    In WebApi you can still use actions-filters. And yes of course in other techniques you need something else.
    But only use Guid for files and think you are safe is not the only option here. In that case you must have an database.
    Save file-names, guids and relation id etc. This is not the silver bullet that fix the costumer needs. It’s an solution and idea as all others that I do agree here :)

    What if the customer don’t want to use a database? or does not need to? maybe they use document database etc…
    Or there own file-server?
    What ever solution you decide to use you always got new or other requirements from the customer, that’s why it’s so important to really understand there needs and use the solution best suited for them, the scenario etc…
    Thats what I really refereed to in my latest comment. It all depends :)

  18. TomaszNo Gravatar says:

    Johan,
    I think Filip in his post pointed out very common and rookie mistake in many solutions. I think he presented reasonable direction of solving this problem.
    You presented Your “Fast fix if the problem exist today rather than spent mony and time to rebuild front-backend…” which value You over estimate because You need time to copy and paste it ( at least code in Your controller to next solution to use it) Your “business logic” is in the controller which is next bad idea, and using ActionFilters tying it up to MVC framework which is another problem. So You end up with copy/paste and framework specific solution. In this particular case serving files securely is very generic requirement in many kinds of applications so it’s worth to do it right and than use it over and over again with some small modifications. Another mistake in Your “solution” logic, Data Store is not a concern of input validation logic, this is another example why Your solution is bad and You have to modify it every time when data store is changed.
    You are writing a lot about rules to justify Your “Fast fix…” there is no rule which justify this kind of “spaghetti” code which You presented unless You are making some quick test fixtures.
    There is no silver bullet solution but one of the crucial thing in enterprise programming is re-usability and maintenance of code if You use a right approach You can reuse a lot of Your code over and over again with out a need to modify it.
    If Your production code looks like You presented above this is impossible.
    Please don’t get me wrong I don’t want to offend You I simple say that Your solution is not good in production code and shouldn’t be used in any case but some very simple and basic apps.

  19. JohanNo Gravatar says:

    Tomasz,
    I don’t see what make DBC “spaghetti”?
    You still need the main functionality to read the file even if it’s via a GUID that gives u the path or not right?
    And what happens if the dude who upload a file upload html-script file and you do not re-code it? or check the
    file-extension is the type you expect it to be? The guid solution only fix one problem. (Hard to read the path to the file because the solution used path.combine and added all the files on the project folder it self.)

    I should have done this contract code anyways. Even if I made a Guid layer that give me the path that I sent to
    my reader. My contract only do two things. Check that the input it what I expect it to be. And my output as well. So nothing happens between (If and only if my service class or something calls other APIs I don’t have control over).

    One interesting point here “enterprise programming”? Some system are small :) and not in the state of
    Enterprise level. Why not aim small and shoot big?
    I put a note on my door “No advertisement” I don’t want to put up the full arsenal at home to prevent some flyer. But it would be a great idea if I was under threat, than my note on the door will not protect me :)

    I can’t see what makes my code with param check “spaghetti” and I cant see what makes an Action Authorization, Authentication filter a “spaghetti” issue either, if the system is based on ASP .Net MVC (if that best suited for the customer.).

    I’m still confused about what I need to copy for one to project to the next. The whole fileloader service class?
    Or the param checks that shall exist there anyway? Or the reusable function that takes a file-name and the extension I want my simple reusable infrastructure SRP method to check for me?

    Maybe I’m to old or need to get a drink… Hopefully the last one :)

  20. Mattias KarlssonNo Gravatar says:

    Hi Filip,

    I tweeted you earlier this week because, well I didn’t agree 100% with your solution (and strongly disagree with some of the commenting solutions here:)). Think it’s kind of security by obscurity,

    I made a gist at GitHub of a solution I think is more secure and also will save disk space and make better indexes… that said it’s a quick n dirty sample done late at night:)
    The gist is here

    To descript basically what my InvoiceController does.
    1. Loads user available invoices from database (well to keep it simple I used an dictionary but you get the point)
    2. Every time a user loades invoice “file” list new temporary id’s are generated
    3. These id’s are stored in a cache (Inproc in sample but can be azure, redis, sql etc.)
    4. As value the internal id of the file is stored
    5. When user goes to download file it tries via the cache to get database id of file via unrelated temporarily id
    6. If it finds an id it tries to load file details via file internal id
    7. If user can access file and it’s found it it’s returned to the user as invoice public id (i.e. ocr) with pdf extension which is not the physical file name so no relation here

    How is this more secure, well I think it’s more secure because of:
    1.Links are user specific and not database record specific
    2.Links are temporary as the cache expires items they die
    3.Theres no relation with exposed filenames/id’s and the physical files/database records
    4.Adds user ip / browser agent to cache key which makes it a little bit harder to use stolen links and if user shares link by “mistake” it most likely wont work.
    5.Opposed to some solutions by commenters only known files can be served as record to physical mapping is done a head of time and not based on user input.

    It could be more secure if I removed cache items as they are downloaded, but this can also affect usability. Also shorter cache time adds more security, but can result in bad usability, analytics can be good here, if you have real world data on average time from list file to download, well then that can guide you in how the cache expiry strategy should be.

    Also a benefit with this solution is that you don’t have to change database schema (if you already have file tables).

    You don’t have to use GUIDs in database which saves bytes, also generates better indexes and insert performance as the randomness of GUIDs.

    To add even more security you could expire entire users cache each time user lists files. This would make it only possible to download “last” listed files.

  21. Alan MosleyNo Gravatar says:

    Why not store the file in the database?
    seems the easiest thing to do,

  22. Filip EkbergNo Gravatar says:

    @Mattias I think that adds more complexity than what is needed. Consider that we have a database table with the following structure:

    InvoiceDocument
    ---------------------------------------------
    Id: uniqueidentifier (guid)
    Filename: nvarchar
    UserId: uniqueidentifier (guid)

    When you request the list of all your invoices you simply fetch all the InvoiceDocuments that correspond with your logged in UserId. You can then use the Filename to display the download link but have the link point to somewhere, where you expect a Guid — the Invoice Id.

    Then when you request to download a file based on the Id you simply verify that the file belongs to the logged in user — by validating the UserId. It’s extremely time consuming to bruteforce a Guid. Surely it does affect performance but just slightly, I don’t think you’d notice a huge difference outside stresstests/benchmarks.

    It doesn’t really matter if the invoice is located on disk, a file server, a blob or in the database that’s up to the controller, but as long as it doesn’t use the string directly passed to the action to locate it but instead use the Guid to “translate” what it really is looking for I think you’re safe.

  23. Arunesh MishraNo Gravatar says:

    I’d implement these points:

    1) Store the download folder name (or path) in your config file. Then match the parent folder(or path) with it just before returning the file to the user in (action method)

    2) For more security check the file extension & content type to verify that it is only desired file ( *.pdf in this case) as Johan says above.

  24. Peter "Shawty" ShawNo Gravatar says:

    I’m going to make one simple comment here, and it goes something like this:

    I don’t care about money, time taken or how quick a fix is. Put simply, security IS SECURITY and if your code, assets and your data are all precious (Which undoubtedly they are) then you have an obligation to make it as secure as you can, it’s that simple. If you have users personal data in your data base you don’t have an obligation, it’s LAW (or it should be) that you make it secure, and if you don’t then your no better than the hackers themselves.

    I apologize if my words are strongly worded, but this whole cut corners to please the enterprise crap really makes my blood boil these days, esp when I hear it from senior guys that should know better.

    Johans comment is typical of what I see from many developers these days:

    The thing is that my solution was just a fast fix for an already open problem. Instead spend hours in rewrite code (if customer don’t want to pay for it) this one was a fast fix and also a way you always shall code anyway

    Your clients and your bosses count the pennies for sure, but they DON’T understand what you understand, there for you should NOT be cutting corners esp not on matters of security.

    Filip’s comments about putting all of this in a database and accessing it via a GUID or some other kind of ID system should always be the chosen method, I don’t care how much shouting you get from your project leader, web security these days is a very big deal, and here in the UK your company (Not you) can get fines ranging into the millions of £’s just for letting some little script kidde get hold of a database of Email’s, as for credit card numbers and other similar stuff, why not let your company get hacked and see what happens…

    I willing to be it’ll cost them waaayyy more than the extra few thousand it cost for you to spend time implementing the code correctly than doing a “Quick Fix”

    @shawty_ds

  25. JohanNo Gravatar says:

    Peter “Shawty” Shaw

    I see. So what happens if a developer did the above code as Filip refer too. Someone sees after a release that this code exist though there was no more time to spend on the project. You have to options here.

    1… Ask for more money to rewrite the code from customer or spend your own money to fix your employees miss.
    2… Add a check on the input param that it is only a file and a file with correct format.
    In this case check so the input string document does not contain path information only a file name and with .pdf?

    I had a comment above that some developers really hate the idea of money, because all they want is creativity, motivation, do fun stuff. Cool unnecessary functions (most of the cases) are more fun to code than money is an option or the stopper that is.

    I don’t think your words are “strongly worded” and no apologize are needed.

    Beside me and Filip has the same boss ;)

    “Filip’s comments about putting all of this in a database and accessing it via a GUID or some other kind of ID system should always be the chosen method”

    I’m more interested in this comment. Why is that?
    What makes a guid instead of a filename more secure that ask for a path to the file? obsecurity?
    If you do not do a check of the input data user pass to your method and not check that the return info is really what the person was asking for? The thing is IT doesn’t matter in this case if you use a guid based input param that guive u a file-path if you do not check the data you accessing or sends as a return. My comment was just plain old defensive principle and DBC principle a kind of Liskov principle. Do not allow input that you do not verify or never return something you never asked for.

    For me there is no problem if you rebuild the whole thing if you pay for the time you must spent on doing so for something that’s not really needed in this scenario thought the only problem with the code above was that no check was doing if the input param was of correct format. you can pass ../../ etc … and that was the real problem. Nothing more and nothing less… right? or wrong?

    best, Johan

  26. rikardNo Gravatar says:

    I agree with not allowing the user to type but making them select from a list.

    Storing files in a DB is still a bad idea even though they keep trying to make it less suboptimal.
    I would put an authorization attribute on the action that returns the file, or the corresponding check in whatever other hosting solution you use to deliver the physical file. You should have secure centralized identity within a large organization, and in the small case you are serving files right out of the same app context.

    I also agree with temporary links that cannot be used externally to determine file locations. Just because a user is who (s)he says (s)he is doesn’t mean they still should have access to invoices requested a while ago.

  27. HassanNo Gravatar says:

    Nice post, i would agree with most of you that we cannot trust the input from user, the following can also be avoided by having url encrption alogo in place.how many of you would agree with that?

  28. Pingback: MVC Async File Upload | Software Engineering

  29. Pingback: 2013 was an amazing year, here's a summary!

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>