Skip to content

Commit f8a51bb

Browse files
RasmusWLRasmusWL
committed
Python: ORM: Add data-flow steps for Django ORM
Added dummy-whitespace to `orm_security_tests.py` so it would be possible to see what the reflected XSS results are in the diff
1 parent ef39968 commit f8a51bb

File tree

5 files changed

+190
-17
lines changed

5 files changed

+190
-17
lines changed

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

Lines changed: 141 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -565,45 +565,129 @@ module PrivateDjango {
565565
* See https://docs.djangoproject.com/en/3.1/topics/db/models/.
566566
*/
567567
module Model {
568-
/** Gets a reference to the `flask.views.View` class or any subclass. */
568+
/** Gets a reference to the `django.db.models.Model` class or any subclass. */
569569
API::Node subclassRef() {
570570
result =
571571
API::moduleImport("django")
572572
.getMember("db")
573573
.getMember("models")
574574
.getMember("Model")
575575
.getASubclass*()
576+
or
577+
result =
578+
API::moduleImport("django")
579+
.getMember("db")
580+
.getMember("models")
581+
.getMember("base")
582+
.getMember("Model")
583+
.getASubclass*()
584+
}
585+
586+
/**
587+
* A source of instances of `django.db.models.Model` class or any subclass, extend this class to model new instances.
588+
*
589+
* This can include instantiations of the class, return values from function
590+
* calls, or a special parameter that will be set when functions are called by an external
591+
* library.
592+
*
593+
* Use the predicate `Model::instance()` to get references to instances of `django.db.models.Model` class or any subclass.
594+
*/
595+
abstract class InstanceSource extends DataFlow::LocalSourceNode {
596+
/** Gets the model class that this is an instance source of. */
597+
abstract API::Node getModelClass();
598+
599+
/** Holds if this instance-source is fetching data from the DB. */
600+
abstract predicate isDbFetch();
601+
}
602+
603+
/** A direct instantiation of `django.db.models.Model` class or any subclass. */
604+
private class ClassInstantiation extends InstanceSource, DataFlow::CallCfgNode {
605+
API::Node modelClass;
606+
607+
ClassInstantiation() {
608+
modelClass = subclassRef() and
609+
this = modelClass.getACall()
610+
}
611+
612+
override API::Node getModelClass() { result = modelClass }
613+
614+
override predicate isDbFetch() { none() }
615+
}
616+
617+
/** A method on a query-set or manager that returns an instance of a django model. */
618+
private class QuerySetMethod extends InstanceSource, DataFlow::CallCfgNode {
619+
API::Node modelClass;
620+
string methodName;
621+
622+
QuerySetMethod() {
623+
modelClass = subclassRef() and
624+
methodName in ["get", "create", "earliest", "latest", "first", "last"] and
625+
this = [manager(modelClass), querySet(modelClass)].getMember(methodName).getACall()
626+
}
627+
628+
override API::Node getModelClass() { result = modelClass }
629+
630+
override predicate isDbFetch() { not methodName = "create" }
631+
}
632+
633+
/**
634+
* Gets a reference to an instance of `django.db.models.Model` class or any subclass,
635+
* where `modelClass` specifies the class.
636+
*/
637+
private DataFlow::TypeTrackingNode instance(DataFlow::TypeTracker t, API::Node modelClass) {
638+
t.start() and
639+
modelClass = result.(InstanceSource).getModelClass()
640+
or
641+
exists(DataFlow::TypeTracker t2 | result = instance(t2, modelClass).track(t2, t))
642+
}
643+
644+
/**
645+
* Gets a reference to an instance of `django.db.models.Model` class or any subclass,
646+
* where `modelClass` specifies the class.
647+
*/
648+
DataFlow::Node instance(API::Node modelClass) {
649+
instance(DataFlow::TypeTracker::end(), modelClass).flowsTo(result)
576650
}
577651
}
578652

579653
/**
580-
* Gets a reference to the Manager (django.db.models.Manager) for a django Model,
581-
* accessed by `<ModelName>.objects`.
654+
* Gets a reference to the Manager (django.db.models.Manager) for the django Model `modelClass`,
655+
* accessed by `<modelClass>.objects`.
582656
*/
583-
API::Node manager() { result = Model::subclassRef().getMember("objects") }
657+
API::Node manager(API::Node modelClass) {
658+
modelClass = Model::subclassRef() and
659+
result = modelClass.getMember("objects")
660+
}
584661

585662
/**
586663
* Gets a method with `name` that returns a QuerySet.
587664
* This method can originate on a QuerySet or a Manager.
665+
* `modelClass` specifies the django Model that this query-set originates from.
588666
*
589667
* See https://docs.djangoproject.com/en/3.1/ref/models/querysets/
590668
*/
591-
API::Node querySetReturningMethod(string name) {
669+
API::Node querySetReturningMethod(API::Node modelClass, string name) {
592670
name in [
593671
"none", "all", "filter", "exclude", "complex_filter", "union", "intersection",
594672
"difference", "select_for_update", "select_related", "prefetch_related", "order_by",
595673
"distinct", "reverse", "defer", "only", "using", "annotate", "extra", "raw",
596674
"datetimes", "dates", "values", "values_list", "alias"
597675
] and
598-
result = [manager(), querySet()].getMember(name)
676+
result = [manager(modelClass), querySet(modelClass)].getMember(name)
677+
or
678+
name = "get_queryset" and
679+
result = manager(modelClass).getMember(name)
599680
}
600681

601682
/**
602683
* Gets a reference to a QuerySet (django.db.models.query.QuerySet).
684+
* `modelClass` specifies the django Model that this query-set originates from.
603685
*
604686
* See https://docs.djangoproject.com/en/3.1/ref/models/querysets/
605687
*/
606-
API::Node querySet() { result = querySetReturningMethod(_).getReturn() }
688+
API::Node querySet(API::Node modelClass) {
689+
result = querySetReturningMethod(modelClass, _).getReturn()
690+
}
607691

608692
/** Gets a reference to the `django.db.models.expressions` module. */
609693
API::Node expressions() { result = models().getMember("expressions") }
@@ -645,6 +729,52 @@ module PrivateDjango {
645729
}
646730
}
647731
}
732+
733+
/** This internal module provides data-flow modeling of Django ORM. */
734+
private module OrmDataflow {
735+
private import semmle.python.dataflow.new.internal.DataFlowPrivate::Orm
736+
737+
/** Gets the (AST) class of the Django model class `modelClass`. */
738+
Class getModelClassClass(API::Node modelClass) {
739+
result.getParent() = modelClass.getAUse().asExpr().(ClassExpr) and
740+
modelClass = Model::subclassRef()
741+
}
742+
743+
/** A synthetic node representing the data for an Django ORM model saved in a DB. */
744+
class SyntheticDjangoOrmModelNode extends SyntheticOrmModelNode {
745+
API::Node modelClass;
746+
747+
SyntheticDjangoOrmModelNode() { this.getClass() = getModelClassClass(modelClass) }
748+
749+
/** Gets the API node for this Django model class. */
750+
API::Node getModelClass() { result = modelClass }
751+
}
752+
753+
/** Additional data-flow steps for Django ORM models. */
754+
class DjangOrmSteps extends AdditionalOrmSteps {
755+
override predicate storeStep(
756+
DataFlow::Node nodeFrom, DataFlow::Content c, DataFlow::Node nodeTo
757+
) {
758+
none()
759+
}
760+
761+
override predicate jumpStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
762+
// save -> synthetic
763+
exists(API::Node modelClass, DataFlow::MethodCallNode saveCall |
764+
nodeTo.(SyntheticDjangoOrmModelNode).getModelClass() = modelClass and
765+
saveCall.calls(Model::instance(modelClass), "save") and
766+
nodeFrom = saveCall.getObject()
767+
)
768+
or
769+
// synthetic -> method-call that returns single ORM model (get/first/...)
770+
exists(API::Node modelClass |
771+
nodeTo.(Model::InstanceSource).getModelClass() = modelClass and
772+
nodeTo.(Model::InstanceSource).isDbFetch() and
773+
nodeFrom.(SyntheticDjangoOrmModelNode).getModelClass() = modelClass
774+
)
775+
}
776+
}
777+
}
648778
}
649779
}
650780

@@ -659,7 +789,7 @@ module PrivateDjango {
659789
DataFlow::Node sql;
660790

661791
ObjectsAnnotate() {
662-
this = django::db::models::querySetReturningMethod("annotate").getACall() and
792+
this = django::db::models::querySetReturningMethod(_, "annotate").getACall() and
663793
django::db::models::expressions::RawSQL::instance(sql) in [
664794
this.getArg(_), this.getArgByName(_)
665795
]
@@ -677,7 +807,7 @@ module PrivateDjango {
677807
DataFlow::Node sql;
678808

679809
ObjectsAlias() {
680-
this = django::db::models::querySetReturningMethod("alias").getACall() and
810+
this = django::db::models::querySetReturningMethod(_, "alias").getACall() and
681811
django::db::models::expressions::RawSQL::instance(sql) in [
682812
this.getArg(_), this.getArgByName(_)
683813
]
@@ -694,7 +824,7 @@ module PrivateDjango {
694824
* - https://docs.djangoproject.com/en/3.1/ref/models/querysets/#raw
695825
*/
696826
private class ObjectsRaw extends SqlExecution::Range, DataFlow::CallCfgNode {
697-
ObjectsRaw() { this = django::db::models::querySetReturningMethod("raw").getACall() }
827+
ObjectsRaw() { this = django::db::models::querySetReturningMethod(_, "raw").getACall() }
698828

699829
override DataFlow::Node getSql() { result = this.getArg(0) }
700830
}
@@ -705,7 +835,7 @@ module PrivateDjango {
705835
* See https://docs.djangoproject.com/en/3.1/ref/models/querysets/#extra
706836
*/
707837
private class ObjectsExtra extends SqlExecution::Range, DataFlow::CallCfgNode {
708-
ObjectsExtra() { this = django::db::models::querySetReturningMethod("extra").getACall() }
838+
ObjectsExtra() { this = django::db::models::querySetReturningMethod(_, "extra").getACall() }
709839

710840
override DataFlow::Node getSql() {
711841
result in [

‎python/ql/test/library-tests/frameworks/django-orm/NormalDataflowTest.ql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,4 @@
11
import python
22
import experimental.dataflow.TestUtil.NormalDataflowTest
3+
// Needs to import Django modeling to get the extra data-flow steps enabled.
4+
import semmle.python.Frameworks

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,45 @@
11
edges
2+
| 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] |
3+
| 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] |
4+
| testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | testapp/orm_security_tests.py:22:23:22:34 | ControlFlowNode for Attribute |
5+
| testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | testapp/orm_security_tests.py:23:22:23:33 | ControlFlowNode for Attribute |
6+
| testapp/orm_security_tests.py:22:9:22:14 | [post store] ControlFlowNode for person [Attribute name] | testapp/orm_security_tests.py:23:9:23:14 | ControlFlowNode for person [Attribute name] |
7+
| testapp/orm_security_tests.py:22:23:22:34 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:22:23:22:42 | ControlFlowNode for Subscript |
8+
| testapp/orm_security_tests.py:22:23:22:42 | ControlFlowNode for Subscript | testapp/orm_security_tests.py:22:9:22:14 | [post store] ControlFlowNode for person [Attribute name] |
9+
| testapp/orm_security_tests.py:23:9:23:14 | ControlFlowNode for person [Attribute name] | testapp/orm_security_tests.py:28:9:28:14 | ControlFlowNode for person [Attribute name] |
10+
| 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] |
11+
| testapp/orm_security_tests.py:23:22:23:33 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:23:22:23:40 | ControlFlowNode for Subscript |
12+
| 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] |
13+
| 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] |
14+
| 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] |
15+
| 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] |
16+
| 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 |
17+
| testapp/orm_security_tests.py:48:46:48:56 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:48:25:48:57 | ControlFlowNode for Attribute() |
18+
| testapp/orm_security_tests.py:51:14:51:53 | ControlFlowNode for Attribute() [Attribute age] | testapp/orm_security_tests.py:55:45:55:50 | ControlFlowNode for person [Attribute age] |
19+
| testapp/orm_security_tests.py:55:45:55:50 | ControlFlowNode for person [Attribute age] | testapp/orm_security_tests.py:55:45:55:54 | ControlFlowNode for Attribute |
20+
| testapp/orm_security_tests.py:55:45:55:54 | ControlFlowNode for Attribute | testapp/orm_security_tests.py:55:25:55:55 | ControlFlowNode for Attribute() |
221
nodes
22+
| testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute age] | semmle.label | [orm-model] Class Person [Attribute age] |
23+
| testapp/orm_security_tests.py:15:1:15:27 | [orm-model] Class Person [Attribute name] | semmle.label | [orm-model] Class Person [Attribute name] |
24+
| testapp/orm_security_tests.py:19:12:19:18 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
25+
| testapp/orm_security_tests.py:22:9:22:14 | [post store] ControlFlowNode for person [Attribute name] | semmle.label | [post store] ControlFlowNode for person [Attribute name] |
26+
| testapp/orm_security_tests.py:22:23:22:34 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
27+
| testapp/orm_security_tests.py:22:23:22:42 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
28+
| testapp/orm_security_tests.py:23:9:23:14 | ControlFlowNode for person [Attribute name] | semmle.label | ControlFlowNode for person [Attribute name] |
29+
| testapp/orm_security_tests.py:23:9:23:14 | [post store] ControlFlowNode for person [Attribute age] | semmle.label | [post store] ControlFlowNode for person [Attribute age] |
30+
| testapp/orm_security_tests.py:23:22:23:33 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
31+
| testapp/orm_security_tests.py:23:22:23:40 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
32+
| testapp/orm_security_tests.py:28:9:28:14 | ControlFlowNode for person [Attribute age] | semmle.label | ControlFlowNode for person [Attribute age] |
33+
| testapp/orm_security_tests.py:28:9:28:14 | ControlFlowNode for person [Attribute name] | semmle.label | ControlFlowNode for person [Attribute name] |
34+
| testapp/orm_security_tests.py:47:14:47:53 | ControlFlowNode for Attribute() [Attribute name] | semmle.label | ControlFlowNode for Attribute() [Attribute name] |
35+
| testapp/orm_security_tests.py:48:25:48:57 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
36+
| testapp/orm_security_tests.py:48:46:48:51 | ControlFlowNode for person [Attribute name] | semmle.label | ControlFlowNode for person [Attribute name] |
37+
| testapp/orm_security_tests.py:48:46:48:56 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
38+
| testapp/orm_security_tests.py:51:14:51:53 | ControlFlowNode for Attribute() [Attribute age] | semmle.label | ControlFlowNode for Attribute() [Attribute age] |
39+
| testapp/orm_security_tests.py:55:25:55:55 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
40+
| testapp/orm_security_tests.py:55:45:55:50 | ControlFlowNode for person [Attribute age] | semmle.label | ControlFlowNode for person [Attribute age] |
41+
| testapp/orm_security_tests.py:55:45:55:54 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
342
subpaths
443
#select
44+
| 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 |
45+
| 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 |

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,14 @@ def person(request):
4545

4646
def show_name(request):
4747
person = Person.objects.get(id=request.GET["id"])
48-
return HttpResponse("Name is: {}".format(person.name)) # NOT OK
48+
return HttpResponse("Name is: {}".format(person.name)) # NOT OK
4949

5050
def show_age(request):
5151
person = Person.objects.get(id=request.GET["id"])
5252
assert isinstance(person.age, int)
5353

5454
# Since the age is an integer, there is not actually XSS in the line below
55-
return HttpResponse("Age is: {}".format(person.age)) # OK
55+
return HttpResponse("Age is: {}".format(person.age)) # OK
5656

5757
# look at the log after doing
5858
"""

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ def test_save4_store():
8080

8181
def test_save4_load():
8282
obj = TestSave4.objects.first()
83-
SINK(obj.text) # $ MISSING: flow
83+
SINK(obj.text) # $ flow="SOURCE, l:-5 -> obj.text"
8484

8585
# --------------------------------------
8686
# Set attribute on existing
@@ -101,7 +101,7 @@ def test_save4b_store():
101101

102102
def test_save4b_load():
103103
obj = TestSave4b.objects.first()
104-
SINK(obj.text) # $ MISSING: flow
104+
SINK(obj.text) # $ flow="SOURCE, l:-5 -> obj.text"
105105

106106
# --------------------------------------
107107
# <Model>.objects.create()
@@ -263,7 +263,7 @@ def test_load_init():
263263

264264
def test_load_single():
265265
obj = TestLoad.objects.get(id=1)
266-
SINK(obj.text) # $ MISSING: flow
266+
SINK(obj.text) # $ flow="SOURCE, l:-5 -> obj.text"
267267

268268
def test_load_many():
269269
objs = TestLoad.objects.all()
@@ -279,7 +279,7 @@ def test_load_many_skip():
279279

280280
def test_load_qs_chain_single():
281281
obj = TestLoad.objects.all().filter(text__contains="s").exclude(text=None).first()
282-
SINK(obj.text) # $ MISSING: flow
282+
SINK(obj.text) # $ flow="SOURCE, l:-21 -> obj.text"
283283

284284
def test_load_qs_chain_many():
285285
objs = TestLoad.objects.all().filter(text__contains="s").exclude(text=None)

0 commit comments

Comments
 (0)