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(vex): CSAF filtering should consider relationships #5923

Merged
merged 11 commits into from Feb 22, 2024

Conversation

juan131
Copy link
Contributor

@juan131 juan131 commented Jan 11, 2024

Description

We're currently not considering relationships within CSAF documents. However, as it's described in the CSAF reference below, relationships could be used to indicate a product is a "default component of/external component of/optional component of/installed on/installed with' another one.

With this in mind, when we detect a vulnerability affecting certain package, we need to inspect relationships because that package could be included in the CSAF document using the above mentioned relationships.

Related PRs

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

Signed-off-by: juan131 <jariza@vmware.com>
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

I am a little concerned about this implementation. Using the VEX added to the test as an example, the Kubernetes library in Argo may not be affected by CVE-2023-2727, but the same library in Kyverno may be affected. This implementation just filters out vulnerabilities, like CVE-2023-2727, with Kubernetes, leading to false suppression. I'm not familiar with CSAF VEX. Please correct me if I'm missing something.

Strictly speaking, a tree of products would need to be built on the Trivy side as well and compared with the CSAF VEX product tree.

However, that implementation will take some time, so the current implementation is fine as a work-around until then!

@juan131
Copy link
Contributor Author

juan131 commented Jan 15, 2024

Hi @knqyf263

Please note the CSAF document added to the test was created to filter out vulnerabilities affecting a container in particular (Bitnami ArgoCD 2.9.3-2 Amd64 Debian12). Therefore, it must be interpreted within that context. That's the reason for the explanation provided:

     "threats": [
        {
          "category": "impact",
          "date": "2024-01-04T17:17:25+01:00",
          "details": "The asset uses the component as a dependency in the code, but the vulnerability only affects Kubernetes clusters https://github.com/kubernetes/kubernetes/issues/118640"
        }
      ]

That CSAF document shouldn't be used to filter vulnerabilities from Kyverno or any other solution.

Does it make sense?

@knqyf263
Copy link
Collaborator

Does it make sense?

I think VEX documents should be interpreted in a context-agnostic way. Both trivy image --vex argo.csaf.vex argo:latest and trivy image --vex argo.csaf.vex kyverno:latest should work properly. Trivy interprets the VEX documents and should not apply irrelevant VEX. In this example, argo.csav.vex should not have any effect on kyverno:latest since it is not created for Kyverno.
You mean users must pass proper VEX documents, right? I think it's a tool's business. Even if they pass the wrong documents, it should not lead to wrong results.

@juan131
Copy link
Contributor Author

juan131 commented Jan 15, 2024

I think VEX documents should be interpreted in a context-agnostic way

I can think of scenarios where VEX documents could be provided within some specific context (e.g. to express that a vulnerability affecting a container is not exploitable when it's part of a specific Helm chart as a consequence of the way that chart configures the K8s manifests that make use of the container).

Regardless of this topic, the relationships described in the document do link the products unambiguously (in the example saying "kubernetes-v1.24.2' is a component of "argo-cd-2.9.3-2-amd64-debian-12":

   "relationships": [
      {
        "product_reference": "kubernetes-v1.24.2",
        "category": "default_component_of",
        "relates_to_product_reference": "argo-cd-2.9.3-2-amd64-debian-12",
        "full_product_name": {
          "product_id": "argo-cd-2.9.3-2-amd64-debian-12-kubernetes",
          "name": "Argo CD uses kubernetes golang library"
        }
      }
    ]
  }

And.. These relationships have its own "product_id" in the CSAF document ("argo-cd-2.9.3-2-amd64-debian-12-kubernetes" in this case). When a vulnerability is tied to a "product_id" identifying a relationship we ensure we're not filtering it for other product that may also have the same component.

{
      "cve": "CVE-2023-2727",
      "flags": [
        {
          "date": "2024-01-04T17:17:25+01:00",
          "label": "vulnerable_code_cannot_be_controlled_by_adversary",
          "product_ids": [
            "argo-cd-2.9.3-2-amd64-debian-12-kubernetes"
          ]
        }
      ],
      (..._

@juan131 juan131 requested a review from knqyf263 January 15, 2024 12:47
@knqyf263
Copy link
Collaborator

knqyf263 commented Jan 15, 2024

I can think of scenarios where VEX documents could be provided within some specific context (e.g. to express that a vulnerability affecting a container is not exploitable when it's part of a specific Helm chart as a consequence of the way that chart configures the K8s manifests that make use of the container).

In this case, the relationship should be described in the document. It should not depend on the context (like the VEX document is included in the Helm chart), IMO. Just to clarify, my thought is applied when relationships are defined.

   "relationships": [
      {
        "product_reference": "kubernetes-v1.24.2",
        "category": "default_component_of",
        "relates_to_product_reference": "argo-cd-2.9.3-2-amd64-debian-12",
        "full_product_name": {
          "product_id": "argo-cd-2.9.3-2-amd64-debian-12-kubernetes",
          "name": "Argo CD uses kubernetes golang library"
        }
      }
    ]
  }

If the relationship is defined as above, we should not filter out the vulnerability on k8s used by other products than Argo CD, even if the VEX document is passed.

And.. These relationships have its own "product_id" in the CSAF document ("argo-cd-2.9.3-2-amd64-debian-12-kubernetes" in this case). When a vulnerability is tied to a "product_id" identifying a relationship we ensure we're not filtering it for other product that may also have the same component.

Yes, it should work as you described. Is the current implementation working like so? This PR seems to just extract product_reference and compare it with the detected component. I might be missing something.

trivy/pkg/vex/csaf.go

Lines 100 to 102 in 2f24ac3

if fpn := rel.FullProductName; fpn != nil && fpn.ProductID != nil &&
lo.FromPtr(fpn.ProductID) == product {
subProducts = append(subProducts, rel.ProductReference)

@juan131
Copy link
Contributor Author

juan131 commented Jan 16, 2024

You're right @knqyf263 ! I was confused since I was under the impression the validity of the provided CSAF doc was questioned. That said, as you mentioned the current implementation doesn't take into account whether the tree of products on the Trivy side.

Do you think we could at least include it even it's not a perfect solution (give the VEX functionality is considered experimental)? I guess we could some some warning when a vulnerability on a package is filtered out as a consequence of being a "default component of/external component of/optional component of/installed on/installed with" of another package. Sth like this:

  • Normal message:
INFO	Filtered out the detected vulnerability	{"VEX format": "CSAF", "vulnerability-id": "CVE-XXXX-YYYY", "status": "not_affected"}
  • Message when we detect this situation:
WARN	Filtered out the detected vulnerability	{"VEX format": "CSAF", "vulnerability-id": "CVE-XXXX-YYYY", "status": "not_affected", "relationship": "default component of ZZZZ"}

@juan131
Copy link
Contributor Author

juan131 commented Jan 22, 2024

@knqyf263 what do you think about my last comment?

@knqyf263
Copy link
Collaborator

However, that implementation will take some time, so the current implementation is fine as a work-around until then!

Yes, I think we can go with the current approach, although it is not perfect. That's what I meant earlier.

I guess we could some some warning when a vulnerability on a package is filtered out as a consequence of being a "default component of/external component of/optional component of/installed on/installed with" of another package. Sth like this:

It sounds like a good idea.

@juan131
Copy link
Contributor Author

juan131 commented Jan 25, 2024

Thanks @knqyf263 !! Implemented at e86aaa7

pkg/vex/csaf.go Outdated Show resolved Hide resolved
pkg/vex/csaf.go Outdated Show resolved Hide resolved
pkg/vex/csaf.go Outdated Show resolved Hide resolved
pkg/vex/csaf.go Outdated Show resolved Hide resolved
if p.Match(pkgURL) {
return true
// getProductPurls returns a slice of PackageURLs associated to a given product
func (v *CSAF) getProductPurls(product csaf.ProductID) []*purl.PackageURL {
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: In what cases can a single product id have multiple PURLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I can't think of any situation like that but the official library is returning a slice:

pkg/vex/csaf.go Outdated Show resolved Hide resolved
juan131 and others added 4 commits February 5, 2024 10:49
Co-authored-by: Teppei Fukuda <knqyf263@gmail.com>
Signed-off-by: juan131 <jariza@vmware.com>
Signed-off-by: juan131 <jariza@vmware.com>
@juan131
Copy link
Contributor Author

juan131 commented Feb 19, 2024

@knqyf263 sorry for the long delay on addressing your comments, I've been involved in other projects these last weeks and I didn't find the time for this.

@knqyf263
Copy link
Collaborator

@juan131 No problem. Thanks for taking your time.

Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Thanks!

@knqyf263 knqyf263 added this pull request to the merge queue Feb 22, 2024
Merged via the queue into aquasecurity:main with commit 9c5e5a0 Feb 22, 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.

None yet

2 participants