First post here, so time to make a fool of myself I guess.
I’ve been working on a file upload script, and as part of debugging an unrelated issue I dumped out form/url/etc parameters, when something jumped out to me. The name of the temporary file was this:
/var/www/html/WEB-INF/lucee/temp/tmp-13.upload
which just seemed really… basic. This was then followed up by another thought. The format looks pretty simple, just something like tmp-#itr#.upload
Could an attacker try submitting a bunch of requests, and just iterating through that value? I don’t even think they would have to submit a file, since it’s essentially just a text field containing the filename at that point.
I was hoping someone would give me a quick rundown on what happens behind the scenes when a file gets uploaded, and how these names are generated, and if there are measures in place that prevent an attack like this from happening?
And would it hurt anything to have the names be a bit more complicated? Usually I do something like file_#getTickCount()#_#CreateUUID()#.temp
when I need a file to have a unique/difficult to guess name.
EDIT: Felt like I should go ahead and note this as well. I am assuming there are measures in place, but I wasn’t able to find anything in the documentation about it. My guess is that the temp name placed in the form field doesn’t actually correspond file name as it’s stored on the disk but is rather mapped in some table, possibly stored in the session variable, which is where the original name, date last modified, etc are also kept. Again though, that’s just a guess.
That is a good point, but I’m not sure we’re thinking of the same thing. My concern wasn’t that the files were accessible by URL, but rather that another user could “upload” another file in the temp directory, simply by sending a filename as a form parameter.
What I do:
I don’t allow any user to specify a filename. Never! He may specify it, but it will be ignored, because I specify it as an uuid during that users session and that will serve as the name of the uploaded file that is also saved in a directory outside the webroot without execution rights (on win). The user can specify a name how he likes. It won’t be saved like he wants. I also place an application.cfc with read only rights in that directory, and that application.cfc has just one and only line with a cfabort, so that even if the attacker could somehow overcome to place a cfm/cfc file there, it would be difficult to execute it. And I would also make use of the nameConflict attibute and set it as makeunique.
I completely agree with all that, but again that’s not the issue. My issue isn’t with the name my code save it as, it’s the temp name coldfusion generates. Whenever you have a script that uploads a file, before your script even begins, Coldfusion has already saved the file to a place in the temp directory with a temporary name. When you call <cffile action="upload"....> You have to pass it the form parameter, which contains that temporary name.
My concern is that the naming scheme Coldfusion uses for the temporary file is extremely basic, and might allow for an attacker to perform an upload with another user’s data, simply by guessing the form name.
They don’t control server side, but they do control what is sent in the form data.
What would happen if instead of sending a HTTP request containing a file to upload, they sent a form field, that has the same name as the file field, but whose value is the name of a temp file uploaded by another user.
I think my main point though is that Lucee should be using something like CreateUUID() whenever the temp file name is created regardless. It would have basically zero performance affect (especially when you consider the overhead of uploading a file), and would only increase security by adding an additional layer of redudancy.
Putting the risk discussion appart, I have to admit that having createUUID() wouldn’t harm at all, and it would make name of a file more unpredictable than the actual way. May be it is valid to create a ticket with security enhancement and let the lucee core devs decide? Thoughts?
Wait, just a point of clarity – are you all saying that by default, the temporary files created during a file-upload are web-accessible unless you explicitly change the location? I’m not sure I’m understanding that correctly.
No Ben, not that I am aware of. That was one of my greatest concerns when I switched from ACF to Railo, and I did some tests. At that time I wasn’t able to do anything like that. From what I understand, @jacobfw has a concern that somebody somehow can overwirte and take over a file that somebody else is trying to submit, by overwriting it somehow. However, I don’t know how this could be achieved. But I admit that UUIDs are more unpredictable than some ID.
I get a nginx Forbidden page when trying to browse /WEB-INF/temp in production, so at least that’s good.
I mostly asked because, historically, ACF and getTempDirectory() has been non-web-accessible as far as I can recall. I’ll keep this stuff in mind for the future.
Not overwrite. Just that another user session could force the upload process to use the same file another user has uploaded if the name is predictable.
Regardless though, I think everyone is agreeing with the conclusion that the names are better off being unpredictable. Even if the attack I’m talking about is not possible now, having a randomized name would provide redundancy in case some one accidentally changes that and it goes unnoticed.
Also @bennadel, huge thank you for all your articles on Coldfusion/Lucee. They’ve been a lifesaver ever since I started working with it several years ago.
I think there’s a tiny chance under load that makeunique might conflict, if multiple users upload the same filename at the same time, as I think there’s no locking between generating a unique filename and actually writing out to the new unique filename. both could end up with the same destination filename. I guess one of the requests would then fail at createNewFile…
still , that’s not a security problem, just that the request would crash
Please correct me if I’m wrong, but I don’t think that’s what I was referring to. That looks like where the filename is generated in the cffile tag. I’m referring to the name of the temp file that gets created and is stored in the form struct, which is then passed to the cffile tag to formally upload the file.
The name of that temp file is what I believe should be randomized.