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: add k8s vulns #332

Merged
merged 18 commits into from
Oct 5, 2023
Merged

Conversation

chen-keinan
Copy link
Contributor

@chen-keinan chen-keinan commented Jul 12, 2023

Description

Feat: add k8s vulns

Related #4029
Related aquasecurity/trivy#5020
Depend on: aquasecurity/vuln-list-update#239

@chen-keinan chen-keinan marked this pull request as draft July 12, 2023 11:24
@chen-keinan chen-keinan marked this pull request as ready for review July 27, 2023 14:19
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 have some feedback on vuln-list-k8s.

  1. CVSS
    https://github.com/aquasecurity/vuln-list-k8s/blob/fedb568387ba3d6258bde457201ba4b1a2b14139/k8s/cves/CVE-2023-2878.json#L22-L24

This should be as below:

	"cvssv3": {
		"vector"* "CVSS:3.1/AV:L/AC:L/PR:L/UI:N/S:C/C:H/I:N/A:N",
		"score": 6.5
	}

The score is calculated based on the vector. They should be put together. Also, CVSS v4 is coming soon.

  1. Component name
    It looks like k8s.io is not only the prefix.

https://github.com/aquasecurity/vuln-list-k8s/blob/fedb568387ba3d6258bde457201ba4b1a2b14139/k8s/cves/CVE-2023-2878.json#L5C1-L6C1

https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/fa9befecb289b9e1a9848a39a18b104278a394bd/go.mod#L1

  1. Affected version
    The following affected version is wrong.

https://github.com/aquasecurity/vuln-list-k8s/blob/fedb568387ba3d6258bde457201ba4b1a2b14139/k8s/cves/CVE-2023-2878.json#L7-L11

The advisory says - secrets-store-csi-driver < 1.3.3, but another advisory says v1.27.0 - v1.27.1, which means 1.27.0 <= k8s <= 1.27.1. The k8s advisory format is not standardized, unfortunately. We must be aware of < or <=.

OSV defines them differently.

			"events": [ {
				"introduced": string,
				"fixed": string,
				"last_affected": string,
				"limit": string
			} ],

fixed is <, and last_affected is <=. It might be a good idea to adopt the same structure.

pkg/vulnsrc/k8svulndb/k8svulndb.go Outdated Show resolved Hide resolved
pkg/vulnsrc/k8svulndb/k8svulndb.go Outdated Show resolved Hide resolved
pkg/vulnsrc/k8svulndb/k8svulndb.go Outdated Show resolved Hide resolved
pkg/vulnsrc/k8svulndb/types.go Outdated Show resolved Hide resolved
pkg/vulnsrc/k8svulndb/types.go Outdated Show resolved Hide resolved
pkg/vulnsrc/k8svulndb/k8svulndb.go Outdated Show resolved Hide resolved
@JonZeolla
Copy link

I think #338 attempts to add support for last_affected

@chen-keinan chen-keinan force-pushed the feat/add-k8s-vulns branch 2 times, most recently from 7e7235f to 859175b Compare August 16, 2023 12:18
@chen-keinan chen-keinan marked this pull request as draft August 17, 2023 07:13
@chen-keinan chen-keinan marked this pull request as ready for review August 17, 2023 11:32
@chen-keinan
Copy link
Contributor Author

@knqyf263 all comments has been addressed, please have another look.

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.

pkg/vulnsrc/k8svulndb/k8svulndb.go Outdated Show resolved Hide resolved
@chen-keinan
Copy link
Contributor Author

@knqyf263 I have fixed the collector and validated all CVEs content.
please review again

@knqyf263
Copy link
Collaborator

@chen-keinan It seems to be challenging to correctly parse the official CVE feed. How did you improve that?

@chen-keinan
Copy link
Contributor Author

chen-keinan commented Aug 28, 2023

@chen-keinan It seems to be challenging to correctly parse the official CVE feed. How did you improve that?

What I did was to pick to vulnerable versions and cve data from a more structure (JSON) external vuln source (as appear in k8s db) [https://cveawg.mitre.org/api/cve/<CVE-ID>] but since real component ID do not appear on external source I had to take the component name from k8s source vulndb.

so in high level its a combination of both

@knqyf263
Copy link
Collaborator

knqyf263 commented Aug 28, 2023

Can I review the script first? Where is it maintained? The vulnerability data is essential.

@chen-keinan
Copy link
Contributor Author

chen-keinan commented Aug 29, 2023

Can I review the script first? Where is it maintained? The vulnerability data is essential.

@knqyf263 sure , the collector start at this point

@knqyf263
Copy link
Collaborator

Thanks. I'll take a look after completing all the tasks for v0.45.0.

Makefile Outdated Show resolved Hide resolved
pkg/vulnsrc/k8svulndb/k8svulndb.go Outdated Show resolved Hide resolved
pkg/vulnsrc/vulnerability/const.go Outdated Show resolved Hide resolved
pkg/vulnsrc/osv/osv.go Outdated Show resolved Hide resolved
pkg/vulnsrc/k8svulndb/k8svulndb.go Outdated Show resolved Hide resolved
@chen-keinan
Copy link
Contributor Author

@knqyf263 comments has been addressed

@knqyf263
Copy link
Collaborator

I had this branch on my local computer, but force-push broke it. Any benefits on force-push?

Actually, I also used to force-push when rebasing the main branch so that a merge commit won't be created, but I stopped doing that after talking with Simar because we currently use "squash and merge". The merge commit doesn't matter. It is more important to keep the commit history in PR, but please let me know if you have a specific reason.

Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
Signed-off-by: chenk <hen.keinan@gmail.com>
@chen-keinan
Copy link
Contributor Author

@DmitriyLewen thanks for the comment test data has been fixed

Signed-off-by: chenk <hen.keinan@gmail.com>
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.

Thanks!
Approved.

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 2, 2023

We'll review the k8s advisories and merge this PR.
https://github.com/aquasecurity/vuln-list-k8s/pulls

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 3, 2023

@chen-keinan What if we skip storing k8s.io/kubectl and k8s.io/client-go? They are not detected in Trivy k8s scanning, right? We will not use these buckets even if we store those vulnerabilities. Or am I missing something? Is KBOM able to detect k8s.io/kubectl?

FYI: k8s.io/client-go is a go dependency, so the vulnerabilities in pkg:golang/k8s.io/client-go are covered by GHSA.

@chen-keinan
Copy link
Contributor Author

chen-keinan commented Oct 3, 2023

@chen-keinan What if we skip storing k8s.io/kubectl and k8s.io/client-go? They are not detected in Trivy k8s scanning, right? We will not use these buckets even if we store those vulnerabilities. Or am I missing something? Is KBOM able to detect k8s.io/kubectl?

FYI: k8s.io/client-go is a go dependency, so the vulnerabilities in pkg:golang/k8s.io/client-go are covered by GHSA.

I prefer to keep kubectl as I'm still looking at simple ways to find it. regarding client-go we can exclude it

@chen-keinan
Copy link
Contributor Author

chen-keinan commented Oct 3, 2023

@chen-keinan What if we skip storing k8s.io/kubectl and k8s.io/client-go? They are not detected in Trivy k8s scanning, right? We will not use these buckets even if we store those vulnerabilities. Or am I missing something? Is KBOM able to detect k8s.io/kubectl?
FYI: k8s.io/client-go is a go dependency, so the vulnerabilities in pkg:golang/k8s.io/client-go are covered by GHSA.

I prefer to keep kubectl as I'm still looking at simple ways to find it.

I also think that we can fallback client-go to k8s.io/kubernetes where its not clear, wdyt ? otherwise lets exclude client-go

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 3, 2023

I prefer to keep kubectl as I'm still looking at simple ways to find it.

Do you mean you have an idea to find kubectl in trivy k8s?

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 3, 2023

I also think that we can fallback client-go to k8s.io/kubernetes where its not clear, wdyt ? otherwise lets exclude client-go

I did that in vuln-list-k8s. If the advisory says client-go is affected, it means the library client-go is affected, and doesn't affect other components.

@chen-keinan
Copy link
Contributor Author

chen-keinan commented Oct 3, 2023

I prefer to keep kubectl as I'm still looking at simple ways to find it.

Do you mean you have an idea to find kubectl in trivy k8s?

I have an idea , but I do not like it , thinking on better way still. anyway I would like to keep the kubectl cves for now

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 3, 2023

I have an idea , but I do not like it , thinking on better way still. anyway I would like to keep the kubectl cves for now

Got it.

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 3, 2023

Oh, my local branch is broken again. @chen-keinan Did you force-push again for some reason?

Signed-off-by: knqyf263 <knqyf263@gmail.com>
We discussed a custom type in PURL, and agreed on "k8s".
It is better to use the same values.

Signed-off-by: knqyf263 <knqyf263@gmail.com>
Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263 knqyf263 merged commit 4fc651f into aquasecurity:main Oct 5, 2023
2 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