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

Enhance ASFF parser to more resemble the SecurityHub parser #10176

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

testaccount90009
Copy link
Contributor

My goal is to make the ASFF findings more actionable than what is currently being added into DefectDojo fields. In the other SecurityHub parser it was enhanced to add ResourceId into the description. I want to also do this with the ASFF findings, in a similar way.

Also, in my opinion (and this change can be removed no worries) but - vuln_id_from_tool is more appropriate for matching to the security hub finding Id/Arn than the 'unique_id_from_tool'.

unique_id_from_tool shouldn't match a vuln_id (which is the finding_id from AWS) - but instead maybe could be matched to a ResourceId. Ideally this would be a host or an endpoint, but not always will a Finding have a host associated with it. HOWEVER, a finding will >always< have a ResourceId and ARN from AWS. So other than adding into the Description, I'm not sure which other field makes sense to add this to. I can remove the unique_id_from_tool and vuln_id_from_tool changes if those are not ideal.

⚠️ Note on feature completeness ⚠️

We are narrowing the scope of acceptable enhancements to DefectDojo in preparation for v3. Learn more here:
https://github.com/DefectDojo/django-DefectDojo/blob/master/readme-docs/CONTRIBUTING.md

Description

Describe the feature / bug fix implemented by this PR.
If this is a new parser, the parser guide may be worth (re)reading.

Test results

Ideally you extend the test suite in tests/ and dojo/unittests to cover the changed in this PR.
Alternatively, describe what you have and haven't tested.

Documentation

Please update any documentation when needed in the documentation folder)

Checklist

This checklist is for your information.

  • Make sure to rebase your PR against the very latest dev.
  • Features/Changes should be submitted against the dev.
  • Bugfixes should be submitted against the bugfix branch.
  • Give a meaningful name to your PR, as it may end up being used in the release notes.
  • Your code is flake8 compliant.
  • Your code is python 3.11 compliant.
  • If this is a new feature and not a bug fix, you've included the proper documentation in the docs at https://github.com/DefectDojo/django-DefectDojo/tree/dev/docs as part of this PR.
  • Model changes must include the necessary migrations in the dojo/db_migrations folder.
  • Add applicable tests to the unit tests.
  • Add the proper label to categorize your PR.

Extra information

Please clear everything below when submitting your pull request, it's here purely for your information.

Moderators: Labels currently accepted for PRs:

  • Import Scans (for new scanners/importers)
  • enhancement
  • performance
  • feature
  • bugfix
  • maintenance (a.k.a chores)
  • dependencies
  • New Migration (when the PR introduces a DB migration)
  • settings_changes (when the PR introduces changes or new settings in settings.dist.py)

Contributors: Git Tips

Rebase on dev branch

If the dev branch has changed since you started working on it, please rebase your work after the current dev.

On your working branch mybranch:

git rebase dev mybranch

In case of conflict:

 git mergetool
 git rebase --continue

When everything's fine on your local branch, force push to your myOrigin remote:

git push myOrigin --force-with-lease

To cancel everything:

git rebase --abort

Squashing commits

git rebase -i origin/dev
  • Replace pick by fixup on the commits you want squashed out
  • Replace pick by reword on the first commit if you want to change the commit message
  • Save the file and quit your editor

Force push to your myOrigin remote:

git push myOrigin --force-with-lease

@github-actions github-actions bot added the parser label May 9, 2024
Copy link

dryrunsecurity bot commented May 9, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Configured Codepaths Analyzer 0 findings
Sensitive Files Analyzer 0 findings
AppSec Analyzer 0 findings
Authn/Authz Analyzer 0 findings
Secrets Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The code change in the provided patch is related to the get_findings method in the AsffParser class. The changes aim to enhance the description of the findings by including additional information from the "Resources" field of the ASFF (AWS Security Finding Format) data. The changes focus on constructing a more informative description for the findings by incorporating the "Id" value from the "Resources" field, if present. This can be useful for security analysts and incident responders to better understand the context of the findings.

While the changes are not directly related to security vulnerabilities or risks, there are a few points worth considering. The code handles the case where the "Resources" field is missing, which is a good practice to ensure the parser can handle a variety of ASFF data formats. Additionally, the code already includes logic to extract endpoint information (EC2 instance IP addresses) from the "Resources" field and associate them with the Finding object, which is valuable for security analysis and remediation. Depending on the use case and requirements, the parser could be further enhanced to extract and display additional relevant information from the ASFF data, such as the AWS resource type, region, or other contextual details that could aid in understanding and triaging the findings.

Files Changed:

  • dojo/tools/asff/parser.py: The changes in this file are related to the get_findings method in the AsffParser class. The code now checks if the "Resources" field is present in the ASFF data item, and if it is, it constructs a new description string that includes the existing description and the "Id" value from the "Resources" field. If the "Resources" field is not present, the original description from the ASFF data is used. The updated description is then assigned to the description attribute of the Finding object.

Powered by DryRun Security

@testaccount90009 testaccount90009 force-pushed the asff-parser-add-resourceid branch 3 times, most recently from 89459e9 to f3b5770 Compare May 13, 2024 19:36
worker_processes defaulting to 1 leads to sub-optimal performance.  Changing this to 'auto' allows for more performance out-of-the-box at no detriment to anything else.  This will help newer users from experiencing performance limitations due to the 1 worker_process, when most modern systems have multiple cores to take advantage of.
My goal is to make the ASFF findings more actionable than what is currently being added into DefectDojo fields.  In the other SecurityHub parser it was enhanced to add ResourceId into the description.  I want to also do this with the ASFF findings, in a similar way.

Also, in my opinion (and this change can be removed no worries) but -  vuln_id_from_tool is more appropriate for matching to the security hub finding Id/Arn than the 'unique_id_from_tool'.

unique_id_from_tool shouldn't match a vuln_id (which is the finding_id from AWS) - but instead maybe could be matched to a ResourceId.  Ideally this would be a host or an endpoint, but not always will a Finding have a host associated with it.  HOWEVER, a finding will >always< have a ResourceId and ARN from AWS.  So other than adding into the Description, I'm not sure which other field makes sense to add this to.  I can remove the unique_id_from_tool and vuln_id_from_tool changes if those are not ideal.
Remove accidental merge of different branch
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.

None yet

1 participant