🐛 fix semgrep severity logic #11218

[manuel-sommer]

No description provided.

[dryrunsecurity]

DryRun Security Summary

The pull request focuses on updates to the unit tests and the parser for the Semgrep security analysis tool integrated into the DefectDojo application, improving the handling and presentation of Semgrep security findings without directly impacting the security of the application itself.

Expand for full summary

Summary:

The code changes in this pull request are primarily focused on updates to the unit tests and the parser for the Semgrep security analysis tool, which is integrated into the DefectDojo application. The changes do not directly impact the security of the application itself, but they do provide valuable insights into the team's approach to application security testing and the tools they are using.

The changes in the test_semgrep_parser.py file involve updating the severity of some findings from "Info" to "Low" in two separate test cases. This could indicate a potential update or refinement in the way the Semgrep parser is classifying the severity of certain types of findings, which could be worth investigating further.

The changes in the dojo/tools/semgrep/parser.py file are focused on improving the handling and presentation of Semgrep security findings within the DefectDojo application. Specifically, the changes ensure that findings with an "INFO" severity level are properly categorized and that code snippets are displayed correctly, which can be crucial for understanding and analyzing the identified security issues.

Files Changed:

1. unittests/tools/test_semgrep_parser.py: The changes in this file update the severity of some findings from "Info" to "Low" in two separate test cases. This could indicate a potential update or refinement in the way the Semgrep parser is classifying the severity of certain types of findings.
2. dojo/tools/semgrep/parser.py: The changes in this file include:
   • Updating the convert_severity() function to handle the "INFO" severity level and map it to the "Low" severity.
   • Modifying the get_description() function to handle a specific issue related to the display of code snippets, where the string "<![" was causing a problem with the rendering of the snippet in the DefectDojo application.

Code Analysis

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

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

[mtesauro]

Approved

[mtesauro]

Whoops, did that approval a bit early.

I'll check back to see if my haste was wasted or not 😉

[mtesauro]

Nope. All's well

[bsterne]

Hi all, I'm the reporter of this bug. Thanks for picking this up. I don't think the logic is correct here in the PR.
https://semgrep.dev/docs/writing-rules/rule-syntax#required

One of the following values: INFO (Low severity), WARNING (Medium severity), or ERROR (High severity). The severity key specifies how critical are the issues that a rule potentially detects. Note: Semgrep Supply Chain differs, as its rules use CVE assignments for severity. For more information, see Filters section in Semgrep Supply Chain documentation.

So there may be some additional cases to consider, but I think the full logic is something closer to:

    if upper_value == "ERROR":
        return "High"
    if upper_value == "WARNING":
        return "Medium"
    if upper_value == "INFO":
        return "Low"

Where are the tests for this? I think we should be looking at actual Semgrep reports and reasoning about the formatting of the findings there. I'm happy to work with you on this if you would like help. Thanks again.

[bsterne]

@blakeaowens @cneill @mtesauro @Maffooch this PR is wrong. Is someone willing to acknowledge my comment:
#11219 (comment)

[manuel-sommer]

@blakeaowens @cneill @mtesauro @Maffooch this PR is wrong. Is someone willing to acknowledge my comment: #11219 (comment)

Hi @bsterne, did you consider the whole method in your comment? https://github.com/DefectDojo/django-DefectDojo/pull/11219/files#diff-6675ec51a8162a810df97675f8c510909e9627872f60ad527c84bd8bd3e5473dR132

[bsterne]

Yes, I did consider the whole method when I made the comment. I'm referencing the Semgrep docs that say:

One of the following values: INFO (Low severity), WARNING (Medium severity), or ERROR (High severity).

In other words, Semgrep INFO maps to "Low" severity, not "Info" severity, as this PR has it. That is for code and secrets findings.

For supply chain findings, there are Critical, High, Medium, or Low as possible options. That's according to their docs. I'm currently looking at a set of supply chain findings (see: semgrep-sca-results.json) that have "sca-severity": "MODERATE", so I think we need to pass in a different field to convert_severity based on what type of report it is.

[manuel-sommer]

I fixed the Info to Low mapping.

[Maffooch]

I believe this fix covers both cases and is implicitly context related (i.e. code and secret reports will not have both Info and low severities). @manuel-sommer great work here