diff mbox series

cve-check: Rework patch parsing

Message ID 20241228182757.2346256-1-colinmca242@gmail.com
State New
Headers show
Series cve-check: Rework patch parsing | expand

Commit Message

Colin McAllister Dec. 28, 2024, 6:27 p.m. UTC
The cve_check functionality to parse CVE IDs from the patch filename and
patch contents have been reworked to improve parsing and also add tests.
This ensures that the parsing works as intended.

Signed-off-by: Colin McAllister <colinmca242@gmail.com>
---
I noticed that there are some patches, especially in older verisons of
Yocto, where the "CVE: " tag was used with multiple CVE IDs in different
formats, like "CVE-YYYY-XXXX & CVE-YYYY-XXXX" or
"CVE-YYYY-XXXX, CVE-YYYY-XXXX". Currently, only space-delimited CVE
IDs will be parsed, but documentation doesn't indicate that is the only
supported format. I figured it'd be nice to update the code to be able
to support multiple formats, that way this patch could be backported to
fix those patches. I also wanted to add unit tests to ensure the patch
parsing behavior is preserved.

I'd also like to update the patch filename parsing to parse multiple CVE
IDs from the filename, but based on the comments, it seems like there
was a reason why only the last CVE ID is extracted from the filename.
I'd be happy to submit a V2 patch or an additional patch to update the
function if that sounds good for the maintainers.

 meta/lib/oe/cve_check.py                  | 141 +++++++++-------
 meta/lib/oeqa/selftest/cases/cve_check.py | 196 ++++++++++++++++++++++
 2 files changed, 278 insertions(+), 59 deletions(-)

Comments

Mathieu Dubois-Briand Dec. 30, 2024, 9:53 a.m. UTC | #1
On Sat Dec 28, 2024 at 7:27 PM CET, Colin McAllister via lists.openembedded.org wrote:
> The cve_check functionality to parse CVE IDs from the patch filename and
> patch contents have been reworked to improve parsing and also add tests.
> This ensures that the parsing works as intended.
>
> Signed-off-by: Colin McAllister <colinmca242@gmail.com>
> ---

Hi Colin,

It looks like this patch is causing a break on the autobuilder, with the
following error:

ERROR: libidn2-2.3.7-r0 do_cve_check: Error executing a python function in exec_func_python() autogenerated:
The stack trace of python calls that resulted in this exception/failure was:
File: 'exec_func_python() autogenerated', lineno: 2, function: <module>
     0001:
 *** 0002:do_cve_check(d)
     0003:
File: '/srv/pokybuild/yocto-worker/oe-selftest-debian/build/meta/classes/cve-check.bbclass', lineno: 184, function: do_cve_check
     0180:            try:
     0181:                patched_cves = get_patched_cves(d)
     0182:            except FileNotFoundError:
     0183:                bb.fatal("Failure in searching patches")
 *** 0184:            cve_data, status = check_cves(d, patched_cves)
     0185:            if len(cve_data) or (d.getVar("CVE_CHECK_COVERAGE") == "1" and status):
     0186:                get_cve_info(d, cve_data)
     0187:                cve_write_data(d, cve_data, status)
     0188:        else:
File: '/srv/pokybuild/yocto-worker/oe-selftest-debian/build/meta/classes/cve-check.bbclass', lineno: 423, function: check_cves
     0419:            product_cursor.close()
     0420:
     0421:            if not vulnerable:
     0422:                bb.note("%s-%s is not vulnerable to %s" % (pn, real_pv, cve))
 *** 0423:                cve_update(d, cve_data, cve, {"abbrev-status": "Patched", "status": "version-not-in-range"})
     0424:        cve_cursor.close()
     0425:
     0426:        if not cves_in_product:
     0427:            bb.note("No CVE records found for product %s, pn %s" % (product, pn))
File: '/srv/pokybuild/yocto-worker/oe-selftest-debian/build/meta/classes/cve-check.bbclass', lineno: 287, function: cve_update
     0283:
     0284:def cve_update(d, cve_data, cve, entry):
     0285:    # If no entry, just add it
     0286:    if cve not in cve_data:
 *** 0287:        cve_data[cve] = entry
     0288:        return
     0289:    # If we are updating, there might be change in the status
     0290:    bb.debug("Trying CVE entry update for %s from %s to %s" % (cve, cve_data[cve]['abbrev-status'], entry['abbrev-status']))
     0291:    if cve_data[cve]['abbrev-status'] == "Unknown":
Exception: TypeError: 'set' object does not support item assignment

https://valkyrie.yoctoproject.org/#/builders/35/builds/683/steps/14/logs/stdio

On first read, it looks like get_patched_cves() was previously returning
a dictionary and is now returning a set().

Can you fix this please ?
diff mbox series

Patch

diff --git a/meta/lib/oe/cve_check.py b/meta/lib/oe/cve_check.py
index 647a94f5af..af88a65655 100644
--- a/meta/lib/oe/cve_check.py
+++ b/meta/lib/oe/cve_check.py
@@ -8,6 +8,7 @@  import collections
 import re
 import itertools
 import functools
+import oe.patch
 
 _Version = collections.namedtuple(
     "_Version", ["release", "patch_l", "pre_l", "pre_v"]
@@ -71,77 +72,99 @@  def _cmpkey(release, patch_l, pre_l, pre_v):
     return _release, _patch, _pre
 
 
-def get_patched_cves(d):
+def parse_cve_from_filename(patch_filename):
     """
-    Get patches that solve CVEs using the "CVE: " tag.
+    Parses CVE ID from the filename
+
+    Matches the last "CVE-YYYY-ID" in the file name, also if written
+    in lowercase. Possible to have multiple CVE IDs in a single
+    file name, but only the last one will be detected from the file name.
     """
+    cve_file_name_match = re.compile(r".*(CVE-\d{4}-\d+)", re.IGNORECASE)
 
-    import re
-    import oe.patch
+    # Check patch file name for CVE ID
+    fname_match = cve_file_name_match.search(patch_filename)
+    return fname_match.group(1).upper() if fname_match else ""
 
-    cve_match = re.compile(r"CVE:( CVE-\d{4}-\d+)+")
 
-    # Matches the last "CVE-YYYY-ID" in the file name, also if written
-    # in lowercase. Possible to have multiple CVE IDs in a single
-    # file name, but only the last one will be detected from the file name.
-    # However, patch files contents addressing multiple CVE IDs are supported
-    # (cve_match regular expression)
-    cve_file_name_match = re.compile(r".*(CVE-\d{4}-\d+)", re.IGNORECASE)
+def parse_cves_from_patch_contents(patch_contents):
+    """
+    Parses CVE IDs from patch contents
+
+    Matches all CVE IDs contained on a line that starts with "CVE: ". Any
+    delimiter (',', '&', "and", etc.) can be used without any issues. Multiple
+    "CVE:" lines can also exist.
+    """
+    patched_cves = set()
+    cve_match = re.compile(r"CVE-\d{4}-\d+")
+    # Search for one or more "CVE: " lines
+    for line in patch_contents.split("\n"):
+        if not line.startswith("CVE:"):
+            continue
+        patched_cves.update(cve_match.findall(line))
+    return patched_cves
+
+
+def parse_cves_from_patch_file(patch_file):
+    """
+    Parses CVE IDs associated with a particular patch file
+    """
+    patched_cves = set()
+    filename_cve = parse_cve_from_filename(patch_file)
+    if filename_cve:
+        bb.debug(2, "Found %s from patch file name %s" % (filename_cve, patch_file))
+        patched_cves.add(parse_cve_from_filename(patch_file))
+
+    # Remote patches won't be present and compressed patches won't be
+    # unpacked, so say we're not scanning them
+    if not os.path.isfile(patch_file):
+        bb.note("%s is remote or compressed, not scanning content" % patch_file)
+        return patched_cves
+
+    with open(patch_file, "r", encoding="utf-8") as f:
+        try:
+            patch_text = f.read()
+        except UnicodeDecodeError:
+            bb.debug(
+                1,
+                "Failed to read patch %s using UTF-8 encoding"
+                " trying with iso8859-1" % patch_file,
+            )
+            f.close()
+            with open(patch_file, "r", encoding="iso8859-1") as f:
+                patch_text = f.read()
 
-    patched_cves = {}
+    patched_cves.update(parse_cves_from_patch_contents(patch_text))
+
+    if not patched_cves:
+        bb.debug(2, "Patch %s doesn't solve CVEs" % patch_file)
+    else:
+        bb.debug(
+            2, "Patch %s solves %s" % (patch_file, ", ".join(sorted(patched_cves)))
+        )
+
+    return patched_cves
+
+
+def get_patched_cves(d):
+    """
+    Get patches that solve CVEs
+    """
+    patched_cves = set()
     patches = oe.patch.src_patches(d)
     bb.debug(2, "Scanning %d patches for CVEs" % len(patches))
+
+    # Check each patch file
     for url in patches:
         patch_file = bb.fetch.decodeurl(url)[2]
-
-        # Check patch file name for CVE ID
-        fname_match = cve_file_name_match.search(patch_file)
-        if fname_match:
-            cve = fname_match.group(1).upper()
-            patched_cves[cve] = {"abbrev-status": "Patched", "status": "fix-file-included", "resource": patch_file}
-            bb.debug(2, "Found %s from patch file name %s" % (cve, patch_file))
-
-        # Remote patches won't be present and compressed patches won't be
-        # unpacked, so say we're not scanning them
-        if not os.path.isfile(patch_file):
-            bb.note("%s is remote or compressed, not scanning content" % patch_file)
-            continue
-
-        with open(patch_file, "r", encoding="utf-8") as f:
-            try:
-                patch_text = f.read()
-            except UnicodeDecodeError:
-                bb.debug(1, "Failed to read patch %s using UTF-8 encoding"
-                        " trying with iso8859-1" %  patch_file)
-                f.close()
-                with open(patch_file, "r", encoding="iso8859-1") as f:
-                    patch_text = f.read()
-
-        # Search for one or more "CVE: " lines
-        text_match = False
-        for match in cve_match.finditer(patch_text):
-            # Get only the CVEs without the "CVE: " tag
-            cves = patch_text[match.start()+5:match.end()]
-            for cve in cves.split():
-                bb.debug(2, "Patch %s solves %s" % (patch_file, cve))
-                patched_cves[cve] = {"abbrev-status": "Patched", "status": "fix-file-included", "resource": patch_file}
-                text_match = True
-
-        if not fname_match and not text_match:
-            bb.debug(2, "Patch %s doesn't solve CVEs" % patch_file)
+        patched_cves.update(parse_cves_from_patch_file(patch_file))
 
     # Search for additional patched CVEs
-    for cve in (d.getVarFlags("CVE_STATUS") or {}):
-        decoded_status = decode_cve_status(d, cve)
-        products = d.getVar("CVE_PRODUCT")
-        if has_cve_product_match(decoded_status, products) == True:
-            patched_cves[cve] = {
-                "abbrev-status": decoded_status["mapping"],
-                "status": decoded_status["detail"],
-                "justification": decoded_status["description"],
-                "affected-vendor": decoded_status["vendor"],
-                "affected-product": decoded_status["product"]
-            }
+    for cve in d.getVarFlags("CVE_STATUS") or {}:
+        decoded_status, _, _ = decode_cve_status(d, cve)
+        if decoded_status == "Patched":
+            bb.debug(2, "CVE %s is additionally patched" % cve)
+            patched_cves.add(cve)
 
     return patched_cves
 
diff --git a/meta/lib/oeqa/selftest/cases/cve_check.py b/meta/lib/oeqa/selftest/cases/cve_check.py
index 3dd3e89d3e..7a73681b84 100644
--- a/meta/lib/oeqa/selftest/cases/cve_check.py
+++ b/meta/lib/oeqa/selftest/cases/cve_check.py
@@ -121,6 +121,202 @@  class CVECheck(OESelftestTestCase):
         self.assertEqual(has_cve_product_match(status, "glibca:glibc test"), True)
 
 
+    def test_parse_cve_from_patch_filename(self):
+        from oe.cve_check import parse_cve_from_filename
+
+        # Patch filename without CVE ID
+        self.assertEqual(parse_cve_from_filename("0001-test.patch"), "")
+
+        # Patch with single CVE ID
+        self.assertEqual(parse_cve_from_filename("CVE-2022-12345.patch"), "CVE-2022-12345")
+
+        # Patch with multiple CVE IDs
+        self.assertEqual(
+            parse_cve_from_filename("CVE-2022-41741-CVE-2022-41742.patch"), "CVE-2022-41742"
+        )
+
+        # Patches with CVE ID and appended text
+        self.assertEqual(
+            parse_cve_from_filename("CVE-2023-3019-0001.patch"), "CVE-2023-3019"
+        )
+        self.assertEqual(
+            parse_cve_from_filename("CVE-2024-21886-1.patch"), "CVE-2024-21886"
+        )
+
+        # Patch with CVE ID and prepended text
+        self.assertEqual(
+            parse_cve_from_filename("grep-CVE-2012-5667.patch"), "CVE-2012-5667"
+        )
+        self.assertEqual(
+            parse_cve_from_filename("0001-CVE-2012-5667.patch"), "CVE-2012-5667"
+        )
+
+        # Patch with CVE ID and both prepended and appended text
+        self.assertEqual(
+            parse_cve_from_filename(
+                "0001-tpm2_import-fix-fixed-AES-key-CVE-2021-3565-0001.patch"
+            ),
+            "CVE-2021-3565",
+        )
+        
+        # Only grab the last CVE ID in the filename
+        self.assertEqual(
+            parse_cve_from_filename(
+                "CVE-2012-5667-CVE-2012-5668.patch"
+            ),
+            "CVE-2012-5668",
+        )
+
+
+    def test_parse_cve_from_patch_contents(self):
+        import textwrap
+        from oe.cve_check import parse_cves_from_patch_contents
+
+        # Standard patch file excerpt without any patches
+        self.assertEqual(
+            parse_cves_from_patch_contents(
+                textwrap.dedent("""\
+            remove "*" for root since we don't have a /etc/shadow so far.
+
+            Upstream-Status: Inappropriate [configuration]
+
+            Signed-off-by: Scott Garman <scott.a.garman@intel.com>
+
+            --- base-passwd/passwd.master~nobash
+            +++ base-passwd/passwd.master
+            @@ -1,4 +1,4 @@
+            -root:*:0:0:root:/root:/bin/sh
+            +root::0:0:root:/root:/bin/sh
+            daemon:*:1:1:daemon:/usr/sbin:/bin/sh
+            bin:*:2:2:bin:/bin:/bin/sh
+            sys:*:3:3:sys:/dev:/bin/sh
+            """)
+            ),
+            set(),
+        )
+
+        # Patch file with multiple CVE IDs (space-separated)
+        self.assertEqual(
+            parse_cves_from_patch_contents(
+                textwrap.dedent("""\
+                There is an assertion in function _cairo_arc_in_direction().
+
+                CVE: CVE-2019-6461 CVE-2019-6462
+                Upstream-Status: Pending
+                Signed-off-by: Ross Burton <ross.burton@intel.com>
+
+                diff --git a/src/cairo-arc.c b/src/cairo-arc.c
+                index 390397bae..1bde774a4 100644
+                --- a/src/cairo-arc.c
+                +++ b/src/cairo-arc.c
+                @@ -186,7 +186,8 @@ _cairo_arc_in_direction (cairo_t          *cr,
+                    if (cairo_status (cr))
+                        return;
+
+                -    assert (angle_max >= angle_min);
+                +    if (angle_max < angle_min)
+                +       return;
+
+                    if (angle_max - angle_min > 2 * M_PI * MAX_FULL_CIRCLES) {
+                    angle_max = fmod (angle_max - angle_min, 2 * M_PI);
+            """),
+            ),
+            {"CVE-2019-6461", "CVE-2019-6462"},
+        )
+
+        # Patch file with multiple CVE IDs (comma-separated w/ both space and no space)
+        self.assertEqual(
+            parse_cves_from_patch_contents(
+                textwrap.dedent("""\
+                There is an assertion in function _cairo_arc_in_direction().
+
+                CVE: CVE-2019-6461,CVE-2019-6462, CVE-2019-6463
+                Upstream-Status: Pending
+                Signed-off-by: Ross Burton <ross.burton@intel.com>
+
+                diff --git a/src/cairo-arc.c b/src/cairo-arc.c
+                index 390397bae..1bde774a4 100644
+                --- a/src/cairo-arc.c
+                +++ b/src/cairo-arc.c
+                @@ -186,7 +186,8 @@ _cairo_arc_in_direction (cairo_t          *cr,
+                    if (cairo_status (cr))
+                        return;
+
+                -    assert (angle_max >= angle_min);
+                +    if (angle_max < angle_min)
+                +       return;
+
+                    if (angle_max - angle_min > 2 * M_PI * MAX_FULL_CIRCLES) {
+                    angle_max = fmod (angle_max - angle_min, 2 * M_PI);
+
+            """),
+            ),
+            {"CVE-2019-6461", "CVE-2019-6462", "CVE-2019-6463"},
+        )
+
+        # Patch file with multiple CVE IDs (&-separated)
+        self.assertEqual(
+            parse_cves_from_patch_contents(
+                textwrap.dedent("""\
+                There is an assertion in function _cairo_arc_in_direction().
+
+                CVE: CVE-2019-6461 & CVE-2019-6462
+                Upstream-Status: Pending
+                Signed-off-by: Ross Burton <ross.burton@intel.com>
+
+                diff --git a/src/cairo-arc.c b/src/cairo-arc.c
+                index 390397bae..1bde774a4 100644
+                --- a/src/cairo-arc.c
+                +++ b/src/cairo-arc.c
+                @@ -186,7 +186,8 @@ _cairo_arc_in_direction (cairo_t          *cr,
+                    if (cairo_status (cr))
+                        return;
+
+                -    assert (angle_max >= angle_min);
+                +    if (angle_max < angle_min)
+                +       return;
+
+                    if (angle_max - angle_min > 2 * M_PI * MAX_FULL_CIRCLES) {
+                    angle_max = fmod (angle_max - angle_min, 2 * M_PI);
+            """),
+            ),
+            {"CVE-2019-6461", "CVE-2019-6462"},
+        )
+
+        # Patch file with multiple lines with CVE IDs
+        self.assertEqual(
+            parse_cves_from_patch_contents(
+                textwrap.dedent("""\
+                There is an assertion in function _cairo_arc_in_direction().
+
+                CVE: CVE-2019-6461 & CVE-2019-6462
+
+                CVE: CVE-2019-6463 & CVE-2019-6464
+                Upstream-Status: Pending
+                Signed-off-by: Ross Burton <ross.burton@intel.com>
+
+                diff --git a/src/cairo-arc.c b/src/cairo-arc.c
+                index 390397bae..1bde774a4 100644
+                --- a/src/cairo-arc.c
+                +++ b/src/cairo-arc.c
+                @@ -186,7 +186,8 @@ _cairo_arc_in_direction (cairo_t          *cr,
+                    if (cairo_status (cr))
+                        return;
+
+                -    assert (angle_max >= angle_min);
+                +    if (angle_max < angle_min)
+                +       return;
+
+                    if (angle_max - angle_min > 2 * M_PI * MAX_FULL_CIRCLES) {
+                    angle_max = fmod (angle_max - angle_min, 2 * M_PI);
+
+            """),
+            ),
+            {"CVE-2019-6461", "CVE-2019-6462", "CVE-2019-6463", "CVE-2019-6464"},
+        )
+
+
+
     def test_recipe_report_json(self):
         config = """
 INHERIT += "cve-check"