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)',