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.
Cool. Yeah, I would suggest we have it insert a UUID into the filename as well.
Just to reiterate. My concern is that if instead of uploading a file to an upload script, someone sent a post with a form parameter with the same name as the file parameter, but instead it had the name of a temp file. Having it make the name of the temp file extremely unpredictable would drastically mitigate it.
WEB-INF folder should be blocked by default on any servlet engine, but we add a .htacces file to WEB-INF and the lucee folder just in case. but the best is to configure the web config somewhere outside the webroot anyway.
But think it is a good idea to make the temp name random and synchronise the method.
Could someone raise a ticket for it?
I’m use to having to fight with people over topics like this, where just a small change is needed, but people either don’t want to admit they’re wrong, or don’t want to have to go through the small effort of making the change. Always nice to see people take these things seriously.
As a matter of fact, I, as an usual cfml dev using Lucee, would like to thank YOU for taking your time and stating your point of view here. Your action will lead to security enhancement and all of us using Lucee will take advantage of your contribution. +1 for it being your first post here in our community. Much appreciated.
The generation of these temp upload file names is now thread safe and the related problem of the files being extracted again for each new thread has been resolved in 5.3.8.12