Merge pull request #4773 from RasmusWL/path-injection-improvements

Python: Path injection improvements
This commit is contained in:
yoff
2020-12-08 14:05:53 +01:00
committed by GitHub
5 changed files with 229 additions and 40 deletions

View File

@@ -0,0 +1,2 @@
lgtm,codescanning
* Added modeling of `os.path.abspath` and `os.path.realpath` for Path Injection (py/path-injection).

View File

@@ -43,7 +43,8 @@ In the second example, it appears that the user is restricted to opening a file
special characters. For example, the string <code>"../../../etc/passwd"</code> will result in the code
reading the file located at <code>"/server/static/images/../../../etc/passwd"</code>, which is the system's
password file. This file would then be sent back to the user, giving them access to all the
system's passwords.
system's passwords. Note that a user could also use an absolute path here, since the result of
<code>os.path.join("/server/static/images/", "/etc/passwd")</code> is <code>"/etc/passwd"</code>.
</p>
<p>

View File

@@ -91,7 +91,7 @@ private module Stdlib {
* For example, using `attr_name = "join"` will get all uses of `os.path.join`.
*/
private DataFlow::Node path_attr(DataFlow::TypeTracker t, string attr_name) {
attr_name in ["join", "normpath"] and
attr_name in ["join", "normpath", "realpath", "abspath"] and
(
t.start() and
result = DataFlow::importNode("os.path." + attr_name)
@@ -157,6 +157,54 @@ private module Stdlib {
}
}
/**
* A call to `os.path.abspath`.
* See https://docs.python.org/3/library/os.path.html#os.path.abspath
*/
private class OsPathAbspathCall extends Path::PathNormalization::Range, DataFlow::CfgNode {
override CallNode node;
OsPathAbspathCall() { node.getFunction() = os::path::path_attr("abspath").asCfgNode() }
DataFlow::Node getPathArg() {
result.asCfgNode() in [node.getArg(0), node.getArgByName("path")]
}
}
/** An additional taint step for calls to `os.path.abspath` */
private class OsPathAbspathCallAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
exists(OsPathAbspathCall call |
nodeTo = call and
nodeFrom = call.getPathArg()
)
}
}
/**
* A call to `os.path.realpath`.
* See https://docs.python.org/3/library/os.path.html#os.path.realpath
*/
private class OsPathRealpathCall extends Path::PathNormalization::Range, DataFlow::CfgNode {
override CallNode node;
OsPathRealpathCall() { node.getFunction() = os::path::path_attr("realpath").asCfgNode() }
DataFlow::Node getPathArg() {
result.asCfgNode() in [node.getArg(0), node.getArgByName("path")]
}
}
/** An additional taint step for calls to `os.path.realpath` */
private class OsPathRealpathCallAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
exists(OsPathRealpathCall call |
nodeTo = call and
nodeFrom = call.getPathArg()
)
}
}
/**
* A call to `os.system`.
* See https://docs.python.org/3/library/os.html#os.system

View File

@@ -1,10 +1,23 @@
edges
| path_injection.py:9:12:9:23 | ControlFlowNode for Attribute | path_injection.py:10:14:10:44 | ControlFlowNode for Attribute() |
| path_injection.py:15:12:15:23 | ControlFlowNode for Attribute | path_injection.py:16:13:16:61 | ControlFlowNode for Attribute() |
| path_injection.py:16:13:16:61 | ControlFlowNode for Attribute() | path_injection.py:17:14:17:18 | ControlFlowNode for npath |
| path_injection.py:24:12:24:23 | ControlFlowNode for Attribute | path_injection.py:25:13:25:61 | ControlFlowNode for Attribute() |
| path_injection.py:25:13:25:61 | ControlFlowNode for Attribute() | path_injection.py:28:14:28:18 | ControlFlowNode for npath |
| path_injection.py:33:12:33:23 | ControlFlowNode for Attribute | path_injection.py:34:13:34:61 | ControlFlowNode for Attribute() |
| path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() |
| path_injection.py:19:16:19:27 | ControlFlowNode for Attribute | path_injection.py:20:13:20:64 | ControlFlowNode for Attribute() |
| path_injection.py:20:13:20:64 | ControlFlowNode for Attribute() | path_injection.py:21:14:21:18 | ControlFlowNode for npath |
| path_injection.py:27:16:27:27 | ControlFlowNode for Attribute | path_injection.py:28:13:28:64 | ControlFlowNode for Attribute() |
| path_injection.py:28:13:28:64 | ControlFlowNode for Attribute() | path_injection.py:31:14:31:18 | ControlFlowNode for npath |
| path_injection.py:37:16:37:27 | ControlFlowNode for Attribute | path_injection.py:38:13:38:64 | ControlFlowNode for Attribute() |
| path_injection.py:46:16:46:27 | ControlFlowNode for Attribute | path_injection.py:47:13:47:64 | ControlFlowNode for Attribute() |
| path_injection.py:47:13:47:64 | ControlFlowNode for Attribute() | path_injection.py:48:14:48:18 | ControlFlowNode for npath |
| path_injection.py:54:16:54:27 | ControlFlowNode for Attribute | path_injection.py:55:13:55:64 | ControlFlowNode for Attribute() |
| path_injection.py:63:16:63:27 | ControlFlowNode for Attribute | path_injection.py:64:13:64:63 | ControlFlowNode for Attribute() |
| path_injection.py:64:13:64:63 | ControlFlowNode for Attribute() | path_injection.py:65:14:65:18 | ControlFlowNode for npath |
| path_injection.py:71:16:71:27 | ControlFlowNode for Attribute | path_injection.py:72:13:72:63 | ControlFlowNode for Attribute() |
| path_injection.py:78:20:78:25 | ControlFlowNode for foo_id | path_injection.py:81:14:81:17 | ControlFlowNode for path |
| path_injection.py:85:20:85:22 | ControlFlowNode for foo | path_injection.py:89:14:89:17 | ControlFlowNode for path |
| path_injection.py:94:16:94:27 | ControlFlowNode for Attribute | path_injection.py:100:14:100:17 | ControlFlowNode for path |
| path_injection.py:105:16:105:27 | ControlFlowNode for Attribute | path_injection.py:111:14:111:17 | ControlFlowNode for path |
| path_injection.py:116:16:116:27 | ControlFlowNode for Attribute | path_injection.py:119:14:119:22 | ControlFlowNode for sanitized |
| path_injection.py:125:16:125:27 | ControlFlowNode for Attribute | path_injection.py:127:30:127:51 | ControlFlowNode for Attribute() |
| path_injection.py:125:16:125:27 | ControlFlowNode for Attribute | path_injection.py:129:14:129:17 | ControlFlowNode for path |
| test.py:9:12:9:23 | ControlFlowNode for Attribute | test.py:9:12:9:39 | ControlFlowNode for Attribute() |
| test.py:9:12:9:23 | ControlFlowNode for Attribute | test.py:9:12:9:39 | ControlFlowNode for Attribute() |
| test.py:9:12:9:39 | ControlFlowNode for Attribute() | test.py:18:9:18:16 | ControlFlowNode for source() |
@@ -39,16 +52,40 @@ edges
| test_chaining.py:41:9:41:16 | ControlFlowNode for source() | test_chaining.py:42:9:42:19 | ControlFlowNode for normpath() |
| test_chaining.py:44:13:44:23 | ControlFlowNode for normpath() | test_chaining.py:45:14:45:14 | ControlFlowNode for z |
nodes
| path_injection.py:9:12:9:23 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:10:14:10:44 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:15:12:15:23 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:16:13:16:61 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:17:14:17:18 | ControlFlowNode for npath | semmle.label | ControlFlowNode for npath |
| path_injection.py:24:12:24:23 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:25:13:25:61 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:28:14:28:18 | ControlFlowNode for npath | semmle.label | ControlFlowNode for npath |
| path_injection.py:33:12:33:23 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:34:13:34:61 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:19:16:19:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:20:13:20:64 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:21:14:21:18 | ControlFlowNode for npath | semmle.label | ControlFlowNode for npath |
| path_injection.py:27:16:27:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:28:13:28:64 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:31:14:31:18 | ControlFlowNode for npath | semmle.label | ControlFlowNode for npath |
| path_injection.py:37:16:37:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:38:13:38:64 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:46:16:46:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:47:13:47:64 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:48:14:48:18 | ControlFlowNode for npath | semmle.label | ControlFlowNode for npath |
| path_injection.py:54:16:54:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:55:13:55:64 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:63:16:63:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:64:13:64:63 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:65:14:65:18 | ControlFlowNode for npath | semmle.label | ControlFlowNode for npath |
| path_injection.py:71:16:71:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:72:13:72:63 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:78:20:78:25 | ControlFlowNode for foo_id | semmle.label | ControlFlowNode for foo_id |
| path_injection.py:81:14:81:17 | ControlFlowNode for path | semmle.label | ControlFlowNode for path |
| path_injection.py:85:20:85:22 | ControlFlowNode for foo | semmle.label | ControlFlowNode for foo |
| path_injection.py:89:14:89:17 | ControlFlowNode for path | semmle.label | ControlFlowNode for path |
| path_injection.py:94:16:94:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:100:14:100:17 | ControlFlowNode for path | semmle.label | ControlFlowNode for path |
| path_injection.py:105:16:105:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:111:14:111:17 | ControlFlowNode for path | semmle.label | ControlFlowNode for path |
| path_injection.py:116:16:116:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:119:14:119:22 | ControlFlowNode for sanitized | semmle.label | ControlFlowNode for sanitized |
| path_injection.py:125:16:125:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:125:16:125:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| path_injection.py:127:30:127:51 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
| path_injection.py:129:14:129:17 | ControlFlowNode for path | semmle.label | ControlFlowNode for path |
| test.py:9:12:9:23 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| test.py:9:12:9:23 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
| test.py:9:12:9:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
@@ -84,9 +121,17 @@ nodes
| test_chaining.py:44:13:44:23 | ControlFlowNode for normpath() | semmle.label | ControlFlowNode for normpath() |
| test_chaining.py:45:14:45:14 | ControlFlowNode for z | semmle.label | ControlFlowNode for z |
#select
| path_injection.py:10:14:10:44 | ControlFlowNode for Attribute() | path_injection.py:9:12:9:23 | ControlFlowNode for Attribute | path_injection.py:10:14:10:44 | ControlFlowNode for Attribute() | This path depends on $@. | path_injection.py:9:12:9:23 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:17:14:17:18 | ControlFlowNode for npath | path_injection.py:15:12:15:23 | ControlFlowNode for Attribute | path_injection.py:17:14:17:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:15:12:15:23 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:28:14:28:18 | ControlFlowNode for npath | path_injection.py:24:12:24:23 | ControlFlowNode for Attribute | path_injection.py:28:14:28:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:24:12:24:23 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | path_injection.py:13:14:13:47 | ControlFlowNode for Attribute() | This path depends on $@. | path_injection.py:12:16:12:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:21:14:21:18 | ControlFlowNode for npath | path_injection.py:19:16:19:27 | ControlFlowNode for Attribute | path_injection.py:21:14:21:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:19:16:19:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:31:14:31:18 | ControlFlowNode for npath | path_injection.py:27:16:27:27 | ControlFlowNode for Attribute | path_injection.py:31:14:31:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:27:16:27:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:48:14:48:18 | ControlFlowNode for npath | path_injection.py:46:16:46:27 | ControlFlowNode for Attribute | path_injection.py:48:14:48:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:46:16:46:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:65:14:65:18 | ControlFlowNode for npath | path_injection.py:63:16:63:27 | ControlFlowNode for Attribute | path_injection.py:65:14:65:18 | ControlFlowNode for npath | This path depends on $@. | path_injection.py:63:16:63:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:81:14:81:17 | ControlFlowNode for path | path_injection.py:78:20:78:25 | ControlFlowNode for foo_id | path_injection.py:81:14:81:17 | ControlFlowNode for path | This path depends on $@. | path_injection.py:78:20:78:25 | ControlFlowNode for foo_id | a user-provided value |
| path_injection.py:89:14:89:17 | ControlFlowNode for path | path_injection.py:85:20:85:22 | ControlFlowNode for foo | path_injection.py:89:14:89:17 | ControlFlowNode for path | This path depends on $@. | path_injection.py:85:20:85:22 | ControlFlowNode for foo | a user-provided value |
| path_injection.py:100:14:100:17 | ControlFlowNode for path | path_injection.py:94:16:94:27 | ControlFlowNode for Attribute | path_injection.py:100:14:100:17 | ControlFlowNode for path | This path depends on $@. | path_injection.py:94:16:94:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:111:14:111:17 | ControlFlowNode for path | path_injection.py:105:16:105:27 | ControlFlowNode for Attribute | path_injection.py:111:14:111:17 | ControlFlowNode for path | This path depends on $@. | path_injection.py:105:16:105:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:119:14:119:22 | ControlFlowNode for sanitized | path_injection.py:116:16:116:27 | ControlFlowNode for Attribute | path_injection.py:119:14:119:22 | ControlFlowNode for sanitized | This path depends on $@. | path_injection.py:116:16:116:27 | ControlFlowNode for Attribute | a user-provided value |
| path_injection.py:129:14:129:17 | ControlFlowNode for path | path_injection.py:125:16:125:27 | ControlFlowNode for Attribute | path_injection.py:129:14:129:17 | ControlFlowNode for path | This path depends on $@. | path_injection.py:125:16:125:27 | ControlFlowNode for Attribute | a user-provided value |
| test.py:19:10:19:10 | ControlFlowNode for x | test.py:9:12:9:23 | ControlFlowNode for Attribute | test.py:19:10:19:10 | ControlFlowNode for x | This path depends on $@. | test.py:9:12:9:23 | ControlFlowNode for Attribute | a user-provided value |
| test.py:26:10:26:10 | ControlFlowNode for y | test.py:9:12:9:23 | ControlFlowNode for Attribute | test.py:26:10:26:10 | ControlFlowNode for y | This path depends on $@. | test.py:9:12:9:23 | ControlFlowNode for Attribute | a user-provided value |
| test.py:33:14:33:14 | ControlFlowNode for x | test.py:9:12:9:23 | ControlFlowNode for Attribute | test.py:33:14:33:14 | ControlFlowNode for x | This path depends on $@. | test.py:9:12:9:23 | ControlFlowNode for Attribute | a user-provided value |

View File

@@ -1,36 +1,129 @@
import os.path
import os
from flask import Flask, request
app = Flask(__name__)
STATIC_DIR = "/server/static/"
@app.route("/path1")
def path_injection():
path = request.args.get('path', '')
f = open(os.path.join(os.getcwd(), path))
filename = request.args.get('filename', '')
f = open(os.path.join(STATIC_DIR, filename)) # NOT OK
@app.route("/path2")
def path_injection():
# Normalized, but not checked
path = request.args.get('path', '')
npath = os.path.normpath(os.path.join(os.getcwd(), path))
f = open(npath) # Path not found
filename = request.args.get('filename', '')
npath = os.path.normpath(os.path.join(STATIC_DIR, filename))
f = open(npath) # NOT OK
SAFE = "/tmp/scratch_area/"
@app.route("/path3")
def safe_path():
# Normalized, but check doesn't reach open().
path = request.args.get('path', '')
npath = os.path.normpath(os.path.join(os.getcwd(), path))
if npath.startswith(SAFE):
def unsafe_path_normpath():
# Normalized, but `open()` is not guarded by `startswith` check
filename = request.args.get('filename', '')
npath = os.path.normpath(os.path.join(STATIC_DIR, filename))
if npath.startswith(STATIC_DIR):
pass
f = open(npath) # Path not found
f = open(npath) # NOT OK
@app.route("/path4")
def safe_path():
def safe_path_normpath():
# Normalized, and checked properly
path = request.args.get('path', '')
npath = os.path.normpath(os.path.join(os.getcwd(), path))
if npath.startswith(SAFE):
f = open(npath)
filename = request.args.get('filename', '')
npath = os.path.normpath(os.path.join(STATIC_DIR, filename))
if npath.startswith(STATIC_DIR):
f = open(npath) # OK
@app.route("/path5")
def unsafe_path_realpath():
# Normalized (by `realpath` that also follows symlinks), but not checked properly
filename = request.args.get('filename', '')
npath = os.path.realpath(os.path.join(STATIC_DIR, filename))
f = open(npath) # NOT OK
@app.route("/path6")
def safe_path_realpath():
# Normalized (by `realpath` that also follows symlinks), and checked properly
filename = request.args.get('filename', '')
npath = os.path.realpath(os.path.join(STATIC_DIR, filename))
if npath.startswith(STATIC_DIR):
f = open(npath) # OK
@app.route("/path6")
def unsafe_path_abspath():
# Normalized (by `abspath`), but not checked properly
filename = request.args.get('filename', '')
npath = os.path.abspath(os.path.join(STATIC_DIR, filename))
f = open(npath) # NOT OK
@app.route("/path7")
def safe_path_abspath():
# Normalized (by `abspath`), and checked properly
filename = request.args.get('filename', '')
npath = os.path.abspath(os.path.join(STATIC_DIR, filename))
if npath.startswith(STATIC_DIR):
f = open(npath) # OK
@app.route("/int-only/<int:foo_id>")
def flask_int_only(foo_id):
# This is OK, since the flask routing ensures that `foo_id` MUST be an integer.
path = os.path.join(STATIC_DIR, foo_id)
f = open(path) # OK TODO: FP
@app.route("/not-path/<foo>")
def flask_not_path(foo):
# On UNIX systems, this is OK, since without being marked as `<path:foo>`, flask
# routing ensures that `foo` cannot contain forward slashes (not by using %2F either).
path = os.path.join(STATIC_DIR, foo)
f = open(path) # OK if only running on UNIX systems, NOT OK if could be running on windows
@app.route("/no-dot-dot")
def no_dot_dot():
filename = request.args.get('filename', '')
path = os.path.join(STATIC_DIR, filename)
# Note: even for UNIX-only programs, this check is not good enough, since it doesn't
# handle if `filename` is an absolute path
if '../' in path:
return "not this time"
f = open(path) # NOT OK
@app.route("/no-dot-dot-with-prefix")
def no_dot_dot_with_prefix():
filename = request.args.get('filename', '')
path = os.path.join(STATIC_DIR, "img-"+filename)
# Note: Since `filename` has a prefix, it's not possible to use an absolute path.
# Therefore, for UNIX-only programs, the `../` check is enough to stop path injections.
if '../' in path:
return "not this time"
f = open(path) # OK if only running on UNIX systems, NOT OK if could be running on windows
@app.route("/replace-slash")
def replace_slash():
filename = request.args.get('filename', '')
path = os.path.join(STATIC_DIR, filename)
sanitized = path.replace("/", "_")
f = open(sanitized) # OK if only running on UNIX systems, NOT OK if could be running on windows
@app.route("/stackoverflow-solution")
def stackoverflow_solution():
# Solution provided in https://stackoverflow.com/a/45188896
filename = request.args.get('filename', '')
path = os.path.join(STATIC_DIR, filename)
if os.path.commonprefix((os.path.realpath(path), STATIC_DIR)) != STATIC_DIR:
return "not this time"
f = open(path) # OK TODO: FP