search

django-helpdesk / django-helpdesk

A Django application to manage tickets for an internal helpdesk. Formerly known as Jutda Helpdesk.
BSD 3-Clause "New" or "Revised" License
1.54k stars 643 forks source link

Helpdesk limits which files can be attached #1162

Closed timthelion closed 4 months ago

timthelion commented 6 months ago

@gwasser What was your initial reason for adding this check? https://github.com/django-helpdesk/django-helpdesk/commit/aff67184d4f4ecb9ccf7045fcebe132c22a43c0d

https://github.com/django-helpdesk/django-helpdesk/blame/f379bbe80e52f4470755b78c2fad91a59fa213ed/helpdesk/validators.py#L36

I find it quite limiting and don't know why it exists.

gwasser commented 6 months ago

An attempt at having some more secure defaults. Whenever allowing file uploads from the public, there is a risk of malicious files being uploaded for command injection, cross-site scripting attacks, etc. So my thought was to have it limited by default, and projects that want to change or remove the limit (perhaps because they restrict users that can upload files and turn off the public portal) can change the configuration for it.

However simply validating file extensions isn't enough on its own to deal with the problem. It was kind of a placeholder until more robust stuff was implemented but then I never got around to it.

The OWASP "cheat sheet" provides more recommendations on handling uploads securely. It does recommend restricting the file extensions, but also many other measures. I think django-helpdesk may already do a lot of this but it would be worth double checking and implementing any gaps. See: https://cheatsheetseries.owasp.org/cheatsheets/File_Upload_Cheat_Sheet.html

timthelion commented 6 months ago

I don't think that this is a meaningful thing to do. If you're trying to prevent xss on display of the attachment, certainly html shouldn't be among the allowed file types. If you are trying to prevent the user from accidentally running an attached .exe file that's the job of the web browser.

gwasser commented 6 months ago

This page from OWASP describes potential security issues and ways to mitigate: https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload. It does recommend restricting both file names and file extensions to smallest known set needed, in order to reduce the attack surface. Plus some other measures. Not all of it can be done by a Django app, I get it, but I recommend trying to do as much as makes sense.

Who uses a Helpdesk? I think in the most common cases, folks will upload logs (text files), Word docs, or screenshots of error messages (images, most likely jpg or png), so that's why I picked this subset.

If the team thinks that's too restrictive default for a generic app, then feel free to change it. I will strongly recommend that the capability is left in place, made a little more friendly/easy-to-use (perhaps make a set of defaults that can be turned on or off easily by users in the project settings), update the install docs to make these risks very clear, suggest that folks configure the smallest subset necessary for their own use case when running in production, and a link to OWASP.