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

Joshua Watt
2024-06-18T15:22:44Z

On Tue, Jun 18, 2024 at 8:48 AM Marta Rybczynska <rybczynska@gmail.com> wrote:
>
> 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...

I think we should look at that later just to keep the scope
reasonable; This is purposely trying to stick close to what SPDX 2.2
gives us.

>
>>
>> +
>> +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.

I've dropped this in the latest version, in favor of only using sha256
to make my life easier :)

>
>
>>
>>
>> +
>> +
>> +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.

Yep, but I think we need to focus on that after the initial
implementation. It's a solvable problem, but it's going to to be
tricky so it will take time, and I'd rather get SPDX 3 support out
early.

>
>> +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?

It's not cryptographic, so it should not matter. I don't want the hash
strings to be _terribly_ long, so unless you have some other short
hashing algorithm, I'd rather stick with this.

>>
>> + 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}

Will do, thanks. Should that be a third entry, or the only entry, or.... ?

>
>>
>> + )
>> + )
>> + 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.

Yes, I think we can do that, but we need cve_check.py to be able to
give us the file names (or hashes?) so we can correlate them.... I'll
log a bug to do that later.

>
>>
>> +
>> + 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.

Ya, I think I can add that based on the not-applicable-platform or
not-applicable-config

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