Re: [OE-core][PATCH 5/6] classes/create-spdx-3.0: Add class

Marta Rybczynska
2024-06-18T14:48:54Z

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

Hello,
This is my first round of the review, before applying the code.

On Mon, Jun 10, 2024 at 11:45 PM Joshua Watt via lists.openembedded.org
<JPEWhacker=gmail.com@lists.openembedded.org> wrote:

| +SPDX_PROFILES ?= "core build software simpleLicensing security"

I would like to discuss this choice by default. People who do security
won't need the 'build' profile (and this likely adds
much data). On the other hand, people who do licensing won't need the
'security' profile.

It would be really nice to generate a basic SPDX without doing the build...

> +
> +SPDX_HASH_ALGORITHMS ?= "sha256 sha1"
>

Any reason to still use sha1? It is obsolete. We could use sha3 instead if
we want to have multiple ones.

>
> +
> +
> +python do_create_spdx() {
> + import oe.sbom30
> + import oe.spdx30
> + from pathlib import Path
> + from contextlib import contextmanager
> + import oe.cve_check
> + from datetime import datetime
> +
> +
> + def set_var_field(var, obj, name, package=None):
> + val = None
> + if package:
> + val = d.getVar("%s:%s" % (var, package))
> +
> + if not val:
> + val = d.getVar(var)
> +
> + if val:
> + setattr(obj, name, val)
> +
> + deploydir = Path(d.getVar("SPDXDEPLOY"))
> + deploy_dir_spdx = Path(d.getVar("DEPLOY_DIR_SPDX"))
> + spdx_workdir = Path(d.getVar("SPDXWORK"))
> + include_sources = d.getVar("SPDX_INCLUDE_SOURCES") == "1"
> + pkg_arch = d.getVar("SSTATE_PKGARCH")
> + is_native = bb.data.inherits_class("native", d) or
> bb.data.inherits_class("cross", d)
> +
> + build_objset = oe.sbom30.ObjectSet.new_objset(d, d.getVar("PN"))
> +
> + build = build_objset.new_sub_build("recipe", "recipe")
> + build_objset.doc.rootElement.append(build)
> +
> + build_objset.set_is_native(is_native)
> +
> + for var in (d.getVar('SPDX_CUSTOM_ANNOTATION_VARS') or "").split():
> + new_annotation(
> + d,
> + build_objset,
> + build,
> + "%s=%s" % (var, d.getVar(var)),
> + oe.spdx30.AnnotationType.other
> + )
> +
> + build_inputs = set()
> +
> + # Add CVEs
> + cve_by_status = {}
> + cve_by_status["Patched"] = {cve: build_objset.new_cve_vuln(cve) for
> cve in oe.cve_check.get_patched_cves(d)}
> +
> + for cve in (d.getVarFlags("CVE_STATUS") or {}):
> + status, _, _ = oe.cve_check.decode_cve_status(d, cve)
> + # Already added above
> + if status == "Patched":
> + continue
> +
> + cve_by_status.setdefault(status, {})[cve] =
> build_objset.new_cve_vuln(cve)
> +
> + cpe_ids = oe.cve_check.get_cpe_ids(d.getVar("CVE_PRODUCT"),
> d.getVar("CVE_VERSION"))
>

This will be adding only those from patches and everything with CVE_STATUS.
In some cases (extra-exclusions) vulnerabilities
that have no real link with the given package. Also, the list isn't
complete, does not contain most of the unpatched ones.

+do_create_spdx[vardepsexclude] += "BB_NUMBER_THREADS"
> +# NOTE: depending on do_unpack is a hack that is necessary to get it's
> dependencies for archive the source
> +addtask do_create_spdx after do_populate_sysroot do_package
> do_packagedata do_unpack do_collect_spdx_deps do_package_write_ipk
> do_pacakge_write_deb do_package_write_rpm before do_populate_sdk do_build
> do_rm_work
> +
>

This is really late... I fear that if we get that in, it will be too hard
to make the generation mode modulable and output
certain profiles at a different place than the whole build. With the same
problem for the world build as we have today
with 2.2.

+def spdxid_hash(*items):
> + h = hashlib.md5()
> + for i in items:
> + if isinstance(i, oe.spdx30.Element):
> + h.update(i._id.encode("utf-8"))
> + else:
> + h.update(i.encode("utf-8"))
> + return h.hexdigest()
> +
>

Can we avoid MD5?

> + def new_cve_vuln(self, cve):
> + v = oe.spdx30.security_Vulnerability()
> + v._id = self.new_spdxid("vulnerability", cve)
> + v.creationInfo = self.doc.creationInfo
> +
> + v.externalIdentifier.append(
> + oe.spdx30.ExternalIdentifier(
> +
> externalIdentifierType=oe.spdx30.ExternalIdentifierType.cve,
> + identifier=cve,
> + identifierLocator=[
> + f"
> https://cve.mitre.org/cgi-bin/cvename.cgi?name={cve}",
> + f"https://www.cve.org/CVERecord?id={cve}",
> + ],
>

Better to link to the JSON format https://cveawg.mitre.org/api/cve/{cve}

> + )
> + )
> + return self.add(v)
> +
> + def new_vex_patched_relationship(self, from_, to):
> + self.add(
> + oe.spdx30.security_VexFixedVulnAssessmentRelationship(
> + _id=self.new_spdxid("vex-fixed", spdxid_hash(from_, *to)),
> + creationInfo=self.doc.creationInfo,
> + from_=from_,
> + relationshipType=oe.spdx30.RelationshipType.fixedIn,
> + to=to,
> + security_vexVersion=VEX_VERSION,
> + )
> + )
>

For machine-readable entries, this should be linking to a proof of a fix,
for example in
a relationship to the fix file.

> +
> + def new_vex_unpatched_relationship(self, from_, to):
> + self.add(
> + oe.spdx30.security_VexAffectedVulnAssessmentRelationship(
> + _id=self.new_spdxid("vex-affected", spdxid_hash(from_,
> *to)),
> + creationInfo=self.doc.creationInfo,
> + from_=from_,
> + relationshipType=oe.spdx30.RelationshipType.affects,
> + to=to,
> + security_vexVersion=VEX_VERSION,
> + )
> + )
> +
> + def new_vex_ignored_relationship(self, from_, to, *,
> impact_statement):
> + self.add(
> + oe.spdx30.security_VexNotAffectedVulnAssessmentRelationship(
> + _id=self.new_spdxid(
> + "vex-not-affected", spdxid_hash(impact_statement,
> from_, *to)
> + ),
> + creationInfo=self.doc.creationInfo,
> + from_=from_,
> + relationshipType=oe.spdx30.RelationshipType.doesNotAffect,
> + to=to,
> + security_vexVersion=VEX_VERSION,
> + security_impactStatement=impact_statement,
> + )
> + )
>

Missing conversion of 'Ignored' to 'justificationType' which is necessary
to allow machine processing of VEX statements. With 'impactStatement' only,
which is freeform text, this is human readable only.

That's it in the first pass. There will be more when I can have a look in
the output.

Regards,
Marta

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