Python: ORM: Add flow to collection/dict queries

Author: RasmusWL

python/ql/lib/semmle/python/frameworks/Django.qll

@@ -614,7 +614,7 @@ module PrivateDjango {
             override predicate isDbFetch() { none() }
           }
 
-          /** A method on a query-set or manager that returns an instance of a django model. */
+          /** A method call on a query-set or manager that returns an instance of a django model. */
           private class QuerySetMethod extends InstanceSource, DataFlow::CallCfgNode {
             API::Node modelClass;
             string methodName;
@@ -630,6 +630,76 @@ module PrivateDjango {
             override predicate isDbFetch() { not methodName = "create" }
           }
 
+          /**
+           * A method call on a query-set or manager that returns a collection
+           * containing instances of a django models.
+           */
+          class QuerySetMethodInstanceCollection extends DataFlow::CallCfgNode {
+            API::Node modelClass;
+            string methodName;
+
+            QuerySetMethodInstanceCollection() {
+              modelClass = subclassRef() and
+              this = querySetReturningMethod(modelClass, methodName).getACall() and
+              not methodName in ["none", "datetimes", "dates", "values", "values_list"]
+              or
+              // TODO: When we have flow-summaries, we should be able to model `values` and `values_list`
+              // Potentially by doing `synthetic ===store of list element==> <Model>.objects`, and then
+              // `.all()` just keeps that content, and `.first()` will do a read step (of the list element).
+              //
+              // So for `Model.objects.filter().exclude().first()` we would have
+              // 1: <Synthetic node for Model> ===store of list element==> Model.objects
+              // 2: Model.objects ==> Model.objects.filter()
+              // 3: Model.objects.filter() ==> Model.objects.filter().exclude()
+              // 4: Model.objects.filter().exclude() ===read of list element==> Model.objects.filter().exclude().first()
+              //
+              // This also means that `.none()` could clear contents. Right now we
+              // think that the result of `Model.objects.none().all()` can contain
+              // Model objects, but it will be empty due to the `.none()` part. Not
+              // that this is important, since no-one would need to write
+              // `.none().all()` code anyway, but it would be cool to be able to model it properly :D
+              //
+              //
+              // The big benefit is for how we could then model `values`/`values_list`. For example,
+              // `Model.objects.value_list(name, description)` would result in (for the attribute description)
+              // 0: <Synthetic node for Model> -- [attr description]
+              // 1: ==> Model.objects [ListElement, attr description]
+              // 2: ==> .value_list(...) [ListElement, TupleIndex 1]
+              //
+              // but for now, we just model a store step directly from the synthetic
+              // node to the method call.
+              // extra method on query-set/manager that does _not_ return a query-set
+              modelClass = subclassRef() and
+              methodName in ["iterator", "bulk_create"] and
+              this = [manager(modelClass), querySet(modelClass)].getMember(methodName).getACall()
+            }
+
+            /** Gets the model class that this is an instance source of. */
+            API::Node getModelClass() { result = modelClass }
+
+            /** Holds if this instance-source is fetching data from the DB. */
+            predicate isDbFetch() { not methodName = "bulk_create" }
+          }
+
+          /**
+           * A method call on a query-set or manager that returns a dictionary
+           * containing instances of a django models as the values.
+           */
+          class QuerySetMethodInstanceDictValue extends DataFlow::CallCfgNode {
+            API::Node modelClass;
+
+            QuerySetMethodInstanceDictValue() {
+              modelClass = subclassRef() and
+              this = [manager(modelClass), querySet(modelClass)].getMember("in_bulk").getACall()
+            }
+
+            /** Gets the model class that this is an instance source of. */
+            API::Node getModelClass() { result = modelClass }
+
+            /** Holds if this instance-source is fetching data from the DB. */
+            predicate isDbFetch() { any() }
+          }
+
           /**
            * Gets a reference to an instance of `django.db.models.Model` class or any subclass,
            * where `modelClass` specifies the class.
@@ -765,6 +835,22 @@ module PrivateDjango {
                 c.(DataFlow::AttributeContent).getAttribute() = fieldName and
                 nodeTo = call
               )
+              or
+              // synthetic -> method-call that returns collection of ORM models (all/filter/...)
+              exists(API::Node modelClass |
+                nodeFrom.(SyntheticDjangoOrmModelNode).getModelClass() = modelClass and
+                nodeTo.(Model::QuerySetMethodInstanceCollection).getModelClass() = modelClass and
+                nodeTo.(Model::QuerySetMethodInstanceCollection).isDbFetch() and
+                c instanceof DataFlow::ListElementContent
+              )
+              or
+              // synthetic -> method-call that returns dictionary with ORM models as values
+              exists(API::Node modelClass |
+                nodeFrom.(SyntheticDjangoOrmModelNode).getModelClass() = modelClass and
+                nodeTo.(Model::QuerySetMethodInstanceDictValue).getModelClass() = modelClass and
+                nodeTo.(Model::QuerySetMethodInstanceDictValue).isDbFetch() and
+                c instanceof DataFlow::DictionaryElementAnyContent
+              )
             }
 
             override predicate jumpStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {

python/ql/test/library-tests/frameworks/django-orm/ReflectedXss.expected

@@ -1,5 +1,7 @@
 edges
+| testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute age] | testapp/orm_security_tests.py:42:23:42:42 | ControlFlowNode for Attribute() [List element, Attribute age] |
 | testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute age] | testapp/orm_security_tests.py:51:14:51:53 | ControlFlowNode for Attribute() [Attribute age] |
+| testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute name] | testapp/orm_security_tests.py:42:23:42:42 | ControlFlowNode for Attribute() [List element, Attribute name] |
 | testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute name] | testapp/orm_security_tests.py:47:14:47:53 | ControlFlowNode for Attribute() [Attribute name] |
 | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | testapp/orm_security_tests.py:22:23:22:34 | ControlFlowNode for Attribute |
 | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | testapp/orm_security_tests.py:23:22:23:33 | ControlFlowNode for Attribute |
@@ -12,6 +14,14 @@ edges
 | testapp/orm_security_tests.py:23:22:23:40 | ControlFlowNode for Subscript | testapp/orm_security_tests.py:23:9:23:14 | [post store] ControlFlowNode for person [Attribute age] |
 | testapp/orm_security_tests.py:28:9:28:14 | ControlFlowNode for person [Attribute age] | testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute age] |
 | testapp/orm_security_tests.py:28:9:28:14 | ControlFlowNode for person [Attribute name] | testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute name] |
+| testapp/orm_security_tests.py:42:13:42:18 | SSA variable person [Attribute age] | testapp/orm_security_tests.py:43:62:43:67 | ControlFlowNode for person [Attribute age] |
+| testapp/orm_security_tests.py:42:13:42:18 | SSA variable person [Attribute name] | testapp/orm_security_tests.py:43:49:43:54 | ControlFlowNode for person [Attribute name] |
+| testapp/orm_security_tests.py:42:23:42:42 | ControlFlowNode for Attribute() [List element, Attribute age] | testapp/orm_security_tests.py:42:13:42:18 | SSA variable person [Attribute age] |
+| testapp/orm_security_tests.py:42:23:42:42 | ControlFlowNode for Attribute() [List element, Attribute name] | testapp/orm_security_tests.py:42:13:42:18 | SSA variable person [Attribute name] |
+| testapp/orm_security_tests.py:43:49:43:54 | ControlFlowNode for person [Attribute name] | testapp/orm_security_tests.py:43:49:43:59 | ControlFlowNode for Attribute |
+| testapp/orm_security_tests.py:43:49:43:59 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:44:29:44:37 | ControlFlowNode for resp_text |
+| testapp/orm_security_tests.py:43:62:43:67 | ControlFlowNode for person [Attribute age] | testapp/orm_security_tests.py:43:62:43:71 | ControlFlowNode for Attribute |
+| testapp/orm_security_tests.py:43:62:43:71 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:44:29:44:37 | ControlFlowNode for resp_text |
 | testapp/orm_security_tests.py:47:14:47:53 | ControlFlowNode for Attribute() [Attribute name] | testapp/orm_security_tests.py:48:46:48:51 | ControlFlowNode for person [Attribute name] |
 | testapp/orm_security_tests.py:48:46:48:51 | ControlFlowNode for person [Attribute name] | testapp/orm_security_tests.py:48:46:48:56 | ControlFlowNode for Attribute |
 | testapp/orm_security_tests.py:48:46:48:56 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:48:25:48:57 | ControlFlowNode for Attribute() |
@@ -47,6 +57,15 @@ nodes
 | testapp/orm_security_tests.py:23:22:23:40 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
 | testapp/orm_security_tests.py:28:9:28:14 | ControlFlowNode for person [Attribute age] | semmle.label | ControlFlowNode for person [Attribute age] |
 | testapp/orm_security_tests.py:28:9:28:14 | ControlFlowNode for person [Attribute name] | semmle.label | ControlFlowNode for person [Attribute name] |
+| testapp/orm_security_tests.py:42:13:42:18 | SSA variable person [Attribute age] | semmle.label | SSA variable person [Attribute age] |
+| testapp/orm_security_tests.py:42:13:42:18 | SSA variable person [Attribute name] | semmle.label | SSA variable person [Attribute name] |
+| testapp/orm_security_tests.py:42:23:42:42 | ControlFlowNode for Attribute() [List element, Attribute age] | semmle.label | ControlFlowNode for Attribute() [List element, Attribute age] |
+| testapp/orm_security_tests.py:42:23:42:42 | ControlFlowNode for Attribute() [List element, Attribute name] | semmle.label | ControlFlowNode for Attribute() [List element, Attribute name] |
+| testapp/orm_security_tests.py:43:49:43:54 | ControlFlowNode for person [Attribute name] | semmle.label | ControlFlowNode for person [Attribute name] |
+| testapp/orm_security_tests.py:43:49:43:59 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
+| testapp/orm_security_tests.py:43:62:43:67 | ControlFlowNode for person [Attribute age] | semmle.label | ControlFlowNode for person [Attribute age] |
+| testapp/orm_security_tests.py:43:62:43:71 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
+| testapp/orm_security_tests.py:44:29:44:37 | ControlFlowNode for resp_text | semmle.label | ControlFlowNode for resp_text |
 | testapp/orm_security_tests.py:47:14:47:53 | ControlFlowNode for Attribute() [Attribute name] | semmle.label | ControlFlowNode for Attribute() [Attribute name] |
 | testapp/orm_security_tests.py:48:25:48:57 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
 | testapp/orm_security_tests.py:48:46:48:51 | ControlFlowNode for person [Attribute name] | semmle.label | ControlFlowNode for person [Attribute name] |
@@ -75,6 +94,7 @@ nodes
 | testapp/orm_security_tests.py:121:25:121:36 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
 subpaths
 #select
+| testapp/orm_security_tests.py:44:29:44:37 | ControlFlowNode for resp_text | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | testapp/orm_security_tests.py:44:29:44:37 | ControlFlowNode for resp_text | Cross-site scripting vulnerability due to $@. | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | a user-provided value |
 | testapp/orm_security_tests.py:48:25:48:57 | ControlFlowNode for Attribute() | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | testapp/orm_security_tests.py:48:25:48:57 | ControlFlowNode for Attribute() | Cross-site scripting vulnerability due to $@. | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | a user-provided value |
 | testapp/orm_security_tests.py:55:25:55:55 | ControlFlowNode for Attribute() | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | testapp/orm_security_tests.py:55:25:55:55 | ControlFlowNode for Attribute() | Cross-site scripting vulnerability due to $@. | testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | a user-provided value |
 | testapp/orm_security_tests.py:102:25:102:36 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:95:37:95:43 | ControlFlowNode for request | testapp/orm_security_tests.py:102:25:102:36 | ControlFlowNode for Attribute | Cross-site scripting vulnerability due to $@. | testapp/orm_security_tests.py:95:37:95:43 | ControlFlowNode for request | a user-provided value |

python/ql/test/library-tests/frameworks/django-orm/testapp/orm_security_tests.py

@@ -41,7 +41,7 @@ def person(request):
         resp_text = "<h1>Persons:</h1>"
         for person in Person.objects.all():
             resp_text += "\n{} (age {})".format(person.name, person.age)
-        return HttpResponse(resp_text) # NOT OK
+        return HttpResponse(resp_text)  # NOT OK
 
 def show_name(request):
     person = Person.objects.get(id=request.GET["id"])

python/ql/test/library-tests/frameworks/django-orm/testapp/orm_tests.py

@@ -307,8 +307,8 @@ def test_load_single():
 def test_load_many():
     objs = TestLoad.objects.all()
     for obj in objs:
-        SINK(obj.text) # $ MISSING: flow
-    SINK(objs[0].text) # $ MISSING: flow
+        SINK(obj.text) # $ flow="SOURCE, l:-10 -> obj.text"
+    SINK(objs[0].text) # $ flow="SOURCE, l:-11 -> objs[0].text"
 
 def test_load_many_skip():
     objs = TestLoad.objects.all()[5:]
@@ -323,8 +323,8 @@ def test_load_qs_chain_single():
 def test_load_qs_chain_many():
     objs = TestLoad.objects.all().filter(text__contains="s").exclude(text=None)
     for obj in objs:
-        SINK(obj.text) # $ MISSING: flow
-    SINK(objs[0].text) # $ MISSING: flow
+        SINK(obj.text) # $ flow="SOURCE, l:-26 -> obj.text"
+    SINK(objs[0].text) # $ flow="SOURCE, l:-27 -> objs[0].text"
 
 def test_load_values():
     # see https://docs.djangoproject.com/en/4.0/ref/models/querysets/#django.db.models.query.QuerySet.values
@@ -363,7 +363,7 @@ def test_load_in_bulk():
     d = TestLoad.objects.in_bulk([1])
     for val in d.values():
         SINK(val.text) # $ MISSING: flow
-    SINK(d[1].text) # $ MISSING: flow
+    SINK(d[1].text) # $ flow="SOURCE, l:-66 -> d[1].text"
 
 
 # Good resources: