Restrict file types #370
Restrict file types #370
Conversation
| @@ -16,6 +16,7 @@ | |||
| from typing import Optional | |||
| from sqlalchemy import text | |||
| from pathvalidate import validate_filename, ValidationError | |||
| import magic | |||
There was a problem hiding this comment.
what about standard python mimetypes library? Look at the example from private repo with images.
|
|
||
|
|
||
| def validate_path(filepath: str): | ||
| if filepath.startswith(("C://", "../", "..\\")) or os.path.isabs(filepath): |
There was a problem hiding this comment.
This could not handle all cases I think. Bonus point: os.path.isabs is not current machine os agnostic. U need to use ntpath module for handling also windows paths, which could be accidentally stored into db. I think then we do not need this hardcoded things. Examples:
Using ntpath on linux could be ok. os.path.isabs is not os agnostic - if you run it on Linux, it's using posixpath. On the other hand, If you run it on windows it's using ntpath.
>>> ntpath.isabs("C:///////fero")
True| validate_filename(filename) | ||
|
|
||
|
|
||
| def validate_file(head, filename): |
There was a problem hiding this comment.
I am not sure why we do not have one function for validating mime types and extensions?
|
|
||
|
|
||
| def supported_type(head) -> bool: | ||
| mime_type = magic.Magic(mime=True).from_buffer(head) |
There was a problem hiding this comment.
u can use mimetypes.guess_type. If not found, use extensions.
I think we can allow all image mimetypes (image/). We could add new mimetype to library, example for webp. This could be also usefull for guessing o geographic files.
>>> mimetypes.add_type('image/webp', '.webp')
>>> mimetypes.guess_type("hej.webp")
('image/webp', None)
Mimetypes is guessing by extension. magic is guessing mimetype somehow?
There was a problem hiding this comment.
I found web with geographic mimetypes too...
There was a problem hiding this comment.
yea, we use mimetypes guess for file download, best to be consistent if possible
Pull Request Test Coverage Report for Build 13237918308Details
💛 - Coveralls |
|
Current approach is (in web security recommended) whitelist. Here's the suggestion: |
This looks more sensible for me now. |
|
whitelist: ALLOWED_MIME_TYPES = { |
| def is_supported_extension(filepath) -> bool: | ||
| """Check whether file's extension is supported.""" | ||
| ext = os.path.splitext(filepath)[1].lower() | ||
| return ext not in FORBIDDEN_EXTENSIONS |
There was a problem hiding this comment.
this will not for some special files
>>> os.path.splitext(".htpasswd")
('.htpasswd', '')
Resolves https://github.com/MerginMaps/server-private/issues/2732 https://github.com/MerginMaps/server-private/issues/2724
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. Block blacklisted extensions.Do not rely only on the extension. Determine the file type from its content and block blacklisted types once we have the file.
Do not allow to sync files with extensions unless whitelistedE.g. block
.exeWhitelist approach is suggested in this PR as it is considered to be safer. However, using blackilists is also a way, see this comment for suggested blacklists.Potential whitelist are in the comment
Extension renaming of unsupported file doesn't help:
Zipping the unsupported makes it possible to upload.