Create a generic SARIF reader

[ericwb]

There are quite a few tools nowadays that support SARIF as an output format. Rather than having to create a new Reader class for each tool, how about adding a generic SARIF reader that can process any tools output that properly supports the SARIF spec.

I see there is now a SarifReader abstract class. Could we get a concrete class that can read any sarif file that matches the spec? I feel this will be much less maintenance prone as more and more tools come about.

[davewichers]

I'd prefer to keep it abstract as each vendor could map their rules to different CWEs or even combinations of CWEs, so having a specific implementation which contains the mapping for that specific tool and any required adjustments is helpful/easier to maintain. For example, I just noticed that CodeQL maps both the Crypto and Hash findings to a single rule, but that maps to both CWEs 327 & 328, so we have to do some custom logic that says if they reported a Crypto finding against a Hashing test case, it still counts as a 'hit' or a false positive it if wasn't supposed to report that.

@darkspirit510 - What do you think?

[ericwb]

Understood, I imagine there can be differences on how CWEs are handled. Technically, the SARIF spec does define how to associate a CWE to a rule. It advises against using tags.

Tags SHOULD NOT be used to label a result or a rule as belonging to a category in a classification system such as the Common Weakness Enumeration [CWE™] (for example, by adding a tag "CWE/622"). Instead, taxonomies (§3.19.3) SHOULD be used for this purpose.

However, I imagine many tools use tags in order for GitHub's Code Scanning to render the value in the UI.

https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html

[darkspirit510]

@ericwb I would prefer having a general SARIF parser. The problem is that I only derived two SARIF readers from my abstract superclass yet (Snyk and Semgrep) and as you mehtioned, one uses tags and the other uses another field. Plus the format does not know if the tool is commercial or not. Sure, we could add a mapping table for tools and commercial names. That's why I created my superclass with abstract methods for CWE mapping, tool name (for the matching) and if it is commercial. I'll try to be even more generic, but I guess, there will always be necessity for some adaptions for each new file.

If it is true, that most readers use the recommended tag field, I'll move the mapping method to superclass and only overwrite it when necessary. Then most readers would be just two one-liner methods 🥹 Edit: Or maybe use an enum - I'll try out some things.

[ericwb]

Some other readers that could probably be ported to the new SarifReader abstract class are CodeQLReader, LGTMReader, ShiftLeftScanReader, and DatadogSastReader (PR #63).

[darkspirit510]

And Contrast too, iirc (Not at my computer at the moment). But yes, I'll create a PR for (hopefully) all mentioned Readers so that they use SarifReader as parent class.

[darkspirit510]

[...] For example, I just noticed that CodeQL maps both the Crypto and Hash findings to a single rule, but that maps to both CWEs 327 & 328, so we have to do some custom logic that says if they reported a Crypto finding against a Hashing test case, it still counts as a 'hit' or a false positive it if wasn't supposed to report that.

@davewichers I don't see any mapping for this in CodeQL Reader class. Only 94 -> 78 and 335 -> 330 🤔

[darkspirit510]

https://github.com/OWASP-Benchmark/BenchmarkUtils/pull/70

[davewichers]

@darkspirit510 - Regarding: "@davewichers I don't see any mapping for this in CodeQL Reader class. Only 94 -> 78 and 335 -> 330 🤔" calling Hash findings a Crypto finding is a common thing, so I suspect this mapping might be in some generic place that applies to LOTS of tools, not just to CodeQL. If you generate a scorecard using the Benchmark-1.2-codeql_java_security-and-quality.sarif file, you'll see that it scores 77% for Crypto and 48% for hashing, but if you look in the SARIF file you should see that Benchmark00003 is flagged by the CodeQL crypto rule, just like Benchmark00005, and yet both test are considered to pass, even though 00003 is a hash test case, and 00005 is a crypto test case.

[darkspirit510]

@ericwb Since #70 is merged, I guess this one can be closed?

[ericwb]

Sounds good