Initial commit of cwe_checker plugin. #163

tbarabosch · 2018-10-09T11:50:53Z

No description provided.

jstucke · 2018-10-09T13:25:18Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+            cwe_message = cwe_message.replace('.', '').replace('32u', '')
+
+            return CweWarning(cwe_name, plugin_version, cwe_message)
+        except Exception as e:


a less general exception handling would be preferred (i.e. catching only expected exceptions like IndexError, etc.)

src/plugins/analysis/cwe_checker/test/test_cwe_checker.py

jstucke · 2018-10-09T14:03:27Z

src/plugins/analysis/cwe_checker/test/test_cwe_checker.py

@@ -0,0 +1,50 @@
+import os
+
+from common_helper_files import get_dir_of_file


imports seem to be redundant

src/plugins/analysis/cwe_checker/code/cwe_checker.py

jstucke · 2018-10-09T14:54:45Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+
+            return CweWarning(cwe_name, plugin_version, cwe_message)
+        except Exception as e:
+            logging.error('[CweWarningParser] Error while parsing CWE warning: %s.' % str(e))


according to our coding guidelines, the .format() function should be used for string formatting

jstucke · 2018-10-09T14:59:51Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+        output, return_code = execute_shell_command_get_return_code(bap_command)
+        if return_code != 0:
+            logging.error('[%s] Could not get module versions from Bap plugin: %i (%s). I tried the following command %s',
+                          self.NAME, return_code, output, bap_command)


jstucke · 2018-10-09T15:22:51Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+        arch_type = file_object.processed_analysis['file_type']['full'].lower()
+        for supported_arch in self.SUPPORTED_ARCHS:
+            if supported_arch in arch_type:
+                return True


the builtins any and all are handy for cases like this, e.g.

if any(supported_arch in arch_type for supported_arch in self.SUPPORTED_ARCHS): return True

(just a suggestion)

jstucke · 2018-10-09T15:29:28Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+            bap_command)
+        if return_code != 0:
+            logging.error('[%s] Could not communicate with Bap plugin: %i (%s).',
+                          self.NAME, return_code, output)


jstucke · 2018-10-09T15:30:30Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+        '''
+        if not self._is_supported_file_type(file_object):
+            logging.debug('[%s] %s is not an ELF executable.',
+                          self.NAME, file_object.file_path)


jstucke · 2018-10-09T15:30:39Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+        elif not self._is_supported_arch(file_object):
+            logging.debug('[%s] %s\'s arch is not supported (%s)',
+                          self.NAME, file_object.file_path,
+                          file_object.processed_analysis['cpu_architecture']['summary'])


jstucke · 2018-10-10T07:15:07Z

Also, it would be nice if you could add some more tests because the coverage is relatively low:
coverage.zip

dorpvom

Tests crash with [Errno 12] Cannot allocate memory. Please check where that is coming from.

tbarabosch · 2018-10-15T10:12:42Z

@dorpvom There were still calls to the dockerized component. I monkeypatched them. There should be no calls to external programs.
@jstucke I think I have tested the relevant components (parsers). The rest of this plugin is just wrapper code that calls cwe_checker (either locally or in Docker).

tbarabosch · 2018-10-15T10:15:08Z

I've pushed the required changes.

jstucke · 2018-10-15T13:05:56Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+        bap_command = self._build_bap_command_for_modules_versions()
+        output, return_code = execute_shell_command_get_return_code(bap_command)
+        if return_code != 0:
+            logging.error('Could not get module versions from Bap plugin: {} ({}). I tried the following command %s'.format(


forgot one %s when fixing the string format style

codecov-io · 2018-10-17T08:01:18Z

Codecov Report

Merging #163 into master will decrease coverage by 0.06%.
The diff coverage is 89.61%.

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
- Coverage   95.79%   95.72%   -0.07%     
==========================================
  Files         309      311       +2     
  Lines       15326    15480     +154     
==========================================
+ Hits        14682    14819     +137     
- Misses        644      661      +17

Impacted Files	Coverage Δ
...gins/analysis/cwe_checker/test/test_cwe_checker.py	`100% <100%> (ø)`
...c/plugins/analysis/cwe_checker/code/cwe_checker.py	`85.04% <85.04%> (ø)`
...test/integration/scheduler/test_cycle_with_tags.py	`98.11% <0%> (-1.89%)`	⬇️

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 ebe2105...678bf82. Read the comment docs.

weidenba

Please increase the test coverage.

weidenba · 2018-10-17T08:30:38Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+        return bap_command
+
+    @staticmethod
+    def _parse_bap_output(output):


This is a static method. It should be easy to test it?

weidenba · 2018-10-17T08:31:08Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+
+        return res
+
+    def _is_supported_arch(self, file_object):


This should be easy to test as well.

weidenba · 2018-10-17T08:33:55Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+        This function handles only ELF executable. Otherwise it returns an empty dictionary.
+        It calls the external BAP plugin cwe_checker.
+        '''
+        if not self._is_supported_file_type(file_object):


Please use the Whitlist functionality

weidenba · 2018-10-17T08:37:38Z

src/plugins/analysis/cwe_checker/code/cwe_checker.py

+            bap_command = '{} {} --pass=cwe-checker --cwe-checker-module_versions=true'.format(PATH_TO_BAP, '/bin/true')
+        return bap_command
+
+    def _build_bap_command(self, file_object):


This should be easy to test?

weidenba · 2018-10-17T08:39:39Z

src/plugins/analysis/cwe_checker/test/test_cwe_checker.py

+
+class MockFileObject(object):
+
+    def __init__(self, file_path):


This Mock is not used at all?

ci issue

Initial commit of cwe_checker plugin.

a3dcb31

tbarabosch assigned jstucke Oct 9, 2018

jstucke requested changes Oct 9, 2018

View reviewed changes

dorpvom previously requested changes Oct 11, 2018

View reviewed changes

Required changes for peer review (#163).

7cd91aa

string format fix

19d47e9

jstucke approved these changes Oct 15, 2018

View reviewed changes

weidenba suggested changes Oct 17, 2018

View reviewed changes

any333 and others added 4 commits October 19, 2018 22:59

test added

a2fa5f1

test added

152591c

cwe_checker now uses file type whitelist

7f53841

cwe checker tests improved

678bf82

weidenba approved these changes Oct 23, 2018

View reviewed changes

weidenba merged commit c7ba4e4 into master Oct 23, 2018

weidenba deleted the cwe_checker_plugin branch October 23, 2018 07:43

		@@ -0,0 +1,50 @@
		import os

		from common_helper_files import get_dir_of_file

Initial commit of cwe_checker plugin. #163

Initial commit of cwe_checker plugin. #163

Uh oh!

Conversation

tbarabosch commented Oct 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstucke Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstucke commented Oct 10, 2018

Uh oh!

dorpvom left a comment

Choose a reason for hiding this comment

Uh oh!

tbarabosch commented Oct 15, 2018

Uh oh!

tbarabosch commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

weidenba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jstucke Oct 9, 2018 •

edited

Loading

codecov-io commented Oct 17, 2018 •

edited

Loading