Fix #9972 (Add support for SARIF output format) by danmar · Pull Request #6863 · cppcheck-opensource/cppcheck

danmar · 2024-10-05T17:43:28Z

No description provided.

danmar · 2024-10-05T17:44:15Z

There is no testing yet. I will see if I can integrate it into github.

danmar · 2024-10-05T17:46:05Z

@mario-campos would you be interested to test/review this Cppcheck SARIF output?

firewave · 2024-10-05T19:17:58Z

See #4651 and #3365.

danmar · 2024-10-05T19:33:47Z

See 4651 and 3365.

4651: very related but this is a simpler approach.

3365: I don't think this is related to the uniq/append output.

danmar · 2024-10-05T19:40:16Z

I believe that we can output findings directly btw. We don't have to construct the complete report at the end. However I am not sure if that will be very useful anyway, the report is so verbose I don't think anybody would like to read the sarif output "live".

I believe a minimal SARIF header would be:

{
"runs": [
{
"results": [

github-advanced-security · 2024-10-06T16:22:12Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@@ -0,0 +1,6 @@
+
+void foo(int x) {
+ if (x >= 0 || x <= 10) {}


danmar · 2024-10-06T17:00:17Z

There is an experimental SARIF upload here:
#6866

mario-campos · 2024-10-07T13:43:23Z

@danmar, sure. I can take a look at it later today.

mario-campos

Looks great!! Can't wait to see this merged!

I left a few comments about some missing SARIF properties, which GitHub requires. I think it would be a good idea to implement them, because I think someone will eventually try to upload the SARIF file to GitHub expecting it to work.

mario-campos · 2024-10-09T03:26:28Z

+ picojson::array serializeRules() const {
+ picojson::array ret;
+ std::set<std::string> ruleIds;
+ for (const auto& finding : mFindings) {
+ if (ruleIds.insert(finding.id).second) {
+ picojson::object rule;
+ rule["id"] = picojson::value(finding.id);
+ picojson::object shortDescription;
+ shortDescription["text"] = picojson::value(finding.shortMessage());
+ rule["shortDescription"] = picojson::value(shortDescription);
+ ret.emplace_back(rule);
+ }
+ }
+ return ret;
+ }


serializeRules() does not serialize the rule's fullDescription.text nor its help.text, which are both required*. See reportingDescriptor object specification.
Also, while not required, it would be nice to see the severity (properties.problem.severity) implemented as well.
* These properties are required by GitHub's Code scanning platform, not necessarily the SARIF specification.

mario-campos · 2024-10-09T03:32:49Z

+ picojson::array serializeResults() const {
+ picojson::array results;
+ for (const auto& finding : mFindings) {
+ picojson::object res;
+ res["level"] = picojson::value(sarifLevel(finding.severity));
+ if (!finding.callStack.empty())
+ res["locations"] = picojson::value(serializeLocations(finding));
+ picojson::object message;
+ message["text"] = picojson::value(finding.shortMessage());
+ res["message"] = picojson::value(message);
+ res["ruleId"] = picojson::value(finding.id);
+ results.emplace_back(res);
+ }
+ return results;
+ }


This function does not implement the required* partialFingerprints property, which is used to prevent creating duplicate alerts for the same vulnerability/finding. See result object specification.
* These properties are required by GitHub's Code scanning platform, not necessarily the SARIF specification.

Also, the locations property is required (by GitHub), so maybe consider hoisting up the if (!finding.callStack.empty()) condition to the for loop, so that if there are no locations, the alert is not serialized.

mario-campos · 2024-10-09T03:38:39Z

+ picojson::object region;
+ region["startLine"] = picojson::value(static_cast<int64_t>(location.line));
+ region["startColumn"] = picojson::value(static_cast<int64_t>(location.column));
+ physicalLocation["region"] = picojson::value(region);


Missing required* properties region.endLine and region.endColumn. See physicalLocation object specification.
* These properties are required by GitHub's Code scanning platform, not necessarily the SARIF specification.

danmar · 2024-10-09T13:12:06Z

@mario-campos thanks for the comments. Can you please check again?

Example output:

{
"$schema": "https:\/\/docs.oasis-open.org\/sarif\/sarif\/v2.1.0\/errata01\/os\/schemas\/sarif-schema-2.1.0.json",
"runs": [
{
"results": [
{
"level": "warning",
"locations": [
{
"physicalLocation": {
"artifactLocation": {
"uri": "1.c"
},
"region": {
"endColumn": 4,
"endLine": 1,
"startColumn": 4,
"startLine": 1
}
}
}
],
"message": {
"text": "Division by zero."
},
"partialFingerprints": {
"hash": "4df90040e1961f0f"
},
"ruleId": "zerodiv"
}
],
"tool": {
"driver": {
"informationUri": "https:\/\/cppcheck.sourceforge.io",
"name": "Cppcheck Premium",
"rules": [
{
"fullDescription": {
"text": "Division by zero."
},
"help": {
"text": "Division by zero."
},
"id": "zerodiv",
"properties": {
"precision": "high",
"problem": {
"severity": "warning"
}
},
"shortDescription": {
"text": "Division by zero."
}
}
],
"version": "24.9.0"
}
}
}
],
"version": "2.1.0"
}

danmar · 2024-10-09T13:16:14Z

@mario-campos please take an extra careful look on the partialFingerprints , I am very unsure what that should be.

mario-campos

Nice job! A few more finishing touches.

Also, you can validate the SARIF file at https://sarifweb.azurewebsites.net/. I've already submitted it and there's a few "issues" but I wouldn't worry about them. According to the docs, they're not required.

Co-authored-by: Mario Campos <mario-campos@github.com>

danmar · 2024-10-10T20:09:22Z

@mario-campos I merged it now. but please feel free to provide additional advice if there is something we can fix..

…6863) Co-authored-by: Mario Campos <mario-campos@github.com>

danmar commented Oct 5, 2024
View reviewed changes

Comment threadcli/cmdlineparser.cpp Outdated

firewave reviewed Oct 5, 2024
View reviewed changes

Comment threadcli/cppcheckexecutor.cpp Outdated

danmar force-pushed the fix-9972-2 branch 2 times, most recently from a9065ff to a798d28Compare October 6, 2024 14:36

github-advanced-securityAI found potential problems Oct 6, 2024
View reviewed changes

Comment threadsamples/incorrectLogicOperator/bad.c
@@ -0,0 +1,6 @@

void foo(int x) {
if (x >= 0 || x <= 10) {}
Check warning
Code scanning / Cppcheck
Logical disjunction always evaluates to true: x >= 0 || x <= 10.
Logical disjunction always evaluates to true: x >= 0 || x <= 10.

danmar mentioned this pull request Oct 6, 2024
Test 9972 #6866
Closed

danmar force-pushed the fix-9972-2 branch from da346ea to cc66c38Compare October 6, 2024 16:56

danmar force-pushed the fix-9972-2 branch from c54dfa2 to fe5ae26Compare October 8, 2024 12:27

mario-campos reviewed Oct 9, 2024
View reviewed changes

danmar added 3 commits October 9, 2024 13:57

Fix #9972 (Add support for SARIF output format)
c1aad91

review
84d128f

relative filename
7a80e05

danmar force-pushed the fix-9972-2 branch from fe5ae26 to 7a80e05Compare October 9, 2024 13:10

fix warning
8680b3a

mario-campos reviewed Oct 9, 2024
View reviewed changes

Comment threadcli/cppcheckexecutor.cpp Outdated
Comment threadcli/cppcheckexecutor.cpp Outdated
Comment threadcli/cppcheckexecutor.cpp
Comment threadcli/cppcheckexecutor.cpp Outdated
Comment threadcli/cppcheckexecutor.cpp Outdated
Comment threadcli/cppcheckexecutor.cpp Outdated

danmarand others added 4 commits October 9, 2024 21:42

Update cli/cppcheckexecutor.cpp
9e1db25
Co-authored-by: Mario Campos <mario-campos@github.com>

Update cli/cppcheckexecutor.cpp
5d76a94
Co-authored-by: Mario Campos <mario-campos@github.com>

review comments
fb9ca69

fix
df6d289

mario-campos reviewed Oct 10, 2024
View reviewed changes

Comment threadcli/cppcheckexecutor.cpp Outdated

danmarand others added 2 commits October 10, 2024 20:22

Update cli/cppcheckexecutor.cpp
816542c
Co-authored-by: Mario Campos <mario-campos@github.com>

test
26ca959

danmar merged commit c49b941 into cppcheck-opensource:mainOct 10, 2024

danmar deleted the fix-9972-2 branch October 10, 2024 20:12

ludviggunne pushed a commit to ludviggunne/cppcheck that referenced this pull request Oct 19, 2024
Fix #9972 (Add support for SARIF output format) (cppcheck-opensource#…
f6e77ba
…6863) Co-authored-by: Mario Campos <mario-campos@github.com>

Uh oh!

Conversation

danmar commented Oct 5, 2024

Uh oh!

danmar commented Oct 5, 2024

Uh oh!

danmar commented Oct 5, 2024

Uh oh!

Uh oh!

firewave commented Oct 5, 2024

Uh oh!

Uh oh!

danmar commented Oct 5, 2024

Uh oh!

danmar commented Oct 5, 2024

Uh oh!

github-advanced-securityAI commented Oct 6, 2024

Uh oh!

Check warning

danmar commented Oct 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mario-campos commented Oct 7, 2024

Uh oh!

mario-campos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mario-camposOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

danmarOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

mario-camposOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

mario-camposOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

danmarOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

mario-camposOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

danmarOct 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danmar commented Oct 9, 2024

Uh oh!

danmar commented Oct 9, 2024

Uh oh!

mario-campos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danmar commented Oct 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danmar commented Oct 6, 2024•
edited
Loading

mario-campos left a comment •
edited
Loading