From b9d406cc80d529e512d3184e4fa515775fb61c10 Mon Sep 17 00:00:00 2001 From: Michael Spallino Date: Mon, 6 Apr 2020 19:55:43 -0400 Subject: [PATCH] support for disabling individual tests - allow disabling tests by id and by name (e.g. B602,assert_used) - update nosec_lines to be a dict keyed on line number to a set of tests to ignore - use an empty set to indicate a blanket nosec (i.e. just #nosec, no individual tests) - use None to indicate that there was no nosec comment on the line in question - track and report metrics on the number of tests skipped by specific tests and the number of tests that fail because they were not included in the list of specific tests to ignore Resolves #211 See also #418 --- README.rst | 17 ++++++ bandit/core/manager.py | 89 +++++++++++++++++++++++++---- bandit/core/metrics.py | 26 +++++++-- bandit/core/node_visitor.py | 13 ++++- bandit/core/tester.py | 49 +++++++++++++--- bandit/formatters/text.py | 6 ++ examples/nosec.py | 7 +++ tests/functional/test_functional.py | 4 +- tests/unit/formatters/test_text.py | 8 ++- 9 files changed, 190 insertions(+), 29 deletions(-) diff --git a/README.rst b/README.rst index f0fbf8341..4bf556698 100644 --- a/README.rst +++ b/README.rst @@ -176,6 +176,23 @@ security issue, it will not be reported:: self.process = subprocess.Popen('/bin/echo', shell=True) # nosec +Because multiple issues can be reported for the same line, specific tests may +be provided to suppress those reports. This will cause other issues not +included to be reported. This can be useful in preventing situations where a +nosec comment is used, but a separate vulnerability may be added to the line +later causing the new vulnerability to be ignored. + +For example, this will suppress the report of B602 and B607:: + + self.process = subprocess.Popen('/bin/ls *', shell=True) #nosec B602, B607 + +Full test names rather than the test ID may also be used. + +For example, this will suppress the report of B101 and continue to report B506 +as an issue.:: + + assert yaml.load("{}") == [] # nosec assert_used + Vulnerability Tests ------------------- diff --git a/bandit/core/manager.py b/bandit/core/manager.py index 466670c8a..9d30f3dfb 100644 --- a/bandit/core/manager.py +++ b/bandit/core/manager.py @@ -9,6 +9,7 @@ import json import logging import os +import re import sys import tokenize import traceback @@ -23,6 +24,8 @@ LOG = logging.getLogger(__name__) +NOSEC_SPECIFIC_IDS_IGNORE = re.compile(r"([bB][\d]+),?[ ]?") +NOSEC_SPECIFIC_NAMES_IGNORE = re.compile(r"([a-z_]+),?[ ]?") class BanditManager(object): @@ -273,18 +276,20 @@ def _parse_file(self, fname, fdata, new_files_list): lines = data.splitlines() self.metrics.begin(fname) self.metrics.count_locs(lines) - if self.ignore_nosec: - nosec_lines = set() - else: - try: - fdata.seek(0) - tokens = tokenize.tokenize(fdata.readline) - nosec_lines = set( - lineno for toktype, tokval, (lineno, _), _, _ in tokens - if toktype == tokenize.COMMENT and - '#nosec' in tokval or '# nosec' in tokval) - except tokenize.TokenError: - nosec_lines = set() + # nosec_lines is a dict of line number -> set of tests to ignore + # for the line + nosec_lines = dict() + try: + fdata.seek(0) + tokens = tokenize.tokenize(fdata.readline) + + if not self.ignore_nosec: + for toktype, tokval, (lineno, _), _, _ in tokens: + if toktype == tokenize.COMMENT: + nosec_lines[lineno] = _parse_nosec_comment(tokval) + + except tokenize.TokenError: + pass score = self._execute_ast_visitor(fname, data, nosec_lines) self.scores.append(score) self.metrics.count_issues([score, ]) @@ -409,3 +414,63 @@ def _find_candidate_matches(unmatched_issues, results_list): unmatched == i]) return issue_candidates + + +def _get_tests_from_nosec_comment(comment): + nosec_no_space = '#nosec' + nosec_space = '# nosec' + nospace = comment.find(nosec_no_space) + space = comment.find(nosec_space) + if nospace > -1: + start_comment = nospace + nosec_length = len(nosec_no_space) + elif space > -1: + start_comment = space + nosec_length = len(nosec_space) + else: + # None is explicitly different to empty set, None indicates no + # nosec comment on a line + return None + + # find the next # separator + end = comment.find('#', start_comment + 1) + # if we don't find one, set end index to the length + if end == -1: + end = len(comment) + + # extract text after #[ ]?nosec and the end index, this is just the + # list of potential test ids or names + return comment[start_comment + nosec_length:end] + + +def _parse_nosec_comment(comment): + nosec_tests = _get_tests_from_nosec_comment(comment) + if nosec_tests is None: + return None + # empty set indicates that there was a nosec comment without specific + # test ids or names + test_ids = set() + if nosec_tests: + extman = extension_loader.MANAGER + # check for IDS to ignore (e.g. B602) + for test in re.finditer(NOSEC_SPECIFIC_IDS_IGNORE, + nosec_tests): + test_id_match = test.group(1) + plugin_id = extman.check_id(test_id_match) + if plugin_id: + test_ids.add(test_id_match) + else: + LOG.warning("Test in comment: %s is not a test id", + test_id_match) + # check for NAMES to ignore (e.g. assert_used) + for test in re.finditer(NOSEC_SPECIFIC_NAMES_IGNORE, + nosec_tests): + plugin_id = extman.get_plugin_id(test.group(1)) + if plugin_id: + test_ids.add(plugin_id) + else: + LOG.warning( + "Test in comment: %s is not a test name, ignoring", + test.group(1)) + + return test_ids diff --git a/bandit/core/metrics.py b/bandit/core/metrics.py index ec26e234a..98cbd4cdc 100644 --- a/bandit/core/metrics.py +++ b/bandit/core/metrics.py @@ -21,7 +21,8 @@ class Metrics(object): def __init__(self): self.data = dict() - self.data['_totals'] = {'loc': 0, 'nosec': 0} + self.data['_totals'] = {'loc': 0, 'nosec': 0, 'nosec_by_test': 0, + 'failed_nosec_by_test': 0} # initialize 0 totals for criteria and rank; this will be reset later for rank in constants.RANKING: @@ -32,21 +33,36 @@ def begin(self, fname): """Begin a new metric block. This starts a new metric collection name "fname" and makes is active. - :param fname: the metrics unique name, normally the file name. """ - self.data[fname] = {'loc': 0, 'nosec': 0} + self.data[fname] = {'loc': 0, 'nosec': 0, 'nosec_by_test': 0, + 'failed_nosec_by_test': 0} self.current = self.data[fname] def note_nosec(self, num=1): - """Note a "nosec" commnet. + """Note a "nosec" comment. Increment the currently active metrics nosec count. - :param num: number of nosecs seen, defaults to 1 """ self.current['nosec'] += num + def note_nosec_by_test(self, num=1): + """Note a "nosec BXXX, BYYY, ..." comment. + + Increment the currently active metrics nosec_by_test count. + :param num: number of nosecs seen, defaults to 1 + """ + self.current['nosec_by_test'] += num + + def note_failed_nosec_by_test(self, num=1): + """Note a test failed not caught when specific tests in comment used + + Increment the currently active metrics failed_nosec_by_test count. + :param num: number of failed nosecs seen, defaults to 1 + """ + self.current['failed_nosec_by_test'] += num + def count_locs(self, lines): """Count lines of code. diff --git a/bandit/core/node_visitor.py b/bandit/core/node_visitor.py index eba93e0d7..32bbd65f1 100644 --- a/bandit/core/node_visitor.py +++ b/bandit/core/node_visitor.py @@ -201,8 +201,10 @@ def pre_visit(self, node): if hasattr(node, 'lineno'): self.context['lineno'] = node.lineno - if node.lineno in self.nosec_lines: - LOG.debug("skipped, nosec") + # explicitly check for empty set to skip all tests for a line + nosec_tests = self.nosec_lines.get(node.lineno) + if nosec_tests is not None and not len(nosec_tests): + LOG.debug("skipped, nosec without test number") self.metrics.note_nosec() return False if hasattr(node, 'col_offset'): @@ -272,6 +274,13 @@ def update_scores(self, scores): severity, this is needed to update the stored list. :param score: The score list to update our scores with ''' + # scores has extra nosec information about nosecs with specific tests + # so pop those out first and track the metrics for them + self.metrics.note_nosec_by_test(scores.pop('nosecs_by_tests')) + self.metrics.note_failed_nosec_by_test( + scores.pop('failed_nosecs_by_test') + ) + # we'll end up with something like: # SEVERITY: {0, 0, 0, 10} where 10 is weighted by finding and level for score_type in self.scores: diff --git a/bandit/core/tester.py b/bandit/core/tester.py index fc46655a1..37aa382c7 100644 --- a/bandit/core/tester.py +++ b/bandit/core/tester.py @@ -32,13 +32,15 @@ def run_tests(self, raw_context, checktype): :param raw_context: Raw context dictionary :param checktype: The type of checks to run - :param nosec_lines: Lines which should be skipped because of nosec - :return: a score based on the number and type of test results + :return: a score based on the number and type of test results with + extra metrics about nosec comments ''' scores = { 'SEVERITY': [0] * len(constants.RANKING), - 'CONFIDENCE': [0] * len(constants.RANKING) + 'CONFIDENCE': [0] * len(constants.RANKING), + 'nosecs_by_tests': 0, + 'failed_nosecs_by_test': 0 } tests = self.testset.get_tests(checktype) @@ -53,10 +55,24 @@ def run_tests(self, raw_context, checktype): else: result = test(context) - # if we have a result, record it and update scores - if (result is not None and - result.lineno not in self.nosec_lines and - temp_context['lineno'] not in self.nosec_lines): + if result is not None: + nosec_tests_to_skip = set() + base_tests = self.nosec_lines.get(result.lineno, None) + context_tests = self.nosec_lines.get( + temp_context['lineno'], None) + + # if both are non there are were no comments + # this is explicitly different than being empty + # empty set indicates blanket nosec comment without + # individual test names or ids + if base_tests is None and context_tests is None: + nosec_tests_to_skip = None + + # combine tests from current line and context line + if base_tests is not None: + nosec_tests_to_skip.update(base_tests) + if context_tests is not None: + nosec_tests_to_skip.update(context_tests) if isinstance(temp_context['filename'], bytes): result.fname = temp_context['filename'].decode('utf-8') @@ -71,6 +87,25 @@ def run_tests(self, raw_context, checktype): if result.test_id == "": result.test_id = test._test_id + # don't skip a the test if there was no nosec comment + if nosec_tests_to_skip is not None: + # if the set is empty or the test id is in the set of + # tests to skip, log and increment the skip by test + # count + if not nosec_tests_to_skip or \ + (result.test_id in nosec_tests_to_skip): + LOG.debug("skipped, nosec for test %s" + % result.test_id) + scores['nosecs_by_tests'] += 1 + continue + # otherwise this test was not called out explicitly by + # a nosec BXX type comment and should fail. Log and + # increment the failed test count + else: + LOG.debug("uncaught test %s in nosec comment" + % result.test_id) + scores['failed_nosecs_by_test'] += 1 + self.results.append(result) LOG.debug("Issue identified by %s: %s", name, result) diff --git a/bandit/formatters/text.py b/bandit/formatters/text.py index 1c77a31a4..f8dd6d302 100644 --- a/bandit/formatters/text.py +++ b/bandit/formatters/text.py @@ -147,6 +147,12 @@ def report(manager, fileobj, sev_level, conf_level, lines=-1): bits.append('\tTotal lines skipped (#nosec): %i' % (manager.metrics.data['_totals']['nosec'])) + bits.append('\tTotal lines skipped (#nosec BXXX,BYYY,...): %i' % + (manager.metrics.data['_totals']['nosec_by_test'])) + bits.append( + '\tTotal other nosec test caught (#nosec BXXX,BYYY but BZZZ): %i' % + (manager.metrics.data['_totals']['failed_nosec_by_test']) + ) skipped = manager.get_skipped() bits.append(get_metrics(manager)) diff --git a/examples/nosec.py b/examples/nosec.py index 6bc2cc296..030ef1959 100644 --- a/examples/nosec.py +++ b/examples/nosec.py @@ -5,3 +5,10 @@ shell=True) #nosec (on the specific kwarg line) subprocess.Popen('#nosec', shell=True) subprocess.Popen('/bin/ls *', shell=True) # type: … # nosec # noqa: E501 ; pylint: disable=line-too-long +subprocess.Popen('/bin/ls *', shell=True) #nosec subprocess_popen_with_shell_equals_true (on the line) +subprocess.Popen('#nosec', shell=True) # nosec B607, B602 +subprocess.Popen('#nosec', shell=True) # nosec B607 B602 +subprocess.Popen('/bin/ls *', shell=True) # nosec subprocess_popen_with_shell_equals_true start_process_with_partial_path +subprocess.Popen('/bin/ls *', shell=True) # type: … # noqa: E501 ; pylint: disable=line-too-long # nosec +subprocess.Popen('#nosec', shell=True) # nosec B607, B101 +subprocess.Popen('#nosec', shell=True) # nosec B602, subprocess_popen_with_shell_equals_true diff --git a/tests/functional/test_functional.py b/tests/functional/test_functional.py index 173d4d67b..804aaa2a9 100644 --- a/tests/functional/test_functional.py +++ b/tests/functional/test_functional.py @@ -722,8 +722,8 @@ def test_flask_debug_true(self): def test_nosec(self): expect = { - 'SEVERITY': {'UNDEFINED': 0, 'LOW': 2, 'MEDIUM': 0, 'HIGH': 0}, - 'CONFIDENCE': {'UNDEFINED': 0, 'LOW': 0, 'MEDIUM': 0, 'HIGH': 2} + 'SEVERITY': {'UNDEFINED': 0, 'LOW': 4, 'MEDIUM': 0, 'HIGH': 0}, + 'CONFIDENCE': {'UNDEFINED': 0, 'LOW': 0, 'MEDIUM': 0, 'HIGH': 4} } self.check_example('nosec.py', expect) diff --git a/tests/unit/formatters/test_text.py b/tests/unit/formatters/test_text.py index 30a45af0e..87a237c35 100644 --- a/tests/unit/formatters/test_text.py +++ b/tests/unit/formatters/test_text.py @@ -99,7 +99,9 @@ def test_report_nobaseline(self, get_issue_list): get_issue_list.return_value = [issue_a, issue_b] - self.manager.metrics.data['_totals'] = {'loc': 1000, 'nosec': 50} + self.manager.metrics.data['_totals'] = {'loc': 1000, 'nosec': 50, + 'nosec_by_test': 0, + 'failed_nosec_by_test': 0} for category in ['SEVERITY', 'CONFIDENCE']: for level in ['UNDEFINED', 'LOW', 'MEDIUM', 'HIGH']: self.manager.metrics.data['_totals']['%s.%s' % @@ -140,6 +142,10 @@ def test_report_nobaseline(self, get_issue_list): 'High: 1', 'Total lines skipped ', '(#nosec): 50', + 'Total lines skipped ', + '(#nosec BXXX,BYYY,...): 0', + 'Total other nosec test caught ', + '(#nosec BXXX,BYYY but BZZZ): 0', 'Total issues (by severity)', 'Total issues (by confidence)', 'Files skipped (1)',