Added Input Engine with triage functionality #777

Niraj-Kamdar · 2020-07-03T11:10:33Z

New features:

Created input_engine that can handle various input files with triage data
Currently, input engine supports csv and json file.
To get cves from input file - Usage: cve-bin-tool -i=test.csv
Input engine can also be used to supplement triage data - Usage: cve-bin-tool -i=test.csv /path/to/scan
User can now add remarks field in csv or json which can have any value from following values ( Here, values in parenthesis are aliases for that specific type. )
1. NewFound (1, n, N)
2. Unexplored (2, u, U)
3. Confirmed (3, c, C)
4. Mitigated, (4, m, M)
5. Ignored (5, i, I)
Output will be also represented in above order.
tests for input engine added under test_input_engine.
removed test for old csv2cve

codecov-commenter · 2020-07-03T11:32:49Z

Codecov Report

Merging #777 into master will decrease coverage by 0.93%.
The diff coverage is 81.55%.

@@            Coverage Diff             @@
##           master     #777      +/-   ##
==========================================
- Coverage   88.32%   87.38%   -0.94%     
==========================================
  Files         139      140       +1     
  Lines        2483     2474       -9     
  Branches      301      297       -4     
==========================================
- Hits         2193     2162      -31     
- Misses        226      243      +17     
- Partials       64       69       +5

Flag	Coverage Δ
#longtests	`87.38% <81.55%> (-0.94%)`	⬇️

Impacted Files	Coverage Δ
cve_bin_tool/csv2cve.py	`0.00% <0.00%> (-63.37%)`	⬇️
cve_bin_tool/cvedb.py	`87.60% <ø> (+2.12%)`	⬆️
cve_bin_tool/version_signature.py	`76.59% <ø> (ø)`
test/test_data/ffmpeg.py	`100.00% <ø> (ø)`
test/test_data/kerberos.py	`100.00% <ø> (ø)`
test/test_data/lighttpd.py	`100.00% <ø> (ø)`
cve_bin_tool/util.py	`70.58% <33.33%> (-13.42%)`	⬇️
cve_bin_tool/cli.py	`82.90% <71.11%> (-3.27%)`	⬇️
test/test_scanner.py	`90.00% <79.16%> (-6.39%)`	⬇️
test/test_output_engine.py	`97.61% <90.00%> (-1.22%)`	⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce183b0...de68871. Read the comment docs.

Niraj-Kamdar · 2020-07-03T12:26:06Z

cve_bin_tool/cli.py

 from .version import VERSION
+from .version_scanner import VersionScanner
+
+ERROR_CODES = {


Custom error codes

Niraj-Kamdar · 2020-07-03T12:27:03Z

cve_bin_tool/cli.py

+def excepthook(exctype, exc, trace):
+    oldHook(exctype, exc, trace)
+    if ERROR_CODES.get(exctype):
+        sys.exit(ERROR_CODES[exctype])


custom except hook to also throw exit code while exception raise

This is really lovely, thanks. One of our users (I think someone running this on a Yocto based system?) was saying that they use the return codes to colourize their CI dashboard, so this may be very helpful to others as well.

Niraj-Kamdar · 2020-07-03T12:31:49Z

cve_bin_tool/cli.py

+    parser.add_argument(
+        "-i",
+        "--input-file",
+        action="store",
+        default=None,
+        help="Provide input filename",
+    )


New option to specify input file

shouldn't it be the actual path to the file? I guess it should be file-path

Niraj-Kamdar · 2020-07-03T12:37:17Z

cve_bin_tool/cli.py


        LOGGER.info("")
        LOGGER.info("Overall CVE summary: ")
        LOGGER.info(
-            f"There are {scanner.files_with_cve} files with known CVEs detected"
+            f"There are {cve_scanner.products_with_cve} products with known CVEs detected"


just a better name because files_with_cves won't make much sense while using it as csv2cve

Good thinking, though I wonder if products will be confusing for people scanning directories, as they may think of a "product" as a .deb or somesuch and we have a different definition. it's tempting to just say "things" instead of files or products, but I guess that doesn't sound quite as formal.

Niraj-Kamdar · 2020-07-03T12:38:09Z

cve_bin_tool/csv2cve.py

-        else:
-            csv2cve.update_logLevel(log_level=args.log_level)
-        csv2cve.generate_output(args.output_file, args.format)
+    argv = argv or sys.argv


dummy csv2cve file

Niraj-Kamdar · 2020-07-03T12:39:50Z

cve_bin_tool/input_engine.py

+class InvalidCsvError(Exception):
+    """ Given File is an Invalid CSV """
+
+
+class MissingFieldsError(Exception):
+    """ Missing needed fields """
+
+
+class InvalidJsonError(Exception):
+    """ Given File is an Invalid JSON """


custom exceptions to raise exceptions

Niraj-Kamdar · 2020-07-03T12:42:09Z

cve_bin_tool/input_engine.py

+class Remarks(OrderedEnum):
+    NewFound = 1, "1", "NewFound", "n", "N"
+    Unexplored = 2, "2", "Unexplored", "u", "U", ""
+    Mitigated = 4, "4", "Mitigated", "m", "M"
+    Confirmed = 3, "3", "Confirmed", "c", "C"
+    Ignored = 5, "5", "Ignored", "i", "I"


custom enum class that can handle multiple aliases easily and can also be sorted.

Niraj-Kamdar · 2020-07-03T12:45:39Z

cve_bin_tool/util.py

@@ -109,3 +110,25 @@ def pattern_match(text, patterns):
            if fnmatch.fnmatch(text, pattern):
                return True
        return False
+
+
+class OrderedEnum(Enum):


This is to provide comparison methods that allows sorting of list of Remarks.

Niraj-Kamdar · 2020-07-03T16:42:13Z

.github/workflows/pythonapp.yml

@@ -118,6 +118,7 @@ jobs:
      ACTIONS: 1
      LONG_TESTS: 0
      NO_EXIT_CVE_NUM: 1
+      PYTHONIOENCODING: 'utf8'


Fixes problem with Windows CI

YAY! Thanks for this

Niraj-Kamdar · 2020-07-05T10:01:51Z

.github/workflows/pythonapp.yml

          test/test_cli.py
+          test/test_extractor.py


Changed because of https://bugs.python.org/issue35621.

terriko

I've got a bunch of nitpicky comments about error codes and better PR splits, but given that @SinghHrmn I think is waitnig for this PR to be merged, I'm going to merge and file issues for my feedback rather than request changes.

@Niraj-Kamdar one big thing though is that you really need to get into the habit of grouping the changes in PRs by function, so the checker changes here should have been in a separate branch and a separate PR. Much of the minor refactoring stuff should also have been split out. We've let you get into bad habits because we had so much refactoring that needed doing that I was expecting to see giant PRs, and that's on me and @pdxjohnny as mentors, but it's something I think is worth working on going forwards.

Here's a couple of helpful articles on how to split up PRs that I think you might find helpful: https://unhashable.com/stacked-pull-requests-keeping-github-diffs-small/ https://www.thedroidsonroids.com/blog/splitting-pull-request

terriko · 2020-07-09T17:41:42Z

cve_bin_tool/checkers/gstreamer.py

-    CONTAINS_PATTERNS = [r"http://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer"]
+    CONTAINS_PATTERNS = [
+        r"http://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer",
+        r"libgstreamer-((\d+\.)+\d+)",


Should we be altering the checker default so it scans for VERSION_PATTERNS as well as CONTAINS_PATTERNS by default? It seems silly to duplicate lines in each checker if we probably actually want version patterns to also be a valid detection string.

Yes that can be helpful.

terriko · 2020-07-09T17:47:09Z

cve_bin_tool/cli.py

+def excepthook(exctype, exc, trace):
+    oldHook(exctype, exc, trace)
+    if ERROR_CODES.get(exctype):
+        sys.exit(ERROR_CODES[exctype])


This is really lovely, thanks. One of our users (I think someone running this on a Yocto based system?) was saying that they use the return codes to colourize their CI dashboard, so this may be very helpful to others as well.

terriko · 2020-07-09T17:49:12Z

cve_bin_tool/cli.py

@@ -37,8 +60,7 @@ def __call__(self, parser, namespace, value, option_string=None):

 def main(argv=None):
    """ Scan a binary file for certain open source libraries that may have CVEs """
-    if argv is None:
-        argv = sys.argv
+    argv = argv or sys.argv


I kind of prefer the clarity of the is None check in some cases (because we get so many new pythonistas looking at this project) but this isn't a thing I expect a lot of new contributors to look at so I think streamlining this is the right choice.

terriko · 2020-07-09T17:52:51Z

cve_bin_tool/cli.py

-        default=None,
-        help="provide output filename (default: output to stdout)",
+        default="",
+        help="provide output filename",


Changing the default is fine, but don't change the help message here: that's to clarify for users because the default messages from argparser didn't make the behaviour clear.

Suggested change

help="provide output filename",

help="provide output filename (default: output to stdout)",

I will add that.

terriko · 2020-07-09T17:56:22Z

cve_bin_tool/cli.py

+        "-x",
+        "--extract",
+        action="store_true",
+        default=False,


I've been wondering about this: should we change the default to auto-extract? I've opened an issue to discuss, since that's out of scope for this PR. #786

terriko · 2020-07-09T18:12:36Z

cve_bin_tool/cli.py


        LOGGER.info("")
        LOGGER.info("Overall CVE summary: ")
        LOGGER.info(
-            f"There are {scanner.files_with_cve} files with known CVEs detected"
+            f"There are {cve_scanner.products_with_cve} products with known CVEs detected"


Good thinking, though I wonder if products will be confusing for people scanning directories, as they may think of a "product" as a .deb or somesuch and we have a different definition. it's tempting to just say "things" instead of files or products, but I guess that doesn't sound quite as formal.

terriko · 2020-07-09T18:16:42Z

cve_bin_tool/csv2cve.py

+    argv = argv or sys.argv
+    if len(argv) < 2:
+        LOGGER.error("Invalid Usage: csv file required")
+        return -2


If we're importing cli, we might as well use the same error code list here.

I will change that

terriko · 2020-07-09T18:17:07Z

cve_bin_tool/csv2cve.py

-            csv2cve.update_logLevel(log_level=args.log_level)
-        csv2cve.generate_output(args.output_file, args.format)
+        LOGGER.error("Invalid Usage: csv file required")
+        return -4


Another error code spot.

terriko · 2020-07-09T18:21:39Z

cve_bin_tool/cve_scanner.py


    @staticmethod
-    def openssl_convert(version):
+    def openssl_convert(version: str) -> str:


yay type hints!

terriko · 2020-07-09T18:29:02Z

test/test_data/ffmpeg.py

@@ -4,7 +4,7 @@
        "version": "4.1.3",
        "version_strings": [
            "%s version 4.1.3",
-            "Codec %s is not recognized by FFmpeg.",
+            "Codec '%s' is not recognized by FFmpeg.",


In future, can you put unrelated changes to checkers in a separate PR? They could all be grouped together, but they shouldn't be grouped with the InputEngine changes. It's considered bad form by most open source projects to group them the way you have.

I have put it here because I have to make VersionScanner generator to use InputEngine parsed data as supplement and it was breaking the test_scanner because I have also included test for contains string previously but I have forget to clean up scanner and test was only passing due to previous scan has detected it. So, that's why when I converted that in generator this bug come to surface. I should have divide this PR in two parts: 1) Making version_scanner generator and 2) building input_engine. Thanks, for the feedback. I will try to keep PR size smaller.

Niraj-Kamdar added 21 commits July 1, 2020 10:00

init input_engine

28d95dc

add more aliases to test csv

8406aa2

generator base scanner added

4fbe089

input_engine working!

56274a6

Added type hint :)

3f09252

working input_engine

60f788f

Fixed issues

ebe9075

Merge remote-tracking branch 'upstream/master' into input-dev

1061fab

update output_engine

5607664

fix issue

c93c4fd

bug fix

f3f8fcc

fix tests

0e48b49

patch for csv2cve

06f0606

remove old unittest

4ee25ad

this may fix

4a337c9

fix long tests

1dde24e

try now

6472bc7

fix test usage

00ad55b

fix bugs

2819a34

legacy windows test

3df3411

Add newfound option

4280905

Niraj-Kamdar added 4 commits July 3, 2020 17:23

add custom except hook to handle error

8b2a175

sortable enum

d9cdf9a

remove unnecessary import

e55e8e9

fix bug

349b8ca

Niraj-Kamdar commented Jul 3, 2020

View reviewed changes

Niraj-Kamdar added 3 commits July 3, 2020 19:16

test now

28c3cdc

legacy false working

84be85b

fix workflow

d5f1147

Merge branch 'input' into input-dev

cca0a5e

Niraj-Kamdar commented Jul 3, 2020

View reviewed changes

Niraj-Kamdar added 11 commits July 4, 2020 18:31

DRY it.

a2c1b55

input_engine tests added

5835c94

fix test_scanner

a2ab3c7

fix CI

23b937e

fix typo

150c239

fix csv2cve

ee94c88

better fix

7714f6d

fix minor bugs

5589f0e

added some more type hints :)

5e443e5

fix test_output_engine

8ce679c

fix test

776b9ce

Niraj-Kamdar force-pushed the input-dev branch from bf1ef44 to 776b9ce Compare July 5, 2020 09:27

run extractor in synchronous for 3.6 and 3.7

de68871

Niraj-Kamdar commented Jul 5, 2020

View reviewed changes

terriko approved these changes Jul 9, 2020

View reviewed changes

terriko merged commit 58df2a7 into intel:master Jul 9, 2020

Niraj-Kamdar deleted the input-dev branch July 10, 2020 07:35

Niraj-Kamdar mentioned this pull request Jul 11, 2020

Use custom exception class instead of error code in csv2cve or new input_engine. #771

Closed

Added Input Engine with triage functionality #777

Added Input Engine with triage functionality #777

Niraj-Kamdar commented Jul 3, 2020 •

edited

codecov-commenter commented Jul 3, 2020 •

edited

Niraj-Kamdar Jul 3, 2020

Niraj-Kamdar Jul 3, 2020

terriko Jul 9, 2020

Niraj-Kamdar Jul 3, 2020

SinghHrmn Jul 3, 2020

Niraj-Kamdar Jul 3, 2020

terriko Jul 9, 2020

Niraj-Kamdar Jul 3, 2020

Niraj-Kamdar Jul 3, 2020

Niraj-Kamdar Jul 3, 2020

Niraj-Kamdar Jul 3, 2020

Niraj-Kamdar Jul 3, 2020

SinghHrmn Jul 3, 2020

Niraj-Kamdar Jul 5, 2020

terriko left a comment

terriko Jul 9, 2020

Niraj-Kamdar Jul 9, 2020

terriko Jul 9, 2020

terriko Jul 9, 2020

terriko Jul 9, 2020

Niraj-Kamdar Jul 9, 2020

terriko Jul 9, 2020

terriko Jul 9, 2020

terriko Jul 9, 2020

Niraj-Kamdar Jul 9, 2020

terriko Jul 9, 2020

terriko Jul 9, 2020

terriko Jul 9, 2020

Niraj-Kamdar Jul 9, 2020 •

edited

	help="provide output filename",
	help="provide output filename (default: output to stdout)",

Added Input Engine with triage functionality #777

Added Input Engine with triage functionality #777

Conversation

Niraj-Kamdar commented Jul 3, 2020 • edited

codecov-commenter commented Jul 3, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terriko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niraj-Kamdar Jul 9, 2020 • edited

Choose a reason for hiding this comment

Niraj-Kamdar commented Jul 3, 2020 •

edited

codecov-commenter commented Jul 3, 2020 •

edited

Niraj-Kamdar Jul 9, 2020 •

edited