remove mysql leftover #10694

manuel-sommer · 2024-08-07T10:24:30Z

No description provided.

dryrunsecurity · 2024-08-07T10:24:46Z

DryRun Security Summary

The pull request covers changes to various files in the DefectDojo application, focusing on improving code quality, maintainability, and performance, while also addressing potential security concerns related to SQL injection risks, sensitive data exposure, and secure configuration management.

Expand for full summary

Summary:

The code changes in this pull request cover several files related to the DefectDojo application, including utility functions, SQL aggregation, and application settings. Overall, the changes appear to be focused on improving code quality, maintainability, and performance, without introducing any obvious security vulnerabilities.

However, as an application security engineer, there are a few areas that warrant further review and consideration:

SQL Injection Risks: The changes to the Sql_GroupConcat class involve dynamic SQL generation, which could potentially lead to SQL injection vulnerabilities if the input values are not properly sanitized and validated.
Sensitive Data Exposure: The use of the GROUP_CONCAT function could potentially expose sensitive data, depending on the data being concatenated. It's important to ensure that the function is used in appropriate scenarios and that the output is properly handled and displayed to the user.
Secure Configuration Management: The changes to the application settings file include sensitive configuration values, such as encryption keys and session/CSRF cookie settings. It's crucial to ensure that these values are properly managed and protected, and that the application's security-related settings are configured correctly.

Overall, the changes in this pull request appear to be generally positive, but it's essential to continue monitoring the application's security posture and address any potential vulnerabilities or security concerns that may arise.

Files Changed:

dojo/metrics/utils.py:
- The changes remove database-specific logic for calculating the age of findings, replacing it with a more generic approach.
- The code has been refactored to improve readability and maintainability, and the changes should also improve performance.
- From a security perspective, the changes do not introduce any obvious vulnerabilities, but it's important to ensure that the underlying data used to generate the metrics is properly secured and access-controlled.
dojo/components/sql_group_concat.py:
- The changes remove the as_mysql method and update the as_sql method to use a more generic approach for generating the SQL query.
- Security considerations include ensuring that input values are properly sanitized and validated to prevent SQL injection vulnerabilities, and that the use of the GROUP_CONCAT function is limited to appropriate scenarios to avoid excessive data retrieval and potential sensitive data exposure.
dojo/settings/settings.dist.py:
- The changes remove the SILENCED_SYSTEM_CHECKS setting and update the DATA_UPLOAD_MAX_NUMBER_FIELDS setting.
- While these changes do not directly introduce security vulnerabilities, it's important to review the CREDENTIAL_AES_256_KEY setting and ensure that the JIRA_SSL_VERIFY setting is configured correctly to prevent potential man-in-the-middle attacks.
dojo/settings/template-env:
- The changes remove the option to use a MySQL database URL, which is a positive change from a security standpoint, as it reduces the risk of SQL injection vulnerabilities.
- The file contains several sensitive configuration values that should be carefully managed and protected, and the enabled Django admin interface should be properly secured.
- The commented-out settings related to session and CSRF cookie security, as well as the secure content type setting, should be reviewed and properly configured to enhance the application's security.

Code Analysis

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

Analyzer	Findings
Configured Codepaths Analyzer	3 findings

Overall Riskiness

🔴 Risk threshold exceeded.

We've notified @mtesauro, @grendel513.

View PR in the DryRun Dashboard.

dojo/metrics/utils.py

kiblik · 2024-08-07T13:19:45Z

Maybe the following might be removed as well:

django-DefectDojo/dojo/components/sql_group_concat.py

Line 21 in 51cea97

def as_mysql(self, compiler, connection):

This might be adjusted just not sure how:

django-DefectDojo/dojo/management/commands/dedupe.py

Line 58 in 51cea97

# add filter on id to make counts not slow on mysql
django-DefectDojo/dojo/utils.py

Line 2171 in 51cea97

# use filter to make count fast on mysql
django-DefectDojo/dojo/finding/views.py

Line 182 in 51cea97

but I'm getting that MySql doesn't support limits in subqueries.
django-DefectDojo/dojo/finding/views.py

Line 250 in 51cea97

but I'm getting that MySql doesn't support limits in subqueries.
django-DefectDojo/dojo/finding/views.py

Line 2305 in 51cea97

# MySQL won't allow an 'update in statement' so loop will have to do
django-DefectDojo/dojo/test/views.py

Line 97 in 51cea97

# we could try to prefetch only the latest note with SubQuery and OuterRef, but I'm getting that MySql doesn't support limits in subqueries.

manuel-sommer · 2024-08-07T13:38:25Z

I also don't know @kiblik

Maffooch · 2024-08-07T17:11:36Z

I was nervous to remove those things tbh. I did not have the time when this removal work occurred to fully test and ensure there were no breakages. The safe option is to leave them there, but it would be nice to get rid of these leftovers

github-actions · 2024-08-12T16:41:10Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

mtesauro · 2024-08-13T02:02:09Z

I was nervous to remove those things tbh. I did not have the time when this removal work occurred to fully test and ensure there were no breakages. The safe option is to leave them there, but it would be nice to get rid of these leftovers

I agree with going slow on these parts of the code that we're not sure about. We have deprecated MySQL and RabbitMQ but I also don't want to make changes that might actually break MySQL (vs removing GHA tests & entries in compose) until the next minor release (2.38.0 / Sept) to give people a bit more time to migrate to PostgreSQL/Redis especially as it seems removing these has a good chance of busting MySQL users.

We keep getting updates to the GH discussion on migrating to Postgres: #9480

github-actions · 2024-10-15T08:05:14Z