Re: [OE-core][PATCH v3 1/3] cve-check: add option to add additional patched CVEs

Marta Rybczynska
2023-05-19T13:12:18Z

[-- Attachment #1: Type: text/plain, Size: 5512 bytes --]

Thank you for this work. I think we are going in a good direction. My
comments in the text.

In general, I would like that we come with the fixed list of possible
statuses and avoid adding new ones too frequently. Changing them will break
my parsing and status scripts each time.

On Fri, May 19, 2023 at 8:24 AM Andrej Valek via lists.openembedded.org
<andrej.valek=siemens.com@lists.openembedded.org> wrote:

> - Replace CVE_CHECK_IGNORE with CVE_STATUS + [CVE_STATUS_REASONING] to be
> more flexible. CVE_STATUS should contain flag for each CVE with accepted
> values "Ignored", "Not applicable" or "Patched". It allows to add
> a status for each CVEs.
>

I'm missing a status to cover the situation when the NVD (or any other
database) has an incorrect entry. We have quite many of those. This might
be a temporary situation, but not always.

SPDX (the 3.0 draft) has some other possible reasons
https://github.com/spdx/spdx-spec/blob/vulnerability-profile/chapters/profile-vulnerabilities.md
What looks like interesting ideas are:
* "Can't fix" / "Will not fix"
* "Not applicable" (SPDX language: Ineffective) when the code is not used
* "Invalid match" (this is our NVD mismatch case)
* "Mitigated" measures taken so that it cannot be exploited
* "Workarounded"

There is still one big missing part: related to configuration options. It
could be used with "Not applicable"/"Ineffective" code, but only in cases
where it is not possible to activate the code. If the user can switch
between vulnerable/not vulnerable versions by a packageconfig change or so,
this is not covered.

Addiional question: why CVE_STATUS_REASONING and not CVE_STATUS_REASON ?
(reason variable is used nearly everywhere)

> diff --git a/meta/classes/cve-check.bbclass
> b/meta/classes/cve-check.bbclass
> index bd9e7e7445c..44462de7445 100644
> --- a/meta/classes/cve-check.bbclass
> +++ b/meta/classes/cve-check.bbclass
> @@ -70,12 +70,15 @@ CVE_CHECK_COVERAGE ??= "1"
> # Skip CVE Check for packages (PN)
> CVE_CHECK_SKIP_RECIPE ?= ""
>
> -# Ingore the check for a given list of CVEs. If a CVE is found,
> -# then it is considered patched. The value is a string containing
> -# space separated CVE values:
> +# Replace NVD DB check status for a given CVE. Each of CVE has to be
> mentioned
> +# separately with optional reason for this status.
> #
> -# CVE_CHECK_IGNORE = 'CVE-2014-2524 CVE-2018-1234'
> +# CVE_STATUS[CVE-1234-0001] = "Ignored" # or "Not applicable" or "Patched"
> +# CVE_STATUS[CVE-1234-0002] = "Not applicable"
> +# CVE_STATUS_REASONING[CVE-1234-0002] = "Issue only applies on Windows"
> #
> +# CVE_CHECK_IGNORE is deprecated and CVE_STATUS has to be used instead.
> +# Keep CVE_CHECK_IGNORE until other layers migrate to new variables
> CVE_CHECK_IGNORE ?= ""
>
> # Layers to be excluded
> @@ -88,6 +91,25 @@ CVE_CHECK_LAYER_INCLUDELIST ??= ""
> # set to "alphabetical" for version using single alphabetical character
> as increment release
> CVE_VERSION_SUFFIX ??= ""
>
> +python () {
> + # Fallback all CVEs from CVE_CHECK_IGNORE to CVE_STATUS
> + cve_check_ignore = d.getVar("CVE_CHECK_IGNORE")
> + if cve_check_ignore:
> + bb.warn("CVE_CHECK_IGNORE has been deprecated, use CVE_STATUS
> instead")
> + set_cves_statuses(d, d.getVar("CVE_CHECK_IGNORE"), "Ignored")
> +
> + # Process CVE_STATUS_GROUPS to set multiple statuses and optional
> reasons at once
> + for cve_status_group in (d.getVar("CVE_STATUS_GROUPS") or "").split():
> + set_cves_statuses(d, d.getVar(cve_status_group) or "",
> + d.getVarFlag(cve_status_group, "status"),
> + d.getVarFlag(cve_status_group, "reason"))
> +}
> +
> +def set_cves_statuses(d, cves, status, reason=""):
> + for cve in cves.split():
> + d.setVarFlag("CVE_STATUS", cve, status)
> + d.setVarFlag("CVE_STATUS_REASONING", cve, reason)
> +
> def generate_json_report(d, out_path, link_path):
> if os.path.exists(d.getVar("CVE_CHECK_SUMMARY_INDEX_PATH")):
> import json
> @@ -282,7 +304,13 @@ def check_cves(d, patched_cves):
> bb.note("Recipe has been skipped by cve-check")
> return ([], [], [], [])
>
> - cve_ignore = d.getVar("CVE_CHECK_IGNORE").split()
> + # Convert CVE_STATUS into ignored CVEs and check validity
> + cve_ignore = []
> + for cve, status in (d.getVarFlags("CVE_STATUS") or {}).items():
> + if status in ["Not applicable", "Ignored"]:
> + cve_ignore.append(cve)
> + elif status not in ["Patched"]:
> + bb.error("Unsupported status %s in CVE_STATUS[%s]" % (status,
> cve))
>
I do not see this entry added into the "Patched" list.

IMO would be better to handle Patched separately, and so a complete "else"
for all other reasons. Allows to avoid hard-coding all possible options.

>
> import sqlite3
> db_file = d.expand("file:${CVE_CHECK_DB_FILE}?mode=ro")
> @@ -455,6 +483,9 @@ def cve_write_data_text(d, patched, unpatched,
> ignored, cve_data):
> else:
> unpatched_cves.append(cve)
> write_string += "CVE STATUS: Unpatched\n"
> + reasoning = d.getVarFlag("CVE_STATUS_REASONING", cve)
> + if reasoning:
> + write_string += "CVE REASON: %s\n" % reasoning
>

Do we want adding new features to the (deprecated) text output?

Kind regards,
Marta

>
>

[-- Attachment #2: Type: text/html, Size: 7820 bytes --]