From ab13f41fa431a6423803986bb73fd80264333558 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 | 39 ++++++++++++ bandit/core/manager.py | 92 ++++++++++++++++++++++++----- bandit/core/metrics.py | 26 ++++++-- bandit/core/node_visitor.py | 12 +++- bandit/core/tester.py | 49 ++++++++++++--- bandit/formatters/text.py | 8 +++ doc/source/config.rst | 17 ++++++ examples/nosec.py | 7 +++ tests/functional/test_functional.py | 4 +- tests/unit/formatters/test_text.py | 8 ++- 10 files changed, 229 insertions(+), 33 deletions(-) diff --git a/README.rst b/README.rst index 3c51ca102..8ecc9a986 100644 --- a/README.rst +++ b/README.rst @@ -48,6 +48,45 @@ later rehomed to PyCQA. References ---------- +In the event that a line of code triggers a Bandit issue, but that the line +has been reviewed and the issue is a false positive or acceptable for some +other reason, the line can be marked with a ``# nosec`` and any results +associated with it will not be reported. + +For example, although this line may cause Bandit to report a potential +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 +------------------- +Vulnerability tests or "plugins" are defined in files in the plugins directory. + +Tests are written in Python and are autodiscovered from the plugins directory. +Each test can examine one or more type of Python statements. Tests are marked +with the types of Python statements they examine (for example: function call, +string, import, etc). + +Tests are executed by the ``BanditNodeVisitor`` object as it visits each node +in the AST. Bandit docs: https://bandit.readthedocs.io/en/latest/ diff --git a/bandit/core/manager.py b/bandit/core/manager.py index 0febb8b53..c2376a7e6 100644 --- a/bandit/core/manager.py +++ b/bandit/core/manager.py @@ -7,6 +7,7 @@ import json import logging import os +import re import sys import tokenize import traceback @@ -21,6 +22,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: @@ -308,21 +311,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 = { - 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( @@ -460,3 +462,63 @@ def _find_candidate_matches(unmatched_issues, results_list): ] 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 14d50d27b..b65644cf0 100644 --- a/bandit/core/metrics.py +++ b/bandit/core/metrics.py @@ -19,7 +19,8 @@ class Metrics: 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: @@ -30,21 +31,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 6acf587e4..33603be6d 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"): @@ -273,6 +275,12 @@ 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 322320919..4a2761434 100644 --- a/bandit/core/tester.py +++ b/bandit/core/tester.py @@ -30,13 +30,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), + "nosecs_by_tests": 0, + "failed_nosecs_by_test": 0 } tests = self.testset.get_tests(checktype) @@ -51,12 +53,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 +85,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 3e821d1d6..29c470a04 100644 --- a/bandit/formatters/text.py +++ b/bandit/formatters/text.py @@ -171,6 +171,14 @@ def report(manager, fileobj, sev_level, conf_level, lines=-1): "\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/doc/source/config.rst b/doc/source/config.rst index b3b4af5df..c28247975 100644 --- a/doc/source/config.rst +++ b/doc/source/config.rst @@ -41,6 +41,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 + ----------------- Scanning Behavior ----------------- 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 ab2ada95e..1dbc75914 100644 --- a/tests/functional/test_functional.py +++ b/tests/functional/test_functional.py @@ -747,8 +747,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 2ce80d499..97e5d87b0 100644 --- a/tests/unit/formatters/test_text.py +++ b/tests/unit/formatters/test_text.py @@ -107,7 +107,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"][f"{category}.{level}"] = 1 @@ -153,6 +155,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)",