[v3,12/12] support/scripts/pkg-stats: add status for cve check
diff mbox series

Message ID 20200222085715.23769-13-heiko.thiery@gmail.com
State Superseded
Headers show
Series
  • pkg-stats json output improvements
Related show

Commit Message

Heiko Thiery Feb. 22, 2020, 8:57 a.m. UTC
Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
 support/scripts/pkg-stats | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Titouan Christophe Feb. 23, 2020, 2:24 p.m. UTC | #1
Heiko, all,

On 2/22/20 9:57 AM, Heiko Thiery wrote:
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
>   support/scripts/pkg-stats | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
> index ed22f6b650..4efff8624f 100755
> --- a/support/scripts/pkg-stats
> +++ b/support/scripts/pkg-stats
> @@ -617,8 +617,14 @@ 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)
> +            if pkg_name in packages:
> +                if cve.affects(packages[pkg_name]):
> +                    packages[pkg_name].cves.append(cve.identifier)
> +                if len(packages[pkg_name].cves):
> +                    packages[pkg_name].status['cve'] = ('error', 'affected by cve')
> +                else:
> +                    packages[pkg_name].status['cve'] = ('ok', 'no cve found')
> +
>   
>   
>   def calculate_stats(packages):
> 

In the current state, that would give:

* If a CVE applies to a br package -> error
* If a CVE does not applies to a br package -> ok
* If no CVE points to a br package -> na (no status check done)

I would argue that the latest case is not correct. The status should be 
ok, because we ran through the whole list of CVEs from the NVD feed, and 
we did not find any of them that applies to this package.

I would rather write it like this:

########################
diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
index ed22f6b650..91477d583e 100755
--- a/support/scripts/pkg-stats
+++ b/support/scripts/pkg-stats
@@ -620,6 +620,12 @@ def check_package_cves(nvd_path, packages):
              if pkg_name in packages and cve.affects(packages[pkg_name]):
                  packages[pkg_name].cves.append(cve.identifier)

+    for pkg_name, pkg in packages.items():
+        if len(pkg.cves) > 0:
+            pkg.status['cve'] = ('error', 'affected by CVE(s)')
+        else:
+            pkg.status['cve'] = ('ok', 'no CVE found')
+

  def calculate_stats(packages):
      stats = defaultdict(int)
########################


Best regards,

Titouan
Heiko Thiery Feb. 24, 2020, 7:06 a.m. UTC | #2
Hi Titouan and all,

Am So., 23. Feb. 2020 um 15:24 Uhr schrieb Titouan Christophe
<titouan.christophe@railnova.eu>:
>
> Heiko, all,
>
> On 2/22/20 9:57 AM, Heiko Thiery wrote:
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> >   support/scripts/pkg-stats | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
> > index ed22f6b650..4efff8624f 100755
> > --- a/support/scripts/pkg-stats
> > +++ b/support/scripts/pkg-stats
> > @@ -617,8 +617,14 @@ 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)
> > +            if pkg_name in packages:
> > +                if cve.affects(packages[pkg_name]):
> > +                    packages[pkg_name].cves.append(cve.identifier)
> > +                if len(packages[pkg_name].cves):
> > +                    packages[pkg_name].status['cve'] = ('error', 'affected by cve')
> > +                else:
> > +                    packages[pkg_name].status['cve'] = ('ok', 'no cve found')
> > +
> >
> >
> >   def calculate_stats(packages):
> >
>
> In the current state, that would give:
>
> * If a CVE applies to a br package -> error
> * If a CVE does not applies to a br package -> ok
> * If no CVE points to a br package -> na (no status check done)
>
> I would argue that the latest case is not correct. The status should be
> ok, because we ran through the whole list of CVEs from the NVD feed, and
> we did not find any of them that applies to this package.

I see .. I think you are right. This will lead to a wrong status. I
will remove the initializer for the status.

>
> I would rather write it like this:
>
> ########################
> diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
> index ed22f6b650..91477d583e 100755
> --- a/support/scripts/pkg-stats
> +++ b/support/scripts/pkg-stats
> @@ -620,6 +620,12 @@ def check_package_cves(nvd_path, packages):
>               if pkg_name in packages and cve.affects(packages[pkg_name]):
>                   packages[pkg_name].cves.append(cve.identifier)
>
> +    for pkg_name, pkg in packages.items():
> +        if len(pkg.cves) > 0:
> +            pkg.status['cve'] = ('error', 'affected by CVE(s)')
> +        else:
> +            pkg.status['cve'] = ('ok', 'no CVE found')
> +

Isn't it right that we loop then (depending on the amount of nvd
pathes) several thousend times?

e.g. packages ~2600, nvds ~ 20 => 20*2600=52000

On the other hand we loop over the list of packages all over the place ;-/

>
>   def calculate_stats(packages):
>       stats = defaultdict(int)
> ########################
>
>
> Best regards,
>
> Titouan
Titouan Christophe Feb. 24, 2020, 9:35 a.m. UTC | #3
On 2/24/20 8:06 AM, Heiko Thiery wrote:
> Hi Titouan and all,
> 
> Am So., 23. Feb. 2020 um 15:24 Uhr schrieb Titouan Christophe
> <titouan.christophe@railnova.eu>:
>>
>> Heiko, all,
>>
>> On 2/22/20 9:57 AM, Heiko Thiery wrote:
>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>

[--SNIP--]

> 
>>
>> I would rather write it like this:
>>
>> ########################
>> diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
>> index ed22f6b650..91477d583e 100755
>> --- a/support/scripts/pkg-stats
>> +++ b/support/scripts/pkg-stats
>> @@ -620,6 +620,12 @@ def check_package_cves(nvd_path, packages):
>>                if pkg_name in packages and cve.affects(packages[pkg_name]):
>>                    packages[pkg_name].cves.append(cve.identifier)
>>
>> +    for pkg_name, pkg in packages.items():
>> +        if len(pkg.cves) > 0:
>> +            pkg.status['cve'] = ('error', 'affected by CVE(s)')
>> +        else:
>> +            pkg.status['cve'] = ('ok', 'no CVE found')
>> +
> 
> Isn't it right that we loop then (depending on the amount of nvd
> pathes) several thousend times?
> 
> e.g. packages ~2600, nvds ~ 20 => 20*2600=52000

Except that each NVD file contains a few thousands CVEs :).

> 
> On the other hand we loop over the list of packages all over the place ;-/

Looping over all CVEs in a single NVD file yields 5 to 10 more 
iterations than looping over all packages (for instance year 2018 alone 
has 16039 CVE items)

> 
>>
>>    def calculate_stats(packages):
>>        stats = defaultdict(int)
>> ########################
>>
>>
>> Best regards,
>>
>> Titouan
Heiko Thiery Feb. 24, 2020, 12:21 p.m. UTC | #4
Hi Titouan,


Am Mo., 24. Feb. 2020 um 10:35 Uhr schrieb Titouan Christophe
<titouan.christophe@railnova.eu>:
>
>
> On 2/24/20 8:06 AM, Heiko Thiery wrote:
> > Hi Titouan and all,
> >
> > Am So., 23. Feb. 2020 um 15:24 Uhr schrieb Titouan Christophe
> > <titouan.christophe@railnova.eu>:
> >>
> >> Heiko, all,
> >>
> >> On 2/22/20 9:57 AM, Heiko Thiery wrote:
> >>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
>

[--SNIP--]

> > Isn't it right that we loop then (depending on the amount of nvd
> > pathes) several thousend times?
> >
> > e.g. packages ~2600, nvds ~ 20 => 20*2600=52000
>
> Except that each NVD file contains a few thousands CVEs :).
>
> >
> > On the other hand we loop over the list of packages all over the place ;-/
>
> Looping over all CVEs in a single NVD file yields 5 to 10 more
> iterations than looping over all packages (for instance year 2018 alone
> has 16039 CVE items)
>

you're right ... compared to this it doesn't matter.

> >
> >>
> >>    def calculate_stats(packages):
> >>        stats = defaultdict(int)
> >> ########################
> >>
> >>
> >> Best regards,
> >>
> >> Titouan

Thank you,
Heiko

Patch
diff mbox series

diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
index ed22f6b650..4efff8624f 100755
--- a/support/scripts/pkg-stats
+++ b/support/scripts/pkg-stats
@@ -617,8 +617,14 @@  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)
+            if pkg_name in packages:
+                if cve.affects(packages[pkg_name]):
+                    packages[pkg_name].cves.append(cve.identifier)
+                if len(packages[pkg_name].cves):
+                    packages[pkg_name].status['cve'] = ('error', 'affected by cve')
+                else:
+                    packages[pkg_name].status['cve'] = ('ok', 'no cve found')
+
 
 
 def calculate_stats(packages):