feat: recommending safe packages #1284
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* fixes: intel#1159 * test: change way pytest is run in CI Switch to using a full pytest gather for the async tests. Several test files are singled out to be run synchronously because they touch NVD and can cause us to get rate limited if they run in parallel. This will ensure that no one needs to add new test files explicitly to CI unless they need to be run synchronously. Several test files are (temporarily) disabled because they are not passing; we'll enable those as they are fixed. Signed-off-by: Terri Oda <[email protected]>
* refactor(scanner): Remove scanned string splitting refactor(strings): Return a string instead of array of string * fix(checker): with respect to changes from b240fa0 fix: xml2 checker fix: sqlite checker fix: glibc checker and test fix: systemd checker fix: libdb checker fix: systemd checker v229 .deb fix: universal python package checker
* feat(checker): Add sudo checker * add condensed downloads for sudo
* Fix yaml and toml tests in test_config * Related to intel#1159 * Add test_config back to updated CI Two of the tests in test_config were failing, presumably because they'd gotten out of date when we changed the default for extraction. These tests were not being run in CI (thanks to @Molkree for noticing that) so they didn't get fixed in a timely manner. Signed-off-by: Terri Oda <[email protected]>
* Add recommdended dev tools list * Moved isort to dev requirements file. * fix: pip install from dev-recommended for isort CI * fix: remove isort also from requirements.csv Signed-off-by: Terri Oda <[email protected]>
* fix: Add gnome-shell checker * fix: Improve regex(multiline) * fix: Windows test for gnome-shell checker
* fix: rename development requirements file for Snyk Snyk picks up files named `*req*.txt` so we're switching dev-recommended.txt to dev-requirements.txt so gets scanned automatically. Signed-off-by: Terri Oda <[email protected]>
* fix: condensed downloads The condensed downloads were made without any proper strings in it due to the changes in intel#1227. * refactor: writelines -> write
…#1246) * refactor: helper script for is_executable() and parse_string() * helper script: Instantiate instance of VersionScanner Previously we were only using VersionScanner.clean_file_path() which is a static method. In this case we did not need to instantiate and instance. We started using the is_executable() and parse_strings() methods, which are regular methods which use self. When a method takes self as the first positional argument we must instantiate its class before we use call the attached method. As such, we've created a new instance of VersionScanner as the version_scanner property within HelperScript. Tested with $ curl -sfLo /tmp/libmatroska-1.5.0-1.el8.aarch64.rpm \ https://download-ib01.fedoraproject.org/pub/epel/8/Everything/x86_64/Packages/l/libmatroska-1.5.0-1.el8.x86_64.rpm $ python cve_bin_tool/helper_script.py /tmp/libmatroska-1.5.0-1.el8.aarch64.rpm [07:09:05] INFO cve_bin_tool.VersionScanner - Updating version_scanner.py:42 egg_info WARNING cve_bin_tool.HelperScript - False helper_script.py:74 WARNING cve_bin_tool.HelperScript - False helper_script.py:74 WARNING cve_bin_tool.HelperScript - False helper_script.py:74 WARNING cve_bin_tool.HelperScript - False helper_script.py:74 WARNING cve_bin_tool.HelperScript - (True, '/tmp helper_script.py:74 /cve-bin-tool-3shnb9ex/libmatroska-1.5.0 -1.el8.aarch64.rpm.extracted/usr/lib64/l ibmatroska.so.6.0.0: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, BuildID[sha1]=4e7b21 76f1dc3f381571a2c873532ab0ce254c24, stripped, too many notes (256)\n') WARNING cve_bin_tool.HelperScript - False helper_script.py:74 ────────────────────────────── LibmatroskaChecker ────────────────────────────── CONTAIN_PATTERNS = [ r"N11libmatroska24KaxChapterProcessCodecIDE", r"N11libmatroska24KaxChapterProcessCommandE", r"N11libmatroska24KaxChapterProcessPrivateE", libmatroska::KaxVideoGamma::RenderData(libebml::IOCallback&, bool, bool)", ... <output shortened> ... r"virtual libmatroska::DataBuffer* libmatroska::DataBuffer::Clone()", r"void libmatroska::KaxBlockBlob::SetBlockGroup(libmatroska::KaxBlockGroup&)", r"void libmatroska::KaxBlockGroup::SetBlockDuration(uint64)", r"void libmatroska::KaxReferenceBlock::SetReferencedBlock(const libmatroska::KaxBlockBlob*)", ] FILENAME_PATTERNS = [ r"libmatroska.so.6.0.0", ] VERSION_PATTERNS = [ r"/builddir/build/BUILD/libmatroska-1.5.0/matroska/KaxBlock.h", r"/builddir/build/BUILD/libmatroska-1.5.0/matroska/KaxTracks.h", r"/builddir/build/BUILD/libmatroska-1.5.0/src/KaxBlock.cpp", r"/builddir/build/BUILD/libmatroska-1.5.0/src/KaxBlockData.cpp", r"/builddir/build/BUILD/libmatroska-1.5.0/matroska/KaxCluster.h", r"/builddir/build/BUILD/libmatroska-1.5.0/src/KaxCluster.cpp", r"/builddir/build/BUILD/libmatroska-1.5.0/matroska/KaxCues.h", r"/builddir/build/BUILD/libmatroska-1.5.0/src/KaxCues.cpp", r"/builddir/build/BUILD/libmatroska-1.5.0/src/KaxCuesData.cpp", r"/builddir/build/BUILD/libmatroska-1.5.0/src/KaxSemantic.cpp", ] VENDOR_PRODUCT = [('matroska', 'libmatroska')] ──────────────────────────────────────────────────────────────────────────────── Signed-off-by: John Andersen <[email protected]> * refactor(helperscript): removed unused imports fix for black and isort commented the log messages * fix: typo * refactor(version_scanner): now is_executable would retrun (True, output) or (False, None) * refactor(helper_script): removed try/except block * fix: black Co-authored-by: John Andersen <[email protected]>
This helps close the gap we were seeing between the new and old API numbers: the new NVD API didn't report the rejected entries, but our code using the old API was storing entries in the db for them.
Making a SECURITY.md file just to make github happy (the information is duplicated from Readme.md)
Disables test_api_cve_count until it can be fixed.
I'm getting this error when passing without the |
|
this is the result that I'm currently get with the the |
|
Oh, also, you probably want to rebase this against origin/main so that it's triggering the new CI rules. |
but, the output does not get printed in the table
now affected-versions get output with their respective color schema based on severity
|
For console, the output looks like: I was not able to output these formatted forms of affected-version patterns and was switching between things. But, now just before the meeting it just clicked me how it could be done 😅
I'll add these :) (since now the output problem is solved 😅) |
Molkree
reviewed
Aug 4, 2021
Molkree
reviewed
Aug 5, 2021
There was a problem hiding this comment.
Thanks for the docstring, now I understand the purpose of this function a bit more, which raises some questions:
- You are not using
VersionInfodefined inutil.py - According to your logic in
format_version_infothere are only 3 cases: no version info, both including endpoints or both excluding endpoints. Is it true?- If it is true then there's no need to always pass 4 versions (when 2 of them will always be empty), just use 2 versions and a flag in your
VersionInfotuple (and don't forget to use it! it's easier to read named fields than just an index into the list) - If it's not true then you will need to cover mixed cases (v1,v2], [v1,v2) (again, don't forget to use your
VersionInfo)
- If it is true then there's no need to always pass 4 versions (when 2 of them will always be empty), just use 2 versions and a flag in your
|
Currently failing (for real, not because of the nvd issue): So looks like this needs a test update to pass CI. |
NOTE: can we short the `Text.style` repeating code? now using VersionInfo NamedTuple shortened `for ...` loop for adding the column
now affected versions do not repeat the last version range of the table
Codecov Report
@@ Coverage Diff @@
## main #1284 +/- ##
==========================================
+ Coverage 79.12% 79.37% +0.25%
==========================================
Files 271 271
Lines 4871 4932 +61
Branches 586 596 +10
==========================================
+ Hits 3854 3915 +61
- Misses 866 868 +2
+ Partials 151 149 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
peb-peb
commented
Aug 8, 2021
Comment on lines
42
to
60
| if versions: | ||
| # should refactor to use python 3.10's "Structural Pattern Matching" | ||
| if start_including and end_including: | ||
| return f"[{start_including} - {end_including}]" | ||
| elif start_including and end_excluding: | ||
| return f"[{start_including} - {end_excluding})" | ||
| elif start_excluding and end_including: | ||
| return f"({start_excluding} - {end_including}]" | ||
| elif start_excluding and end_excluding: | ||
| return f"({start_excluding} - {end_excluding})" | ||
| elif start_including: | ||
| return f">= {start_including}" | ||
| elif start_excluding: | ||
| return f"> {start_excluding}" | ||
| elif end_including: | ||
| return f"<= {end_including}" | ||
| elif end_excluding: | ||
| return f"< {end_excluding}" | ||
| return "-" |
There was a problem hiding this comment.
Can anyone think of a better approach for this? OR we could just wait for python 3.10 and add the "Structural Pattern Matching" (Maybe we should open a "good first issue" issue for this as a reminder for future)
There was a problem hiding this comment.
Waiting for Python 3.10 is not an option as cve-bin-tool supports all currently supported Python versions. So 3.9 will go out of support in 2025, only then 3.10 will become the minimum supported version (of course if cve-bin-tool continues to support all versions).
I would still rewrite it a bit because if versions check is useless (tuple of empty strings evaluates to True):
def format_version_range(version_info: VersionInfo) -> str:
(start_including, start_excluding, end_including, end_excluding) = version_info
if start_including and end_including:
return f"[{start_including} - {end_including}]"
if start_including and end_excluding:
return f"[{start_including} - {end_excluding})"
if start_excluding and end_including:
return f"({start_excluding} - {end_including}]"
if start_excluding and end_excluding:
return f"({start_excluding} - {end_excluding})"
if start_including:
return f">= {start_including}"
if start_excluding:
return f"> {start_excluding}"
if end_including:
return f"<= {end_including}"
if end_excluding:
return f"< {end_excluding}"
return "-"
Comment on lines
160
to
190
| if affected_versions != 0: | ||
| table.add_row( | ||
| Text.styled(cve_data["vendor"], color), | ||
| Text.styled(cve_data["product"], color), | ||
| Text.styled(cve_data["version"], color), | ||
| linkify_cve(Text.styled(cve_data["cve_number"], color)), | ||
| Text.styled(cve_data["severity"], color), | ||
| Text.styled( | ||
| str(cve_data["score"]) | ||
| + " (v" | ||
| + str(cve_data["cvss_version"]) | ||
| + ")", | ||
| color, | ||
| ), | ||
| Text.styled(cve_data["affected_versions"], color), | ||
| ) | ||
| else: | ||
| table.add_row( | ||
| Text.styled(cve_data["vendor"], color), | ||
| Text.styled(cve_data["product"], color), | ||
| Text.styled(cve_data["version"], color), | ||
| linkify_cve(Text.styled(cve_data["cve_number"], color)), | ||
| Text.styled(cve_data["severity"], color), | ||
| Text.styled( | ||
| str(cve_data["score"]) | ||
| + " (v" | ||
| + str(cve_data["cvss_version"]) | ||
| + ")", | ||
| color, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Is there a way to deduplicate this? 🤔
There was a problem hiding this comment.
Yeah
cells = [
Text.styled(cve_data["vendor"], color),
Text.styled(cve_data["product"], color),
Text.styled(cve_data["version"], color),
linkify_cve(Text.styled(cve_data["cve_number"], color)),
Text.styled(cve_data["severity"], color),
Text.styled(
f"{cve_data['score']} (v{cve_data['cvss_version']})", color
),
]
if affected_versions != 0:
cells.append(Text.styled(cve_data["affected_versions"], color))
table.add_row(*cells)
|
Sorry for replying late :( @Molkree reviews were really helpful and now I've implemented those changes. :) |
peb-peb
commented
Aug 8, 2021
Comment on lines
114
to
142
| if affected_versions != 0: | ||
| try: | ||
| start_including = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].start_including | ||
| start_excluding = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].start_excluding | ||
| end_including = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].end_including | ||
| end_excluding = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].end_excluding | ||
| except KeyError: # TODO: handle 'UNKNOWN' and some cves more cleanly | ||
| start_including = "" | ||
| start_excluding = "" | ||
| end_including = "" | ||
| end_excluding = "" | ||
| cve_by_remarks[cve.remarks][-1].update( | ||
| { | ||
| "affected_versions": format_version_range( | ||
| start_including, | ||
| start_excluding, | ||
| end_including, | ||
| end_excluding, | ||
| ) | ||
| } | ||
| ) |
There was a problem hiding this comment.
I wrote a comment for this, but am not able to find it now. So, I'm writing it again :(
Can we do something else (instead of try... except block) to handle the UNKNOWN and some cves which are not found in the all_cve_version_info dictionary?
For UNKNOWN, maybe we could could add a key: value pair of "UNKNOWN": VersionInfo('', '', '', '') at the start/end of this dictionary.
But, when some cves are not found in the all_cve_version_info, what could be a more clean/absolute way of handling both these error?
I'm facing this issue when running it over wireshark-1.10.14-25.el7.i686.rpm and currently I get this:
(cve_env) peb@ooo:/cve-bin-tool$ python3 -m cve_bin_tool.cli -u never --affected-versions ../test/wireshark-1.10.14-25.el7.i686.rpm
┏━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━┓
┃ Vendor ┃ Product ┃ Version ┃ CVE Number ┃ Severity ┃ Score (CVSS Version) ┃ Affected Versions ┃
┡━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━┩
│ wireshark │ wireshark │ 1.10.14 │ CVE-2015-3182 │ MEDIUM │ 5.5 (v3) │ - │
│ wireshark │ wireshark │ 1.10.14 │ CVE-2015-3814 │ MEDIUM │ 5 (v2) │ - │
│ wireshark │ wireshark │ 1.10.14 │ CVE-2017-17935 │ HIGH │ 7.5 (v3) │ <= 2.2.11 │
│ wireshark │ wireshark │ 1.10.14 │ CVE-2017-17997 │ HIGH │ 7.5 (v3) │ <= 2.2.11 │
│ wireshark │ wireshark │ 1.10.14 │ CVE-2017-6014 │ HIGH │ 7.5 (v3) │ <= 2.2.4 │
│ wireshark │ wireshark │ 1.10.14 │ CVE-2018-14438 │ HIGH │ 7.5 (v3) │ <= 2.6.2 │
│ wireshark │ wireshark │ 1.10.14 │ CVE-2018-6836 │ CRITICAL │ 9.8 (v3) │ <= 2.4.4 │
│ wireshark │ wireshark │ 1.10.14 │ CVE-2020-26575 │ HIGH │ 7.5 (v3) │ <= 3.2.7 │
└───────────┴───────────┴─────────┴────────────────┴──────────┴──────────────────────┴───────────────────┘
here, all_cve_version_info returns
defaultdict(<class 'cve_bin_tool.util.VersionInfo'>,
{
'CVE-2017-17935': VersionInfo(start_including='', start_excluding='', end_including='2.2.11', end_excluding=''),
'CVE-2017-17997': VersionInfo(start_including='', start_excluding='', end_including='2.2.11', end_excluding=''),
'CVE-2017-6014': VersionInfo(start_including='', start_excluding='', end_including='2.2.4', end_excluding=''),
'CVE-2018-14438': VersionInfo(start_including='', start_excluding='', end_including='2.6.2', end_excluding=''),
'CVE-2018-6836': VersionInfo(start_including='', start_excluding='', end_including='2.4.4', end_excluding=''),
'CVE-2020-26575': VersionInfo(start_including='', start_excluding='', end_including='3.2.7', end_excluding='')
}
)
Here, all_cve_version_info doesn't contain CVE-2015-3182 and CVE-2015-3814.
Molkree
reviewed
Aug 8, 2021
Comment on lines
116
to
127
| start_including = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].start_including | ||
| start_excluding = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].start_excluding | ||
| end_including = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].end_including | ||
| end_excluding = dict(all_cve_version_info)[ | ||
| cve.cve_number | ||
| ].end_excluding |
There was a problem hiding this comment.
First of all, all_cve_version_info is already a dictionary so you don't need to convert it to dict again.
Second, just use unpacking:
Suggested change
| start_including = dict(all_cve_version_info)[ | |
| cve.cve_number | |
| ].start_including | |
| start_excluding = dict(all_cve_version_info)[ | |
| cve.cve_number | |
| ].start_excluding | |
| end_including = dict(all_cve_version_info)[ | |
| cve.cve_number | |
| ].end_including | |
| end_excluding = dict(all_cve_version_info)[ | |
| cve.cve_number | |
| ].end_excluding | |
| ( | |
| start_including, | |
| start_excluding, | |
| end_including, | |
| end_excluding, | |
| ) = all_cve_version_info[cve.cve_number] |
There was a problem hiding this comment.
Or go further and:
- Add default values
""to yourVersionInfoclass. - Change
format_version_rangeto acceptVersionInfoclass. - Rewrite this whole
iflike this:
if affected_versions != 0:
try:
version_info = all_cve_version_info[cve.cve_number]
except KeyError: # TODO: handle 'UNKNOWN' and some cves more cleanly
version_info = VersionInfo()
cve_by_remarks[cve.remarks][-1].update(
{"affected_versions": format_version_range(version_info)}
)There was a problem hiding this comment.
I was using all_cve_version_info with defaultdict and was facing issues with it and this was the solution I decided to go with (it never occurred to me that I could just use dict here) :(
if affected_versions != 0: try: version_info = all_cve_version_info[cve.cve_number] except KeyError: # TODO: handle 'UNKNOWN' and some cves more cleanly version_info = VersionInfo() cve_by_remarks[cve.remarks][-1].update( {"affected_versions": format_version_range(version_info)} )
This was a very clean change :)
But, can we do something better? (OR can we remove # TODO: handle 'UNKNOWN' and some cves more cleanly)
| start_including, start_excluding, end_including, end_excluding | ||
| ): | ||
| """ | ||
| formats version info to desirable output |
There was a problem hiding this comment.
Suggested change
| formats version info to desirable output | |
| Format version info to desirable output |
Just to be consistent with another function in this module
Comment on lines
42
to
60
| if versions: | ||
| # should refactor to use python 3.10's "Structural Pattern Matching" | ||
| if start_including and end_including: | ||
| return f"[{start_including} - {end_including}]" | ||
| elif start_including and end_excluding: | ||
| return f"[{start_including} - {end_excluding})" | ||
| elif start_excluding and end_including: | ||
| return f"({start_excluding} - {end_including}]" | ||
| elif start_excluding and end_excluding: | ||
| return f"({start_excluding} - {end_excluding})" | ||
| elif start_including: | ||
| return f">= {start_including}" | ||
| elif start_excluding: | ||
| return f"> {start_excluding}" | ||
| elif end_including: | ||
| return f"<= {end_including}" | ||
| elif end_excluding: | ||
| return f"< {end_excluding}" | ||
| return "-" |
There was a problem hiding this comment.
Waiting for Python 3.10 is not an option as cve-bin-tool supports all currently supported Python versions. So 3.9 will go out of support in 2025, only then 3.10 will become the minimum supported version (of course if cve-bin-tool continues to support all versions).
I would still rewrite it a bit because if versions check is useless (tuple of empty strings evaluates to True):
def format_version_range(version_info: VersionInfo) -> str:
(start_including, start_excluding, end_including, end_excluding) = version_info
if start_including and end_including:
return f"[{start_including} - {end_including}]"
if start_including and end_excluding:
return f"[{start_including} - {end_excluding})"
if start_excluding and end_including:
return f"({start_excluding} - {end_including}]"
if start_excluding and end_excluding:
return f"({start_excluding} - {end_excluding})"
if start_including:
return f">= {start_including}"
if start_excluding:
return f"> {start_excluding}"
if end_including:
return f"<= {end_including}"
if end_excluding:
return f"< {end_excluding}"
return "-"
Comment on lines
160
to
190
| if affected_versions != 0: | ||
| table.add_row( | ||
| Text.styled(cve_data["vendor"], color), | ||
| Text.styled(cve_data["product"], color), | ||
| Text.styled(cve_data["version"], color), | ||
| linkify_cve(Text.styled(cve_data["cve_number"], color)), | ||
| Text.styled(cve_data["severity"], color), | ||
| Text.styled( | ||
| str(cve_data["score"]) | ||
| + " (v" | ||
| + str(cve_data["cvss_version"]) | ||
| + ")", | ||
| color, | ||
| ), | ||
| Text.styled(cve_data["affected_versions"], color), | ||
| ) | ||
| else: | ||
| table.add_row( | ||
| Text.styled(cve_data["vendor"], color), | ||
| Text.styled(cve_data["product"], color), | ||
| Text.styled(cve_data["version"], color), | ||
| linkify_cve(Text.styled(cve_data["cve_number"], color)), | ||
| Text.styled(cve_data["severity"], color), | ||
| Text.styled( | ||
| str(cve_data["score"]) | ||
| + " (v" | ||
| + str(cve_data["cvss_version"]) | ||
| + ")", | ||
| color, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Yeah
cells = [
Text.styled(cve_data["vendor"], color),
Text.styled(cve_data["product"], color),
Text.styled(cve_data["version"], color),
linkify_cve(Text.styled(cve_data["cve_number"], color)),
Text.styled(cve_data["severity"], color),
Text.styled(
f"{cve_data['score']} (v{cve_data['cvss_version']})", color
),
]
if affected_versions != 0:
cells.append(Text.styled(cve_data["affected_versions"], color))
table.add_row(*cells)changed `all_cve_version_info` from defaultdict -> dict handling `KeyError` more cleanly shortened `table.add_row()` implementation fixed `format_version_info` docstring
changed variable name to be more pythonic: versionStartIncluding -> version_start_including versionStartExcluding -> version_start_excluding versionEndIncluding -> version_end_including versionEndExcluding -> version_end_excluding
terriko
approved these changes
Aug 11, 2021
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.