Skip to content

Commit 8c3349f

Browse files
RasmusWLRasmusWL
committed
Python: Properly model flask.send_from_directory
To not include `filename` as path-injection sink.
1 parent 228e9e9 commit 8c3349f

File tree

5 files changed

+33
-19
lines changed

5 files changed

+33
-19
lines changed

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ private import semmle.python.Concepts
1111
private import semmle.python.frameworks.Werkzeug
1212
private import semmle.python.ApiGraphs
1313
private import semmle.python.frameworks.internal.InstanceTaintStepsHelper
14+
private import semmle.python.security.dataflow.PathInjectionCustomizations
1415

1516
/**
1617
* Provides models for the `flask` PyPI package.
@@ -537,11 +538,21 @@ module Flask {
537538
// the provided directory, so is not exposed to path-injection. (but is still a
538539
// path-argument).
539540
this.getArg(1), this.getArgByName("filename")
540-
// TODO: Exclude filename as path-injection sink
541541
]
542542
}
543543
}
544544

545+
/**
546+
* To exclude `filename` argument to `flask.send_from_directory` as a path-injection sink.
547+
*/
548+
private class FlaskSendFromDirectoryCallFilenameSanitizer extends PathInjection::Sanitizer {
549+
FlaskSendFromDirectoryCallFilenameSanitizer() {
550+
this = any(FlaskSendFromDirectoryCall c).getArg(1)
551+
or
552+
this = any(FlaskSendFromDirectoryCall c).getArgByName("filename")
553+
}
554+
}
555+
545556
/**
546557
* A call to `flask.send_file`.
547558
*

‎python/ql/lib/semmle/python/security/dataflow/PathInjection.qll‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ class PathNotNormalizedConfiguration extends TaintTracking::Configuration {
2626

2727
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
2828

29-
override predicate isSanitizer(DataFlow::Node node) { node instanceof Path::PathNormalization }
29+
override predicate isSanitizer(DataFlow::Node node) {
30+
node instanceof Sanitizer
31+
or
32+
node instanceof Path::PathNormalization
33+
}
3034

3135
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
3236
guard instanceof SanitizerGuard
@@ -52,6 +56,8 @@ class FirstNormalizationConfiguration extends TaintTracking::Configuration {
5256

5357
override predicate isSink(DataFlow::Node sink) { sink instanceof Path::PathNormalization }
5458

59+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
60+
5561
override predicate isSanitizerOut(DataFlow::Node node) { node instanceof Path::PathNormalization }
5662

5763
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
@@ -67,6 +73,8 @@ class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuratio
6773

6874
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
6975

76+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
77+
7078
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
7179
guard instanceof Path::SafeAccessCheck
7280
or

‎python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ module PathInjection {
3232
*/
3333
abstract class Sink extends DataFlow::Node { }
3434

35+
/**
36+
* A sanitizer for "path injection" vulnerabilities.
37+
*
38+
* This should only be used for things like calls to library functions that perform their own
39+
* (correct) normalization/escaping of untrusted paths.
40+
*
41+
* Please also see `Path::SafeAccessCheck` and `Path::PathNormalization` Concepts.
42+
*/
43+
abstract class Sanitizer extends DataFlow::Node { }
44+
3545
/**
3646
* A sanitizer guard for "path injection" vulnerabilities.
3747
*/

‎python/ql/test/library-tests/frameworks/flask/file_sending.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
from flask import send_from_directory, send_file
22

3-
send_from_directory("filepath", "file") # $ getAPathArgument="filepath" getAPathArgument="file"
4-
send_from_directory(directory="filepath", filename="file") # $ getAPathArgument="filepath" getAPathArgument="file"
3+
send_from_directory("dir", "file") # $ getAPathArgument="dir" getAPathArgument="file"
4+
send_from_directory(directory="dir", filename="file") # $ getAPathArgument="dir" getAPathArgument="file"
55

66
send_file("file") # $ getAPathArgument="file"
77
send_file(filename_or_fp="file") # $ getAPathArgument="file"

‎python/ql/test/query-tests/Security/CWE-022-PathInjection/PathInjection.expected‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,6 @@
11
edges
2-
| flask_path_injection.py:11:16:11:22 | ControlFlowNode for request | flask_path_injection.py:11:16:11:27 | ControlFlowNode for Attribute |
3-
| flask_path_injection.py:11:16:11:27 | ControlFlowNode for Attribute | flask_path_injection.py:13:44:13:51 | ControlFlowNode for filename |
42
| flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | flask_path_injection.py:19:15:19:26 | ControlFlowNode for Attribute |
5-
| flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | flask_path_injection.py:20:16:20:22 | ControlFlowNode for request |
6-
| flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | flask_path_injection.py:20:16:20:27 | ControlFlowNode for Attribute |
73
| flask_path_injection.py:19:15:19:26 | ControlFlowNode for Attribute | flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname |
8-
| flask_path_injection.py:20:16:20:22 | ControlFlowNode for request | flask_path_injection.py:20:16:20:27 | ControlFlowNode for Attribute |
9-
| flask_path_injection.py:20:16:20:27 | ControlFlowNode for Attribute | flask_path_injection.py:21:41:21:48 | ControlFlowNode for filename |
104
| path_injection.py:12:16:12:22 | ControlFlowNode for request | path_injection.py:12:16:12:27 | ControlFlowNode for Attribute |
115
| path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() |
126
| path_injection.py:19:16:19:22 | ControlFlowNode for request | path_injection.py:19:16:19:27 | ControlFlowNode for Attribute |
@@ -76,15 +70,9 @@ edges
7670
| test_chaining.py:41:9:41:16 | ControlFlowNode for source() | test_chaining.py:42:9:42:19 | ControlFlowNode for normpath() |
7771
| test_chaining.py:44:13:44:23 | ControlFlowNode for normpath() | test_chaining.py:45:14:45:14 | ControlFlowNode for z |
7872
nodes
79-
| flask_path_injection.py:11:16:11:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
80-
| flask_path_injection.py:11:16:11:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
81-
| flask_path_injection.py:13:44:13:51 | ControlFlowNode for filename | semmle.label | ControlFlowNode for filename |
8273
| flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
8374
| flask_path_injection.py:19:15:19:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
84-
| flask_path_injection.py:20:16:20:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
85-
| flask_path_injection.py:20:16:20:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
8675
| flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname | semmle.label | ControlFlowNode for dirname |
87-
| flask_path_injection.py:21:41:21:48 | ControlFlowNode for filename | semmle.label | ControlFlowNode for filename |
8876
| path_injection.py:12:16:12:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
8977
| path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
9078
| path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
@@ -170,10 +158,7 @@ nodes
170158
| test_chaining.py:44:13:44:23 | ControlFlowNode for normpath() | semmle.label | ControlFlowNode for normpath() |
171159
| test_chaining.py:45:14:45:14 | ControlFlowNode for z | semmle.label | ControlFlowNode for z |
172160
#select
173-
| flask_path_injection.py:13:44:13:51 | ControlFlowNode for filename | flask_path_injection.py:11:16:11:22 | ControlFlowNode for request | flask_path_injection.py:13:44:13:51 | ControlFlowNode for filename | This path depends on $@. | flask_path_injection.py:11:16:11:22 | ControlFlowNode for request | a user-provided value |
174161
| flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname | flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | flask_path_injection.py:21:32:21:38 | ControlFlowNode for dirname | This path depends on $@. | flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | a user-provided value |
175-
| flask_path_injection.py:21:41:21:48 | ControlFlowNode for filename | flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | flask_path_injection.py:21:41:21:48 | ControlFlowNode for filename | This path depends on $@. | flask_path_injection.py:19:15:19:21 | ControlFlowNode for request | a user-provided value |
176-
| flask_path_injection.py:21:41:21:48 | ControlFlowNode for filename | flask_path_injection.py:20:16:20:22 | ControlFlowNode for request | flask_path_injection.py:21:41:21:48 | ControlFlowNode for filename | This path depends on $@. | flask_path_injection.py:20:16:20:22 | ControlFlowNode for request | a user-provided value |
177162
| path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | path_injection.py:12:16:12:22 | ControlFlowNode for request | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | This path depends on $@. | path_injection.py:12:16:12:22 | ControlFlowNode for request | a user-provided value |
178163
| path_injection.py:21:14:21:18 | ControlFlowNode for npath | path_injection.py:19:16:19:22 | ControlFlowNode for request | path_injection.py:21:14:21:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:19:16:19:22 | ControlFlowNode for request | a user-provided value |
179164
| path_injection.py:31:14:31:18 | ControlFlowNode for npath | path_injection.py:27:16:27:22 | ControlFlowNode for request | path_injection.py:31:14:31:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:27:16:27:22 | ControlFlowNode for request | a user-provided value |

0 commit comments

Comments
 (0)