On Wed, Jul 17, 2024 at 9:11 AM Marko, Peter <[email protected]> wrote:
> Hi Marta, > > Thanks for the great work on this topic. > I have left 3 comments below. > > Thanks for considering them. > Peter > > > -----Original Message----- > > From: [email protected] <openembedded- > > [email protected]> On Behalf Of Marta Rybczynska via > > lists.openembedded.org > > Sent: Monday, July 15, 2024 12:20 > > To: [email protected] > > Cc: Marta Rybczynska <[email protected]>; Samantha Jalabert > > <[email protected]> > > Subject: [OE-core] [PATCH 1/3] cve-check: enrich annotation of CVEs > > > > Previously the information passed between different function of the > > cve-check class included only tables of patched, unpatched, ignored > > vulnerabilities and the general status of the recipe. > > > > The VEX work requires more information, and we need to pass them > > between different functions, so that it can be enriched as the > > analysis progresses. Instead of multiple tables, use a single one > > with annotations for each CVE encountered. For example, a patched > > CVE will have: > > > > {"abbrev-status": "Patched", "status": "version-not-in-range"} > > > > abbrev-status contains the general status (Patched, Unpatched, > > Ignored and Unknown that will be added in the VEX code) > > status contains more detailed information that can come from > > CVE_STATUS and the analysis. > > > > Additional fields of the annotation include for example the name > > of the patch file fixing a given CVE. > > > > Signed-off-by: Marta Rybczynska <[email protected]> > > Signed-off-by: Samantha Jalabert <[email protected]> > > --- > > meta/classes/cve-check.bbclass | 208 +++++++++++++++++---------------- > > meta/lib/oe/cve_check.py | 12 +- > > 2 files changed, 113 insertions(+), 107 deletions(-) > > > > diff --git a/meta/classes/cve-check.bbclass > b/meta/classes/cve-check.bbclass > > index 93a2a1413d..f177223568 100644 > > --- a/meta/classes/cve-check.bbclass > > +++ b/meta/classes/cve-check.bbclass > > @@ -188,10 +188,10 @@ python do_cve_check () { > > patched_cves = get_patched_cves(d) > > except FileNotFoundError: > > bb.fatal("Failure in searching patches") > > - ignored, patched, unpatched, status = check_cves(d, > patched_cves) > > - if patched or unpatched or (d.getVar("CVE_CHECK_COVERAGE") > == "1" > > and status): > > - cve_data = get_cve_info(d, patched + unpatched + > ignored) > > - cve_write_data(d, patched, unpatched, ignored, > cve_data, status) > > + cve_data, status = check_cves(d, patched_cves) > > + if len(cve_data) or (d.getVar("CVE_CHECK_COVERAGE") == "1" > and > > status): > > + get_cve_info(d, cve_data) > > + cve_write_data(d, cve_data, status) > > else: > > bb.note("No CVE database found, skipping CVE check") > > > > @@ -294,7 +294,51 @@ ROOTFS_POSTPROCESS_COMMAND:prepend = > > "${@'cve_check_write_rootfs_manifest ' if d > > do_rootfs[recrdeptask] += "${@'do_cve_check' if > > d.getVar('CVE_CHECK_CREATE_MANIFEST') == '1' else ''}" > > do_populate_sdk[recrdeptask] += "${@'do_cve_check' if > > d.getVar('CVE_CHECK_CREATE_MANIFEST') == '1' else ''}" > > > > -def check_cves(d, patched_cves): > > +def cve_is_ignored(d, cve_data, cve): > > + if cve not in cve_data: > > + return False > > + if cve_data[cve]['abbrev-status'] == "Ignored": > > + return True > > + return False > > + > > +def cve_is_patched(d, cve_data, cve): > > + if cve not in cve_data: > > + return False > > + if cve_data[cve]['abbrev-status'] == "Patched": > > + return True > > + return False > > + > > +def cve_update(d, cve_data, cve, entry): > > I think that there is a fundamental change in behavior here. > Previously we were taking (NVD) DB as base and only vulnerable CVEs were > compared annotated with CVE_STATUS or our presence of CVE patches. > Now we take the CVE_STATUS and CVE patches as base and add entries from DB > only if they were not annotated yet. > This was a little more complicated than that. get_patched_cves() was taking a part of CVE_STATUS at the beginning of the process, then applying the NVD database. The change is to import the totality and then update the status in the process. Now, the entries in CVE_STATUS had priority before, and they still have. Now it is explicit, before it was hidden in the code. I do not see changes in the end result, do you have a case in mind? > I am not arguing against it, I actually like it much more as we will be > able to insert also CVEs not in DB into our reports. > But I have two comments on this: > * this should be explicitly described in commit message > Yes, we can add it here. The logic will be documented more in the standalone tool, because with the CVE database, the update rule is even more complex. (this is also good, it forces to write the list of priorities) > * this makes global cve includes spill into all recipe reports, so this > commit series should also get rid of cve-extra-exclusions.inc file finally > (or at least add a comment into it with this sideeffect) > This is the plan. I do not want to push a series removing cve-extra-inclusions.inc with this one, but maybe, as you suggest, we add a warning to this file and then remove with a separate series? > > > + # If no entry, just add it > > + if cve not in cve_data: > > + cve_data[cve] = entry > > + return > > + # If we are updating, there might be change in the status > > + bb.debug("Trying CVE entry update for %s from %s to %s" % (cve, > > cve_data[cve]['abbrev-status'], entry['abbrev-status'])) > > + if cve_data[cve]['abbrev-status'] == "Unknown": > > + cve_data[cve] = entry > > + return > > + if cve_data[cve]['abbrev-status'] == entry['abbrev-status']: > > + return > > + # Update like in {'abbrev-status': 'Patched', 'status': > 'version-not-in-range'} > > to {'abbrev-status': 'Unpatched', 'status': 'version-in-range'} > > + if entry['abbrev-status'] == "Unpatched" and cve_data[cve]['abbrev- > > status'] == "Patched": > > + if entry['status'] == "version-in-range" and > cve_data[cve]['status'] == > > "version-not-in-range": > > + # New result from the scan, vulnerable > > + cve_data[cve] = entry > > + bb.debug("CVE entry %s update from Patched to Unpatched > from the > > scan result" % cve) > > + return > > + if entry['abbrev-status'] == "Patched" and > cve_data[cve]['abbrev-status'] > > == "Unpatched": > > + if entry['status'] == "version-not-in-range" and > cve_data[cve]['status'] == > > "version-in-range": > > + # Range does not match the scan, but we already have a > vulnerable > > match, ignore > > + bb.debug("CVE entry %s update from Patched to Unpatched > from the > > scan result - not applying" % cve) > > + return > > + # If we have an "Ignored", it has a priority > > + if cve_data[cve]['abbrev-status'] == "Ignored": > > + bb.debug("CVE %s not updating because Ignored" % cve) > > + return > > + bb.warn("Unhandled CVE entry update for %s from %s to %s" % (cve, > > cve_data[cve], entry)) > > + > > +def check_cves(d, cve_data): > > """ > > Connect to the NVD database and find unpatched cves. > > """ > > @@ -304,28 +348,19 @@ def check_cves(d, patched_cves): > > real_pv = d.getVar("PV") > > suffix = d.getVar("CVE_VERSION_SUFFIX") > > > > - cves_unpatched = [] > > - cves_ignored = [] > > cves_status = [] > > cves_in_recipe = False > > # CVE_PRODUCT can contain more than one product (eg. curl/libcurl) > > products = d.getVar("CVE_PRODUCT").split() > > # If this has been unset then we're not scanning for CVEs here (for > example, > > image recipes) > > if not products: > > - return ([], [], [], []) > > + return ([], []) > > pv = d.getVar("CVE_VERSION").split("+git")[0] > > > > # If the recipe has been skipped/ignored we return empty lists > > if pn in d.getVar("CVE_CHECK_SKIP_RECIPE").split(): > > bb.note("Recipe has been skipped by cve-check") > > - return ([], [], [], []) > > - > > - # Convert CVE_STATUS into ignored CVEs and check validity > > - cve_ignore = [] > > - for cve in (d.getVarFlags("CVE_STATUS") or {}): > > - decoded_status, _, _ = decode_cve_status(d, cve) > > - if decoded_status == "Ignored": > > - cve_ignore.append(cve) > > + return ([], []) > > > > import sqlite3 > > db_file = d.expand("file:${CVE_CHECK_DB_FILE}?mode=ro") > > @@ -344,11 +379,10 @@ def check_cves(d, patched_cves): > > for cverow in cve_cursor: > > cve = cverow[0] > > > > - if cve in cve_ignore: > > + if cve_is_ignored(d, cve_data, cve): > > bb.note("%s-%s ignores %s" % (product, pv, cve)) > > - cves_ignored.append(cve) > > continue > > - elif cve in patched_cves: > > + elif cve_is_patched(d, cve_data, cve): > > bb.note("%s has been patched" % (cve)) > > continue > > # Write status once only for each product > > @@ -364,7 +398,7 @@ def check_cves(d, patched_cves): > > for row in product_cursor: > > (_, _, _, version_start, operator_start, version_end, > operator_end) = > > row > > #bb.debug(2, "Evaluating row " + str(row)) > > - if cve in cve_ignore: > > + if cve_is_ignored(d, cve_data, cve): > > ignored = True > > > > version_start = convert_cve_version(version_start) > > @@ -403,16 +437,16 @@ def check_cves(d, patched_cves): > > if vulnerable: > > if ignored: > > bb.note("%s is ignored in %s-%s" % (cve, pn, > real_pv)) > > - cves_ignored.append(cve) > > + cve_update(d, cve_data, cve, {"abbrev-status": > "Ignored"}) > > I think that this case happens only when we ignore the CVE via CVE_STATUS. > So calling this is not necessary as it is already prefilled from for it, > correct? > In case it would not be prefilled, then it would drop our enrichment... > > > else: > > bb.note("%s-%s is vulnerable to %s" % (pn, > real_pv, cve)) > > - cves_unpatched.append(cve) > > + cve_update(d, cve_data, cve, {"abbrev-status": > "Unpatched", > > "status": "version-in-range"}) > > Could you please include the new statuses in cve-check-map.conf > (e.g. version-in-range, version-not-in-range, fix-file-included) > I think it's important to have all possible statuses there to be able to > correct the DB manually. > Will add. There will be a bigger pass on the doc after it is all merged, because there's one additional state "Unknown". Kind regards, Marta
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#202185): https://lists.openembedded.org/g/openembedded-core/message/202185 Mute This Topic: https://lists.openembedded.org/mt/107228576/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
