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

["Valek, Andrej" <andrej.valek@siemens.com>]

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

Hello Marta,

On Fri, 2023-05-19 at 15:11 +0200, Marta Rybczynska wrote:
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<http://lists.openembedded.org> <andrej.valek=siemens.com@lists.openembedded.org<mailto: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"

I would say, "Ignored", "Not applicable" or "Patched" are enough, because everything important is covered. Of course we can extend some keywords in the feature, but we shouldn't confuse users.

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)

See explanation here: https://lists.openembedded.org/g/openembedded-core/message/181551 . Once we have a decision, I can change it.


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.

Of course this code is not covering the "Patched" ;). Check cve_check.py how the "Patched" is handled. Elif case is covering the typos.

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?

Is "reasoning/reason" deprecated? We're just adding new entry.

Kind regards,
Marta


Regards,
Andrej

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