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

feat(report): update gitlab template to populate operating_system value #7735

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

aarongoldenthal
Copy link
Contributor

Description

Update the gitlab.tpl container scanning template to populate the vulnerabilities[].location.operating_system value from the Target. The current template extracts the image from Target, but the OS is not used and has the value hardcoded as "operating_system": "Unknown". This update extracts the OS from Target, if specified (simply checking for ( in the Target), to populate the operating_system value (for example "operating_system": "alpine 3.10.2"). If not specified, the default of Unknown is used.

Related issues

Checklist

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

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2024

CLA assistant check
All committers have signed the CLA.

@aarongoldenthal
Copy link
Contributor Author

The existing test was updated to reflect the operating_system value being populated, but what's missing is a test with a report returning "operating_system": "Unknown". The example I was thinking of was an image FROM scratch with at least one vulnerability, but all of the existing container image test fixtures appear to use a known OS, so I thought I'd check before going that direction.

I did manually test against anchore/syft:v0.20.0 (I was looking for something older likely to have vulnerabilities, FROM scratch, and it was on my mind), and it did properly report "Unknown":

{
  ...
  "vulnerabilities": [
    {
      "id": "CVE-2024-24579",
      "name": "stereoscope vulnerable to tar path traversal when processing OCI tar archives",
      "description": "stereoscope is a go library for processing container images and simulating a squash filesystem.  Prior to version 0.0.1, it is possible to craft an OCI tar archive that, when stereoscope attempts to unarchive the contents, will result in writing to paths outside of the unarchive temporary directory. Specifically, use of `github.com/anchore/stereoscope/pkg/file.UntarToDirectory()` function, the  `github.com/anchore/stereoscope/pkg/image/oci.TarballImageProvider` struct, or the higher level `github.com/anchore/stereoscope/pkg/image.Image.Read()` function express this vulnerability. As a workaround, if you are using the OCI archive as input into stereoscope then you can switch to using an OCI layout by unarchiving the tar archive and provide the unarchived directory to stereoscope.",
      "severity": "Medium",
      "solution": "Upgrade github.com/anchore/stereoscope to 0.0.1",
      "location": {
        "dependency": {
          "package": {
            "name": "github.com/anchore/stereoscope"
          },
          "version": "v0.0.0-20210817160504-0f4abc2a5a5a"
        },
        "operating_system": "Unknown",
        "image": "syft"
      },
      "identifiers": [
        {
          "type": "cve",
          "name": "CVE-2024-24579",
          "value": "CVE-2024-24579",
          "url": "https://avd.aquasec.com/nvd/cve-2024-24579"
        }
      ],
      "links": [{
          "url": "https://github.com/anchore/stereoscope"
        },{
          "url": "https://github.com/anchore/stereoscope/commit/09dacab4d9ee65ee8bc7af8ebf4aa7b5aaa36204"
        },{
          "url": "https://github.com/anchore/stereoscope/security/advisories/GHSA-hpxr-w9w7-g4gv"
        },{
          "url": "https://nvd.nist.gov/vuln/detail/CVE-2024-24579"
        }
      ]
    },
    ...
  ],
  ...
}

@knqyf263 knqyf263 requested a review from DmitriyLewen October 15, 2024 07:15
@@ -29,6 +29,10 @@
{{- range . }}
{{- $target := .Target }}
{{- $image := $target | regexFind "[^\\s]+" }}
{{- $os := "Unknown" }}
{{- if contains "(" $target -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am worried that filepath might contain (:

Target: targetName(app.Type, app.FilePath),

Can Class field be used to detect OS package vulnerabilities?

Target: fmt.Sprintf("%s (%s %s)", target.Name, target.OS.Family, target.OS.Name),
Class: types.ClassOSPkg,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal here was to replicate the data from GitLab's container scanning tool using Trivy directly. Within GitLab, the format is unique to container scanning (i.e. container images in a registry). In that use case, there's shouldn't be parentheses in the .Target value as they're invalid. If it's being used in other cases with file paths then parentheses could be an issue, but then spaces could also be an issue with line 31

{{- $image := $target | regexFind "[^\\s]+" }}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the Class question, GitLab actually displays the results, not just looking to identify OS packages, for example:
image

Copy link
Contributor

@DmitriyLewen DmitriyLewen Oct 21, 2024

Choose a reason for hiding this comment

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

If it's being used in other cases with file paths then parentheses could be an issue, but then spaces could also be an issue with line 31

Hm.. you are right.
We need to think about language-specific files from container.

I think we can check all targets to find os and image and use them for all targets.

Something like this:

  {{- $image := "Unknown" -}}
  {{- $os := "Unknown" -}}
  {{- range . }}
    {{- if eq .Class "os-pkgs" -}}
      {{- $target := .Target }}
        {{- $image = $target | regexFind "[^\\s]+" }}
        {{- $os = $target | splitList "(" | last | trimSuffix ")" }}
    {{- end }}
  {{- end }}
  "vulnerabilities": [
  {{- $t_first := true }}
  {{- range . }}
  {{- $target := .Target }}
    {{- range .Vulnerabilities -}}

wdyt?

test:

FROM alpine

COPY log4j-core-2.17.0.jar /te(st)/log4j-core-2.17.0.jar

Before:

➜ trivy -q image test:7735 -f template -t @./contrib/gitlab.tpl | grep "operating_system" -A 1
        "operating_system": "alpine 3.20.3",
        "image": "test:7735"
--
        "operating_system": "alpine 3.20.3",
        "image": "test:7735"
--
        "operating_system": "Unknown",
        "image": "Java"

After:

➜  trivy -q image test:7735 -f template -t @./contrib/gitlab.tpl | grep "operating_system" -A 1
        "operating_system": "alpine 3.20.3",
        "image": "test:7735"
--
        "operating_system": "alpine 3.20.3",
        "image": "test:7735"
--
        "operating_system": "alpine 3.20.3",
        "image": "test:7735"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DmitriyLewen I like it. I did a quick test and it is slightly different than what GitLab output, but I think it's more logical. By default they run with --vuln-type os, which I had forgotten, but after overriding they output "operating_system": "Unknown" for non-OS packages. There are some cases, for example I can think of some npm package examples, that have language-specific installations (installing binaries), so I think the OS is useful data.

At the moment there's one test for this template, which I had updated, but what are your thoughts on tests for the "Unknown" cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment there's one test for this template, which I had updated, but what are your thoughts on tests for the "Unknown" cases?

Hm... You can add test for testdata/fixtures/images/vulnimage.tar.gz (use low severity to skip a few vulns).

➜ trivy -q image  -f template -t @./contrib/gitlab.tpl  --input ./integration/testdata/fixtures/images/vulnimage.tar.gz --severity LOW | grep "operating_system" -A 1
        "operating_system": "alpine 3.7.1",
        "image": "./integration/testdata/fixtures/images/vulnimage.tar.gz"
--
        "operating_system": "alpine 3.7.1",
        "image": "./integration/testdata/fixtures/images/vulnimage.tar.gz"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually thinking the other direction - an image, maybe something FROM scratch, that would report "operating_system": "Unknown", just to test the defaults. I'm not sure it's critical, but didn't see an image in the existing testcases that looked like it would work. I think the image/os values are already covered in the test for integration/testdata/alpine-310.gitlab.golden.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have provided vulnimage.tar.gz to show that vulnerabilities for language packages now contain the correct operating_system and image values.

Actual behavior:

➜  trivy -q image  -f template -t @./contrib/gitlab.tpl  --input ./integration/testdata/fixtures/images/vulnimage.tar.gz --severity LOW | grep "operating_system" -A 1
        "operating_system": "Unknown",
        "image": "./integration/testdata/fixtures/images/vulnimage.tar.gz"
--
        "operating_system": "Unknown",
        "image": "rust-app/Cargo.lock"

that would report "operating_system": "Unknown", just to test the defaults

We can use fs mode (use one dir from repo to check this. In this case OS is not detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test and the actions are all passing in my fork, so from my perspective I think this is ready.

contrib/gitlab.tpl Outdated Show resolved Hide resolved
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

@aarongoldenthal Thanks for your work and patience!

LGTM

cc. @knqyf263

@knqyf263 knqyf263 enabled auto-merge October 24, 2024 07:02
@knqyf263 knqyf263 added this pull request to the merge queue Oct 24, 2024
Merged via the queue into aquasecurity:main with commit c0d79fa Oct 24, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants