-
Notifications
You must be signed in to change notification settings - Fork 1.8k
OpenVAS parser improvments #13214
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
OpenVAS parser improvments #13214
Conversation
|
DryRun Findings:
I will fix the rest. |
|
I don't think my changes caused the remaining failing tests. Are they failing on the default DEV branch or did i break something? |
|
|
@valentijnscholten Thank you for the sniped. I was to much distracted by the other errors (sorry). The failing test in unittests.test_tool_config.TestApiScanConfigEntry seem to be caused by a deprecation warning in api_blackduck. Is the k8s test failing a known problem (otherwise i will try to investigate this)? |
|
the blackduck thing is new to me but appears on more PRs since today. |
|
@jostaub If you rebase/merge the blackduck failure should go away. |
aa8d641 to
e913275
Compare
|
This pull request contains a CSV injection finding: OpenVASParserV2 in dojo/tools/openvas/parser.py assigns CSV column values directly to Finding fields (title, summary, impact, mitigation, openvas_result) without sanitizing leading formula characters, and the existing cleanup_openvas_text and escape_restructured_text functions (which only remove newlines or wrap text) do not mitigate spreadsheet-formula injection. As a result, malicious inputs could survive import/export and execute commands or exfiltrate data when the CSV is opened in a spreadsheet.
CSV Injection in
|
| Vulnerability | CSV Injection |
|---|---|
| Description | The OpenVASParserV2 processes CSV files and directly assigns column values to Finding model fields such as title, summary, impact, mitigation, and openvas_result without specific sanitization against spreadsheet formula injection. While cleanup_openvas_text removes newlines and escape_restructured_text wraps text in triple backticks for display within DefectDojo, these functions do not prevent malicious formulas (e.g., starting with '=', '+', '-', '@') from being interpreted as commands if the exported data is opened in a spreadsheet program. If a malicious CSV is imported and then its findings are exported, an attacker could craft inputs that, when opened in a spreadsheet, execute arbitrary commands or exfiltrate data. |
django-DefectDojo/dojo/tools/openvas/parser.py
Lines 20 to 40 in e913275
| if str(filename.name).endswith(".xml"): | |
| return OpenVASXMLParser().get_findings(filename, test) | |
| return None | |
| class OpenVASParserV2: | |
| def get_scan_types(self): | |
| return ["OpenVAS Parser v2"] | |
| def get_label_for_scan_types(self, scan_type): | |
| return scan_type | |
| def get_description_for_scan_types(self, scan_type): | |
| return "Import CSV or XML output of Greenbone OpenVAS report." | |
| def get_findings(self, file, test): | |
| if str(file.name).endswith(".csv"): | |
| return get_findings_from_csv(file, test) | |
| if str(file.name).endswith(".xml"): | |
| return get_findings_from_xml(file, test) | |
| return None |
All finding details can be found in the DryRun Security Dashboard.
valentijnscholten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and nice edit on the v2 suffix support.
Maffooch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on the version suffix - excellent idea!
valentijnscholten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved (again :-))
|
I used vscode to review and request reviews from others.. seems like that did not work the way I thought 😅 |
mtesauro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
This PR should address the problems described in:
Here an overview of the most important changes:
In #12378 we discussed why it is a new parser version instead of an update of the existing parser. The concrete fields (that where the reason mentioned in the issue) are no longer the same since i changed the dedpulication algo since then. However the base reason is still the same (hashing fields not present in original parser).
For a long time, this PR did not deduplicate on endpoints. However, this resulted in duplication markings during reimport in the default engagement configuration, which is why this functionality was removed (for now).