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

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?

cli/cmdlineparser.cpp

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

See #4651 and #3365.

cli/cppcheckexecutor.cpp

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.

samples/incorrectLogicOperator/bad.c

@@ -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

cli/cppcheckexecutor.cpp

+        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

cli/cppcheckexecutor.cpp

+        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

cli/cppcheckexecutor.cpp

+                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.

cli/cppcheckexecutor.cpp

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.

cli/cppcheckexecutor.cpp

Co-authored-by: Mario Campos <[email protected]>

cli/cppcheckexecutor.cpp

Co-authored-by: Mario Campos <[email protected]>

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..

danmar commented Oct 5, 2024

View reviewed changes

cli/cmdlineparser.cpp Outdated Show resolved Hide resolved

firewave reviewed Oct 5, 2024

View reviewed changes

cli/cppcheckexecutor.cpp Outdated Show resolved Hide resolved

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

github-advanced-security bot found potential problems Oct 6, 2024

View reviewed changes

samples/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

Draft

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

danmar force-pushed the fix-9972-2 branch from c54dfa2 to fe5ae26 Compare 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 7a80e05 Compare October 9, 2024 13:10

fix warning

8680b3a

mario-campos reviewed Oct 9, 2024

View reviewed changes

danmar and others added 4 commits October 9, 2024 21:42

Update cli/cppcheckexecutor.cpp

9e1db25

Co-authored-by: Mario Campos <[email protected]>

Update cli/cppcheckexecutor.cpp

5d76a94

Co-authored-by: Mario Campos <[email protected]>

review comments

fb9ca69

fix

df6d289

mario-campos reviewed Oct 10, 2024

View reviewed changes

cli/cppcheckexecutor.cpp Outdated Show resolved Hide resolved

danmar and others added 2 commits October 10, 2024 20:22

Update cli/cppcheckexecutor.cpp

816542c

Co-authored-by: Mario Campos <[email protected]>

test

26ca959

danmar merged commit c49b941 into danmar:main Oct 10, 2024
63 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danmar commented Oct 5, 2024

danmar commented Oct 5, 2024

danmar commented Oct 5, 2024

firewave commented Oct 5, 2024

danmar commented Oct 5, 2024

danmar commented Oct 5, 2024

github-advanced-security bot commented Oct 6, 2024

danmar commented Oct 6, 2024 •

edited

Loading

mario-campos commented Oct 7, 2024

mario-campos left a comment •

edited

Loading

mario-campos Oct 9, 2024

danmar Oct 9, 2024

mario-campos Oct 9, 2024

mario-campos Oct 9, 2024

danmar Oct 9, 2024

mario-campos Oct 9, 2024

danmar Oct 9, 2024

danmar commented Oct 9, 2024

danmar commented Oct 9, 2024

mario-campos left a comment

danmar commented Oct 10, 2024

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

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

Conversation

danmar commented Oct 5, 2024

danmar commented Oct 5, 2024

danmar commented Oct 5, 2024

firewave commented Oct 5, 2024

danmar commented Oct 5, 2024

danmar commented Oct 5, 2024

github-advanced-security bot commented Oct 6, 2024

danmar commented Oct 6, 2024 • edited Loading

mario-campos commented Oct 7, 2024

mario-campos left a comment • edited Loading

Choose a reason for hiding this comment

mario-campos Oct 9, 2024

Choose a reason for hiding this comment

danmar Oct 9, 2024

Choose a reason for hiding this comment

mario-campos Oct 9, 2024

Choose a reason for hiding this comment

mario-campos Oct 9, 2024

Choose a reason for hiding this comment

danmar Oct 9, 2024

Choose a reason for hiding this comment

mario-campos Oct 9, 2024

Choose a reason for hiding this comment

danmar Oct 9, 2024

Choose a reason for hiding this comment

danmar commented Oct 9, 2024

danmar commented Oct 9, 2024

mario-campos left a comment

Choose a reason for hiding this comment

danmar commented Oct 10, 2024

danmar commented Oct 6, 2024 •

edited

Loading

mario-campos left a comment •

edited

Loading