django-DefectDojo icon indicating copy to clipboard operation
django-DefectDojo copied to clipboard

Mayhem SARIF support (new parser)

Open xansec opened this issue 5 months ago • 3 comments

Description

This supports parsing Mayhem-generated SARIF reports. In general, the existing SARIF support should work, however, there are some idiosyncrasies as Mayhem is a DAST tool, where the output fields are used in a somewhat non-idiomatic way. Rather than creating a patch change for the existing SARIF report support, I propose this new Mayhem specific support, so that we can keep up with the changes to the SARIF output without burdening or imposing on the existing SARIF parser.

Test results

Tests have been included in dojo/unittests

Documentation

Documentation has been added under dojo/docs/content/en/connecting_your_tools/parsers/file/mayhem.md

Checklist

This checklist is for your information.

  • [x] Make sure to rebase your PR against the very latest dev.
  • [x] Features/Changes should be submitted against the dev.
  • [X] Give a meaningful name to your PR, as it may end up being used in the release notes.
  • [X] Your code is flake8 compliant.
  • [X] Your code is python 3.11 compliant.
  • [X] 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.
  • [X] Add applicable tests to the unit tests.
  • [X] Add the proper label to categorize your PR.

xansec avatar Jun 18 '25 02:06 xansec

DryRun Security

No security concerns detected in this pull request.


All finding details can be found in the DryRun Security Dashboard.

dryrunsecurity[bot] avatar Jun 18 '25 02:06 dryrunsecurity[bot]

I believe the issue raised by the security bot are by design - the SARIF included various examples of exploits, and credentials are known demo credentials for public demo endpoints.

xansec avatar Jun 18 '25 03:06 xansec

Thank you for raising the PR. I just wanted to double check if there's really no way around copying and pasting the existing parser. It's not a lot of code, but I can imagine if SARIF gets more populat we might get more parsers that need slightly different handling of things. Have you considered instead to inherit/extend the existing parser class and overwrite some methods? When I do a "lazy" compare of the SARIF parser with the Mayhem parsers the difference seem very limited, mainly some markdown handling and title cleanup + cwe handling?

valentijnscholten avatar Jun 18 '25 06:06 valentijnscholten

Good catch - I went ahead and extended the SarifParser class instead of copying code. There were a couple of issues with how SarifParser uses member functions vs package functions, so I ended up having to copy code anyways (otherwise polymorphism wasn't working and reverting back to the original SarifParser functions), but I tried to keep this to a minimum. Let me know if there's anything else.

xansec avatar Jun 18 '25 22:06 xansec

Looks like we need to make some changes to the Sarif parser to make it more extensible. That might be too much to try and achieve in this PR. If you could ensure the Mayhem unit tests are testing all the Mayhem specific fields/data, that will help in the future when we make the Sarif parser more extensible.

valentijnscholten avatar Jun 20 '25 20:06 valentijnscholten

@xansec What do you think about the unit tests? I believe the cwe handling is different from the default SARIF parser, so it's good to have that present in the unit test assertions. What else is different/extended in the Mayhem parser and would be good to assert on in the unit tests?

valentijnscholten avatar Jun 30 '25 22:06 valentijnscholten

I have added more comprehensive unit tests for both CWEs as well as a few other fields in a finding. This should cover most cases.

xansec avatar Jul 11 '25 19:07 xansec