trivy icon indicating copy to clipboard operation
trivy copied to clipboard

feat(image): updated json report added package locations fields

Open parvez0 opened this issue 1 year ago • 11 comments

Description

This pull request introduces a new feature to include package location indexes in the JSON report.

Changes

  1. Modified pkg/scanner/local/server.go to use the location data from the detected vulnerability.
  2. Updated pkg/report/sarif.go to reuse the calculated location information if available.
  3. Added a new key Location under DetectedVulnerability in pkg/types/vulnerability.go to update the extracted location data.
  4. Updated integration/testdata/*.json.golden to verify Location field in json reports

Usage

$ git clone https://github.com/knqyf263/trivy-ci-test.git
$ cd trivy-ci-test
$ trivy fs --format template --format json -o trivy_report.json --scanners vuln .

Command output before this feature change

{
  "SchemaVersion": 2,
  "CreatedAt": "2024-03-16T21:54:44.700427+05:30",
  "ArtifactName": ".",
  "ArtifactType": "filesystem",
  "Metadata": {
    ...
  },
  "Results": [
    {
      "Target": "Cargo.lock",
      "Class": "lang-pkgs",
      "Type": "cargo",
      "Packages": [...],
      "Vulnerabilities": [
        {
          "VulnerabilityID": "CVE-2019-15542",
          "PkgID": "[email protected]",
          "PkgName": "ammonia",
          "PkgIdentifier": {
            "PURL": "pkg:cargo/[email protected]"
          },
          "InstalledVersion": "1.9.0",
          "FixedVersion": "2.1.0",
          "Status": "fixed",
          "Layer": {},
          "SeveritySource": "ghsa",
          "PrimaryURL": "https://avd.aquasec.com/nvd/cve-2019-15542",
          "DataSource": {...},
          "Title": "Uncontrolled recursion in ammonia",
          "Description": "An issue was discovered in the ammonia crate before 2.1.0 for Rust. There is uncontrolled recursion during HTML DOM tree serialization.",
          "Severity": "HIGH",
          "CweIDs": ["CWE-674],
          "VendorSeverity": {...},
          "CVSS": {...},
          "References": [...],
          "PublishedDate": "2019-08-26T18:15:12.467Z",
          "LastModifiedDate": "2020-08-24T17:37:01.14Z"
        }
      ]
    }
  ]
}

Command output after introducing this feature

{
  "SchemaVersion": 2,
  "CreatedAt": "2024-03-16T21:54:44.700427+05:30",
  "ArtifactName": ".",
  "ArtifactType": "filesystem",
  "Metadata": {
    ...
  },
  "Results": [
    {
      "Target": "Cargo.lock",
      "Class": "lang-pkgs",
      "Type": "cargo",
      "Packages": [...],
      "Vulnerabilities": [
        {
          "VulnerabilityID": "CVE-2019-15542",
          "PkgID": "[email protected]",
          "PkgName": "ammonia",
          "PkgIdentifier": {
            "PURL": "pkg:cargo/[email protected]"
          },
          "InstalledVersion": "1.9.0",
          "FixedVersion": "2.1.0",
          "Status": "fixed",
          "Layer": {},
          "SeveritySource": "ghsa",
          "PrimaryURL": "https://avd.aquasec.com/nvd/cve-2019-15542",
          "Locations": [
            {
              "StartLine": 2,
              "EndLine": 13
            }
          ],
          "DataSource": {...},
          "Title": "Uncontrolled recursion in ammonia",
          "Description": "An issue was discovered in the ammonia crate before 2.1.0 for Rust. There is uncontrolled recursion during HTML DOM tree serialization.",
          "Severity": "HIGH",
          "CweIDs": [ "CWE-674"],
          "VendorSeverity": {...},
          "CVSS": {...},
          "References": [...],
          "PublishedDate": "2019-08-26T18:15:12.467Z",
          "LastModifiedDate": "2020-08-24T17:37:01.14Z"
        }
      ]
    }
  ]
}

Related issues

  • Close #4418

Checklist

  • [x] I've read the guidelines for contributing to this repository.
  • [x] I've followed the conventions in the PR title.
  • [x] I've added tests that prove my fix is effective or that my feature works.
  • [x] I've updated the documentation with the relevant information (if needed).
  • [x] I've added usage information (if the PR introduces new options)
  • [x] I've included a "before" and "after" example to the description (if the PR is a user interface change).

parvez0 avatar Mar 16 '24 16:03 parvez0

Hey @DmitriyLewen, needed your opinion on this. After integrating the Location Changes, I noticed that integration_test/client_server.go started failing for the busybox_with_cargo.lock and scan pox.xml test cases. The issue from these test cases is expecting DetectedVulnerability.Location to be nil. However, the output golden files busybox-with-lockfile.json.golden and pom.json.golden are also used in other integration tests where non-nil Location data is expected. To address this, I created two duplicate files that includes Location data. Please let me know if you have a better solution for this.

parvez0 avatar Mar 20 '24 03:03 parvez0

@parvez0 You need to update rpc package. Let me know when you're done making changes (to avoid conflicts) and i will update client/server part.

DmitriyLewen avatar Mar 20 '24 04:03 DmitriyLewen

@parvez0 You need to update rpc package. Let me know when you're done making changes (to avoid conflicts) and i will update client/server part.

Hey @DmitriyLewen what do I have to do in rpc package I haven't seen any changes that need to be made there, can you please let me know I'll try to do it as soon as possible

parvez0 avatar Mar 21 '24 01:03 parvez0

Hm... we don't insert Locations in client/server mode... I created #6366 for this bug. After merge #6366 you need to merge main branch into this PR and follow these steps:

  • add locations to Vulnerabilities (use 26 number). Use Location struct.
  • run mage fmt.
  • run mage:protoc.
  • add Locations to Vulnerabilties as for Packages ( use ConvertToRPCLocations and ConvertFromRPCLocation functions)
  • add Locations in testcase in convert_test.go.
  • recheck that you don't need new test golden files in integration tests.

DmitriyLewen avatar Mar 21 '24 09:03 DmitriyLewen

Originally, our policy was to include only minimal data in reports. It may be better to embed Package in DetectedVulnerability than to gradually add more and more attributes.

knqyf263 avatar Apr 22 '24 13:04 knqyf263

sounds logical. @parvez0 Do you have the time and opportunity to do this?

DmitriyLewen avatar Apr 24 '24 03:04 DmitriyLewen

Hey @DmitriyLewen, I'll take a look at this and update

parvez0 avatar Apr 24 '24 13:04 parvez0

We're so sorry. We discussed this offline and found out that embedding packages will get lots of duplicate data if multiple vulnerabilities are found in one package. We need to carefully think about it.

knqyf263 avatar Apr 29 '24 06:04 knqyf263

Hey @knqyf263, I had the same question and was just about to reach out to you. Please let me know if you've settled on an approach for this and need my assistance to finalize it. In the meantime, I'll start looking into other issues that I can address.

parvez0 avatar Apr 29 '24 14:04 parvez0

@parvez0 Thanks for your understanding. I implemented my idea and would collect feedback from other maintainers.

knqyf263 avatar Apr 30 '24 17:04 knqyf263

This PR is stale because it has been labeled with inactivity.

github-actions[bot] avatar Jun 30 '24 00:06 github-actions[bot]