buildroot.busybox.net archive mirror
 help / color / mirror / Atom feed
* [Buildroot] [PATCH v2 0/2] Add CVE reporting to pkg-stats
@ 2020-02-08 21:57 Titouan Christophe
  2020-02-08 21:57 ` [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting Titouan Christophe
  2020-02-08 21:57 ` [Buildroot] [PATCH v2 2/2] docs/manual: describe the new <pkg>_IGNORE_CVES variable Titouan Christophe
  0 siblings, 2 replies; 9+ messages in thread
From: Titouan Christophe @ 2020-02-08 21:57 UTC (permalink / raw)
  To: buildroot

Hello,

This set of commit extends the pkg-stats tool to use the NVD database
(https://nvd.nist.gov/vuln/data-feeds) to see if the current version
of each Buildroot package is affected by a CVE.

An example result can be seen here:

 - Human readable HTML:       https://bootlin.com/~thomas/stats-cve.html
 - Machine parseable JSON:    https://bootlin.com/~thomas/stats-cve.json

Thanks to this, we can see that 60 of our packages are apparently
affected by a total of 185 CVEs.

A new per-package variable, <pkg>_IGNORE_CVES, is introduced, and
allows to tell the tool to ignore some CVEs, for example because it is
fixed by a local patch in Buildroot, or because the CVE does not apply
to the Buildroot package (the CVE only affects a non-Linux operating
system, or affect a functionality of the package that isn't built in
Buildroot).

Of course, the results are not perfect:

 - The NVD database product names certainly don't 100% match the
   Buildroot package names. We might have to add some extra metadata
   information in each package (CPE ID ?) to map to the correct NVD
   database product name.

 - Language-specific packages (for example: python-paho-mqtt and paho-mqtt-c)
   are probably not correctly handled.

 - Buildroot packages that have a version selection are not correctly
   handled.

But overall, it already provide useful results. The plan is of course
to implement e-mail notification to Buildroot developers in charge of
packages with unfixed CVEs, in a second step.

Thanks to Thomas Petazzoni, Thomas DS and all the reviewers for this effort !


Best regards,

Titouan

---
Thomas Petazzoni (2):
  support/scripts/pkg-stats: add support for CVE reporting
  docs/manual: describe the new <pkg>_IGNORE_CVES variable

 docs/manual/adding-packages-generic.txt |  14 +++
 support/scripts/pkg-stats               | 149 +++++++++++++++++++++++-
 2 files changed, 162 insertions(+), 1 deletion(-)

-- 
2.24.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting
  2020-02-08 21:57 [Buildroot] [PATCH v2 0/2] Add CVE reporting to pkg-stats Titouan Christophe
@ 2020-02-08 21:57 ` Titouan Christophe
  2020-02-11 10:02   ` Thomas De Schampheleire
  2020-02-08 21:57 ` [Buildroot] [PATCH v2 2/2] docs/manual: describe the new <pkg>_IGNORE_CVES variable Titouan Christophe
  1 sibling, 1 reply; 9+ messages in thread
From: Titouan Christophe @ 2020-02-08 21:57 UTC (permalink / raw)
  To: buildroot

From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

This commit extends the pkg-stats script to grab information about the
CVEs affecting the Buildroot packages.

To do so, it downloads the NVD database from
https://nvd.nist.gov/vuln/data-feeds in JSON format, and processes the
JSON file to determine which of our packages is affected by which
CVE. The information is then displayed in both the HTML output and the
JSON output of pkg-stats.

To use this feature, you have to pass the new --nvd-path option,
pointing to a writable directory where pkg-stats will store the NVD
database. If the local database is less than 24 hours old, it will not
re-download it. If it is more than 24 hours old, it will re-download
only the files that have really been updated by upstream NVD.

Packages can use the newly introduced <pkg>_IGNORE_CVES variable to
tell pkg-stats that some CVEs should be ignored: it can be because a
patch we have is fixing the CVE, or because the CVE doesn't apply in
our case.

From an implementation point of view:

 - A new class CVE implement most of the required functionalities:
   - Downloading the yearly NVD files
   - Reading and extracting relevant data from these files
   - Matching Packages against a CVE
   - Support for the format "1.0" of the NVD feeds, currently much more easier
     to process than the version "1.1", as the latter only provides CPE IDs.
     Both feed versions seem to provide the same data anyway.

 - The statistics are extended with the total number of CVEs, and the
   total number of packages that have at least one CVE pending.

 - The HTML output is extended with these new details. There are no
   changes to the code generating the JSON output because the existing
   code is smart enough to automatically expose the new information.

This development is a collective effort with Titouan Christophe
<titouan.christophe@railnova.eu> and Thomas De Schampheleire
<thomas.de_schampheleire@nokia.com>.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu>
---
Changes v1 -> v2 (Titouan):
 * Don't extract database files from gzip to json in downloader
 * Refactor CVEs traversal and matching in the CVE class
 * Simplify the NVD files downloader
 * Index the packages by name in a dict for faster CVE matching
 * Fix small typos and python idioms
---
 support/scripts/pkg-stats | 149 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 148 insertions(+), 1 deletion(-)

diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
index e477828f7b..2784a43d05 100755
--- a/support/scripts/pkg-stats
+++ b/support/scripts/pkg-stats
@@ -26,10 +26,17 @@ import subprocess
 import requests  # URL checking
 import json
 import certifi
+import distutils.version
+import time
+import gzip
 from urllib3 import HTTPSConnectionPool
 from urllib3.exceptions import HTTPError
 from multiprocessing import Pool
 
+NVD_START_YEAR = 2002
+NVD_JSON_VERSION = "1.0"
+NVD_BASE_URL = "https://nvd.nist.gov/feeds/json/cve/" + NVD_JSON_VERSION
+
 INFRA_RE = re.compile(r"\$\(eval \$\(([a-z-]*)-package\)\)")
 URL_RE = re.compile(r"\s*https?://\S*\s*$")
 
@@ -47,6 +54,7 @@ class Package:
     all_licenses = list()
     all_license_files = list()
     all_versions = dict()
+    all_ignored_cves = dict()
 
     def __init__(self, name, path):
         self.name = name
@@ -61,6 +69,7 @@ class Package:
         self.url = None
         self.url_status = None
         self.url_worker = None
+        self.cves = list()
         self.latest_version = (RM_API_STATUS_ERROR, None, None)
 
     def pkgvar(self):
@@ -152,6 +161,12 @@ class Package:
                 self.warnings = int(m.group(1))
                 return
 
+    def is_cve_ignored(self, cve):
+        """
+        Tells if the CVE is ignored by the package
+        """
+        return cve in self.all_ignored_cves.get(self.pkgvar(), [])
+
     def __eq__(self, other):
         return self.path == other.path
 
@@ -163,6 +178,103 @@ class Package:
             (self.name, self.path, self.has_license, self.has_license_files, self.has_hash, self.patch_count)
 
 
+class CVE:
+    """An accessor class for CVE Items in NVD files"""
+    def __init__(self, nvd_cve):
+        """Initialize a CVE from its NVD JSON representation"""
+        self.nvd_cve = nvd_cve
+
+    @staticmethod
+    def download_nvd_year(nvd_path, year):
+        metaf = "nvdcve-%s-%s.meta" % (NVD_JSON_VERSION, year)
+        path_metaf = os.path.join(nvd_path, metaf)
+        jsonf_gz = "nvdcve-%s-%s.json.gz" % (NVD_JSON_VERSION, year)
+        path_jsonf_gz = os.path.join(nvd_path, jsonf_gz)
+
+        # If the database file is less than a day old, we assume the NVD data
+        # locally available is recent enough.
+        if os.path.exists(path_jsonf_gz) and os.stat(path_jsonf_gz).st_mtime >= time.time() - 86400:
+            return path_jsonf_gz
+
+        # If not, we download the meta file
+        url = "%s/%s" % (NVD_BASE_URL, metaf)
+        print("Getting %s" % url)
+        page_meta = requests.get(url)
+        page_meta.raise_for_status()
+        if os.path.exists(path_metaf):
+            # If the meta file already existed, we compare the existing
+            # one with the data newly downloaded. If they are different,
+            # we need to re-download the database.
+            meta_known = open(path_metaf, "r").read()
+            if page_meta.text == meta_known:
+                return path_jsonf_gz
+
+        # Grab the compressed JSON NVD, and write files to disk
+        url = "%s/%s" % (NVD_BASE_URL, jsonf_gz)
+        print("Getting %s" % url)
+        page_data = requests.get(url)
+        page_data.raise_for_status()
+        open(path_jsonf_gz, "wb").write(page_data.content)
+        open(path_metaf, "w").write(page_meta.text)
+        return path_jsonf_gz
+
+    @classmethod
+    def read_nvd_dir(cls, nvd_dir):
+        """
+        Iterate over all the CVEs contained in NIST Vulnerability Database
+        feeds since NVD_START_YEAR. If the files are missing or outdated in
+        nvd_dir, a fresh copy will be downloaded, and kept in .json.gz
+        """
+        for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
+            filename = CVE.download_nvd_year(nvd_dir, year)
+            content = json.load(gzip.GzipFile(filename))
+            for cve in content["CVE_Items"]:
+                yield cls(cve['cve'])
+
+    def each_product(self):
+        """Iterate over each product section of this cve"""
+        for vendor in self.nvd_cve['affects']['vendor']['vendor_data']:
+            for product in vendor['product']['product_data']:
+                yield product
+
+    @property
+    def identifier(self):
+        """The CVE unique identifier"""
+        return self.nvd_cve['CVE_data_meta']['ID']
+
+    @property
+    def pkg_names(self):
+        """The set of package names referred by this CVE definition"""
+        return set(p['product_name'] for p in self.each_product())
+
+    def affects(self, br_pkg):
+        """
+        True if the Buildroot Package object passed as argument is affected
+        by this CVE.
+        """
+        for product in self.each_product():
+            if product['product_name'] != br_pkg.name:
+                continue
+
+            for v in product['version']['version_data']:
+                if v["version_affected"] == "=":
+                    if br_pkg.current_version == v["version_value"]:
+                        return True
+                elif v["version_affected"] == "<=":
+                    pkg_version = distutils.version.LooseVersion(br_pkg.current_version)
+                    if not hasattr(pkg_version, "version"):
+                        print("Cannot parse package '%s' version '%s'" % (br_pkg.name, br_pkg.current_version))
+                        continue
+                    cve_affected_version = distutils.version.LooseVersion(v["version_value"])
+                    if not hasattr(cve_affected_version, "version"):
+                        print("Cannot parse CVE affected version '%s'" % v["version_value"])
+                        continue
+                    return pkg_version <= cve_affected_version
+                else:
+                    print("version_affected: %s" % v['version_affected'])
+        return False
+
+
 def get_pkglist(npackages, package_list):
     """
     Builds the list of Buildroot packages, returning a list of Package
@@ -227,7 +339,7 @@ def get_pkglist(npackages, package_list):
 def package_init_make_info():
     # Fetch all variables@once
     variables = subprocess.check_output(["make", "BR2_HAVE_DOT_CONFIG=y", "-s", "printvars",
-                                         "VARS=%_LICENSE %_LICENSE_FILES %_VERSION"])
+                                         "VARS=%_LICENSE %_LICENSE_FILES %_VERSION %_IGNORE_CVES"])
     variable_list = variables.splitlines()
 
     # We process first the host package VERSION, and then the target
@@ -261,6 +373,10 @@ def package_init_make_info():
             pkgvar = pkgvar[:-8]
             Package.all_versions[pkgvar] = value
 
+        elif pkgvar.endswith("_IGNORE_CVES"):
+            pkgvar = pkgvar[:-12]
+            Package.all_ignored_cves[pkgvar] = value.split(" ")
+
 
 def check_url_status_worker(url, url_status):
     if url_status != "Missing" and url_status != "No Config.in":
@@ -355,6 +471,13 @@ def check_package_latest_version(packages):
     del http_pool
 
 
+def check_package_cves(nvd_path, packages):
+    for cve in CVE.read_nvd_dir(nvd_path):
+        for pkg_name in cve.pkg_names:
+            if pkg_name in packages and cve.affects(packages[pkg_name]):
+                packages[pkg_name].cves.append(cve.identifier)
+
+
 def calculate_stats(packages):
     stats = defaultdict(int)
     for pkg in packages:
@@ -390,6 +513,9 @@ def calculate_stats(packages):
         else:
             stats["version-not-uptodate"] += 1
         stats["patches"] += pkg.patch_count
+        stats["total-cves"] += len(pkg.cves)
+        if len(pkg.cves) != 0:
+            stats["pkg-cves"] += 1
     return stats
 
 
@@ -601,6 +727,17 @@ def dump_html_pkg(f, pkg):
     f.write("  <td class=\"%s\">%s</td>\n" %
             (" ".join(td_class), url_str))
 
+    # CVEs
+    td_class = ["centered"]
+    if len(pkg.cves) == 0:
+        td_class.append("correct")
+    else:
+        td_class.append("wrong")
+    f.write("  <td class=\"%s\">\n" % " ".join(td_class))
+    for cve in pkg.cves:
+        f.write("   <a href=\"https://security-tracker.debian.org/tracker/%s\">%s<br/>\n" % (cve, cve))
+    f.write("  </td>\n")
+
     f.write(" </tr>\n")
 
 
@@ -618,6 +755,7 @@ def dump_html_all_pkgs(f, packages):
 <td class=\"centered\">Latest version</td>
 <td class=\"centered\">Warnings</td>
 <td class=\"centered\">Upstream URL</td>
+<td class=\"centered\">CVEs</td>
 </tr>
 """)
     for pkg in sorted(packages):
@@ -656,6 +794,10 @@ def dump_html_stats(f, stats):
             stats["version-not-uptodate"])
     f.write("<tr><td>Packages with no known upstream version</td><td>%s</td></tr>\n" %
             stats["version-unknown"])
+    f.write("<tr><td>Packages affected by CVEs</td><td>%s</td></tr>\n" %
+            stats["pkg-cves"])
+    f.write("<tr><td>Total number of CVEs affecting all packages</td><td>%s</td></tr>\n" %
+            stats["total-cves"])
     f.write("</table>\n")
 
 
@@ -714,6 +856,8 @@ def parse_args():
                           help='Number of packages')
     packages.add_argument('-p', dest='packages', action='store',
                           help='List of packages (comma separated)')
+    parser.add_argument('--nvd-path', dest='nvd_path',
+                        help='Path to the local NVD database')
     args = parser.parse_args()
     if not args.html and not args.json:
         parser.error('at least one of --html or --json (or both) is required')
@@ -746,6 +890,9 @@ def __main__():
     check_package_urls(packages)
     print("Getting latest versions ...")
     check_package_latest_version(packages)
+    if args.nvd_path:
+        print("Checking packages CVEs")
+        check_package_cves(args.nvd_path, {p.name: p for p in packages})
     print("Calculate stats")
     stats = calculate_stats(packages)
     if args.html:
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 2/2] docs/manual: describe the new <pkg>_IGNORE_CVES variable
  2020-02-08 21:57 [Buildroot] [PATCH v2 0/2] Add CVE reporting to pkg-stats Titouan Christophe
  2020-02-08 21:57 ` [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting Titouan Christophe
@ 2020-02-08 21:57 ` Titouan Christophe
  2020-02-09 21:07   ` Thomas De Schampheleire
  1 sibling, 1 reply; 9+ messages in thread
From: Titouan Christophe @ 2020-02-08 21:57 UTC (permalink / raw)
  To: buildroot

From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu>
---
 docs/manual/adding-packages-generic.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt
index baa052e31c..9a77923a92 100644
--- a/docs/manual/adding-packages-generic.txt
+++ b/docs/manual/adding-packages-generic.txt
@@ -488,6 +488,20 @@ not and can not work as people would expect it should:
   locations, `/lib/firmware`, `/usr/lib/firmware`, `/lib/modules`,
   `/usr/lib/modules`, and `/usr/share`, which are automatically excluded.
 
+* +LIBFOO_IGNORE_CVES+ is a space-separated list of CVEs that tells
+  Buildroot CVE tracking tools which CVEs should be ignored for this
+  package. This is typically used when the CVE is fixed by a patch in
+  the package, or when the CVE for some reason does not affect the
+  Buildroot package. A Makefile comment must always preceed the
+  addition of a CVE to this variable. Example:
+
+----------------------
+# 0001-fix-cve-2020-12345.patch
+LIBFOO_IGNORE_CVES += CVE-2020-12345
+# only when built with libbaz, which Buildroot doesn't support
+LIBFOO_IGNORE_CVES += CVE-2020-54321
+----------------------
+
 The recommended way to define these variables is to use the following
 syntax:
 
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 2/2] docs/manual: describe the new <pkg>_IGNORE_CVES variable
  2020-02-08 21:57 ` [Buildroot] [PATCH v2 2/2] docs/manual: describe the new <pkg>_IGNORE_CVES variable Titouan Christophe
@ 2020-02-09 21:07   ` Thomas De Schampheleire
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas De Schampheleire @ 2020-02-09 21:07 UTC (permalink / raw)
  To: buildroot

Hello Titouan, Thomas,

El s?b., 8 feb. 2020 a las 22:58, Titouan Christophe
(<titouan.christophe@railnova.eu>) escribi?:
>
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu>
> ---
>  docs/manual/adding-packages-generic.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt
> index baa052e31c..9a77923a92 100644
> --- a/docs/manual/adding-packages-generic.txt
> +++ b/docs/manual/adding-packages-generic.txt
> @@ -488,6 +488,20 @@ not and can not work as people would expect it should:
>    locations, `/lib/firmware`, `/usr/lib/firmware`, `/lib/modules`,
>    `/usr/lib/modules`, and `/usr/share`, which are automatically excluded.
>
> +* +LIBFOO_IGNORE_CVES+ is a space-separated list of CVEs that tells
> +  Buildroot CVE tracking tools which CVEs should be ignored for this
> +  package. This is typically used when the CVE is fixed by a patch in
> +  the package, or when the CVE for some reason does not affect the
> +  Buildroot package. A Makefile comment must always preceed the

'preceed' is incorrect and should be 'precede'.

Best regards,
Thomas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting
  2020-02-08 21:57 ` [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting Titouan Christophe
@ 2020-02-11 10:02   ` Thomas De Schampheleire
  2020-02-11 11:15     ` Titouan Christophe
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas De Schampheleire @ 2020-02-11 10:02 UTC (permalink / raw)
  To: buildroot

Hi Titouan, Thomas,

El s?b., 8 feb. 2020 a las 22:58, Titouan Christophe
(<titouan.christophe@railnova.eu>) escribi?:
>
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>
> This commit extends the pkg-stats script to grab information about the
> CVEs affecting the Buildroot packages.
>
> To do so, it downloads the NVD database from
> https://nvd.nist.gov/vuln/data-feeds in JSON format, and processes the
> JSON file to determine which of our packages is affected by which
> CVE. The information is then displayed in both the HTML output and the
> JSON output of pkg-stats.
>
> To use this feature, you have to pass the new --nvd-path option,
> pointing to a writable directory where pkg-stats will store the NVD
> database. If the local database is less than 24 hours old, it will not
> re-download it. If it is more than 24 hours old, it will re-download
> only the files that have really been updated by upstream NVD.
>
> Packages can use the newly introduced <pkg>_IGNORE_CVES variable to
> tell pkg-stats that some CVEs should be ignored: it can be because a
> patch we have is fixing the CVE, or because the CVE doesn't apply in
> our case.
>
> >From an implementation point of view:
>

>  - A new class CVE implement most of the required functionalities:
>    - Downloading the yearly NVD files
>    - Reading and extracting relevant data from these files
>    - Matching Packages against a CVE
>    - Support for the format "1.0" of the NVD feeds, currently much more easier
>      to process than the version "1.1", as the latter only provides CPE IDs.
>      Both feed versions seem to provide the same data anyway.
>
>  - The statistics are extended with the total number of CVEs, and the
>    total number of packages that have at least one CVE pending.
>
>  - The HTML output is extended with these new details. There are no
>    changes to the code generating the JSON output because the existing
>    code is smart enough to automatically expose the new information.
>
> This development is a collective effort with Titouan Christophe
> <titouan.christophe@railnova.eu> and Thomas De Schampheleire
> <thomas.de_schampheleire@nokia.com>.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu>
> ---
> Changes v1 -> v2 (Titouan):
>  * Don't extract database files from gzip to json in downloader
>  * Refactor CVEs traversal and matching in the CVE class
>  * Simplify the NVD files downloader
>  * Index the packages by name in a dict for faster CVE matching
>  * Fix small typos and python idioms

Thanks for iterating on this..


> ---
>  support/scripts/pkg-stats | 149 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 148 insertions(+), 1 deletion(-)
>
> diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
> index e477828f7b..2784a43d05 100755
> --- a/support/scripts/pkg-stats
> +++ b/support/scripts/pkg-stats
> @@ -26,10 +26,17 @@ import subprocess
>  import requests  # URL checking
>  import json
>  import certifi
> +import distutils.version
> +import time
> +import gzip
>  from urllib3 import HTTPSConnectionPool
>  from urllib3.exceptions import HTTPError
>  from multiprocessing import Pool
>
> +NVD_START_YEAR = 2002
> +NVD_JSON_VERSION = "1.0"
> +NVD_BASE_URL = "https://nvd.nist.gov/feeds/json/cve/" + NVD_JSON_VERSION
> +
>  INFRA_RE = re.compile(r"\$\(eval \$\(([a-z-]*)-package\)\)")
>  URL_RE = re.compile(r"\s*https?://\S*\s*$")
>
> @@ -47,6 +54,7 @@ class Package:
>      all_licenses = list()
>      all_license_files = list()
>      all_versions = dict()
> +    all_ignored_cves = dict()
>
>      def __init__(self, name, path):
>          self.name = name
> @@ -61,6 +69,7 @@ class Package:
>          self.url = None
>          self.url_status = None
>          self.url_worker = None
> +        self.cves = list()
>          self.latest_version = (RM_API_STATUS_ERROR, None, None)
>
>      def pkgvar(self):
> @@ -152,6 +161,12 @@ class Package:
>                  self.warnings = int(m.group(1))
>                  return
>
> +    def is_cve_ignored(self, cve):
> +        """
> +        Tells if the CVE is ignored by the package
> +        """
> +        return cve in self.all_ignored_cves.get(self.pkgvar(), [])
> +
>      def __eq__(self, other):
>          return self.path == other.path
>
> @@ -163,6 +178,103 @@ class Package:
>              (self.name, self.path, self.has_license, self.has_license_files, self.has_hash, self.patch_count)
>
>
> +class CVE:
> +    """An accessor class for CVE Items in NVD files"""
> +    def __init__(self, nvd_cve):
> +        """Initialize a CVE from its NVD JSON representation"""
> +        self.nvd_cve = nvd_cve
> +
> +    @staticmethod
> +    def download_nvd_year(nvd_path, year):
> +        metaf = "nvdcve-%s-%s.meta" % (NVD_JSON_VERSION, year)
> +        path_metaf = os.path.join(nvd_path, metaf)
> +        jsonf_gz = "nvdcve-%s-%s.json.gz" % (NVD_JSON_VERSION, year)
> +        path_jsonf_gz = os.path.join(nvd_path, jsonf_gz)
> +
> +        # If the database file is less than a day old, we assume the NVD data
> +        # locally available is recent enough.
> +        if os.path.exists(path_jsonf_gz) and os.stat(path_jsonf_gz).st_mtime >= time.time() - 86400:
> +            return path_jsonf_gz
> +
> +        # If not, we download the meta file
> +        url = "%s/%s" % (NVD_BASE_URL, metaf)
> +        print("Getting %s" % url)
> +        page_meta = requests.get(url)
> +        page_meta.raise_for_status()
> +        if os.path.exists(path_metaf):
> +            # If the meta file already existed, we compare the existing
> +            # one with the data newly downloaded. If they are different,
> +            # we need to re-download the database.
> +            meta_known = open(path_metaf, "r").read()
> +            if page_meta.text == meta_known:
> +                return path_jsonf_gz

While I was testing and playing around, I found that the when the
json.gz file is removed, it is not redownloaded.
This is because here, the json.gz path is returned without checking
that it exists.
(I have some patch for this and next suggestions at the end of this mail).

> +
> +        # Grab the compressed JSON NVD, and write files to disk
> +        url = "%s/%s" % (NVD_BASE_URL, jsonf_gz)
> +        print("Getting %s" % url)
> +        page_data = requests.get(url)
> +        page_data.raise_for_status()
> +        open(path_jsonf_gz, "wb").write(page_data.content)
> +        open(path_metaf, "w").write(page_meta.text)
> +        return path_jsonf_gz
> +
> +    @classmethod
> +    def read_nvd_dir(cls, nvd_dir):
> +        """
> +        Iterate over all the CVEs contained in NIST Vulnerability Database
> +        feeds since NVD_START_YEAR. If the files are missing or outdated in
> +        nvd_dir, a fresh copy will be downloaded, and kept in .json.gz
> +        """
> +        for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
> +            filename = CVE.download_nvd_year(nvd_dir, year)
> +            content = json.load(gzip.GzipFile(filename))

During testing I initially got an error in the 2017 file:

Traceback (most recent call last):
  File "support/scripts/pkg-stats", line 917, in <module>
    __main__()
  File "support/scripts/pkg-stats", line 906, in __main__
    check_package_cves(args.nvd_path, {p.name: p for p in packages})
  File "support/scripts/pkg-stats", line 484, in check_package_cves
    for cve in CVE.read_nvd_dir(nvd_path):
  File "support/scripts/pkg-stats", line 231, in read_nvd_dir
    content = json.load(gzip.GzipFile(filename))
  File "/usr/lib64/python2.7/json/__init__.py", line 287, in load
    return loads(fp.read(),
  File "/usr/lib64/python2.7/gzip.py", line 260, in read
    self._read(readsize)
  File "/usr/lib64/python2.7/gzip.py", line 314, in _read
    self._read_eof()
  File "/usr/lib64/python2.7/gzip.py", line 353, in _read_eof
    hex(self.crc)))
IOError: CRC check failed 0x9b7b00d7 != 0x790947a4L

I'm not sure how this happened, but after removing the file and
redownloading it got fixed.
I then tried reproducing such problem by manually messing with the
json.gz file, and this created another error:

Traceback (most recent call last):
  File "support/scripts/pkg-stats", line 919, in <module>
    __main__()
  File "support/scripts/pkg-stats", line 908, in __main__
    check_package_cves(args.nvd_path, {p.name: p for p in packages})
  File "support/scripts/pkg-stats", line 486, in check_package_cves
    for cve in CVE.read_nvd_dir(nvd_path):
  File "support/scripts/pkg-stats", line 233, in read_nvd_dir
    content = json.load(gzip.GzipFile(filename))
  File "/usr/lib64/python2.7/json/__init__.py", line 287, in load
    return loads(fp.read(),
  File "/usr/lib64/python2.7/gzip.py", line 260, in read
    self._read(readsize)
  File "/usr/lib64/python2.7/gzip.py", line 318, in _read
    uncompress = self.decompress.decompress(buf)
zlib.error: Error -3 while decompressing: invalid distance too far back

I suggest a better error handling for such case (see below).

> +            for cve in content["CVE_Items"]:
> +                yield cls(cve['cve'])
> +
> +    def each_product(self):
> +        """Iterate over each product section of this cve"""
> +        for vendor in self.nvd_cve['affects']['vendor']['vendor_data']:
> +            for product in vendor['product']['product_data']:
> +                yield product
> +
> +    @property
> +    def identifier(self):
> +        """The CVE unique identifier"""
> +        return self.nvd_cve['CVE_data_meta']['ID']
> +
> +    @property
> +    def pkg_names(self):
> +        """The set of package names referred by this CVE definition"""
> +        return set(p['product_name'] for p in self.each_product())
> +
> +    def affects(self, br_pkg):
> +        """
> +        True if the Buildroot Package object passed as argument is affected
> +        by this CVE.
> +        """
> +        for product in self.each_product():
> +            if product['product_name'] != br_pkg.name:
> +                continue
> +
> +            for v in product['version']['version_data']:
> +                if v["version_affected"] == "=":
> +                    if br_pkg.current_version == v["version_value"]:
> +                        return True
> +                elif v["version_affected"] == "<=":
> +                    pkg_version = distutils.version.LooseVersion(br_pkg.current_version)
> +                    if not hasattr(pkg_version, "version"):
> +                        print("Cannot parse package '%s' version '%s'" % (br_pkg.name, br_pkg.current_version))
> +                        continue
> +                    cve_affected_version = distutils.version.LooseVersion(v["version_value"])
> +                    if not hasattr(cve_affected_version, "version"):
> +                        print("Cannot parse CVE affected version '%s'" % v["version_value"])
> +                        continue
> +                    return pkg_version <= cve_affected_version
> +                else:
> +                    print("version_affected: %s" % v['version_affected'])
> +        return False
> +
> +
>  def get_pkglist(npackages, package_list):
>      """
>      Builds the list of Buildroot packages, returning a list of Package
> @@ -227,7 +339,7 @@ def get_pkglist(npackages, package_list):
>  def package_init_make_info():
>      # Fetch all variables at once
>      variables = subprocess.check_output(["make", "BR2_HAVE_DOT_CONFIG=y", "-s", "printvars",
> -                                         "VARS=%_LICENSE %_LICENSE_FILES %_VERSION"])
> +                                         "VARS=%_LICENSE %_LICENSE_FILES %_VERSION %_IGNORE_CVES"])
>      variable_list = variables.splitlines()
>
>      # We process first the host package VERSION, and then the target
> @@ -261,6 +373,10 @@ def package_init_make_info():
>              pkgvar = pkgvar[:-8]
>              Package.all_versions[pkgvar] = value
>
> +        elif pkgvar.endswith("_IGNORE_CVES"):
> +            pkgvar = pkgvar[:-12]
> +            Package.all_ignored_cves[pkgvar] = value.split(" ")
> +
>
>  def check_url_status_worker(url, url_status):
>      if url_status != "Missing" and url_status != "No Config.in":
> @@ -355,6 +471,13 @@ def check_package_latest_version(packages):
>      del http_pool
>
>
> +def check_package_cves(nvd_path, packages):
> +    for cve in CVE.read_nvd_dir(nvd_path):
> +        for pkg_name in cve.pkg_names:
> +            if pkg_name in packages and cve.affects(packages[pkg_name]):
> +                packages[pkg_name].cves.append(cve.identifier)
> +
> +
>  def calculate_stats(packages):
>      stats = defaultdict(int)
>      for pkg in packages:
> @@ -390,6 +513,9 @@ def calculate_stats(packages):
>          else:
>              stats["version-not-uptodate"] += 1
>          stats["patches"] += pkg.patch_count
> +        stats["total-cves"] += len(pkg.cves)
> +        if len(pkg.cves) != 0:
> +            stats["pkg-cves"] += 1
>      return stats
>
>
> @@ -601,6 +727,17 @@ def dump_html_pkg(f, pkg):
>      f.write("  <td class=\"%s\">%s</td>\n" %
>              (" ".join(td_class), url_str))
>
> +    # CVEs
> +    td_class = ["centered"]
> +    if len(pkg.cves) == 0:
> +        td_class.append("correct")
> +    else:
> +        td_class.append("wrong")
> +    f.write("  <td class=\"%s\">\n" % " ".join(td_class))
> +    for cve in pkg.cves:
> +        f.write("   <a href=\"https://security-tracker.debian.org/tracker/%s\">%s<br/>\n" % (cve, cve))
> +    f.write("  </td>\n")
> +
>      f.write(" </tr>\n")
>
>
> @@ -618,6 +755,7 @@ def dump_html_all_pkgs(f, packages):
>  <td class=\"centered\">Latest version</td>
>  <td class=\"centered\">Warnings</td>
>  <td class=\"centered\">Upstream URL</td>
> +<td class=\"centered\">CVEs</td>
>  </tr>
>  """)
>      for pkg in sorted(packages):
> @@ -656,6 +794,10 @@ def dump_html_stats(f, stats):
>              stats["version-not-uptodate"])
>      f.write("<tr><td>Packages with no known upstream version</td><td>%s</td></tr>\n" %
>              stats["version-unknown"])
> +    f.write("<tr><td>Packages affected by CVEs</td><td>%s</td></tr>\n" %
> +            stats["pkg-cves"])
> +    f.write("<tr><td>Total number of CVEs affecting all packages</td><td>%s</td></tr>\n" %
> +            stats["total-cves"])
>      f.write("</table>\n")
>
>
> @@ -714,6 +856,8 @@ def parse_args():
>                            help='Number of packages')
>      packages.add_argument('-p', dest='packages', action='store',
>                            help='List of packages (comma separated)')
> +    parser.add_argument('--nvd-path', dest='nvd_path',
> +                        help='Path to the local NVD database')
>      args = parser.parse_args()
>      if not args.html and not args.json:
>          parser.error('at least one of --html or --json (or both) is required')
> @@ -746,6 +890,9 @@ def __main__():
>      check_package_urls(packages)
>      print("Getting latest versions ...")
>      check_package_latest_version(packages)
> +    if args.nvd_path:
> +        print("Checking packages CVEs")
> +        check_package_cves(args.nvd_path, {p.name: p for p in packages})

If the the passed nvd path does not exist, the script will fail. I
suggest creating the dir automatically.


The below changes implement the suggestions above:


diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
index 2784a43d05..e8f708537e 100755
--- a/support/scripts/pkg-stats
+++ b/support/scripts/pkg-stats
@@ -27,8 +27,10 @@ import requests  # URL checking
 import json
 import certifi
 import distutils.version
+import sys
 import time
 import gzip
+import zlib
 from urllib3 import HTTPSConnectionPool
 from urllib3.exceptions import HTTPError
 from multiprocessing import Pool
@@ -201,10 +203,12 @@ class CVE:
         print("Getting %s" % url)
         page_meta = requests.get(url)
         page_meta.raise_for_status()
-        if os.path.exists(path_metaf):
-            # If the meta file already existed, we compare the existing
-            # one with the data newly downloaded. If they are different,
-            # we need to re-download the database.
+        # If the meta file already existed, we compare the existing
+        # one with the data newly downloaded. If they are different,
+        # we need to re-download the database.
+        # If the database does not exist locally, we need to redownload it in
+        # any case.
+        if os.path.exists(path_metaf) and os.path.exists(path_jsonf_gz):
             meta_known = open(path_metaf, "r").read()
             if page_meta.text == meta_known:
                 return path_jsonf_gz
@@ -227,7 +231,13 @@ class CVE:
         """
         for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
             filename = CVE.download_nvd_year(nvd_dir, year)
-            content = json.load(gzip.GzipFile(filename))
+            try:
+                content = json.load(gzip.GzipFile(filename))
+            except (zlib.error, IOError) as e:
+                print('ERROR: problem reading %s, please remove the
file and rerun this script.' % filename)
+                print(e)
+                sys.exit(1)
+
             for cve in content["CVE_Items"]:
                 yield cls(cve['cve'])

@@ -892,6 +902,8 @@ def __main__():
     check_package_latest_version(packages)
     if args.nvd_path:
         print("Checking packages CVEs")
+        if not os.path.exists(args.nvd_path):
+            os.makedirs(args.nvd_path)
         check_package_cves(args.nvd_path, {p.name: p for p in packages})
     print("Calculate stats")
     stats = calculate_stats(packages)


Best regards,
Thomas

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting
  2020-02-11 10:02   ` Thomas De Schampheleire
@ 2020-02-11 11:15     ` Titouan Christophe
  2020-02-11 11:38       ` yann.morin at orange.com
  2020-02-11 14:06       ` Thomas De Schampheleire
  0 siblings, 2 replies; 9+ messages in thread
From: Titouan Christophe @ 2020-02-11 11:15 UTC (permalink / raw)
  To: buildroot

Hello Thomas,

Thanks for your review !

On 2/11/20 11:02 AM, Thomas De Schampheleire wrote:
> Hi Titouan, Thomas,
> 
> El s?b., 8 feb. 2020 a las 22:58, Titouan Christophe
> (<titouan.christophe@railnova.eu>) escribi?:
>>
>> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>>
>> This commit extends the pkg-stats script to grab information about the
>> CVEs affecting the Buildroot packages.
>>
> 
> While I was testing and playing around, I found that the when the
> json.gz file is removed, it is not redownloaded.
> This is because here, the json.gz path is returned without checking
> that it exists.
> (I have some patch for this and next suggestions at the end of this mail).

Good catch, that probably happened because I flipped around the conditions.

> 
> During testing I initially got an error in the 2017 file:
> 
> Traceback (most recent call last):
>    File "support/scripts/pkg-stats", line 917, in <module>
>      __main__()
>    File "support/scripts/pkg-stats", line 906, in __main__
>      check_package_cves(args.nvd_path, {p.name: p for p in packages})
>    File "support/scripts/pkg-stats", line 484, in check_package_cves
>      for cve in CVE.read_nvd_dir(nvd_path):
>    File "support/scripts/pkg-stats", line 231, in read_nvd_dir
>      content = json.load(gzip.GzipFile(filename))
>    File "/usr/lib64/python2.7/json/__init__.py", line 287, in load
>      return loads(fp.read(),
>    File "/usr/lib64/python2.7/gzip.py", line 260, in read
>      self._read(readsize)
>    File "/usr/lib64/python2.7/gzip.py", line 314, in _read
>      self._read_eof()
>    File "/usr/lib64/python2.7/gzip.py", line 353, in _read_eof
>      hex(self.crc)))
> IOError: CRC check failed 0x9b7b00d7 != 0x790947a4L

I'm curious to understand how that happened :o

> 
> I'm not sure how this happened, but after removing the file and
> redownloading it got fixed.
> I then tried reproducing such problem by manually messing with the
> json.gz file, and this created another error:
> 
> Traceback (most recent call last):
>    File "support/scripts/pkg-stats", line 919, in <module>
>      __main__()
>    File "support/scripts/pkg-stats", line 908, in __main__
>      check_package_cves(args.nvd_path, {p.name: p for p in packages})
>    File "support/scripts/pkg-stats", line 486, in check_package_cves
>      for cve in CVE.read_nvd_dir(nvd_path):
>    File "support/scripts/pkg-stats", line 233, in read_nvd_dir
>      content = json.load(gzip.GzipFile(filename))
>    File "/usr/lib64/python2.7/json/__init__.py", line 287, in load
>      return loads(fp.read(),
>    File "/usr/lib64/python2.7/gzip.py", line 260, in read
>      self._read(readsize)
>    File "/usr/lib64/python2.7/gzip.py", line 318, in _read
>      uncompress = self.decompress.decompress(buf)
> zlib.error: Error -3 while decompressing: invalid distance too far back
> 
> I suggest a better error handling for such case (see below).

[SNIP]

> If the the passed nvd path does not exist, the script will fail. I
> suggest creating the dir automatically.
> 
> 
> The below changes implement the suggestions above:
> 
> 
> diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
> index 2784a43d05..e8f708537e 100755
> --- a/support/scripts/pkg-stats
> +++ b/support/scripts/pkg-stats
> @@ -27,8 +27,10 @@ import requests  # URL checking
>   import json
>   import certifi
>   import distutils.version
> +import sys
>   import time
>   import gzip
> +import zlib
>   from urllib3 import HTTPSConnectionPool
>   from urllib3.exceptions import HTTPError
>   from multiprocessing import Pool
> @@ -201,10 +203,12 @@ class CVE:
>           print("Getting %s" % url)
>           page_meta = requests.get(url)
>           page_meta.raise_for_status()
> -        if os.path.exists(path_metaf):
> -            # If the meta file already existed, we compare the existing
> -            # one with the data newly downloaded. If they are different,
> -            # we need to re-download the database.
> +        # If the meta file already existed, we compare the existing
> +        # one with the data newly downloaded. If they are different,
> +        # we need to re-download the database.
> +        # If the database does not exist locally, we need to redownload it in
> +        # any case.
> +        if os.path.exists(path_metaf) and os.path.exists(path_jsonf_gz):
>               meta_known = open(path_metaf, "r").read()
>               if page_meta.text == meta_known:
>                   return path_jsonf_gz

=> agreed

> @@ -227,7 +231,13 @@ class CVE:
>           """
>           for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
>               filename = CVE.download_nvd_year(nvd_dir, year)
> -            content = json.load(gzip.GzipFile(filename))
> +            try:
> +                content = json.load(gzip.GzipFile(filename))
> +            except (zlib.error, IOError) as e:
> +                print('ERROR: problem reading %s, please remove the
> file and rerun this script.' % filename)
> +                print(e)
> +                sys.exit(1)
> +

I don't find it pythonic to catch the exception then exit. Unless 
there's a really good reason to do so, I'd prefer to let the exception 
bubble up, as it helps diagnosing the issue. Maybe something like:

try:
     content = json.load(gzip.GzipFile(filename))
except:
     # Display an informative message about the problematic file
     print("ERROR: cannot read %s. Please remove the file then rerun 
this script" % filename)
     # Then bubble up the exception
     raise

(Also, exit() is included in the global namespace, so no need to import 
the sys module for that)

>               for cve in content["CVE_Items"]:
>                   yield cls(cve['cve'])
> 
> @@ -892,6 +902,8 @@ def __main__():
>       check_package_latest_version(packages)
>       if args.nvd_path:
>           print("Checking packages CVEs")
> +        if not os.path.exists(args.nvd_path):
> +            os.makedirs(args.nvd_path)

I thought that not creating the directory was a design choice, but this 
indeed makes much more sense.

>           check_package_cves(args.nvd_path, {p.name: p for p in packages})
>       print("Calculate stats")
>       stats = calculate_stats(packages)
> 
> 
> Best regards,
> Thomas
> 

Regards,

Titouan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting
  2020-02-11 11:15     ` Titouan Christophe
@ 2020-02-11 11:38       ` yann.morin at orange.com
  2020-02-11 12:11         ` Titouan Christophe
  2020-02-11 14:06       ` Thomas De Schampheleire
  1 sibling, 1 reply; 9+ messages in thread
From: yann.morin at orange.com @ 2020-02-11 11:38 UTC (permalink / raw)
  To: buildroot

Titouan, All,

On 2020-02-11 12:15 +0100, Titouan Christophe spake thusly:
> On 2/11/20 11:02 AM, Thomas De Schampheleire wrote:
> >El s?b., 8 feb. 2020 a las 22:58, Titouan Christophe
> >(<titouan.christophe@railnova.eu>) escribi?:
> >>From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> >>This commit extends the pkg-stats script to grab information about the
> >>CVEs affecting the Buildroot packages.
[--SNIP--]
> >@@ -227,7 +231,13 @@ class CVE:
> >          """
> >          for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
> >              filename = CVE.download_nvd_year(nvd_dir, year)
> >-            content = json.load(gzip.GzipFile(filename))
> >+            try:
> >+                content = json.load(gzip.GzipFile(filename))
> >+            except (zlib.error, IOError) as e:
> >+                print('ERROR: problem reading %s, please remove the file and rerun this script.' % filename)
> >+                print(e)
> >+                sys.exit(1)
> >+
> 
> I don't find it pythonic to catch the exception then exit. Unless there's a
> really good reason to do so, I'd prefer to let the exception bubble up, as
> it helps diagnosing the issue.

The exception is print()ed, at least (not sure if that also prints the
barcktrace, though?).

[--SNIP--]
> >@@ -892,6 +902,8 @@ def __main__():
> >      check_package_latest_version(packages)
> >      if args.nvd_path:
> >          print("Checking packages CVEs")
> >+        if not os.path.exists(args.nvd_path):
> >+            os.makedirs(args.nvd_path)
> I thought that not creating the directory was a design choice, but this
> indeed makes much more sense.

In python3, makedirs() can take the option exist_ok=False to tell it to
not fail on an already existing directory, so that would avoid this
test-and-create racy code.

Aha, this is a python2 script. Damned... ;-p

Regards,
Yann E. MORIN.

> >          check_package_cves(args.nvd_path, {p.name: p for p in packages})
> >      print("Calculate stats")
> >      stats = calculate_stats(packages)
> >
> >
> >Best regards,
> >Thomas
> >
> 
> Regards,
> 
> Titouan
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
                                        ____________
.-----------------.--------------------:       _    :------------------.
|  Yann E. MORIN  | Real-Time Embedded |    __/ )   | /"\ ASCII RIBBON |
| +33 534.541.179 | Software  Designer |  _/ - /'   | \ / CAMPAIGN     |
| +33 638.411.245 '--------------------: (_    `--, |  X  AGAINST      |
|      yann.morin (at) orange.com      |_="    ,--' | / \ HTML MAIL    |
'--------------------------------------:______/_____:------------------'


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting
  2020-02-11 11:38       ` yann.morin at orange.com
@ 2020-02-11 12:11         ` Titouan Christophe
  0 siblings, 0 replies; 9+ messages in thread
From: Titouan Christophe @ 2020-02-11 12:11 UTC (permalink / raw)
  To: buildroot

Hi Yann,

On 2/11/20 12:38 PM, yann.morin at orange.com wrote:
> Titouan, All,
> 
> On 2020-02-11 12:15 +0100, Titouan Christophe spake thusly:
>> On 2/11/20 11:02 AM, Thomas De Schampheleire wrote:
>>> El s?b., 8 feb. 2020 a las 22:58, Titouan Christophe
>>> (<titouan.christophe@railnova.eu>) escribi?:
>>>> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>>>> This commit extends the pkg-stats script to grab information about the
>>>> CVEs affecting the Buildroot packages.
> [--SNIP--]
>>> @@ -227,7 +231,13 @@ class CVE:
>>>           """
>>>           for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
>>>               filename = CVE.download_nvd_year(nvd_dir, year)
>>> -            content = json.load(gzip.GzipFile(filename))
>>> +            try:
>>> +                content = json.load(gzip.GzipFile(filename))
>>> +            except (zlib.error, IOError) as e:
>>> +                print('ERROR: problem reading %s, please remove the file and rerun this script.' % filename)
>>> +                print(e)
>>> +                sys.exit(1)
>>> +
>>
>> I don't find it pythonic to catch the exception then exit. Unless there's a
>> really good reason to do so, I'd prefer to let the exception bubble up, as
>> it helps diagnosing the issue.
> 
> The exception is print()ed, at least (not sure if that also prints the
> barcktrace, though?).

No, it doesn't print the backtrace, only the error message. To obtain 
the backtrace, one must

import traceback
traceback.print_exc()

> 
> [--SNIP--]
>>> @@ -892,6 +902,8 @@ def __main__():
>>>       check_package_latest_version(packages)
>>>       if args.nvd_path:
>>>           print("Checking packages CVEs")
>>> +        if not os.path.exists(args.nvd_path):
>>> +            os.makedirs(args.nvd_path)
>> I thought that not creating the directory was a design choice, but this
>> indeed makes much more sense.
> 
> In python3, makedirs() can take the option exist_ok=False to tell it to
> not fail on an already existing directory, so that would avoid this
> test-and-create racy code.
> 
> Aha, this is a python2 script. Damned... ;-p

Yes there's a race condition over here, though I'm not sure it's 
critical enough to develop advanced techniques in this utility script.

As stated on IRC, I'll post another series to make all support scripts 
Py3, so we shall get that race condition fixed at that time O:-)

> 
> Regards,
> Yann E. MORIN.
> 
>>>           check_package_cves(args.nvd_path, {p.name: p for p in packages})
>>>       print("Calculate stats")
>>>       stats = calculate_stats(packages)
>>>
>>>
>>> Best regards,
>>> Thomas
>>>
>>
>> Regards,
>>
>> Titouan
>> _______________________________________________
>> buildroot mailing list
>> buildroot at busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
> 

All the best,

Titouan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting
  2020-02-11 11:15     ` Titouan Christophe
  2020-02-11 11:38       ` yann.morin at orange.com
@ 2020-02-11 14:06       ` Thomas De Schampheleire
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas De Schampheleire @ 2020-02-11 14:06 UTC (permalink / raw)
  To: buildroot

Hi Titouan,

El mar., 11 feb. 2020 a las 12:15, Titouan Christophe
(<titouan.christophe@railnova.eu>) escribi?:
[..]
>
> > @@ -227,7 +231,13 @@ class CVE:
> >           """
> >           for year in range(NVD_START_YEAR, datetime.datetime.now().year + 1):
> >               filename = CVE.download_nvd_year(nvd_dir, year)
> > -            content = json.load(gzip.GzipFile(filename))
> > +            try:
> > +                content = json.load(gzip.GzipFile(filename))
> > +            except (zlib.error, IOError) as e:
> > +                print('ERROR: problem reading %s, please remove the
> > file and rerun this script.' % filename)
> > +                print(e)
> > +                sys.exit(1)
> > +
>
> I don't find it pythonic to catch the exception then exit. Unless
> there's a really good reason to do so, I'd prefer to let the exception
> bubble up, as it helps diagnosing the issue. Maybe something like:
>
> try:
>      content = json.load(gzip.GzipFile(filename))
> except:
>      # Display an informative message about the problematic file
>      print("ERROR: cannot read %s. Please remove the file then rerun
> this script" % filename)
>      # Then bubble up the exception
>      raise

Ok for me, thanks.

>
> (Also, exit() is included in the global namespace, so no need to import
> the sys module for that)

The documentation for exit() says:
https://docs.python.org/2/library/constants.html#exit

"They are useful for the interactive interpreter shell and should not
be used in programs."

This is also described here:
https://stackoverflow.com/questions/19747371/python-exit-commands-why-so-many-and-when-should-each-be-used

Best regards,
Thomas

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-02-11 14:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-08 21:57 [Buildroot] [PATCH v2 0/2] Add CVE reporting to pkg-stats Titouan Christophe
2020-02-08 21:57 ` [Buildroot] [PATCH v2 1/2] support/scripts/pkg-stats: add support for CVE reporting Titouan Christophe
2020-02-11 10:02   ` Thomas De Schampheleire
2020-02-11 11:15     ` Titouan Christophe
2020-02-11 11:38       ` yann.morin at orange.com
2020-02-11 12:11         ` Titouan Christophe
2020-02-11 14:06       ` Thomas De Schampheleire
2020-02-08 21:57 ` [Buildroot] [PATCH v2 2/2] docs/manual: describe the new <pkg>_IGNORE_CVES variable Titouan Christophe
2020-02-09 21:07   ` Thomas De Schampheleire

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).