Restrict upload for dangerous file types#370
Conversation
Pull Request Test Coverage Report for Build 13318729670Details
💛 - Coveralls |
|
Current approach is (in web security recommended) whitelist. Here's the suggestion: |
This looks more sensible for me now. |
|
whitelist: ALLOWED_MIME_TYPES = { |
harminius
force-pushed
the
file_upload_restriction
branch
from
edbca50 to
67ac1ee
Compare
tomasMizera
left a comment
tomasMizera
left a comment
There was a problem hiding this comment.
The platform is getting more and more stable 🚀
Let's make sure we are ready to quickly adjust the list in case we accidentally block something we should not have blocked.
| abort(400, f"File {item.path} has been already uploaded") | ||
| if not is_valid_path(item.path): | ||
| abort(400, f"File {item.path} contains invalid characters.") | ||
| abort(400, f"File '{item.path}' contains invalid characters.") |
There was a problem hiding this comment.
How about
| abort(400, f"File '{item.path}' contains invalid characters.") | |
| abort(400, f"Unsupported file name detected: {item.path}") |
to make it clear it relates to the file name(path), not the content of the file
| abort(400, f"File {item.path} contains invalid characters.") | ||
| abort(400, f"File '{item.path}' contains invalid characters.") | ||
| if not is_supported_extension(item.path): | ||
| abort(400, f"Unsupported extension of '{item.path}' file.") |
There was a problem hiding this comment.
I'd suggest
| abort(400, f"Unsupported extension of '{item.path}' file.") | |
| abort(400, f"Unsupported file type detected: {item.path}") |
File type is easier to understand than extension. While it is obvious to us, end users might not know what that refers to :)
There was a problem hiding this comment.
Shall we add also a hint what to do to unblock upload? Please remove the file from the upload folder or try zipping the file before uploading.
""
There was a problem hiding this comment.
Yeah, good idea! 👍🏻 How about Please remove the file or try compressing it into a ZIP file before uploading.?
| continue | ||
| if not is_supported_type(dest_file): | ||
| logging.info(f"Rejecting blacklisted file: {dest_file}") | ||
| abort(400, f"Unsupported file type of '{f.path}' file.") |
There was a problem hiding this comment.
Similarly here:
| abort(400, f"Unsupported file type of '{f.path}' file.") | |
| abort(400, f"Unsupported file type detected: {f.path}") |
harminius
changed the title
5 participants
Resolves https://github.com/MerginMaps/server-private/issues/2732
Problem:
We allow users to upload any file, even the (potentially) malicious ones 🏴☠️
See tickets for more details and the Pentest report
Solution:
We are more confident with using blacklists. 🛡️
Validate extension during project push - aka
push_start. Abort the upload if it includes files with blacklisted extensions.Do not rely solely on the extension check. Determine the file type from its header and block blacklisted types - once we have the file on our filesystem (in
push_finish).Zipping the unsupported makes it possible to upload it.
E.g. block
.exeExtension renaming of the unsupported file doesn't help ⛔
Potential whitelists are in the comment
Do not allow to sync files with extensions unless whitelistedWhitelist approach is suggested in this PR as it is considered to be safer. However, using blacklists is also a way, see this comment for suggested blacklists.