0

I've created a file upload for my client and I'm trying to make things as secure as possible. I'm using the following code to handle the file upload. The idea is to rename the file and write it to a folder outside the web root.

The question is, during the 'write' process is there any chance that ColdFusion will allow a malicious file to execute before the file is written to the folder and renamed with the following code?

This is at the top of my component...

<cfset destdir = "/folder/upload/">

This is part of the code that handles the file...

<cfset var local = structNew()>
<cfset local.response = structNew()>
<cfset local.response['catcher'] = ''>
<cfset local.filename = listGetAt(#arguments.file#, 1, ".")>
<cfset local.fileext = ListLast(#arguments.file#, ".")>
<cfset local.nfile = #CreateUUID()# & "." & #local.fileext#>

<cftry>
  <cffile action="write" file="#destdir##local.nfile#" output="#arguments.content#">
  <cfset local.response['newfilename'] = local.nfile>
  <cfcatch type="any">
  <cfset local.response['catcher'] = "Write Exception " & #cfcatch.Detail# & " | " & #cfcatch.Message#>
  <cfset local.response['success'] = true>
  <cfreturn local.response>
  </cfcatch>
</cftry>

I should mention that the file upload procedure is being handled by a CFC and Valums' AjaxUpload Plugin...

Ofeargall
  • 5,340
  • 5
  • 28
  • 33

3 Answers3

4

Your sample code looks like you're doing something different from a normal file upload. You don't have a cffile action="upload", and it looks like you're already retrieved the content of the file. You should be limiting the local.fileext to file types that you consider safe and the Arguments.content should be checked to make sure it's not malicious. As soon as you've written the file to the webroot it can be probed through the url, so you must verify that everything is safe before writing it.

With a normal form post file upload the process should be something like:

  1. Use cffile action="upload" to write the file upload into a temp folder outside the webroot
  2. Verify the integrity of the file to make sure it's not malicious (delete if bad file)
  3. Move the verified file into its final location with cffile action="move"
nosilleg
  • 2,143
  • 1
  • 22
  • 36
  • I am indeed using a different method of uploading. I'm using a method written by Sid Maestre and Martin Webb that handles XHR for the AjaxUpload. Here's the sanitized paste bin: http://pastebin.com/d30rXQRh – Ofeargall Apr 20 '12 at 15:45
  • That code has a number of bugs in it (e.g. incorrectly dealing with files with no extension/only an extension/multiple dots in the filename). In addition it doesn't do anything to prevent malicious file uploads as per the 3 steps above. The code should be modified as per my initial statement above. – nosilleg Apr 20 '12 at 16:39
1

To answer the question you asked - your "write" opertation is a single operation. You are not moving and renaming the original file (at least not in the code above). Instead you are creating a file handle, outputting a buffer and closing the handle. The code cannot be executed prior to the release of the handle. If you were moving and renaming or copying the file itself then there could be a gap as you fear - enough to allow an execution. You should also know that file I/O might create problems if you intend to write then execute the file in a single request thread (could get an error trying to get access to the file as Java might hit slightly ahead of the OS on getting notice of the handle release if you see what I'm saying).

Here's a post on cffile hacking that might shed light around the edges of your issue.

http://www.coldfusionmuse.com/index.cfm/2009/9/18/script.insertion.attack.vector

Note - this is my understanding... pretty solid, but there some pretty smart folks on this list including the ones who have responded already. Not trying to steal anyone's thunder here.

Mark A Kruger
  • 7,183
  • 20
  • 21
  • While the above answers have indeed helped me discover flaws in my code and added to my knowledge, this answer specifically addresses my question. – Ofeargall Apr 20 '12 at 18:46
  • I'm not confident this answer is correct. There is the given example of the OS and Java firing in the wrong order, but there's also the fact that CF will parse and execute .cfm files that are mid ftp upload; and the CF directory watcher gateway will also report on files that haven't been finalised. And it's not just CFM files that you have to worry about. In terms of being "as secure as possible" I wouldn't assume that the file isn't probable from the url before it's finalised. If you follow the advise in my answer, which is also in the link in this answer, your app will be more secure. – nosilleg Apr 22 '12 at 10:17
  • This is all dependant on if your /folder/upload/ is under the webroot, and if not then the real answer to your question is 'it doesn't matter'. – nosilleg Apr 22 '12 at 10:17
  • nosilleg - you have a good point, but I believe the issue there is the type of file handle and locking being implemented by the given system. I don't believe that CF "releases" the file till the whole buffer is written so I think the answer is safe (though I've been wrong many times before :) – Mark A Kruger Apr 23 '12 at 14:27
0

Renaming the file at upload and placing in outside the web root is a good idea, but there are still some basic points how you can improve security at fileupload in coldfusion.

For starters cffile with action upload has an attribute "accept" where you can specify which mime-types (comma delimited list) your fileupload will allow.
cffile also has a "mode" attribute (for linux only) to set permissions for the file.
source: http://livedocs.adobe.com/coldfusion/8/htmldocs/help.html?content=Tags_f_02.html

I don't think that an uploaded malicious file can be executed automaticly when it's uploaded that easy, but it's a good practice to take precautions.

jan
  • 2,879
  • 2
  • 19
  • 28
  • 1
    A controversial opinion here... Every CF programmer should pretend that the `accept` attribute doesn't exist in cffile. The mime-type can be falsified meaning you still have to check the file in other ways. The only thing the `accept` attribute does is give you a false sense of security. – nosilleg Apr 20 '12 at 08:58
  • I disagree. It definitely improves security. I'm not saying you should only rely on mime-type check, because it can indeed be masked. But it's still an extra validation on your upload. – jan Apr 20 '12 at 09:06
  • 1
    As I said, controversial. In addition to my points above, using it requires additional code so that you can provide friendly error messages to your users, otherwise they will get a hard CF 500 error. Extra code and a false sense of security makes me dislike the attribute very much. – nosilleg Apr 20 '12 at 09:33
  • I am running on a Linux environment. I hadn't thought of setting the file permissions upon write. Which would work great since all I need to do is attach the file to an email and then delete after the email is sent. – Ofeargall Apr 20 '12 at 15:47