Merge pull request #119 from python-security/113_fix_self_false_positive

Skip making self a TaintedNode

.pre-commit-config.yaml

@@ -8,4 +8,4 @@
     -   id: check-ast
     -   id: check-symlinks
     -   id: flake8
-        args: ['--ignore=E501']
+        args: ['--exclude=examples/*', '--ignore=E501,E741']

examples/example_inputs/def_with_self_as_first_arg.py

@@ -0,0 +1,4 @@
+
+
+def my_data(self, foo, bar):
+	return redirect(self.something)

pyt/framework_adaptor.py

@@ -5,7 +5,10 @@
 from .ast_helper import Arguments
 from .expr_visitor import make_cfg
 from .module_definitions import project_definitions
-from .node_types import TaintedNode
+from .node_types import (
+    AssignmentNode,
+    TaintedNode
+)
 
 
 class FrameworkAdaptor():
@@ -37,19 +40,27 @@ def get_func_cfg_with_tainted_args(self, definition):
             first_node_after_args = func_cfg.nodes[1]
             first_node_after_args.ingoing = list()
 
-            # We're just gonna give all the tainted args the lineno of the def
+            # We are just going to give all the tainted args the lineno of the def
             definition_lineno = definition.node.lineno
 
             # Taint all the arguments
-            for arg in args:
-                tainted_node = TaintedNode(arg, arg,
-                                           None, [],
-                                           line_number=definition_lineno,
-                                           path=definition.path)
-                function_entry_node.connect(tainted_node)
+            for i, arg in enumerate(args):
+                node_type = TaintedNode
+                if i == 0 and arg == 'self':
+                    node_type = AssignmentNode
+
+                arg_node = node_type(
+                    label=arg,
+                    left_hand_side=arg,
+                    ast_node=None,
+                    right_hand_side_variables=[],
+                    line_number=definition_lineno,
+                    path=definition.path
+                )
+                function_entry_node.connect(arg_node)
                 # 1 and not 0 so that Entry Node remains first in the list
-                func_cfg.nodes.insert(1, tainted_node)
-                tainted_node.connect(first_node_after_args)
+                func_cfg.nodes.insert(1, arg_node)
+                arg_node.connect(first_node_after_args)
 
         return func_cfg
 

pyt/node_types.py

@@ -176,6 +176,10 @@ def __repr__(self):
 
 
 class TaintedNode(AssignmentNode):
+    """CFG Node that represents a tainted node.
+
+    Only created in framework_adaptor.py and only used in `identify_triggers` of vulnerabilities.py
+    """
     pass
 
 

tests/vulnerabilities_test.py

@@ -15,9 +15,10 @@
 from pyt.constraint_table import initialize_constraint_table
 from pyt.fixed_point import analyse
 from pyt.framework_adaptor import FrameworkAdaptor
-from pyt.framework_helper import(
+from pyt.framework_helper import (
     is_django_view_function,
-    is_flask_route_function
+    is_flask_route_function,
+    is_function
 )
 from pyt.node_types import Node
 from pyt.reaching_definitions_taint import ReachingDefinitionsTaintAnalysis
@@ -95,17 +96,15 @@ def test_find_triggers(self):
         l = vulnerabilities.find_triggers(XSS1.nodes, trigger_words)
         self.assert_length(l, expected_length=1)
 
-
     def test_find_sanitiser_nodes(self):
         cfg_node = Node(None, None, line_number=None, path=None)
-        sanitiser_tuple  = vulnerabilities.Sanitiser('escape', cfg_node)
+        sanitiser_tuple = vulnerabilities.Sanitiser('escape', cfg_node)
         sanitiser = 'escape'
 
         result = list(vulnerabilities.find_sanitiser_nodes(sanitiser, [sanitiser_tuple]))
         self.assert_length(result, expected_length=1)
         self.assertEqual(result[0], cfg_node)
 
-
     def test_build_sanitiser_node_dict(self):
         self.cfg_create_from_file('examples/vulnerable_code/XSS_sanitised.py')
         cfg_list = [self.cfg]
@@ -114,7 +113,7 @@ def test_build_sanitiser_node_dict(self):
 
         cfg = cfg_list[1]
 
-        cfg_node = Node(None, None,  line_number=None, path=None)
+        cfg_node = Node(None, None, line_number=None, path=None)
         sinks_in_file = [vulnerabilities.TriggerNode('replace', ['escape'], cfg_node)]
 
         sanitiser_dict = vulnerabilities.build_sanitiser_node_dict(cfg, sinks_in_file)
@@ -142,7 +141,6 @@ def run_analysis(self, path):
             )
         )
 
-
     def test_find_vulnerabilities_assign_other_var(self):
         vulnerabilities = self.run_analysis('examples/vulnerable_code/XSS_assign_to_other_var.py')
         self.assert_length(vulnerabilities, expected_length=1)
@@ -555,3 +553,37 @@ def test_django_view_param(self):
                 ~call_1 = ret_render(request, 'templates/xss.html', 'param'param)
         """
         self.assertTrue(self.string_compare_alpha(vulnerability_description, EXPECTED_VULNERABILITY_DESCRIPTION))
+
+
+class EngineEveryTest(BaseTestCase):
+    def run_empty(self):
+        return
+
+    def run_analysis(self, path):
+        self.cfg_create_from_file(path)
+        cfg_list = [self.cfg]
+
+        FrameworkAdaptor(cfg_list, [], [], is_function)
+        initialize_constraint_table(cfg_list)
+
+        analyse(cfg_list, analysis_type=ReachingDefinitionsTaintAnalysis)
+
+        trigger_word_file = os.path.join(
+            'pyt',
+            'vulnerability_definitions',
+            'all_trigger_words.pyt'
+        )
+
+        return vulnerabilities.find_vulnerabilities(
+            cfg_list,
+            ReachingDefinitionsTaintAnalysis,
+            UImode.NORMAL,
+            VulnerabilityFiles(
+                default_blackbox_mapping_file,
+                trigger_word_file
+            )
+        )
+
+    def test_self_is_not_tainted(self):
+        vulnerabilities = self.run_analysis('examples/example_inputs/def_with_self_as_first_arg.py')
+        self.assert_length(vulnerabilities, expected_length=0)

tox.ini

@@ -7,5 +7,5 @@ deps = -rrequirements-dev.txt
 commands =
     coverage erase
     coverage run tests
-    coverage report --show-missing --fail-under 88
+    coverage report --show-missing --fail-under 89
     pre-commit run