Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Threat Uploads: Server side file extension validation + force downloads #11135

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

Maffooch
Copy link
Contributor

@Maffooch Maffooch commented Oct 25, 2024

To be consistent with the main Manage Files features across objects, the threat uploads feature needs to have server side extension validation, as well as forcing files to be downloaded

[sc-8130]

Copy link

dryrunsecurity bot commented Oct 25, 2024

DryRun Security Summary

The pull request focuses on improving the file serving and handling functionality of the application, including replacing the use of FileResponse with a more robust generate_file_response_from_file_path utility function, enhancing the generate_file_response function to accept a FileUpload object, and adding a new UploadThreatForm to validate file extensions for threat model file uploads.

Expand for full summary

Summary:

The code changes in this pull request focus on improving the file serving and handling functionality of the application. The changes include:

  1. Replacing the use of FileResponse with a more robust generate_file_response_from_file_path utility function in the dojo/engagement/views.py file. This change ensures that the correct content type and headers are set in the response when serving files.

  2. Enhancing the generate_file_response function in the dojo/utils.py file to accept a FileUpload object as input and call the new generate_file_response_from_file_path function. This provides a more uniform and maintainable way of serving files.

  3. Adding a new UploadThreatForm in the dojo/forms.py file, which allows users to upload threat model files (JPG, PNG, PDF) and validates the file extension to ensure only valid file types are accepted.

From an application security perspective, these changes are generally positive, as they focus on improving the security and reliability of file handling and serving within the application. However, there are a few additional security considerations that could be addressed, such as:

  • Validating the actual file type, not just the extension
  • Enforcing file size limits to prevent denial-of-service attacks
  • Sanitizing and encoding file names or metadata to prevent injection attacks
  • Ensuring secure file storage with appropriate permissions and access controls

Overall, the code changes demonstrate a good understanding of security best practices and a commitment to improving the application's security posture.

Files Changed:

  1. dojo/engagement/views.py: The changes replace the use of FileResponse with a call to the generate_file_response_from_file_path utility function, which ensures the correct content type and headers are set when serving files.

  2. dojo/utils.py: The changes enhance the generate_file_response function to accept a FileUpload object as input and call the new generate_file_response_from_file_path function, providing a more uniform and maintainable way of serving files.

  3. dojo/forms.py: A new UploadThreatForm has been added, which allows users to upload threat model files (JPG, PNG, PDF) and validates the file extension to ensure only valid file types are accepted.

Code Analysis

We ran 9 analyzers against 3 files and 1 analyzer had findings. 8 analyzers had no findings.

Analyzer Findings
Authn/Authz Analyzer 1 finding

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@mtesauro mtesauro merged commit b60eefd into DefectDojo:bugfix Oct 28, 2024
73 checks passed
@Maffooch Maffooch deleted the threat branch October 28, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants