Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions vulnerabilities/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@
from django.contrib import admin

from vulnerabilities.models import (
ImpactedPackage,
Vulnerability_Package_Relation,
Importer,
Package,
ResolvedPackage,
Vulnerability,
VulnerabilityReference,
)
Expand All @@ -48,16 +47,11 @@ class PackageAdmin(admin.ModelAdmin):
pass


@admin.register(ImpactedPackage)
@admin.register(Vulnerability_Package_Relation)
class ImpactedPackageAdmin(admin.ModelAdmin):
pass


@admin.register(ResolvedPackage)
class ResolvedPackageAdmin(admin.ModelAdmin):
pass


@admin.register(Importer)
class ImporterAdmin(admin.ModelAdmin):
pass
148 changes: 75 additions & 73 deletions vulnerabilities/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,69 @@

from vulnerabilities.data_source import DataSource

class Importer(models.Model):
"""
Metadata and pointer to the implementation for a source of vulnerability data (aka security
advisories)
"""
name = models.CharField(max_length=100, unique=True, help_text='Name of the importer')

license = models.CharField(
max_length=100,
blank=True,
help_text='License of the vulnerability data',
)

last_run = models.DateTimeField(null=True, help_text='UTC Timestamp of the last run')

data_source = models.CharField(
max_length=100,
help_text='Name of the data source implementation importable from vulnerabilities.importers'
)
data_source_cfg = pgfields.JSONField(
null=False,
default=dict,
help_text='Implementation-specific configuration for the data source',
)

def make_data_source(self, batch_size: int, cutoff_date: datetime = None) -> DataSource:
"""
Return a configured and ready to use instance of this importers data source implementation.

batch_size - max. number of records to return on each iteration
cutoff_date - optional timestamp of the oldest data to include in the import
"""
importers_module = importlib.import_module('vulnerabilities.importers')
klass = getattr(importers_module, self.data_source)

ds = klass(
batch_size,
last_run_date=self.last_run,
cutoff_date=cutoff_date,
config=self.data_source_cfg,
)

return ds

def __str__(self):
return self.name


class Vulnerability(models.Model):
"""
A software vulnerability with minimal information. Identifiers other than CVE ID are stored as
VulnerabilityReference.
"""
cve_id = models.CharField(max_length=50, help_text='CVE ID', unique=True, null=True)
summary = models.TextField(help_text='Summary of the vulnerability', blank=True)
cvss = models.FloatField(max_length=100, help_text='CVSS Score', null=True)
vuln_id = models.CharField(max_length=50, help_text='eg CVE ID, RUST SEC ID', unique=True, null=True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this field really be nullable? What do we do with a vulnerability that has no vuln_id and no reference_ids? If your response is that there is always at least one reference ID, then why not store that in vuln_id? Or in other words; I haven't quite understood the difference between vuln_id and reference_ids here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't quite understood the difference between vuln_id and reference_ids here.

I expected that, hence I had added comments in the code to define what is vuln_id and what is reference_id, here is a repaste:

Whatever goes into vuln_id is a vulnerability identifier
which is undivisible i.e atomic vulnerability id. All CVEs fit into this.
reference_ids are usually but not limited to advisory ids like USN-4399-1
Contents of reference_ids are a name/id given to collection of
other small vulnerbilties. For example USN-4399-1 refers to CVE-2020-8618, CVE-2020-8619

If your response is that there is always at least one reference ID

As far as the advisories I've looked at, yes there is some sort of id present, but I'm not 100% confident whether this will stay true.

then why not store that in vuln_id?

Yes. If it is a atomic vulnerability id, then it probably didn't belonged in the reference_id in the first place. RUST-SEC ids are stored in reference_id no matter whether they have CVE or not. If they don't have a CVE, they become atomic, because no other id will denote that specific vulnerability.

I also had this idea, which I didn't mentioned here, but a vuln_id's value should also be present along with(if present) other reference_ids in the reference_ids column. The idea being vuln_id is also it's own reference_id.

Should this field really be nullable? What do we do with a vulnerability that has no vuln_id and no reference_ids?

As @pombredanne mentioned, we have to give them our ids, but that's gonna introduce a whole lot of other complexities(how to make id's consistent ?).

My other point is , should we really worry about vulnerabilities without any id's . As far as I have inspected these advisories, only FriendsOfPHP were missing these , which was solved, because GH provide their ids for FriendsOfPHP advisories.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have mentioned that I read the comment and still didn't get it (undivisible/atomic and "small vulnerabilities" confused me). But now I think I understand. Some advisories cover multiple vulnerabilities and if those have CVE IDs, they will all be mentioned.

The problem with storing IDs other than CVE in vuln_id is that we require them to be unique across all publishers of advisories. That might be the case coincidentally, but I don't think there are any efforts to ensure that. But, in practice it will probably work and if not that problem can be solved when it occurs.

As @pombredanne mentioned, we have to give them our ids, but that's gonna introduce a whole lot of other complexities(how to make id's consistent ?).

I think that was just referring to the automatically added primary key column.

I don't think we should worry about vulnerabilities without IDs. But the only reason I can think of for making this column nullable is to be able to store vulnerabilities without IDs. Hence my question. :)

@pombredanne pombredanne Jul 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbs2001 re:

As far as I have inspected these advisories, only FriendsOfPHP were missing these
AFAIK, they use date as ID then FriendsOfPHP/security-advisories@c6fc722#diff-a1ec953bbcb767e15ba1a9edbe828550

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbs2001 I thin we could do better with a simpler model.

  • On the vuln_id, if we want and need this, it would then becomes our own id that we assign automatically IMHO. I am not sure we need an id though I can some benefit for users.
  • On the reference side, IMHO one URL + reference ID is a reference, I cannot see when we need more than one URL. Can you elaborate that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pombredanne re

On the reference side, IMHO one URL + reference ID is a reference, I cannot see when we need more than one URL. Can you elaborate that?

I have explained it in this ticket itself, can you take a look at Problem 1 ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pombredanne

AFAIK, they use date as ID then

Probably yes.

On the reference side, IMHO one URL + reference ID is a reference, I cannot see when we need more than one URL. Can you elaborate that?

Sure, I have done that in a comment below

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pombredanne

On the vuln_id, if we want and need this, it would then becomes our own id that we assign automatically IMHO. I am not sure we need an id though I can some benefit for users.

I don't understand this, can you elaborate this further?

reference_ids = pgfields.JSONField()

# Whatever goes into vuln_id is a vulnerability identifier
# which is undivisible i.e atomic vulnerability id. All CVEs fit into this.

# reference_ids are usually but not limited to `advisory` ids like USN-4399-1
# https://usn.ubuntu.com/4399-1/.
# Contents of reference_ids are a name/id given to collection of
# other small vulnerbilties. For example USN-4399-1 refers to CVE-2020-8618, CVE-2020-8619

def __str__(self):
return self.cve_id or self.summary
Expand All @@ -56,26 +110,25 @@ class VulnerabilityReference(models.Model):
package manager.
"""
vulnerability = models.ForeignKey(
Vulnerability, on_delete=models.CASCADE)
source = models.CharField(
max_length=50, help_text='Source(s) name eg:NVD', blank=True)
reference_id = models.CharField(
max_length=50, help_text='Reference ID, eg:DSA-4465-1', blank=True)
url = models.URLField(
max_length=1024, help_text='URL of Vulnerability data', blank=True)
Vulnerability, on_delete=models.CASCADE)
source = models.ForeignKey(
Importer, on_delete=models.CASCADE)
urls = pgfields.JSONField()
summary = models.TextField()

class Meta:
unique_together = ('vulnerability', 'source', 'reference_id', 'url')

def __str__(self):
return f'{self.source} {self.reference_id} {self.url}'
unique_together = ('vulnerability', 'source')

class VulnerabilityScore(models.Model):
vulnerability_reference = models.ForeignKey(VulnerabilityReference, on_delete=models.CASCADE)
type = models.CharField(max_length=50, help_text='Vulnerability score type', blank=True)
score = models.CharField(max_length=50)

class Package(PackageURLMixin):
"""
A software package with links to relevant vulnerabilities.
"""
vulnerabilities = models.ManyToManyField(to='Vulnerability', through='ImpactedPackage')
vulnerabilities = models.ManyToManyField(to='Vulnerability', through='Vulnerability_Package_Relation')

class Meta:
unique_together = ('name', 'namespace', 'type', 'version', 'qualifiers', 'subpath')
Expand Down Expand Up @@ -111,68 +164,17 @@ def __str__(self):
return self.package_url


class ImpactedPackage(models.Model):
class Vulnerability_Package_Relation(models.Model):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this name is a placeholder, right? How about VulnerabilityImpact? Not great, not terrible, IMHO.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VulnerabilityImpact doesn't mention package anywhere, we need a name which should make sense that the table is about vulnerability and package

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that would be better.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbs2001 I am not convinced by the name too (and we should not use snake case for Class or model names).
The attributes are about a Package (is_vulnerable and version_range are all about Package) so a better name could might PackageAssignedVulnerability or PackageRelatedVulnerability ... but I need to think more about that change as what are the benefits to combine the Impacted and Resolved models in one?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pombredanne

About the name thing, it's just a placeholder, Btw PackageRelatedVulnerability makes tad more sense here.

what are the benefits to combine the Impacted and Resolved models in one?

Good question. There are some issues with having 2 tables, Impacted and Resolved .

Issue 1 :
You can do this, which doesn't make any sense.

In [1]: from vulnerabilities import models                                                                                   

In [2]: v1 = models.Vulnerability.objects.create(cve_id="CVE-foo")                                                           

In [3]: p1 = models.Package.objects.create(name="cream",type="ice",version='mango') 
   
In [4]: vp1 = models.ImpactedPackage.objects.create(vulnerability=v1, package=p1)                                            

In [5]: vp2 = models.ResolvedPackage.objects.create(vulnerability=v1, package=p1)    

This is pure garbage, nothing can be interpreted from these entries.

With a single table + flag, I can use a unique_together=('vulnerability','package')

Issue 2 : Check https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/import_runner.py#L121 , I am not sure I understand the exact issue but it's something along the lines that updating vulnerability status of a already existing package is not possible. @haikoschol can you explain this, with a snippet?

Having a single table, changes delete to an update(of the flag), which bypasses this issue.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""
Relates a vulnerability to package(s) impacted by it.
"""
# {
vulnerability = models.ForeignKey(Vulnerability, on_delete=models.CASCADE)
package = models.ForeignKey(Package, on_delete=models.CASCADE)
is_vulnerable = models.BooleanField()
# } till this point we have a consensus in this model

class Meta:
unique_together = ('vulnerability', 'package')

version_range = models.CharField(max_length=50)

class ResolvedPackage(models.Model):
"""
Relates a vulnerability to package(s) that contain a fix or resolution of this vulnerability.
"""
vulnerability = models.ForeignKey(Vulnerability, on_delete=models.CASCADE)
package = models.ForeignKey(Package, on_delete=models.CASCADE)


class Importer(models.Model):
"""
Metadata and pointer to the implementation for a source of vulnerability data (aka security
advisories)
"""
name = models.CharField(max_length=100, unique=True, help_text='Name of the importer')

license = models.CharField(
max_length=100,
blank=True,
help_text='License of the vulnerability data',
)

last_run = models.DateTimeField(null=True, help_text='UTC Timestamp of the last run')

data_source = models.CharField(
max_length=100,
help_text='Name of the data source implementation importable from vulnerabilities.importers'
)
data_source_cfg = pgfields.JSONField(
null=False,
default=dict,
help_text='Implementation-specific configuration for the data source',
)

def make_data_source(self, batch_size: int, cutoff_date: datetime = None) -> DataSource:
"""
Return a configured and ready to use instance of this importers data source implementation.

batch_size - max. number of records to return on each iteration
cutoff_date - optional timestamp of the oldest data to include in the import
"""
importers_module = importlib.import_module('vulnerabilities.importers')
klass = getattr(importers_module, self.data_source)

ds = klass(
batch_size,
last_run_date=self.last_run,
cutoff_date=cutoff_date,
config=self.data_source_cfg,
)

return ds

def __str__(self):
return self.name
class Meta:
unique_together = ('vulnerability', 'package')
1 change: 1 addition & 0 deletions vulnerablecode/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
'vulnerabilities',
'rest_framework',
'django_filters',
'django_extensions'
Comment thread
haikoschol marked this conversation as resolved.
]

MIDDLEWARE = [
Expand Down