Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sarif parser location files processing #11265

Open
wants to merge 5 commits into
base: bugfix
Choose a base branch
from

Conversation

dmarushkin
Copy link
Contributor

@dmarushkin dmarushkin commented Nov 14, 2024

Description

In sarif results -> locations provided list of files by sarif standart. In many scanners like semgrep each item in results contain one file in locations, but in some scanners like mobsfscan scanner reports contain multiple files for one item:

Example for located in tests mobsfscan report:

      "results": [
        {
          "message": {
            "text": "The App logs information. Please ensure that sensitive information is never logged."
          },
          "level": "note",
          "locations": [
            {
              "physicalLocation": {
                "region": {
                  "snippet": {
                    "text": "            Log.d(\"Diva\", \"Error occurred while creating database: \" + e.getMessage());"
                  },
                  "endColumn": 88,
                  "endLine": 57,
                  "startColumn": 13,
                  "startLine": 57
                },
                "artifactLocation": {
                  "uri": "app/src/main/java/jakhar/aseem/diva/InsecureDataStorage2Activity.java"
                }
              }
            },
            {
              "physicalLocation": {
                "region": {
                  "snippet": {
                    "text": "            Log.d(\"Diva\", \"Error occurred while inserting into database: \" + e.getMessage());"
                  },
                  "endColumn": 94,
                  "endLine": 71,
                  "startColumn": 13,
                  "startLine": 71
                },
                "artifactLocation": {
                  "uri": "app/src/main/java/jakhar/aseem/diva/InsecureDataStorage2Activity.java"
                }
              }
            },

In old parser logic all files in one result except the first file are lost because locations[0] used for item filling.
In new logic information about scanner hit in each file saved in files list and after from this list created separate vuln items.

Test results

Fixed tests for mobsfscan and njsscan sarif reports.

Copy link

dryrunsecurity bot commented Nov 14, 2024

DryRun Security Summary

The pull request focuses on improving the SARIF parser in the Dojo application security tool, enhancing the handling of SARIF results with multiple locations, improving snippet extraction and description generation, and expanding the range of security findings from different scanning tools that the parser can process.

Expand for full summary

Summary:

The code changes in this pull request focus on improving the SARIF (Static Analysis Results Interchange Format) parser in the Dojo application security tool. The key changes include:

  1. Enhanced handling of SARIF results with multiple locations, where the parser now processes all locations and creates a separate Finding object for each one.
  2. Improved snippet extraction, with the get_snippet() function now retrieving code snippets from both the "region" and "contextRegion" fields in the SARIF data.
  3. Enhanced description generation, where the get_description() function includes more detailed information in the finding descriptions, such as the rule name, short and full descriptions, and the code flow description (if available).

Additionally, the changes to the unit tests for the SARIF parser indicate that the parser is being updated to handle a wider range of security findings from different scanning tools, such as njsscan and mobsfscan. This is a positive change, as it allows the DefectDojo project to process and display a more comprehensive set of security findings, providing a better understanding of the application's security posture.

Files Changed:

  1. dojo/tools/sarif/parser.py: This file contains the core SARIF parser logic, and the changes improve the parser's ability to accurately represent the findings reported in the SARIF format. The changes enhance the handling of multiple locations, snippet extraction, and finding description generation, which will benefit the application security team's ability to triage and remediate identified issues.

  2. unittests/tools/test_sarif_parser.py: This file contains the unit tests for the SARIF parser, and the changes reflect updates to the expected number of findings from different security scanning tools, such as njsscan and mobsfscan. These changes indicate that the SARIF parser is being updated to handle a wider range of security findings, improving the overall coverage and robustness of the application security program.

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.

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants