SemgrepParser.convert_severity has broken logic

[bsterne]

Bug description
convert_severity tries to map a Semgrep finding to a DefectDojo severity level. There are several tests at the end of the function that will never return True.

    def convert_severity(self, val):
        upper_value = val.upper()
        if upper_value == "CRITICAL":
            return "Critical"
        if upper_value in ["WARNING", "MEDIUM"]:
            return "Medium"
        if upper_value in ["ERROR", "HIGH"]:
            return "High"
        if upper_value == "LOW":
            return "Low"
        if upper_value == "INFO":
            if "WARNING" == val.upper():
                return "Medium"
            if "ERROR" == val.upper() or "HIGH" == val.upper():
                return "High"
            if "INFO" == val.upper():
                return "Info"
        msg = f"Unknown value for severity: {val}"
        raise ValueError(msg)

Steps to reproduce
Steps to reproduce the behavior:

1. Inspect the code
2. Note that upper_value is statically defined in the first line of the function
3. Once we enter the block if upper_value == "INFO": it doesn't make sense to compare it with other strings.

Expected behavior
I believe a different property needs to be passed in an tested, but it's unclear what the correct behavior is. It is clear that this logic is wrong.

Deployment method (select with an X)

• Docker Compose
• Kubernetes
• GoDojo
• AWS ECS

[manuel-sommer]

Could we close this @bsterne ?

[0xDC0DE]

I am a security researcher at Semgrep. I can give some additional information about the metadata that we add to our SAST rules if this can be helpful.

The rule author sets the likelihood and the impact of a rule. These two values are based on the vulnerability the rule is trying to catch. They are usually decided by the CWE the rule is targeting. But they are sometimes manually overridden by the rule author.

The likelihood and impact are used to compute the severity automatically. We don't publish in our docs what the actual computation looks like, because this is not set in stone. It is sometimes overridden or might still change in the future. But for the majority of rules it looks like this:

local likelihood = {
  HIGH: 100,
  MEDIUM: 10,
  LOW: 1,
};

local impact = {
  HIGH: 100,
  MEDIUM: 10,
  LOW: 1,
};

local severity(l, i) =
  local score = likelihood[std.asciiUpper(l)] * impact[std.asciiUpper(i)];
  if score < 100 then
    'INFO'
  else if score == 100 then
    'WARNING'
  else
    'ERROR';

The confidence level is also set by rule author but is more a value to describe the rule, rather than the vulnerability it is trying to catch. It is a score to reflect how confident the rule writer is that these patterns will capture the vulnerability and not result in too many False Positives. Right now this is done manually, and many of our advanced taint mode rules end up being HIGH confidence. However, this might change as well. I think this could, for example, at some point be automated based on the actual triage rate of our customers.

[0xDC0DE]

We don't currently use the CRITICAL severity, but have plans to do so in the near future. The code above will then most likely be updated like this:

 else if score < 10000 then
    'ERROR'
  else
    'CRITICAL';