Collect Mozilla

[Hritik14]

Fixes #78. Most of the work is done. Here's the checklist

• Parsing for mozilla data source
• Working mozilla importer
• Migrate to GitDataSource
• Write test cases
• More verbose comments
• Update dependencies

Currently, it requires two extra dependencies: PyGithub and markdown
I would eliminate the PyGithub dependency next but I think markdown has
to stay.
Final dependencies would be comitted later.

I would also really like advices on

• The FIXMEs present in current code.
• Should we mark all the pks <= resolved pkg as vulnerable as mozilla doesn't ship that information

This is how it looks for now

Signed-off-by: Hritik Vijay hritikxx8@gmail.com

[sbs2001]

@Hritik14 I've done a light review for now, will do another review once changes are made.

[Hritik14]

@sbs2001 any update ?

[sbs2001]

@Hritik14 are you sure, you pushed the changes ?

[Hritik14]

@sbs2001 ah, not yet. I actually replied on the requested changes. I'm not sure if you get notifications for those replies. Do let me know and I'll leave a comment next time as well.

[Hritik14]

@sbs2001 I've refactored as suggested and also added a helper functions to split markdown and the front matter. I'd add it to the istio importer after this PR is merged.

[Hritik14]

rebased

[sbs2001]

Thanks @Hritik14 I've left some comments inline for your consideration. This looks good btw .

[sbs2001]

What's wrong at alpine linux ?

[Hritik14]

Should we add the severity of the CVE itself ?
Severity is only available in references, so maybe we should consider adding the CVE Mitre as a reference (as done in this PR) more uniformly.
In Alpine, the CVE is not added as a reference but only as id. It won't really be required there as no severity is mentioned in the advisory for alpine, I wrote it just to maintain uniformaty.
This is what I was thinking
https://github.com/nexB/vulnerablecode/blob/a95a0e9c21ba9a2689dffe46c6c55fe7b8dacf78/vulnerabilities/importers/mozilla.py#L150

[sbs2001]

Just curious, could you point me to an example where this is not true ?

[Hritik14]

This is a precautionary measure. I'm only collecting if a p tag follows an h3 which seems more or less like a standard. There is no strict rule about how to format the markdown file, so I figured we should get the correct data if at all.

[sbs2001]

Would that collect the strong tag too ? Like the one at https://github.com/mozilla/foundation-security-advisories/blob/master/announce/2016/mfsa2016-14.md ? Not a big deal but we should avoid collecting those tags.

[Hritik14]

No.
For the linked document it returns

Security researcher Holger Fuhrmannek reported that a malicious\nGraphite....

Only consecutive p tags are collected and get_text() makes sure we're getting only textual representation of the content present in p tags.

[Hritik14]

@sbs2001 any update ?

[pombredanne]

Thanks!
see a few comments inline for you consideration.

Also, can you avoid having a test zip in vulnerabilities/tests/test_data/mozilla.zip ? text is fine instead.
This will simplify the tests and help review too.

[pombredanne]

What about this instead?

import saneyaml

# normalize line endings just in case:
text = text.replace("\r\n", "\n")
front_matter, _, body = text.rpartition("\n---\n")
front_matter = saneyaml.load(front_matter)

For instance:

>>> import saneyaml
>>> text = """---
... title: ISTIO-SECURITY-2019-001
... subtitle: 安全公告
... description: 错误的权限控制。
... cve: [CVE-2019-12243]
... publishdate: 2019-05-28
... keywords: [CVE]
... skip_seealso: true
... aliases:
...     - /zh/blog/2019/cve-2019-12243
...     - /zh/news/2019/cve-2019-12243
... ---
... 
... {{< security_bulletin
...         cves="CVE-2019-12243"
...         cvss="8.9"
...         vector="CVSS:3.0/AV:A/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:N/E:H/RL:O/RC:C"
...         releases="1.1 to 1.1.6" >}}
... 
... ## 内容{#context}
... """
>>> text = text.replace("\r\n", "\n")
>>> front_matter, _, body = text.rpartition("\n---\n")
>>> front_matter = saneyaml.load(front_matter)
>>> print(front_matter)
{'title': 'ISTIO-SECURITY-2019-001', 'subtitle': '安全公告', 'description': '错误的权限控制。', 'cve': ['CVE-2019-12243'], 'publishda
te': '2019-05-28', 'keywords': ['CVE'], 'skip_seealso': True, 'aliases': ['/zh/blog/2019/cve-2019-12243', '/zh/news/2019/cve-2019-122
43']}

[Hritik14]

We have not been using saneyaml anywhere in the project. If that is preferred, I can add it to requirements and we can start using that but I don't see any specific reason to do so here.
Regarding rpartition. We cannot have that either as a markdown is allowed to have a "\n---"\n.
Consider this

>>> text=""""---
... announced: February 4, 2014
... fixed_in:
... - Firefox 27
... - Firefox ESR 24.3
... - Thunderbird 24.3
... - Seamonkey 2.24
... impact: High
... reporter: Cody Crews
... title: Clone protected content with XBL scopes
... ---
... 
... <h3>Description</h3>
... 
... Text
... ---
... other text
... """
>>> text = text.replace("\r\n", "\n")
>>> front_matter, _, body = text.rpartition("\n---\n")
>>> 
>>> front_matter
'"---\nannounced: February 4, 2014\nfixed_in:\n- Firefox 27\n- Firefox ESR 24.3\n- Thunderbird 24.3\n- Seamonkey 2.24\nimpact: High\nreporter: Cody Crews\ntitle: Clone protected content with XBL scopes\n---\n\n<h3>Description</h3>\n\nText'
>>> 
>>> body
'other text\n'
>>> 

The official validator by mozilla parses it something like this
https://github.com/mozilla/foundation-security-advisories/blob/d43d09d204ab5da014e83b7d1743df289cefee92/check_advisories.py#L183-L208

Further, as it is a helper, we cannot have it mozilla specific. All it should do is to split the front matter and markdown.
I could have something like this

import yaml

text="""---
announced: February 4, 2014
fixed_in:
- Firefox 27
- Firefox ESR 24.3
- Thunderbird 24.3
- Seamonkey 2.24
impact: High
reporter: Cody Crews
title: Clone protected content with XBL scopes
---

<h3>Description</h3>
---
other text
"""
# normalize line endings just in case:
text = text.replace("\r\n", "\n")
linezero,_, text = text.partition("---\n")
if not linezero: # nothing before first ---
    front_matter,_, body = text.partition("---")
    front_matter = yaml.safe_load(front_matter)
else:
    front_matter = ""
    body = linezero + "---\n" + text
print(front_matter)
print(body)

which prints

{'announced': 'February 4, 2014', 'fixed_in': ['Firefox 27', 'Firefox ESR 24.3', 'Thunderbird 24.3', 'Seamonkey 2.24'], 'impact': 'High', 'reporter': 'Cody Crews', 'title': 'Clone protected content with XBL scopes'}


<h3>Description</h3>
---
other text

but doesn't look any better to me.

[pombredanne]

but doesn't look any better to me.

It feels a bit more readable may be?

The official validator by mozilla parses it something like this
https://github.com/mozilla/foundation-security-advisories/blob/d43d09d204ab5da014e83b7d1743df289cefee92/check_advisories.py#L183-L208

We could very much reuse it as-is as well. This is under an MPL license so we would have to do it in an orderly fashion with proper license tracking and code separation though.
If we do not copy it, we cannot reuse code from it though.

[Hritik14]

As it is required by multiple importers, let's move this to it's own PR. Opened #443

[pombredanne]

How can you get no impacted packages and fixed packages?

[Hritik14]

Upstream doesn't provide with a list of impacted package. We might consider all packages until this package as impacted but I'm not sure about this. There are many other importers that do this too. Eg: https://github.com/nexB/vulnerablecode/blob/f254b0d4ac54b70c648055a7e8eda16c05dce0f9/vulnerabilities/importers/alpine_linux.py#L194

[pombredanne]

good point. We need a ticket though, as this may need to be interpreted as "all versions before this version are vulnerable" and it warrant some research and discussions.

[sbs2001]

Ouch, is this going to be redundant now, just like the suse_backport importer. We now have a improved notion of fixed/resolved/patched package now. Due to #436

[Hritik14]

@sbs2001 That is scary. I've opened #449 to discuss this further.

[pombredanne]

I would like to merge this sooner than later. ... See also #449 (comment)

[Hritik14]

@pombredanne Thank you for the through review. Please check unresolved conversations and I'll push the updated code.

[pombredanne]

@Hritik14 done :) Thank you for having looked at all these in details!

[sbs2001]

@pombredanne re zip files: see #442 (comment) .

@Hritik14 IMHO mocking the whole GItDataSource's logic of collecting changes is redundant, since there are thorough tests of that already. There is some mock db things which IMHO is also not required . Instead you could just test the to_advisories and see whether correct Advisory objects are returned. It's fast (no need to setup DB and such) and easy to implement.

Something like https://github.com/nexB/vulnerablecode/blob/main/vulnerabilities/tests/test_retiredotnet.py . There are tests which do check what ends up in db, but I think those should be limited and ideally should write small amounts of data.

[pombredanne]

@Hritik14 do you mind to rebase?

[pombredanne]

Let's merge this first and fix it later.
Please enter an issue to track that

We are merging first and will fix issues afterward.