fix(mariner): use `advisory_id` for definition file names #271

DmitriyLewen · 2024-02-13T05:45:18Z

Description

There are times when CBL-Mariner uses multiple definitions for a single CVE.
e.g:

<definition class="vulnerability" id="oval:com.microsoft.cbl-mariner:def:31880" version="1">
      <metadata>
        <title>CVE-2023-5678 affecting package openssl for versions less than 1.1.1k-28</title>
        <affected family="unix">
          <platform>CBL-Mariner</platform>
        </affected>
        <reference ref_id="CVE-2023-5678" ref_url="https://nvd.nist.gov/vuln/detail/CVE-2023-5678" source="CVE"/>
        <patchable>true</patchable>
        <advisory_id>31880-1</advisory_id>
        <severity>Medium</severity>
        <description>CVE-2023-5678 affecting package openssl for versions less than 1.1.1k-28. A patched version of the package is available.</description>
      </metadata>
      <criteria operator="AND">
        <criterion comment="Package openssl is earlier than 1.1.1k-28, affected by CVE-2023-5678" test_ref="oval:com.microsoft.cbl-mariner:tst:31880000"/>
      </criteria>
    </definition>
    <definition class="vulnerability" id="oval:com.microsoft.cbl-mariner:def:31872" version="1">
      <metadata>
        <title>CVE-2023-5678 affecting package edk2 for versions less than 20230301gitf80f052277c8-38</title>
        <affected family="unix">
          <platform>CBL-Mariner</platform>
        </affected>
        <reference ref_id="CVE-2023-5678" ref_url="https://nvd.nist.gov/vuln/detail/CVE-2023-5678" source="CVE"/>
        <patchable>true</patchable>
        <advisory_id>31872-1</advisory_id>
        <severity>Medium</severity>
        <description>CVE-2023-5678 affecting package edk2 for versions less than 20230301gitf80f052277c8-38. A patched version of the package is available.</description>
      </metadata>
      <criteria operator="AND">
        <criterion comment="Package edk2 is earlier than 20230301gitf80f052277c8-38, affected by CVE-2023-5678" test_ref="oval:com.microsoft.cbl-mariner:tst:31872000"/>
      </criteria>
    </definition>

To avoid overwriting - use advisory_id field for file names.
See #271 (comment)

Related Issues

[Mariner] trivy doesn't detect CVE-2023-5678 for package earlier than where the fix has been first introduced. trivy-db#379

knqyf263 · 2024-02-15T10:57:38Z

I'm checking with the Aqua commercial team.

eric-desrochers · 2024-03-11T13:45:54Z

Any ETA for this PR to be merged in Trivy ?

eric-desrochers · 2024-03-20T17:45:40Z

@knqyf263 any development based on your check with Aqua commercial team ?

knqyf263 · 2024-04-01T05:56:20Z

I'll remind them

eric-desrochers · 2024-04-08T14:27:40Z

I'll remind them

@knqyf263 any update ? This is impacting AKS Image Cleaner in its ability to retire unsecure images for which CVE are found.

knqyf263 · 2024-04-15T06:35:06Z

I reminded them again, but they seem to be busy. If you're in a hurry, you can update your data as described below.
aquasecurity/trivy-db#379 (comment)

knqyf263 · 2024-05-15T05:35:55Z

@DmitriyLewen Can't we save two files named the advisory id instead of CVE-ID, like /definitions/2023/31872-1.json?

DmitriyLewen · 2024-05-15T06:05:00Z

hm... i didn't think about this.

I see 2 points:

I checked oval files. Looks like all definitions contain advisory_id. Also all advisory_id are unique.

➜ cat cbl-mariner-2.0-oval.xml| grep advisory_id | sort | uniq -d   
➜ cat cbl-mariner-1.0-oval.xml| grep advisory_id | sort | uniq -d
➜ cat cbl-mariner-1.0-oval.xml| grep '<definition class="vulnerability"' | wc -l
    2252
➜ cat cbl-mariner-1.0-oval.xml| grep advisory_id | wc -l         
    2252
➜ cat cbl-mariner-2.0-oval.xml| grep '<definition class="vulnerability"' | wc -l
    2702
➜ cat cbl-mariner-2.0-oval.xml| grep advisory_id | wc -l
    2702

But advisory_id is custom field - https://github.com/OVALProject/Language/blob/7fa7bba7b48f09decb732d00b2be032a487ff9fc/schemas/oval-definitions-schema.xsd#L242-L276. Therefore, we cannot be 100% sure about this field.

Working with these files will be more difficult. advisory_id means nothing to user. Therefore, to find file for CVE, you need to use search. It's not a big problem, but it's still 1 extra action.

knqyf263 · 2024-05-15T06:27:08Z

But advisory_id is custom field - https://github.com/OVALProject/Language/blob/7fa7bba7b48f09decb732d00b2be032a487ff9fc/schemas/oval-definitions-schema.xsd#L242-L276. Therefore, we cannot be 100% sure about this field.

id and version are required. I think we can construct an advisory id from them.
https://github.com/OVALProject/Language/blob/7fa7bba7b48f09decb732d00b2be032a487ff9fc/schemas/oval-definitions-schema.xsd#L237-L238

    <definition class="vulnerability" id="oval:com.microsoft.cbl-mariner:def:31880" version="1">

Working with these files will be more difficult. advisory_id means nothing to user. Therefore, to find file for CVE, you need to use search. It's not a big problem, but it's still 1 extra action.

It's no big deal. We already use custom advisory IDs for others.
https://github.com/aquasecurity/vuln-list/tree/main/ghsa/go/github.com/aws/aws-sdk-go
https://github.com/aquasecurity/vuln-list/tree/main/alma/9/2024

DmitriyLewen · 2024-05-15T06:48:20Z

hm... looks like your are right.
IIUC in some cases mariner uses only latest from version:

    <definition class="vulnerability" id="oval:com.microsoft.cbl-mariner:def:27423" version="2000000001">
...
        <advisory_id>27423-1</advisory_id>

But it doesn't have much impact.

knqyf263 · 2024-05-15T07:28:43Z

IIUC in some cases mariner uses only latest from version:

What do you mean? 2000000001 is the latest?

DmitriyLewen · 2024-05-15T07:32:22Z

hm... i missed word 😞
I meant mariner uses only latest number from version
version="2000000001" -> 1
version="2000000002" -> 2
`version="2000000000" -> without version

knqyf263 · 2024-05-15T07:38:16Z

Got it, you mean they use the last digit. Even 27423-2000000001 is not bad since we just need a unique identifier.

DmitriyLewen · 2024-05-15T07:42:04Z

only cbl-1.0 uses version 2000...
cbl-2.0 uses single digit as version (1,2,3, etc.).
I think we can reproduce their logic.

knqyf263 · 2024-05-15T07:47:04Z

If we go with this approach, what changes do we need in trivy-db compared to the current PR?

DmitriyLewen · 2024-05-15T07:49:33Z

we will only need to update names of test files to match the names in vuln-list.

mariner/mariner.go

DmitriyLewen · 2024-05-15T08:27:57Z

mariner/mariner.go


-func (c Config) saveAdvisoryPerYear(dirName string, vulnID string, def Definition) error {
+	vulnID := def.Metadata.Reference.RefID


I thought about using advisory_date field.
But this field does not always exist:

➜ cat cbl-mariner-1.0-oval.xml| grep ' <definition class="vulnerability"' | toilet -l 2252 ➜ cat cbl-mariner-1.0-oval.xml| grep '<advisory_date>' | toilet -l 2070

So I'm leaving logic with year number from CVE.

DmitriyLewen · 2024-05-15T08:28:36Z

@knqyf263 I updated this PR. Can you take a look?

knqyf263 · 2024-05-15T09:43:15Z

Even after we merge this PR, will trivy-db not fail?

DmitriyLewen · 2024-05-15T09:53:35Z

right.
I used mariner dir with new file names:

➜  make db-build
./trivy-db build --cache-dir cache --update-interval 6h
2024/05/15 15:51:45 Updating vulnerability database...
2024/05/15 15:51:45 Updating cbl-mariner data...
2024/05/15 15:51:45     Parsing cache/vuln-list/mariner/1.0
2024/05/15 15:51:45     Parsing cache/vuln-list/mariner/2.0

knqyf263 · 2024-05-15T10:37:43Z

Thanks for confirming. I'll merge this PR, then.

knqyf263

I left some small comments.

knqyf263 · 2024-05-15T10:40:58Z

mariner/mariner.go

@@ -154,8 +152,24 @@ func (c Config) update(version, path string) error {

 	return nil
 }
+func (c Config) saveAdvisoryPerYear(dirName string, def Definition) error {
+	// Mariner uses `<ID>_<last_number_from_version>` format for `advisory_id`.


AFAIK, they use hyphens.

Suggested change

// Mariner uses `<ID>_<last_number_from_version>` format for `advisory_id`.

// Mariner uses `<ID>-<last_number_from_version>` format for `advisory_id`.

We should start calling Azure Linux.

Suggested change

// Mariner uses `<ID>_<last_number_from_version>` format for `advisory_id`.

// Azure Linux uses `<ID>_<last_number_from_version>` format for `advisory_id`.

fixed in 995ea97

knqyf263 · 2024-05-15T10:41:58Z

mariner/mariner.go

+	// cf. https://github.com/aquasecurity/vuln-list-update/pull/271#issuecomment-2111678641
+	advisoryID := def.Metadata.AdvisoryID
+	if advisoryID == "" {
+		advisoryID = def.ID


The ID is something like oval:com.microsoft.cbl-mariner:obj:31880001. Shouldn't we extract the last digits, 31880001?

Nice catch!
Changed in 995ea97
Also added tests - 7074604

knqyf263 · 2024-05-15T10:42:11Z

mariner/mariner.go

+		advisoryID = def.ID
+		// for `0` versions `_0` suffix is omitted.
+		if def.Version != "" && def.Version[len(def.Version)-1:] != "0" {
+			advisoryID = fmt.Sprintf("%s_%s", advisoryID, def.Version[len(def.Version)-1:])


Suggested change

advisoryID = fmt.Sprintf("%s_%s", advisoryID, def.Version[len(def.Version)-1:])

advisoryID = fmt.Sprintf("%s-%s", advisoryID, def.Version[len(def.Version)-1:])

fixed in 995ea97

knqyf263 · 2024-05-15T10:42:26Z

mariner/mariner.go

+func (c Config) saveAdvisoryPerYear(dirName string, def Definition) error {
+	// Mariner uses `<ID>_<last_number_from_version>` format for `advisory_id`.
+	// But `advisory_id` is not required field.
+	// Therefore, if `advisory_id` is not exist, we create this field independently.


nit

Suggested change

// Therefore, if `advisory_id` is not exist, we create this field independently.

// Therefore, if `advisory_id` does not exist, we create this field independently.

updated in 995ea97

knqyf263 · 2024-05-15T11:42:20Z

Triggered the workflow
https://github.com/aquasecurity/vuln-list-update/actions/runs/9095013101

DmitriyLewen added 2 commits February 13, 2024 11:37

fix(mariner): save defs as array

2be1bc7

test: update tests

b94e1d3

DmitriyLewen self-assigned this Feb 13, 2024

DmitriyLewen mentioned this pull request Feb 13, 2024

test(mariner): update tests after changing filenames in vuln-list aquasecurity/trivy-db#382

Merged

1 task

DmitriyLewen marked this pull request as ready for review February 14, 2024 08:38

DmitriyLewen requested a review from knqyf263 February 14, 2024 08:38

knqyf263 mentioned this pull request Feb 15, 2024

[Mariner] trivy doesn't detect CVE-2023-5678 for package earlier than where the fix has been first introduced. aquasecurity/trivy-db#379

Closed

knqyf263 mentioned this pull request May 8, 2024

fix(mariner): support multiple definitions for a single vulnerability aquasecurity/trivy#6661

Closed

DmitriyLewen added 4 commits May 15, 2024 14:16

revert: save Definition as struct

af63f0f

refactor: use advisory_id as struct

1b7c548

tests: update golden files

092ba40

fix: update repoURL

b76577c

DmitriyLewen force-pushed the fix/mariner-defs-as-array branch from 48b9969 to b76577c Compare May 15, 2024 08:20

DmitriyLewen commented May 15, 2024

View reviewed changes

mariner/mariner.go Show resolved Hide resolved

DmitriyLewen commented May 15, 2024

View reviewed changes

DmitriyLewen changed the title ~~fix(mariner): save defenitions as array~~ fix(mariner): use advisory_id for definition file names May 15, 2024

knqyf263 reviewed May 15, 2024

View reviewed changes

DmitriyLewen added 3 commits May 15, 2024 17:15

refactor: move advisoryID assembly to separate function

995ea97

test: add tests for AdvisoryID

7074604

refactor: mix comment mistake

089504c

knqyf263 approved these changes May 15, 2024

View reviewed changes

knqyf263 merged commit 80e370d into aquasecurity:main May 15, 2024
2 checks passed

DmitriyLewen deleted the fix/mariner-defs-as-array branch May 16, 2024 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mariner): use `advisory_id` for definition file names #271

fix(mariner): use `advisory_id` for definition file names #271

DmitriyLewen commented Feb 13, 2024 •

edited

knqyf263 commented Feb 15, 2024

eric-desrochers commented Mar 11, 2024

eric-desrochers commented Mar 20, 2024

knqyf263 commented Apr 1, 2024

eric-desrochers commented Apr 8, 2024

knqyf263 commented Apr 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

DmitriyLewen May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024 •

edited

knqyf263 commented May 15, 2024

knqyf263 left a comment

knqyf263 May 15, 2024

knqyf263 May 15, 2024

DmitriyLewen May 15, 2024

knqyf263 May 15, 2024

DmitriyLewen May 15, 2024

knqyf263 May 15, 2024

DmitriyLewen May 15, 2024

knqyf263 May 15, 2024

DmitriyLewen May 15, 2024

knqyf263 commented May 15, 2024


		func (c Config) saveAdvisoryPerYear(dirName string, vulnID string, def Definition) error {
		vulnID := def.Metadata.Reference.RefID

	// Mariner uses `<ID>_<last_number_from_version>` format for `advisory_id`.
	// Mariner uses `<ID>-<last_number_from_version>` format for `advisory_id`.

	// Mariner uses `<ID>_<last_number_from_version>` format for `advisory_id`.
	// Azure Linux uses `<ID>_<last_number_from_version>` format for `advisory_id`.

	advisoryID = fmt.Sprintf("%s_%s", advisoryID, def.Version[len(def.Version)-1:])
	advisoryID = fmt.Sprintf("%s-%s", advisoryID, def.Version[len(def.Version)-1:])

	// Therefore, if `advisory_id` is not exist, we create this field independently.
	// Therefore, if `advisory_id` does not exist, we create this field independently.

fix(mariner): use advisory_id for definition file names #271

fix(mariner): use advisory_id for definition file names #271

Conversation

DmitriyLewen commented Feb 13, 2024 • edited

Description

Related Issues

knqyf263 commented Feb 15, 2024

eric-desrochers commented Mar 11, 2024

eric-desrochers commented Mar 20, 2024

knqyf263 commented Apr 1, 2024

eric-desrochers commented Apr 8, 2024

knqyf263 commented Apr 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024

Choose a reason for hiding this comment

DmitriyLewen commented May 15, 2024

knqyf263 commented May 15, 2024

DmitriyLewen commented May 15, 2024 • edited

knqyf263 commented May 15, 2024

knqyf263 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knqyf263 commented May 15, 2024

fix(mariner): use `advisory_id` for definition file names #271

fix(mariner): use `advisory_id` for definition file names #271

DmitriyLewen commented Feb 13, 2024 •

edited

DmitriyLewen commented May 15, 2024 •

edited