-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
scripts: zephyr_module: Add CPE support #66495
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -569,6 +569,45 @@ Build files located in a ``MODULE_EXT_ROOT`` can be described as: | |
This allows control of the build inclusion to be described externally to the | ||
Zephyr module. | ||
|
||
.. _modules-vulnerability-monitoring: | ||
|
||
Vulnerability monitoring | ||
======================== | ||
|
||
The module description file :file:`zephyr/module.yml` can be used to improve vulnerability monitoring. | ||
|
||
If your module needs to track vulnerabilities using an external reference | ||
(e.g your module is forked from another repository), you can use the ``security`` section. | ||
It contains the field ``external-references`` that contains a list of references that needs to | ||
be monitored for your module. The supported formats are: | ||
|
||
- CPE (Common Platform Enumeration) | ||
- PURL (Package URL) | ||
|
||
.. code-block:: yaml | ||
kartben marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
security: | ||
external-references: | ||
kartben marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- <module-related-cpe> | ||
- <an-other-module-related-cpe> | ||
- <module-related-purl> | ||
|
||
A real life example for `mbedTLS` module could look like this: | ||
|
||
.. code-block:: yaml | ||
|
||
security: | ||
external-references: | ||
- cpe:2.3:a:arm:mbed_tls:3.5.2:*:*:*:*:*:*:* | ||
- pkg:github/Mbed-TLS/[email protected] | ||
Comment on lines
+599
to
+602
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually poses a good question regarding what CPE and PURLs should be used for the Zephyr forks of the upstream project. BTW I think it would be great if the sample code block would show a bit more of the "surroundings" so that folks have an immediate clue regarding the nesting level of the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Zephyr forks must used the CPE/PURL with the version it forked for the current version. For example, for mbedTLS, the last commit merge the upstream 3.5.2 into the zephyr fork (zephyrproject-rtos/mbedtls@6ec4abd) , so For the example, I did not want to add keys that are not related to my modification (it would probably not be updated if a changes occurs in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I now understand why you wouldn't list other keys in case the schema changes, still wonder if it would make sense to at least mention something like " ... you can use the top-level Re: CPE/PURL, part of my question was also (but really just a question for my own education and not something that should get in the way of this PR in any way) how to deal with the Zephyr module forks where it's more than just mirroring the upstream module but also including our own patches (which can come with their own vulnerabilities) on top of it. Should the "external references" list both the CPE/PURL of the upstream module + a CPE/PURL corresponding to the Zephyr fork? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are totally right. This was part of the initial discussion (#53479). At first I wanted zephyr modules maintainers to have the responsibility of the CVE for their project. We finally decided, for now, it wouldn't be the case. The future steps I would like to have:
But this is a discussion to have with the security team / @ceolin and see what's possible or not. Let's take this step by step :) I hope it answers your question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
it very much does :) Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, ideally Zephyr forks will have their on CPE, but adding it right now would be useless since these CPEs would no match any CVE database. From a practical point of view, what we really need now is a way to monitor CVEs in the projects we have forked. |
||
|
||
.. note:: | ||
CPE field must follow the CPE 2.3 schema provided by `NVD | ||
<https://csrc.nist.gov/projects/security-content-automation-protocol/specifications/cpe>`_. | ||
PURL field must follow the PURL specification provided by `Github | ||
<https://github.com/package-url/purl-spec/blob/master/PURL-SPECIFICATION.rst>`_. | ||
|
||
|
||
Build system integration | ||
======================== | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
|
||
import os | ||
import yaml | ||
import re | ||
|
||
from west import log | ||
from west.util import west_topdir, WestNotFound | ||
|
@@ -47,6 +48,7 @@ def __init__(self, cfg): | |
self.docZephyr = None | ||
self.docApp = None | ||
self.docSDK = None | ||
self.docModulesExtRefs = None | ||
|
||
# dict of absolute file path => the Document that owns that file | ||
self.allFileLinks = {} | ||
|
@@ -69,6 +71,36 @@ def __init__(self, cfg): | |
# SDK install path from parsed CMake cache | ||
self.sdkPath = "" | ||
|
||
def _build_purl(self, url, version=None): | ||
if not url: | ||
return None | ||
|
||
purl = None | ||
# This is designed to match repository with the following url pattern: | ||
# '<protocol><base_url>/<namespace>/<package> | ||
COMMON_GIT_URL_REGEX=r'((git@|http(s)?:\/\/)(?P<base_url>[\w\.@]+)(\/|:))(?P<namespace>[\w,\-,\_]+)\/(?P<package>[\w,\-,\_]+)(.git){0,1}((\/){0,1})$' | ||
|
||
match = re.fullmatch(COMMON_GIT_URL_REGEX, url) | ||
if match: | ||
purl = f'pkg:{match.group("base_url")}/{match.group("namespace")}/{match.group("package")}' | ||
|
||
if purl and (version or len(version) > 0): | ||
purl += f'@{version}' | ||
|
||
return purl | ||
|
||
def _add_describe_relationship(self, doc, cfgpackage): | ||
# create DESCRIBES relationship data | ||
rd = RelationshipData() | ||
rd.ownerType = RelationshipDataElementType.DOCUMENT | ||
rd.ownerDocument = doc | ||
rd.otherType = RelationshipDataElementType.PACKAGEID | ||
rd.otherPackageID = cfgpackage.spdxID | ||
rd.rlnType = "DESCRIBES" | ||
|
||
# add it to pending relationships queue | ||
self.pendingRelationships.append(rd) | ||
|
||
# primary entry point | ||
def makeDocuments(self): | ||
# parse CMake cache file and get compiler path | ||
|
@@ -163,16 +195,7 @@ def setupAppDocument(self): | |
pkgApp = Package(cfgPackageApp, self.docApp) | ||
self.docApp.pkgs[pkgApp.cfg.spdxID] = pkgApp | ||
|
||
# create DESCRIBES relationship data | ||
rd = RelationshipData() | ||
rd.ownerType = RelationshipDataElementType.DOCUMENT | ||
rd.ownerDocument = self.docApp | ||
rd.otherType = RelationshipDataElementType.PACKAGEID | ||
rd.otherPackageID = cfgPackageApp.spdxID | ||
rd.rlnType = "DESCRIBES" | ||
|
||
# add it to pending relationships queue | ||
self.pendingRelationships.append(rd) | ||
self._add_describe_relationship(self.docApp, cfgPackageApp) | ||
|
||
def setupBuildDocument(self): | ||
# set up build document | ||
|
@@ -196,7 +219,7 @@ def setupBuildDocument(self): | |
# add it to pending relationships queue | ||
self.pendingRelationships.append(rd) | ||
|
||
def setupZephyrDocument(self, modules): | ||
def setupZephyrDocument(self, zephyr, modules): | ||
# set up zephyr document | ||
cfgZephyr = DocumentConfig() | ||
cfgZephyr.name = "zephyr-sources" | ||
|
@@ -217,40 +240,66 @@ def setupZephyrDocument(self, modules): | |
cfgPackageZephyr.spdxID = "SPDXRef-zephyr-sources" | ||
cfgPackageZephyr.relativeBaseDir = relativeBaseDir | ||
|
||
zephyr_url = zephyr.get("remote", "") | ||
if zephyr_url: | ||
cfgPackageZephyr.url = zephyr_url | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand https://spdx.github.io/spdx-spec/v2.3/package-information/#771-description correctly, I think the URL should be more than "just" the https:// url to the github repo ; i.e. it should be in the form There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But if looking at the examples given in 7.7.3, then it states:
so |
||
|
||
if zephyr.get("revision"): | ||
cfgPackageZephyr.revision = zephyr.get("revision") | ||
|
||
purl = None | ||
zephyr_tags = zephyr.get("tags", "") | ||
if zephyr_tags: | ||
# Find tag vX.Y.Z | ||
for tag in zephyr_tags: | ||
version = re.fullmatch(r'^v(?P<version>\d+\.\d+\.\d+)$', tag) | ||
purl = self._build_purl(zephyr_url, tag) | ||
|
||
if purl: | ||
cfgPackageZephyr.externalReferences.append(purl) | ||
|
||
# Extract version from tag once | ||
if cfgPackageZephyr.version == "" and version: | ||
cfgPackageZephyr.version = version.group('version') | ||
|
||
if len(cfgPackageZephyr.version) > 0: | ||
cpe = f'cpe:2.3:o:zephyrproject:zephyr:{cfgPackageZephyr.version}:-:*:*:*:*:*:*' | ||
cfgPackageZephyr.externalReferences.append(cpe) | ||
|
||
pkgZephyr = Package(cfgPackageZephyr, self.docZephyr) | ||
self.docZephyr.pkgs[pkgZephyr.cfg.spdxID] = pkgZephyr | ||
|
||
self._add_describe_relationship(self.docZephyr, cfgPackageZephyr) | ||
|
||
for module in modules: | ||
module_name = module.get("name", None) | ||
module_path = module.get("path", None) | ||
module_url = module.get("remote", None) | ||
module_revision = module.get("revision", None) | ||
|
||
if not module_name: | ||
log.err(f"cannot find module name in meta file; bailing") | ||
return False | ||
|
||
# Replace "_" by "-" since it's not allowed in spdx ID | ||
module_name = module_name.replace("_", "-") | ||
|
||
# set up zephyr sources package | ||
cfgPackageZephyrModule = PackageConfig() | ||
cfgPackageZephyrModule.name = module_name | ||
cfgPackageZephyrModule.name = module_name + "-sources" | ||
cfgPackageZephyrModule.spdxID = "SPDXRef-" + module_name + "-sources" | ||
cfgPackageZephyrModule.relativeBaseDir = module_path | ||
cfgPackageZephyrModule.primaryPurpose = "SOURCE" | ||
|
||
if module_revision: | ||
cfgPackageZephyrModule.revision = module_revision | ||
|
||
if module_url: | ||
cfgPackageZephyrModule.url = module_url | ||
|
||
pkgZephyrModule = Package(cfgPackageZephyrModule, self.docZephyr) | ||
self.docZephyr.pkgs[pkgZephyrModule.cfg.spdxID] = pkgZephyrModule | ||
|
||
# create DESCRIBES relationship data | ||
rd = RelationshipData() | ||
rd.ownerType = RelationshipDataElementType.DOCUMENT | ||
rd.ownerDocument = self.docZephyr | ||
rd.otherType = RelationshipDataElementType.PACKAGEID | ||
rd.otherPackageID = cfgPackageZephyr.spdxID | ||
rd.rlnType = "DESCRIBES" | ||
self._add_describe_relationship(self.docZephyr, cfgPackageZephyrModule) | ||
|
||
# add it to pending relationships queue | ||
self.pendingRelationships.append(rd) | ||
return True | ||
|
||
def setupSDKDocument(self): | ||
# set up SDK document | ||
|
@@ -280,6 +329,40 @@ def setupSDKDocument(self): | |
# add it to pending relationships queue | ||
self.pendingRelationships.append(rd) | ||
|
||
def setupModulesDocument(self, modules): | ||
# set up zephyr document | ||
cfgModuleExtRef = DocumentConfig() | ||
cfgModuleExtRef.name = "modules-deps" | ||
cfgModuleExtRef.namespace = self.cfg.namespacePrefix + "/modules-deps" | ||
cfgModuleExtRef.docRefID = "DocumentRef-modules-deps" | ||
self.docModulesExtRefs = Document(cfgModuleExtRef) | ||
|
||
for module in modules: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not an SPDX expert so please pardon the dumb question but does it really make sense to list all modules enabled in the workspace, even the ones (and that's potentially a long list) that don't end up being part of the final application? Related: AFAICS there doesn't seem to be a way to figure out what modules are actually used, or what files/packages (as per the contents of build.spdx and zephyr.spdx) "belong" to what module? So how would one know if they are effectively impacted by a vulnerability reported against a particular package? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a question in the first PR I did (#66182 (comment)). I'm not sure what we should do on this, so for now, I did not changed this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SPDX does not require listing the entire set of modules. SPDX is essentially a language that lets you describe a set of software packages to a greater or lesser degree of specificity. @kartben I'm happy to share some thoughts about how to potentially use these sorts of SPDX documents to "trace" the impact of a vulnerability, but agree with @tgagneret-embedded that it probably makes more sense in a separate issue or PR thread. |
||
module_name = module.get("name", None) | ||
module_security = module.get("security", None) | ||
|
||
if not module_name: | ||
log.err(f"cannot find module name in meta file; bailing") | ||
return False | ||
|
||
module_ext_ref = [] | ||
if module_security: | ||
module_ext_ref = module_security.get("external-references") | ||
|
||
# set up zephyr sources package | ||
cfgPackageModuleExtRef = PackageConfig() | ||
cfgPackageModuleExtRef.name = module_name + "-deps" | ||
cfgPackageModuleExtRef.spdxID = "SPDXRef-" + module_name + "-deps" | ||
|
||
for ref in module_ext_ref: | ||
cfgPackageModuleExtRef.externalReferences.append(ref) | ||
|
||
pkgModule = Package(cfgPackageModuleExtRef, self.docModulesExtRefs) | ||
self.docModulesExtRefs.pkgs[pkgModule.cfg.spdxID] = pkgModule | ||
|
||
self._add_describe_relationship(self.docModulesExtRefs, cfgPackageModuleExtRef) | ||
|
||
|
||
# set up Documents before beginning | ||
def setupDocuments(self): | ||
log.dbg("setting up placeholder documents") | ||
|
@@ -289,7 +372,8 @@ def setupDocuments(self): | |
try: | ||
with open(self.metaFile) as file: | ||
content = yaml.load(file.read(), yaml.SafeLoader) | ||
self.setupZephyrDocument(content["modules"]) | ||
if not self.setupZephyrDocument(content["zephyr"], content["modules"]): | ||
return False | ||
except (FileNotFoundError, yaml.YAMLError): | ||
log.err(f"cannot find a valid zephyr_meta.yml required for SPDX generation; bailing") | ||
return False | ||
|
@@ -299,6 +383,8 @@ def setupDocuments(self): | |
if self.cfg.includeSDK: | ||
self.setupSDKDocument() | ||
|
||
self.setupModulesDocument(content["modules"]) | ||
|
||
return True | ||
|
||
# walk through targets and gather information | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good.
Maybe at this location specify the
version:
field should be used.For example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your comment makes me think this
version
field creates confusion. There was a misunderstanding from my part in the original issue.The goal was to provide a version for the module and help complete the SBOM for the module (and not searching vulnerabilities from the forked repository for example). The
external-references
field must be used for that (and CPE and PURL specifications allow to include the version)I think we should remove the
version
field from this PR and I will clarify this for a next PR. Are you okay with this ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there are some clarification (raised by your comment) to be done on this PR with the security group. In the meantime, I converted it into draft and I'll update it when I have all the information I need.
I'm sorry for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you believe it clarifies the usage and intentions of the field, then it's fine.
The clearer the intention of a PR, the better.