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

summarizing multiple similar findings into problems #11432

Closed
wants to merge 15 commits into from

Conversation

LeoOMaia
Copy link

Description

  • Feature:
    We created a "Problems" tab that disambiguates similar findings based on the script_id that detected them. This allows us to consolidate most findings into a single problem, enabling the vulnerability analyst to more accurately identify all types of issues without duplication.
  • Bug fix implemented by this PR:
    A bug in the OpenVAS XML parser has been fixed, where it was not correctly identifying the script_id and was always returning None.

Test results

We tested the creation and association of findings to a specific problem according to the JSON file we provided, which already identifies similar findings by grouping them based on their script_id. We also tested that after creating the findings and problems, deleting a finding would update the problem by reducing the number of associated findings, and if all findings related to a problem were deleted, the problem would be automatically removed. Additionally, we verified the logic where if all findings become inactive, the problem status changes from open to closed, and if at least one finding remains active, the problem stays open.

dependabot bot and others added 2 commits December 16, 2024 18:06
Bumps [nanoid](https://github.com/ai/nanoid) from 3.3.7 to 3.3.8.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md)
- [Commits](ai/nanoid@3.3.7...3.3.8)

---
updated-dependencies:
- dependency-name: nanoid
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@github-actions github-actions bot added New Migration Adding a new migration file. Take care when merging. docs ui parser labels Dec 17, 2024
Copy link

dryrunsecurity bot commented Dec 17, 2024

DryRun Security Summary

The code changes to the Defect Dojo application focus on enhancing security problem management through improved problem tracking, vulnerability metadata handling, performance optimizations, and the introduction of a new Problem model to better organize and prioritize security findings.

Expand for full summary

Summary:

The provided code changes cover a wide range of updates and enhancements to the Defect Dojo application, with a focus on improving the management and tracking of security problems and findings. Key highlights from a security perspective include:

  1. Problem Management Integration: The application now has the ability to associate security findings with specific "problems" or issues, allowing for better organization, prioritization, and tracking of remediation efforts.

  2. Vulnerability Metadata Handling: The code changes include improvements to how vulnerability information, such as CVSS scores, CPE data, and vulnerability IDs, are extracted and utilized within the application.

  3. Caching and Asynchronous Tasks: The application has introduced caching mechanisms and asynchronous tasks to improve performance and efficiency, which require careful review to ensure they do not introduce any security vulnerabilities.

  4. Input Validation and Sanitization: While the changes generally appear to be well-implemented, it is crucial to ensure that all user-supplied input is properly validated and sanitized throughout the application to prevent common web application vulnerabilities, such as SQL injection and cross-site scripting (XSS).

  5. Access Control and Authorization: The application should have robust access control and authorization mechanisms in place to ensure that users can only access and modify the data and functionality they are authorized to interact with.

Overall, the code changes demonstrate a focus on improving the security management capabilities of the Defect Dojo application, but it is essential to thoroughly review the implementation and conduct regular security assessments to maintain a secure and reliable application.

Files Changed:

  • .dryrunsecurity.yaml: This file contains a list of sensitive code paths and authorized contributors, as well as a notification list for changes to these paths. This is a positive security practice.
  • dojo/db_migrations/0219_problem_finding_problem.py: This migration introduces a new Problem model and associates it with the existing Finding model, enabling better tracking and management of security issues.
  • dojo/models.py: This file includes the implementation of the new Problem model, which provides a structured way to manage and track security problems.
  • dojo/problem/helper.py: This file handles the caching and loading of a JSON mapping between script IDs and problem IDs, which should be reviewed for potential security implications.
  • dojo/problem/update_mappings.py: This file includes an asynchronous task to update the problem-to-finding mapping cache, which should be reviewed for secure implementation.
  • dojo/problem/urls.py: This file defines the URL patterns for the "problem" functionality, which should be reviewed for proper access control and input validation.
  • dojo/problem/views.py: This file includes the implementation of the views for managing and displaying problems and their associated findings, which should be reviewed for security vulnerabilities.
  • dojo/templates/dojo/*: These template files are responsible for rendering the user interface for the "problem" functionality, and they should be reviewed for proper input sanitization and secure coding practices.
  • dojo/templatetags/*: These custom template tags are used throughout the application, and they should be reviewed for security implications.
  • dojo/tools/*: These files handle the parsing of security findings from various tools, such as Nmap, Nuclei, and OpenVAS, and they should be reviewed to ensure the secure integration of these tools.
  • dojo/urls.py: This file includes the addition of the "dojo.problem.urls" module to the application's URL configuration, which should be reviewed for proper access control and input validation.

Code Analysis

We ran 9 analyzers against 21 files and 2 analyzers had findings. 7 analyzers had no findings.

Analyzer Findings
Configured Codepaths Analyzer 25 findings
Sensitive Files Analyzer 3 findings

Overall Riskiness

🔴 Risk threshold exceeded.

We've notified @mtesauro, @grendel513.

View PR in the DryRun Dashboard.

@LeoOMaia LeoOMaia marked this pull request as draft December 17, 2024 17:01
@github-actions github-actions bot added the settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR label Dec 21, 2024
Copy link

@cunha cunha left a comment

Choose a reason for hiding this comment

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

The most important comments are changing some of the algorithms to have better asymptotic performance.

dojo/problem/config.json Outdated Show resolved Hide resolved
dojo/problem/helper.py Show resolved Hide resolved
dojo/problem/helper.py Outdated Show resolved Hide resolved
dojo/problem/helper.py Outdated Show resolved Hide resolved
dojo/problem/helper.py Outdated Show resolved Hide resolved
dojo/settings/settings.dist.py Outdated Show resolved Hide resolved
dojo/tools/nmap/parser.py Outdated Show resolved Hide resolved
dojo/tools/nmap/parser.py Outdated Show resolved Hide resolved
dojo/tools/openvas/xml_parser.py Outdated Show resolved Hide resolved
dojo/tools/openvas/xml_parser.py Outdated Show resolved Hide resolved
@LeoOMaia LeoOMaia requested a review from cunha December 23, 2024 00:18
Copy link

@cunha cunha left a comment

Choose a reason for hiding this comment

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

Some additional suggestions in the comments.

dojo/settings/settings.dist.py Outdated Show resolved Hide resolved
dojo/templatetags/get_different_total_script_id.py Outdated Show resolved Hide resolved
dojo/problem/helper.py Outdated Show resolved Hide resolved
dojo/problem/helper.py Outdated Show resolved Hide resolved
dojo/problem/helper.py Show resolved Hide resolved
@LeoOMaia LeoOMaia requested a review from cunha December 23, 2024 23:44
Copy link

@cunha cunha left a comment

Choose a reason for hiding this comment

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

Minor comments inside.

dojo/settings/settings.dist.py Show resolved Hide resolved
dojo/templates/base.html Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the docs label Dec 24, 2024
@LeoOMaia LeoOMaia marked this pull request as ready for review December 24, 2024 17:05
@LeoOMaia LeoOMaia requested a review from cunha December 24, 2024 17:06
Copy link

@cunha cunha left a comment

Choose a reason for hiding this comment

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

Minor comments inside.

dojo/templatetags/navigation_tags.py Outdated Show resolved Hide resolved
dojo/templates/base.html Show resolved Hide resolved
dojo/settings/settings.dist.py Outdated Show resolved Hide resolved
@LeoOMaia LeoOMaia requested a review from cunha December 24, 2024 18:30
@Maffooch
Copy link
Contributor

Maffooch commented Jan 3, 2025

Hi @LeoOMaia this is a neat idea, but we are unable to take this contribution due to the added complexity of the finding model. In the future, please create an issue in GitHub, or make a post in slack about new features before writing any code.

@Maffooch Maffooch closed this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Migration Adding a new migration file. Take care when merging. parser settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants