From 68fd75ca34dd9d792cdce7f400a72acd8b473b54 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Mon, 5 Dec 2022 17:20:22 +0100 Subject: [PATCH 01/52] UnpackUnsafe query and tests --- .../Security/CWE-022bis/UnsafeUnpack.qhelp | 56 +++++++++++++++++++ .../Security/CWE-022bis/UnsafeUnpack.ql | 56 +++++++++++++++++++ .../CWE-022bis/examples/HIT_UnsafeUnpack.py | 12 ++++ .../CWE-022bis/examples/NoHIT_UnsafeUnpack.py | 17 ++++++ .../Security/CWE-022/UnsafeUnpack.expected | 10 ++++ .../Security/CWE-022/UnsafeUnpack.py | 12 ++++ .../Security/CWE-022/UnsafeUnpack.qlref | 1 + 7 files changed, 164 insertions(+) create mode 100644 python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp create mode 100644 python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql create mode 100644 python/ql/src/experimental/Security/CWE-022bis/examples/HIT_UnsafeUnpack.py create mode 100644 python/ql/src/experimental/Security/CWE-022bis/examples/NoHIT_UnsafeUnpack.py create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.qlref diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp new file mode 100644 index 000000000000..c1115e819b95 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp @@ -0,0 +1,56 @@ + + + + +

Extracting files from a malicious tarball without validating that the destination file path +is within the destination directory using `shutil.unpack_archive()` can cause files outside the +destination directory to be overwritten, due to the possible presence of directory traversal elements + (..) in archive path names.

+ +

Tarball contain archive entries representing each file in the archive. These entries +include a file path for the entry, but these file paths are not restricted and may contain +unexpected special elements such as the directory traversal element (..). If these +file paths are used to determine an output file to write the contents of the archive item to, then +the file may be written to an unexpected location. This can result in sensitive information being +revealed or deleted, or an attacker being able to influence behavior by modifying unexpected +files.

+ +

For example, if a tarball contains a file entry ../sim4n6.txt, and the tarball +is extracted to the directory /tmp/tmp123, then naively combining the paths would result +in an output file path of /tmp/tmp123/../sim4n6.txt, which would cause the file to be +written to /tmp/.

+ + + + +

Ensure that output paths constructed from tarball entries are validated +to prevent writing files to unexpected locations.

+ +

Consider using a safer module, such as: zipfile

+ + + + +

+In this example an archive is extracted without validating file paths. +

+ + + +

To fix this vulnerability, we need to call the function tarfile.extract() + on each member after verifying that it does not contain either `..` or startswith `/`. +

+ + + + + + +
  • + Shutil official documentation + shutil.unpack_archive() warning. +
    • +     +     diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql new file mode 100644 index 000000000000..eb78b13cd0ac --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -0,0 +1,56 @@ +/** + * @name Arbitrary file write during a remotely downloaded tarball extraction + * @description Extracting files from a malicious tarball using `shutil.unpack_archive()` without validating + * that the destination file path is within the destination directory can cause files outside + * the destination directory to be overwritten. More precisely, if the tarball comes from a remote location. + * @kind path-problem + * @id py/unsafe-unpacking + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @tags security + * external/cwe/cwe-022bis + */ + +import python +import experimental.semmle.python.Concepts +import DataFlow::PathGraph +import semmle.python.ApiGraphs +import semmle.python.dataflow.new.internal.Attributes +import semmle.python.dataflow.new.DataFlow +import semmle.python.ApiGraphs +import semmle.python.dataflow.new.TaintTracking +import semmle.python.Concepts + +class UnsafeUnpackingConfig extends TaintTracking::Configuration { + UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" } + + override predicate isSource(DataFlow::Node source) { + // A source coming from a remote location + exists(Http::Client::Request request | source = request) and + not source.getScope().getLocation().getFile().inStdlib() + } + + override predicate isSink(DataFlow::Node sink) { + // A sink capturing method calls to `unpack_archive`. + sink = + API::moduleImport("shutil").getMember("unpack_archive").getACall().getParameter(0).asSink() and + not sink.getScope().getLocation().getFile().inStdlib() + } + + override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { + // Writing the response data to the archive + exists(API::CallNode call, MethodCallNode w | + nodeTo = call.getArg(0) and + call = API::builtin("open").getACall() and + w.getMethodName() = "write" and + w.getObject() = call.getReturn().getAValueReachableFromSource() and + nodeFrom = w.getArg(0) + ) + } +} + +from UnsafeUnpackingConfig config, DataFlow::PathNode source, DataFlow::PathNode sink +where config.hasFlowPath(source, sink) +select source.getNode(), source, sink, "Unsafe extraction from a malicious tarball, is used in a $@", + source.getAQlClass(), "during archive unpacking." \ No newline at end of file diff --git a/python/ql/src/experimental/Security/CWE-022bis/examples/HIT_UnsafeUnpack.py b/python/ql/src/experimental/Security/CWE-022bis/examples/HIT_UnsafeUnpack.py new file mode 100644 index 000000000000..cc0f857f8c07 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022bis/examples/HIT_UnsafeUnpack.py @@ -0,0 +1,12 @@ +import requests +import shutil + +url = "https://www.someremote.location/tarball.tar.gz" +response = requests.get(url, stream=True) + +tarpath = "/tmp/tmp456/tarball.tar.gz" +with open(tarpath, "wb") as f: + f.write(response.raw.read()) + +untarredpath = "/tmp/tmp123" +shutil.unpack_archive(tarpath, untarredpath) \ No newline at end of file diff --git a/python/ql/src/experimental/Security/CWE-022bis/examples/NoHIT_UnsafeUnpack.py b/python/ql/src/experimental/Security/CWE-022bis/examples/NoHIT_UnsafeUnpack.py new file mode 100644 index 000000000000..426bcd714817 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022bis/examples/NoHIT_UnsafeUnpack.py @@ -0,0 +1,17 @@ +import requests +import tarfile + +url = "https://www.someremote.location/tarball.tar.gz" +response = requests.get(url, stream=True) + +tarpath = "/tmp/tmp456/tarball.tar.gz" +with open(tarpath, "wb") as f: + f.write(response.raw.read()) + +untarredpath = "/tmp/tmp123" +with tarfile.open(tarpath) as tar: + for member in tar.getmembers(): + if member.name.startswith("/") or ".." in member.name: + raise Exception("Path traversal identified in tarball") + + tar.extract(untarredpath, member) \ No newline at end of file diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected new file mode 100644 index 000000000000..96d25c65a30a --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -0,0 +1,10 @@ +edges +| UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | +nodes +| UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | +subpaths +#select +| UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball, is used in a $@ | PathNode | during archive unpacking. | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py new file mode 100644 index 000000000000..cc0f857f8c07 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -0,0 +1,12 @@ +import requests +import shutil + +url = "https://www.someremote.location/tarball.tar.gz" +response = requests.get(url, stream=True) + +tarpath = "/tmp/tmp456/tarball.tar.gz" +with open(tarpath, "wb") as f: + f.write(response.raw.read()) + +untarredpath = "/tmp/tmp123" +shutil.unpack_archive(tarpath, untarredpath) \ No newline at end of file diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.qlref b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.qlref new file mode 100644 index 000000000000..90e5db651a0d --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.qlref @@ -0,0 +1 @@ +experimental/Security/CWE-022bis/UnsafeUnpack.ql \ No newline at end of file From 054c06be6579ef2338f900df58539a29c8a3fc75 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Tue, 6 Dec 2022 02:51:07 +0100 Subject: [PATCH 02/52] Update UnsafeUnpack.ql --- .../Security/CWE-022bis/UnsafeUnpack.ql | 43 +++++++++++-------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index eb78b13cd0ac..c80dda11cea0 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -8,49 +8,54 @@ * @problem.severity error * @security-severity 7.5 * @precision high - * @tags security + * @tags securityimport semmle.python.dataflow.TaintTracking * external/cwe/cwe-022bis */ import python -import experimental.semmle.python.Concepts -import DataFlow::PathGraph -import semmle.python.ApiGraphs -import semmle.python.dataflow.new.internal.Attributes -import semmle.python.dataflow.new.DataFlow +import semmle.python.Concepts +import semmle.python.dataflow.new.internal.DataFlowPublic import semmle.python.ApiGraphs +import DataFlow::PathGraph import semmle.python.dataflow.new.TaintTracking -import semmle.python.Concepts class UnsafeUnpackingConfig extends TaintTracking::Configuration { UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" } override predicate isSource(DataFlow::Node source) { // A source coming from a remote location - exists(Http::Client::Request request | source = request) and - not source.getScope().getLocation().getFile().inStdlib() + exists(Http::Client::Request request | source = request) } override predicate isSink(DataFlow::Node sink) { // A sink capturing method calls to `unpack_archive`. - sink = - API::moduleImport("shutil").getMember("unpack_archive").getACall().getParameter(0).asSink() and - not sink.getScope().getLocation().getFile().inStdlib() + sink = API::moduleImport("shutil").getMember("unpack_archive").getACall().getArg(0) } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { // Writing the response data to the archive - exists(API::CallNode call, MethodCallNode w | - nodeTo = call.getArg(0) and + (exists(API::CallNode call, MethodCallNode mc, Node f | + mc.getMethodName() = "write" and + f = mc.getObject() and + nodeTo = mc.getArg(0) and call = API::builtin("open").getACall() and - w.getMethodName() = "write" and - w.getObject() = call.getReturn().getAValueReachableFromSource() and - nodeFrom = w.getArg(0) + call.flowsTo(f) and + nodeFrom = call.getArg(0) + ) + or + // Reading the response + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and + mc.getMethodName() = "read" and + nodeTo = mc ) + or + // Accessing the name + exists(AttrRead ar | ar.accesses(nodeFrom, "name") and nodeTo = ar)) } } from UnsafeUnpackingConfig config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) -select source.getNode(), source, sink, "Unsafe extraction from a malicious tarball, is used in a $@", - source.getAQlClass(), "during archive unpacking." \ No newline at end of file +select sink.getNode(), source, sink, + "Unsafe extraction from a malicious tarball retrieved from a remote location." From a5849eb9b0cd434a14dfb849adad7c7ca194ba52 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Tue, 6 Dec 2022 14:00:08 +0100 Subject: [PATCH 03/52] Improved the additional taint step using InstanceSource --- .../Security/CWE-022bis/UnsafeUnpack.ql | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index c80dda11cea0..bd3d17ff71b8 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -18,6 +18,7 @@ import semmle.python.dataflow.new.internal.DataFlowPublic import semmle.python.ApiGraphs import DataFlow::PathGraph import semmle.python.dataflow.new.TaintTracking +import semmle.python.frameworks.Stdlib class UnsafeUnpackingConfig extends TaintTracking::Configuration { UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" } @@ -34,24 +35,25 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { // Writing the response data to the archive - (exists(API::CallNode call, MethodCallNode mc, Node f | - mc.getMethodName() = "write" and - f = mc.getObject() and - nodeTo = mc.getArg(0) and - call = API::builtin("open").getACall() and - call.flowsTo(f) and - nodeFrom = call.getArg(0) + ( + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc.getMethodName() = "write" and + f = mc.getObject() and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) + ) + or + // Reading the response + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and + mc.getMethodName() = "read" and + nodeTo = mc + ) + or + // Accessing the name + exists(AttrRead ar | ar.accesses(nodeFrom, "name") and nodeTo = ar) ) - or - // Reading the response - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and - mc.getMethodName() = "read" and - nodeTo = mc - ) - or - // Accessing the name - exists(AttrRead ar | ar.accesses(nodeFrom, "name") and nodeTo = ar)) } } From c22c0b502940478b5434eab1c43895a691c22f59 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Tue, 6 Dec 2022 14:39:16 +0100 Subject: [PATCH 04/52] Update python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- .../ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp index c1115e819b95..d1fd1f4f414a 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp @@ -40,7 +40,7 @@ In this example an archive is extracted without validating file paths.

    To fix this vulnerability, we need to call the function tarfile.extract() - on each member after verifying that it does not contain either `..` or startswith `/`. + on each member after verifying that it does not contain either .. or startswith /.

    From 9a60202de62da63e842b35eddbef838c4de08ca1 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Tue, 6 Dec 2022 14:40:35 +0100 Subject: [PATCH 05/52] Update python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- .../ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp index d1fd1f4f414a..cc42d88de1d7 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp @@ -5,7 +5,7 @@

    Extracting files from a malicious tarball without validating that the destination file path -is within the destination directory using `shutil.unpack_archive()` can cause files outside the +is within the destination directory using shutil.unpack_archive() can cause files outside the destination directory to be overwritten, due to the possible presence of directory traversal elements (..) in archive path names.

    From 58570b4d2c2b09dae1558b1f6bee2c77d0ffcdbb Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Tue, 6 Dec 2022 14:40:48 +0100 Subject: [PATCH 06/52] Update python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index bd3d17ff71b8..4c57e087dd30 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -8,7 +8,7 @@ * @problem.severity error * @security-severity 7.5 * @precision high - * @tags securityimport semmle.python.dataflow.TaintTracking + * @tags security * external/cwe/cwe-022bis */ From 4896e62117c20dd2f5737a80f0e1cdd21401f8c4 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Tue, 6 Dec 2022 14:44:52 +0100 Subject: [PATCH 07/52] Use of more generic terms --- .../src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp index cc42d88de1d7..1219bbe43bc8 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp @@ -17,9 +17,9 @@ the file may be written to an unexpected location. This can result in sensitive revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.

    -

    For example, if a tarball contains a file entry ../sim4n6.txt, and the tarball +

    For example, if a tarball contains a file entry ../sneaky-file.txt, and the tarball is extracted to the directory /tmp/tmp123, then naively combining the paths would result -in an output file path of /tmp/tmp123/../sim4n6.txt, which would cause the file to be +in an output file path of /tmp/tmp123/../sneaky-file.txt, which would cause the file to be written to /tmp/.

     From 2801b8495aefcdcf7710bfcd5a5a3748c6da0145 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Tue, 6 Dec 2022 14:50:47 +0100 Subject: [PATCH 08/52] A fix of the tag name --- python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 4c57e087dd30..d550b2a4a379 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -9,7 +9,7 @@ * @security-severity 7.5 * @precision high * @tags security - * external/cwe/cwe-022bis + * external/cwe/cwe-022 */ import python From 9336f4f1a2db7d557f398a229985505870697a91 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Thu, 8 Dec 2022 12:26:59 +0100 Subject: [PATCH 09/52] Considering the use of contextlib.closing() method --- .../Security/CWE-022bis/UnsafeUnpack.ql | 11 ++++- .../Security/CWE-022/UnsafeUnpack.expected | 6 ++- .../Security/CWE-022/UnsafeUnpack.py | 46 ++++++++++++++++++- 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index d550b2a4a379..e793e6707718 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -34,8 +34,8 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - // Writing the response data to the archive ( + // Writing the response data to the archive exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | is.flowsTo(f) and mc.getMethodName() = "write" and @@ -48,11 +48,18 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { exists(MethodCallNode mc | nodeFrom = mc.getObject() and mc.getMethodName() = "read" and - nodeTo = mc + mc.flowsTo(nodeTo) ) or // Accessing the name exists(AttrRead ar | ar.accesses(nodeFrom, "name") and nodeTo = ar) + or + // Considering closing use + exists(API::Node closing | + closing = API::moduleImport("contextlib").getMember("closing") and + closing.getACall().flowsTo(nodeTo) and + nodeFrom = closing.getACall().getArg(0) + ) ) } } diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected index 96d25c65a30a..b1e93bf3ab28 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -1,10 +1,14 @@ edges | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | +| UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | nodes | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | +| UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | semmle.label | ControlFlowNode for to_path | subpaths #select -| UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball, is used in a $@ | PathNode | during archive unpacking. | +| UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index cc0f857f8c07..c7820e52b04b 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -9,4 +9,48 @@ f.write(response.raw.read()) untarredpath = "/tmp/tmp123" -shutil.unpack_archive(tarpath, untarredpath) \ No newline at end of file +shutil.unpack_archive(tarpath, untarredpath) + + +import tempfile +import os +from urllib import request +import contextlib +import shutil + +unpack = True +to_path = "/tmp/tmp123" +uri = "https://www.goog.com/zzz.tar.gz" +scheme = "https" + +with tempfile.TemporaryDirectory() as temp_dir: + if unpack and (str(uri).endswith("zip") or str(uri).endswith("tar.gz")): + unpack_path = to_path + to_path = temp_dir + else: + unpack_path = None + if scheme in ["http", "https", "ftp"]: + if os.path.isdir(to_path): + to_path = os.path.join(to_path, os.path.basename(uri)) + url = uri + url_response = request.urlopen(url) + with contextlib.closing(url_response) as fp: + with open(to_path, "wb") as out_file: + block_size = DEFAULT_BUFFER_SIZE * 8 + while True: + block = fp.read(block_size) + if not block: + break + out_file.write(block) + else: + if scheme == "oci" and not storage_options: + storage_options = default_signer() + fs = fsspec.filesystem(scheme, **storage_options) + if os.path.isdir(to_path): + to_path = os.path.join( + to_path, os.path.basename(str(uri).rstrip("/")) + ) + fs.get(uri, to_path, recursive=True) + if unpack_path: + shutil.unpack_archive(to_path, unpack_path) + to_path = unpack_path From 545aab0e07d5ee9cbfdb56b16e057db59fecd9e7 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Fri, 9 Dec 2022 15:54:43 +0100 Subject: [PATCH 10/52] tarball path provided using CLI argument (source) --- .../Security/CWE-022bis/UnsafeUnpack.ql | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index e793e6707718..33ecb3d5dc29 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -1,8 +1,9 @@ /** - * @name Arbitrary file write during a remotely downloaded tarball extraction - * @description Extracting files from a malicious tarball using `shutil.unpack_archive()` without validating + * @name Arbitrary file write during a tarball extraction from user controlled source + * @description Extracting files from a potentially malicious tarball using `shutil.unpack_archive()` without validating * that the destination file path is within the destination directory can cause files outside - * the destination directory to be overwritten. More precisely, if the tarball comes from a remote location. + * the destination directory to be overwritten. More precisely, if the tarball comes from a user controlled + * location either a remote one or cli argument. * @kind path-problem * @id py/unsafe-unpacking * @problem.severity error @@ -26,6 +27,14 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { // A source coming from a remote location exists(Http::Client::Request request | source = request) + or + // A source coming from a CLI argparse module + exists(Node o, API::Node ap, MethodCallNode args | + ap = API::moduleImport("argparse").getMember("ArgumentParser").getACall().getReturn() and + args = ap.getMember("parse_args").getACall() and + args.flowsTo(o) and + source.(AttrRead).accesses(o, any(string s)) + ) } override predicate isSink(DataFlow::Node sink) { @@ -54,7 +63,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // Accessing the name exists(AttrRead ar | ar.accesses(nodeFrom, "name") and nodeTo = ar) or - // Considering closing use + // Considering the use of closing() exists(API::Node closing | closing = API::moduleImport("contextlib").getMember("closing") and closing.getACall().flowsTo(nodeTo) and From eff132512c3bae3140c87e477d5d62c89d0a7db8 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Sat, 10 Dec 2022 08:15:42 +0100 Subject: [PATCH 11/52] Copying the response data to the archive --- .../Security/CWE-022bis/UnsafeUnpack.ql | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 33ecb3d5dc29..cb87b16432e6 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -53,6 +53,15 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeTo = is.(CallCfgNode).getArg(0) ) or + // Copying the response data to the archive + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc = API::moduleImport("shutil").getMember("copyfileobj").getACall() and + f = mc.getArg(1) and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) + ) + or // Reading the response exists(MethodCallNode mc | nodeFrom = mc.getObject() and @@ -60,8 +69,8 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { mc.flowsTo(nodeTo) ) or - // Accessing the name - exists(AttrRead ar | ar.accesses(nodeFrom, "name") and nodeTo = ar) + // Accessing the name or raw content + exists(AttrRead ar | ar.accesses(nodeFrom, ["name","raw"]) and nodeTo = ar) or // Considering the use of closing() exists(API::Node closing | From b19452467df781552790be5fc5bdcce2acacc5f7 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Sat, 10 Dec 2022 21:59:14 +0100 Subject: [PATCH 12/52] read by chunks as additional step --- .../Security/CWE-022bis/UnsafeUnpack.ql | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index cb87b16432e6..159a19b4beec 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -1,8 +1,8 @@ /** - * @name Arbitrary file write during a tarball extraction from user controlled source + * @name Arbitrary file write during a tarball extraction from a user controlled source * @description Extracting files from a potentially malicious tarball using `shutil.unpack_archive()` without validating * that the destination file path is within the destination directory can cause files outside - * the destination directory to be overwritten. More precisely, if the tarball comes from a user controlled + * the destination directory to be overwritten. More precisely, if the tarball comes from a user controlled * location either a remote one or cli argument. * @kind path-problem * @id py/unsafe-unpacking @@ -28,11 +28,11 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // A source coming from a remote location exists(Http::Client::Request request | source = request) or - // A source coming from a CLI argparse module + //A source coming from a CLI argparse module exists(Node o, API::Node ap, MethodCallNode args | ap = API::moduleImport("argparse").getMember("ArgumentParser").getACall().getReturn() and args = ap.getMember("parse_args").getACall() and - args.flowsTo(o) and + args.flowsTo(o) and source.(AttrRead).accesses(o, any(string s)) ) } @@ -57,7 +57,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | is.flowsTo(f) and mc = API::moduleImport("shutil").getMember("copyfileobj").getACall() and - f = mc.getArg(1) and + f = mc.getArg(1) and nodeFrom = mc.getArg(0) and nodeTo = is.(CallCfgNode).getArg(0) ) @@ -70,13 +70,25 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { ) or // Accessing the name or raw content - exists(AttrRead ar | ar.accesses(nodeFrom, ["name","raw"]) and nodeTo = ar) + exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) + or + //Use of join of filename + exists(API::CallNode mcn | + mcn = API::moduleImport("os").getMember("path").getMember("join").getACall() and + nodeFrom = mcn.getArg(1) and + mcn.flowsTo(nodeTo) + ) + or + // Read by chunks + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and mc.getMethodName() = "chunks" and mc.flowsTo(nodeTo) + ) or // Considering the use of closing() - exists(API::Node closing | - closing = API::moduleImport("contextlib").getMember("closing") and - closing.getACall().flowsTo(nodeTo) and - nodeFrom = closing.getACall().getArg(0) + exists(API::CallNode closing | + closing = API::moduleImport("contextlib").getMember("closing").getACall() and + closing.flowsTo(nodeTo) and + nodeFrom = closing.getArg(0) ) ) } From 2f68b54b27a6e6ba51e27351715823bde362f176 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Mon, 12 Dec 2022 19:46:34 +0100 Subject: [PATCH 13/52] A simple download_file() call from maybe boto3 --- python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 159a19b4beec..17604c004263 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -35,6 +35,9 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { args.flowsTo(o) and source.(AttrRead).accesses(o, any(string s)) ) + or + // A source catching a S3 filename download + exists(API::Node s3 | source = s3.getMember("download_file").getACall().getArg(2)) } override predicate isSink(DataFlow::Node sink) { From 54109b8ea7ce7b413f2d672b52e2ccfde8795e72 Mon Sep 17 00:00:00 2001 From: ALJI Mohamed Date: Tue, 13 Dec 2022 15:34:01 +0100 Subject: [PATCH 14/52] Add source wget.download --- .../ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 17604c004263..f8dfb78fced0 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -38,6 +38,11 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { or // A source catching a S3 filename download exists(API::Node s3 | source = s3.getMember("download_file").getACall().getArg(2)) + or + // A source download a file using wget + exists(MethodCallNode mcn | + mcn = API::moduleImport("wget").getMember("download").getACall() and source = mcn.getArg(1) + ) } override predicate isSink(DataFlow::Node sink) { From 4376870a518262146c3c2446cd2512be2038b577 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 15 Dec 2022 23:39:02 +0100 Subject: [PATCH 15/52] An uploded file is considered a source --- .../Security/CWE-022bis/UnsafeUnpack.ql | 122 +++++++++++------- 1 file changed, 75 insertions(+), 47 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index f8dfb78fced0..1ebdf48397c3 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -43,6 +43,21 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { exists(MethodCallNode mcn | mcn = API::moduleImport("wget").getMember("download").getACall() and source = mcn.getArg(1) ) + or + // catch the uploaded files as a source + exists(Subscript s, Attribute at | + at = s.getObject() and at.getAttr() = "FILES" and source.asExpr() = s + ) + or + exists(Node obj, AttrRead ar | + ar.getAMethodCall("get").flowsTo(source) and + ar.accesses(obj, "FILES") + ) + or + exists(Node obj, AttrRead ar | + ar.getAMethodCall("getlist").flowsTo(source) and + ar.accesses(obj, "FILES") + ) } override predicate isSink(DataFlow::Node sink) { @@ -51,53 +66,66 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - ( - // Writing the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc.getMethodName() = "write" and - f = mc.getObject() and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) - or - // Copying the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc = API::moduleImport("shutil").getMember("copyfileobj").getACall() and - f = mc.getArg(1) and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) - or - // Reading the response - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and - mc.getMethodName() = "read" and - mc.flowsTo(nodeTo) - ) - or - // Accessing the name or raw content - exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) - or - //Use of join of filename - exists(API::CallNode mcn | - mcn = API::moduleImport("os").getMember("path").getMember("join").getACall() and - nodeFrom = mcn.getArg(1) and - mcn.flowsTo(nodeTo) - ) - or - // Read by chunks - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and mc.getMethodName() = "chunks" and mc.flowsTo(nodeTo) - ) - or - // Considering the use of closing() - exists(API::CallNode closing | - closing = API::moduleImport("contextlib").getMember("closing").getACall() and - closing.flowsTo(nodeTo) and - nodeFrom = closing.getArg(0) - ) + // Writing the response data to the archive + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc.getMethodName() = "write" and + f = mc.getObject() and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) + ) + or + // Copying the response data to the archive + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc = API::moduleImport("shutil").getMember("copyfileobj").getACall() and + f = mc.getArg(1) and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) + ) + or + // Reading the response + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and + mc.getMethodName() = "read" and + mc.flowsTo(nodeTo) + ) + or + // Accessing the name or raw content + exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) + or + //Use of join of filename + exists(API::CallNode mcn | + mcn = API::moduleImport("os").getMember("path").getMember("join").getACall() and + nodeFrom = mcn.getArg(1) and + mcn.flowsTo(nodeTo) + ) + or + // Read by chunks + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and mc.getMethodName() = "chunks" and mc.flowsTo(nodeTo) + ) + or + // Considering the use of closing() + exists(API::CallNode closing | + closing = API::moduleImport("contextlib").getMember("closing").getACall() and + closing.flowsTo(nodeTo) and + nodeFrom = closing.getArg(0) + ) + or + // Considering the use of "fs" + exists(API::CallNode fs, MethodCallNode mcn | + fs = + API::moduleImport("django") + .getMember("core") + .getMember("files") + .getMember("storage") + .getMember("FileSystemStorage") + .getACall() and + fs.flowsTo(mcn.getObject()) and + mcn.getMethodName() = ["save", "path"] and + nodeFrom = mcn.getArg(0) and + nodeTo = mcn ) } } From 10d6ebf95b5231c09440f401521376049c2325a6 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Wed, 25 Jan 2023 19:28:05 +0100 Subject: [PATCH 16/52] Use of inline tests for dataflow queries --- .../query-tests/Security/CWE-022/DataflowQueryTest.expected | 4 ++++ .../query-tests/Security/CWE-022/DataflowQueryTest.ql | 2 ++ .../experimental/query-tests/Security/CWE-022/UnsafeUnpack.py | 4 ++-- 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected new file mode 100644 index 000000000000..34c941949244 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected @@ -0,0 +1,4 @@ +missingAnnotationOnSink +failures +| UnsafeUnpack.py:12:46:12:58 | Comment # $result=BAD | Missing result:result=BAD | +| UnsafeUnpack.py:55:53:55:65 | Comment # $result=BAD | Missing result:result=BAD | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql new file mode 100644 index 000000000000..29ccba982aac --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql @@ -0,0 +1,2 @@ +import python +import experimental.dataflow.TestUtil.DataflowQueryTest diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index c7820e52b04b..b6015bed9083 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -9,7 +9,7 @@ f.write(response.raw.read()) untarredpath = "/tmp/tmp123" -shutil.unpack_archive(tarpath, untarredpath) +shutil.unpack_archive(tarpath, untarredpath) # $result=BAD import tempfile @@ -52,5 +52,5 @@ ) fs.get(uri, to_path, recursive=True) if unpack_path: - shutil.unpack_archive(to_path, unpack_path) + shutil.unpack_archive(to_path, unpack_path) # $result=BAD to_path = unpack_path From 0ed480855a8b38dd573e29f6b8112fa4890fd4c9 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Wed, 25 Jan 2023 19:44:28 +0100 Subject: [PATCH 17/52] Update python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql Yes, definitely Co-authored-by: yoff --- python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 1ebdf48397c3..2fdbc29dc43f 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -69,8 +69,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // Writing the response data to the archive exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | is.flowsTo(f) and - mc.getMethodName() = "write" and - f = mc.getObject() and + mc.calls(f, "write") nodeFrom = mc.getArg(0) and nodeTo = is.(CallCfgNode).getArg(0) ) From 2d38993075a6a5a319b6e493b536f0647e15bf9b Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Wed, 25 Jan 2023 19:46:13 +0100 Subject: [PATCH 18/52] Add a missing "and" --- python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 2fdbc29dc43f..6cb52a489948 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -69,7 +69,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // Writing the response data to the archive exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | is.flowsTo(f) and - mc.calls(f, "write") + mc.calls(f, "write") and nodeFrom = mc.getArg(0) and nodeTo = is.(CallCfgNode).getArg(0) ) From 22af6f518272f6e9883bbbc5af3cc1b7dd25c3d1 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Wed, 25 Jan 2023 23:00:00 +0100 Subject: [PATCH 19/52] Restrict download_file() to boto3 lib --- .../Security/CWE-022bis/UnsafeUnpack.ql | 10 ++++++++-- .../query-tests/Security/CWE-022/UnsafeUnpack.py | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 6cb52a489948..965392856c56 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -36,8 +36,14 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { source.(AttrRead).accesses(o, any(string s)) ) or - // A source catching a S3 filename download - exists(API::Node s3 | source = s3.getMember("download_file").getACall().getArg(2)) + // A source catching an S3 filename download + // see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file + exists(MethodCallNode mcn, Node s3, Node bc | + bc = API::moduleImport("boto3").getMember("client").getACall() and + bc = s3.getALocalSource() and + mcn.calls(s3, "download_file") and + source = mcn.getArg(2) + ) or // A source download a file using wget exists(MethodCallNode mcn | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index b6015bed9083..13c3a32cd5a7 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -54,3 +54,17 @@ if unpack_path: shutil.unpack_archive(to_path, unpack_path) # $result=BAD to_path = unpack_path + + +# A source catching an S3 filename download +# see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file +import boto3 + +remote_ziped_name = "remote_name.tar.gz" +base_dir = "/tmp/basedir" +local_ziped_path = os.path.join(base_dir, remote_ziped_name) +bucket_name = "mybucket" + +s3 = boto3.client('s3') +s3.download_file(bucket_name, remote_ziped_name, local_ziped_path) +shutil.unpack_archive(local_ziped_path, base_dir) # $result=BAD \ No newline at end of file From 9b5b0c60b83fcf112dc0e5778d2ead8c77c86bbc Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 00:02:20 +0100 Subject: [PATCH 20/52] Handle the download of a tarball using wget pkg. --- .../Security/CWE-022bis/UnsafeUnpack.ql | 10 +++++++-- .../Security/CWE-022/UnsafeUnpack.py | 21 ++++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 965392856c56..d9c5ffd6f766 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -46,8 +46,14 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { ) or // A source download a file using wget - exists(MethodCallNode mcn | - mcn = API::moduleImport("wget").getMember("download").getACall() and source = mcn.getArg(1) + // see wget: https://pypi.org/project/wget/ + exists(API::CallNode mcn | + mcn = API::moduleImport("wget").getMember("download").getACall() and + ( + source = mcn.getArg(1) + or + source = mcn.getReturn().asSource() and not exists(Node arg | arg = mcn.getArg(1)) + ) ) or // catch the uploaded files as a source diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index 13c3a32cd5a7..8386f2770b50 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -59,6 +59,7 @@ # A source catching an S3 filename download # see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file import boto3 +import os remote_ziped_name = "remote_name.tar.gz" base_dir = "/tmp/basedir" @@ -67,4 +68,22 @@ s3 = boto3.client('s3') s3.download_file(bucket_name, remote_ziped_name, local_ziped_path) -shutil.unpack_archive(local_ziped_path, base_dir) # $result=BAD \ No newline at end of file +shutil.unpack_archive(local_ziped_path, base_dir) # $result=BAD + + +# wget +# see wget: https://pypi.org/project/wget/ +import wget +import os + +url = "https://some.remote/location/remote_name.tar.xz" +compressed_file = "/tmp/basedir/local_name.tar.xz" +base_dir = "/tmp/basedir" + +# download(url, out, bar) contains out parameter +wget.download(url, compressed_file) +shutil.unpack_archive(compressed_file, base_dir) # $result=BAD + +# download(url) returns filename +compressed_file = wget.download(url) +shutil.unpack_archive(compressed_file, base_dir) # $result=BAD From f867c9008fdd5c4d03331b581bf8594149291d30 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 00:08:54 +0100 Subject: [PATCH 21/52] Commit the expected results --- .../Security/CWE-022/DataflowQueryTest.expected | 3 +++ .../Security/CWE-022/UnsafeUnpack.expected | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected index 34c941949244..99e42e4cccb8 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected @@ -2,3 +2,6 @@ missingAnnotationOnSink failures | UnsafeUnpack.py:12:46:12:58 | Comment # $result=BAD | Missing result:result=BAD | | UnsafeUnpack.py:55:53:55:65 | Comment # $result=BAD | Missing result:result=BAD | +| UnsafeUnpack.py:71:51:71:63 | Comment # $result=BAD | Missing result:result=BAD | +| UnsafeUnpack.py:85:50:85:62 | Comment # $result=BAD | Missing result:result=BAD | +| UnsafeUnpack.py:89:50:89:62 | Comment # $result=BAD | Missing result:result=BAD | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected index b1e93bf3ab28..78214a634b9f 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -2,13 +2,25 @@ edges | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | | UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | +| UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | nodes | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | | UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | semmle.label | ControlFlowNode for to_path | +| UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | subpaths #select | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | From 2e4cb63049307e68d6228969c7fefe682effedaa Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 01:00:01 +0100 Subject: [PATCH 22/52] Optimize the Argparse filename as a source. --- .../Security/CWE-022bis/UnsafeUnpack.ql | 17 +++++++++++------ .../Security/CWE-022/UnsafeUnpack.py | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index d9c5ffd6f766..052ca3e5d983 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -28,12 +28,17 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // A source coming from a remote location exists(Http::Client::Request request | source = request) or - //A source coming from a CLI argparse module - exists(Node o, API::Node ap, MethodCallNode args | - ap = API::moduleImport("argparse").getMember("ArgumentParser").getACall().getReturn() and - args = ap.getMember("parse_args").getACall() and - args.flowsTo(o) and - source.(AttrRead).accesses(o, any(string s)) + // A source coming from a CLI argparse module + // see argparse: https://docs.python.org/3/library/argparse.html + exists(MethodCallNode args | + args = source.(AttrRead).getObject().getALocalSource() and + args = + API::moduleImport("argparse") + .getMember("ArgumentParser") + .getACall() + .getReturn() + .getMember("parse_args") + .getACall() ) or // A source catching an S3 filename download diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index 8386f2770b50..eb0ac597a1a0 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -87,3 +87,17 @@ # download(url) returns filename compressed_file = wget.download(url) shutil.unpack_archive(compressed_file, base_dir) # $result=BAD + + +# A source coming from a CLI argparse module +# see argparse: https://docs.python.org/3/library/argparse.html +import argparse + +parser = argparse.ArgumentParser(description='Process some integers.') +parser.add_argument('integers', metavar='N', type=int, nargs='+', + help='an integer for the accumulator') +parser.add_argument('filename', help='filename to be provided') + +args = parser.parse_args() +compressed_file = args.filename +shutil.unpack_archive(compressed_file, base_dir) # $result=BAD \ No newline at end of file From 9464940214d59899cc16b61cdb4eaed629010fbf Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 01:00:19 +0100 Subject: [PATCH 23/52] Add expected results for argparse source --- .../query-tests/Security/CWE-022/DataflowQueryTest.expected | 1 + .../query-tests/Security/CWE-022/UnsafeUnpack.expected | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected index 99e42e4cccb8..ae1beb23a86c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected @@ -5,3 +5,4 @@ failures | UnsafeUnpack.py:71:51:71:63 | Comment # $result=BAD | Missing result:result=BAD | | UnsafeUnpack.py:85:50:85:62 | Comment # $result=BAD | Missing result:result=BAD | | UnsafeUnpack.py:89:50:89:62 | Comment # $result=BAD | Missing result:result=BAD | +| UnsafeUnpack.py:103:50:103:62 | Comment # $result=BAD | Missing result:result=BAD | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected index 78214a634b9f..a2eff1ddd1da 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -5,6 +5,7 @@ edges | UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | | UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | | UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:102:19:102:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | nodes | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | @@ -17,6 +18,8 @@ nodes | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | | UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:102:19:102:31 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | subpaths #select | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | @@ -24,3 +27,4 @@ subpaths | UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:102:19:102:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | From aaa004061252b19ef77c6084652dbc2bd4ca487c Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 08:53:47 +0100 Subject: [PATCH 24/52] Seperate the dataflow config from the query --- .../Security/CWE-022bis/UnsafeUnpack.ql | 132 +---------------- .../Security/CWE-022bis/UnsafeUnpackQuery.qll | 137 ++++++++++++++++++ .../CWE-022/DataflowQueryTest.expected | 6 - .../Security/CWE-022/DataflowQueryTest.ql | 1 + 4 files changed, 140 insertions(+), 136 deletions(-) create mode 100644 python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 052ca3e5d983..1e17cb728cab 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -10,141 +10,13 @@ * @security-severity 7.5 * @precision high * @tags security + * experimental * external/cwe/cwe-022 */ import python -import semmle.python.Concepts -import semmle.python.dataflow.new.internal.DataFlowPublic -import semmle.python.ApiGraphs +import UnsafeUnpackQuery import DataFlow::PathGraph -import semmle.python.dataflow.new.TaintTracking -import semmle.python.frameworks.Stdlib - -class UnsafeUnpackingConfig extends TaintTracking::Configuration { - UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" } - - override predicate isSource(DataFlow::Node source) { - // A source coming from a remote location - exists(Http::Client::Request request | source = request) - or - // A source coming from a CLI argparse module - // see argparse: https://docs.python.org/3/library/argparse.html - exists(MethodCallNode args | - args = source.(AttrRead).getObject().getALocalSource() and - args = - API::moduleImport("argparse") - .getMember("ArgumentParser") - .getACall() - .getReturn() - .getMember("parse_args") - .getACall() - ) - or - // A source catching an S3 filename download - // see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file - exists(MethodCallNode mcn, Node s3, Node bc | - bc = API::moduleImport("boto3").getMember("client").getACall() and - bc = s3.getALocalSource() and - mcn.calls(s3, "download_file") and - source = mcn.getArg(2) - ) - or - // A source download a file using wget - // see wget: https://pypi.org/project/wget/ - exists(API::CallNode mcn | - mcn = API::moduleImport("wget").getMember("download").getACall() and - ( - source = mcn.getArg(1) - or - source = mcn.getReturn().asSource() and not exists(Node arg | arg = mcn.getArg(1)) - ) - ) - or - // catch the uploaded files as a source - exists(Subscript s, Attribute at | - at = s.getObject() and at.getAttr() = "FILES" and source.asExpr() = s - ) - or - exists(Node obj, AttrRead ar | - ar.getAMethodCall("get").flowsTo(source) and - ar.accesses(obj, "FILES") - ) - or - exists(Node obj, AttrRead ar | - ar.getAMethodCall("getlist").flowsTo(source) and - ar.accesses(obj, "FILES") - ) - } - - override predicate isSink(DataFlow::Node sink) { - // A sink capturing method calls to `unpack_archive`. - sink = API::moduleImport("shutil").getMember("unpack_archive").getACall().getArg(0) - } - - override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - // Writing the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc.calls(f, "write") and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) - or - // Copying the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc = API::moduleImport("shutil").getMember("copyfileobj").getACall() and - f = mc.getArg(1) and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) - or - // Reading the response - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and - mc.getMethodName() = "read" and - mc.flowsTo(nodeTo) - ) - or - // Accessing the name or raw content - exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) - or - //Use of join of filename - exists(API::CallNode mcn | - mcn = API::moduleImport("os").getMember("path").getMember("join").getACall() and - nodeFrom = mcn.getArg(1) and - mcn.flowsTo(nodeTo) - ) - or - // Read by chunks - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and mc.getMethodName() = "chunks" and mc.flowsTo(nodeTo) - ) - or - // Considering the use of closing() - exists(API::CallNode closing | - closing = API::moduleImport("contextlib").getMember("closing").getACall() and - closing.flowsTo(nodeTo) and - nodeFrom = closing.getArg(0) - ) - or - // Considering the use of "fs" - exists(API::CallNode fs, MethodCallNode mcn | - fs = - API::moduleImport("django") - .getMember("core") - .getMember("files") - .getMember("storage") - .getMember("FileSystemStorage") - .getACall() and - fs.flowsTo(mcn.getObject()) and - mcn.getMethodName() = ["save", "path"] and - nodeFrom = mcn.getArg(0) and - nodeTo = mcn - ) - } -} from UnsafeUnpackingConfig config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll new file mode 100644 index 000000000000..5794d9e32f2e --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll @@ -0,0 +1,137 @@ +/** + * + * Provides a taint-tracking configuration for detecting "UnsafeUnpacking" vulnerabilities. + * + */ + +import python +import semmle.python.Concepts +import semmle.python.dataflow.new.internal.DataFlowPublic +import semmle.python.ApiGraphs +import semmle.python.dataflow.new.TaintTracking +import semmle.python.frameworks.Stdlib + +class UnsafeUnpackingConfig extends TaintTracking::Configuration { + UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" } + + override predicate isSource(DataFlow::Node source) { + // A source coming from a remote location + exists(Http::Client::Request request | source = request) + or + // A source coming from a CLI argparse module + // see argparse: https://docs.python.org/3/library/argparse.html + exists(MethodCallNode args | + args = source.(AttrRead).getObject().getALocalSource() and + args = + API::moduleImport("argparse") + .getMember("ArgumentParser") + .getACall() + .getReturn() + .getMember("parse_args") + .getACall() + ) + or + // A source catching an S3 filename download + // see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file + exists(MethodCallNode mcn, Node s3, Node bc | + bc = API::moduleImport("boto3").getMember("client").getACall() and + bc = s3.getALocalSource() and + mcn.calls(s3, "download_file") and + source = mcn.getArg(2) + ) + or + // A source download a file using wget + // see wget: https://pypi.org/project/wget/ + exists(API::CallNode mcn | + mcn = API::moduleImport("wget").getMember("download").getACall() and + ( + source = mcn.getArg(1) + or + source = mcn.getReturn().asSource() and not exists(Node arg | arg = mcn.getArg(1)) + ) + ) + or + // catch the uploaded files as a source + exists(Subscript s, Attribute at | + at = s.getObject() and at.getAttr() = "FILES" and source.asExpr() = s + ) + or + exists(Node obj, AttrRead ar | + ar.getAMethodCall("get").flowsTo(source) and + ar.accesses(obj, "FILES") + ) + or + exists(Node obj, AttrRead ar | + ar.getAMethodCall("getlist").flowsTo(source) and + ar.accesses(obj, "FILES") + ) + } + + override predicate isSink(DataFlow::Node sink) { + // A sink capturing method calls to `unpack_archive`. + sink = API::moduleImport("shutil").getMember("unpack_archive").getACall().getArg(0) + } + + override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { + // Writing the response data to the archive + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc.calls(f, "write") and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) + ) + or + // Copying the response data to the archive + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc = API::moduleImport("shutil").getMember("copyfileobj").getACall() and + f = mc.getArg(1) and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) + ) + or + // Reading the response + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and + mc.getMethodName() = "read" and + mc.flowsTo(nodeTo) + ) + or + // Accessing the name or raw content + exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) + or + //Use of join of filename + exists(API::CallNode mcn | + mcn = API::moduleImport("os").getMember("path").getMember("join").getACall() and + nodeFrom = mcn.getArg(1) and + mcn.flowsTo(nodeTo) + ) + or + // Read by chunks + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and mc.getMethodName() = "chunks" and mc.flowsTo(nodeTo) + ) + or + // Considering the use of closing() + exists(API::CallNode closing | + closing = API::moduleImport("contextlib").getMember("closing").getACall() and + closing.flowsTo(nodeTo) and + nodeFrom = closing.getArg(0) + ) + or + // Considering the use of "fs" + exists(API::CallNode fs, MethodCallNode mcn | + fs = + API::moduleImport("django") + .getMember("core") + .getMember("files") + .getMember("storage") + .getMember("FileSystemStorage") + .getACall() and + fs.flowsTo(mcn.getObject()) and + mcn.getMethodName() = ["save", "path"] and + nodeFrom = mcn.getArg(0) and + nodeTo = mcn + ) + } +} diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected index ae1beb23a86c..3875da4e143c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected @@ -1,8 +1,2 @@ missingAnnotationOnSink failures -| UnsafeUnpack.py:12:46:12:58 | Comment # $result=BAD | Missing result:result=BAD | -| UnsafeUnpack.py:55:53:55:65 | Comment # $result=BAD | Missing result:result=BAD | -| UnsafeUnpack.py:71:51:71:63 | Comment # $result=BAD | Missing result:result=BAD | -| UnsafeUnpack.py:85:50:85:62 | Comment # $result=BAD | Missing result:result=BAD | -| UnsafeUnpack.py:89:50:89:62 | Comment # $result=BAD | Missing result:result=BAD | -| UnsafeUnpack.py:103:50:103:62 | Comment # $result=BAD | Missing result:result=BAD | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql index 29ccba982aac..e3cd5f784ff3 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql @@ -1,2 +1,3 @@ import python import experimental.dataflow.TestUtil.DataflowQueryTest +import UnsafeUnpackQuery \ No newline at end of file From 54cc4d6498d90a5b711a9eead2dca298ae860b64 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 12:51:55 +0100 Subject: [PATCH 25/52] Opt for any source from RemoteFlowSource. --- .../Security/CWE-022bis/UnsafeUnpackQuery.qll | 5 +- .../Security/CWE-022/UnsafeUnpack.expected | 62 +++++++----- .../Security/CWE-022/UnsafeUnpack.py | 97 ++++++++----------- 3 files changed, 79 insertions(+), 85 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll index 5794d9e32f2e..c041b710ae0d 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll @@ -1,7 +1,5 @@ /** - * * Provides a taint-tracking configuration for detecting "UnsafeUnpacking" vulnerabilities. - * */ import python @@ -10,13 +8,14 @@ import semmle.python.dataflow.new.internal.DataFlowPublic import semmle.python.ApiGraphs import semmle.python.dataflow.new.TaintTracking import semmle.python.frameworks.Stdlib +import semmle.python.dataflow.new.RemoteFlowSources class UnsafeUnpackingConfig extends TaintTracking::Configuration { UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" } override predicate isSource(DataFlow::Node source) { // A source coming from a remote location - exists(Http::Client::Request request | source = request) + source instanceof RemoteFlowSource or // A source coming from a CLI argparse module // see argparse: https://docs.python.org/3/library/argparse.html diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected index a2eff1ddd1da..cda42c13267c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -1,30 +1,40 @@ edges -| UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | -| UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | -| UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | -| UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | -| UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:102:19:102:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | UnsafeUnpack.py:11:16:11:22 | ControlFlowNode for request | +| UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | +| UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | +| UnsafeUnpack.py:11:16:11:22 | ControlFlowNode for request | UnsafeUnpack.py:11:16:11:27 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:11:16:11:27 | ControlFlowNode for Attribute | UnsafeUnpack.py:17:23:17:34 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:17:23:17:34 | ControlFlowNode for Attribute | UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | +| UnsafeUnpack.py:34:50:34:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:48:20:48:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:52:19:52:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:66:19:66:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:80:16:80:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:86:15:86:26 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:86:15:86:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | nodes -| UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| UnsafeUnpack.py:9:15:9:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | -| UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | -| UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | semmle.label | ControlFlowNode for to_path | -| UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | -| UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | -| UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:102:19:102:31 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | -| UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | semmle.label | ModuleVariableNode for UnsafeUnpack.request | +| UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember | +| UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | semmle.label | GSSA Variable request | +| UnsafeUnpack.py:11:16:11:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| UnsafeUnpack.py:11:16:11:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:17:23:17:34 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | +| UnsafeUnpack.py:34:50:34:65 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:48:20:48:34 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:52:19:52:36 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:66:19:66:31 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:80:16:80:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:86:15:86:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | subpaths #select -| UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:12:5:41 | ControlFlowNode for Attribute() | UnsafeUnpack.py:12:23:12:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | UnsafeUnpack.py:36:24:36:43 | ControlFlowNode for Attribute() | UnsafeUnpack.py:55:31:55:37 | ControlFlowNode for to_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:70:50:70:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:71:23:71:38 | ControlFlowNode for local_ziped_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:84:20:84:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:85:23:85:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:88:19:88:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:89:23:89:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:102:19:102:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:103:23:103:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:34:50:34:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:48:20:48:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:52:19:52:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:66:19:66:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | UnsafeUnpack.py:80:16:80:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index eb0ac597a1a0..b2d23f2dbe01 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -1,60 +1,24 @@ import requests import shutil - -url = "https://www.someremote.location/tarball.tar.gz" -response = requests.get(url, stream=True) - -tarpath = "/tmp/tmp456/tarball.tar.gz" -with open(tarpath, "wb") as f: - f.write(response.raw.read()) - -untarredpath = "/tmp/tmp123" -shutil.unpack_archive(tarpath, untarredpath) # $result=BAD - - -import tempfile -import os -from urllib import request -import contextlib -import shutil - -unpack = True -to_path = "/tmp/tmp123" -uri = "https://www.goog.com/zzz.tar.gz" -scheme = "https" - -with tempfile.TemporaryDirectory() as temp_dir: - if unpack and (str(uri).endswith("zip") or str(uri).endswith("tar.gz")): - unpack_path = to_path - to_path = temp_dir - else: - unpack_path = None - if scheme in ["http", "https", "ftp"]: - if os.path.isdir(to_path): - to_path = os.path.join(to_path, os.path.basename(uri)) - url = uri - url_response = request.urlopen(url) - with contextlib.closing(url_response) as fp: - with open(to_path, "wb") as out_file: - block_size = DEFAULT_BUFFER_SIZE * 8 - while True: - block = fp.read(block_size) - if not block: - break - out_file.write(block) - else: - if scheme == "oci" and not storage_options: - storage_options = default_signer() - fs = fsspec.filesystem(scheme, **storage_options) - if os.path.isdir(to_path): - to_path = os.path.join( - to_path, os.path.basename(str(uri).rstrip("/")) - ) - fs.get(uri, to_path, recursive=True) - if unpack_path: - shutil.unpack_archive(to_path, unpack_path) # $result=BAD - to_path = unpack_path - +import os + +from flask import Flask, request +app = Flask(__name__) + +# Consider any RemoteFlowSource as a source +@app.route("/download_from_url") +def download_from_url(): + filename = request.args.get('filename', '') + if not filename: + response = requests.get(filename, stream=True) + + tarpath = "/tmp/tmp456/tarball.tar.gz" + with open(tarpath, "wb") as f: + f.write(response.raw.read()) + + untarredpath = "/tmp/tmp123" + shutil.unpack_archive(tarpath, untarredpath) # $result=BAD + # A source catching an S3 filename download # see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file @@ -100,4 +64,25 @@ args = parser.parse_args() compressed_file = args.filename -shutil.unpack_archive(compressed_file, base_dir) # $result=BAD \ No newline at end of file +shutil.unpack_archive(compressed_file, base_dir) # $result=BAD + + +# A source coming from a CLI and downloaded +import argparse +import requests + +parser = argparse.ArgumentParser(description='Process some integers.') +parser.add_argument('integers', metavar='N', type=int, nargs='+', + help='an integer for the accumulator') +parser.add_argument('filename', help='url to filename to be provided') + +args = parser.parse_args() +url_filename = args.filename + +response = requests.get(url_filename, stream=True) + +tarpath = "/tmp/tmp456/tarball.tar.gz" +with open(tarpath, "wb") as f: + f.write(response.raw.read()) + +shutil.unpack_archive(tarpath, base_dir) # $result=BAD \ No newline at end of file From 51b11de44a24926449543f837d9d1ef8c91408b9 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 15:16:24 +0100 Subject: [PATCH 26/52] Add a Django Upload examples --- .../Security/CWE-022bis/UnsafeUnpackQuery.qll | 25 ++++++++---- .../Security/CWE-022/UnsafeUnpack.py | 40 ++++++++++++++++++- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll index c041b710ae0d..29c41b74dab7 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll @@ -55,13 +55,10 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { at = s.getObject() and at.getAttr() = "FILES" and source.asExpr() = s ) or + // Retrieve Django uploaded files + // see HttpRequest.FILES: https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.HttpRequest.FILES exists(Node obj, AttrRead ar | - ar.getAMethodCall("get").flowsTo(source) and - ar.accesses(obj, "FILES") - ) - or - exists(Node obj, AttrRead ar | - ar.getAMethodCall("getlist").flowsTo(source) and + ar.getAMethodCall(["getlist", "get"]).flowsTo(source) and ar.accesses(obj, "FILES") ) } @@ -93,7 +90,21 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { exists(MethodCallNode mc | nodeFrom = mc.getObject() and mc.getMethodName() = "read" and - mc.flowsTo(nodeTo) + nodeTo = mc + ) + or + // Open for access + exists(MethodCallNode cn | + nodeTo = cn.getObject() and + cn.getMethodName() = "open" and + cn.flowsTo(nodeFrom) + ) + or + // Write for access + exists(MethodCallNode cn | + nodeTo = cn.getObject() and + cn.getMethodName() = "write" and + nodeFrom = cn.getArg(0) ) or // Accessing the name or raw content diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index b2d23f2dbe01..fe1b6fa6d456 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -85,4 +85,42 @@ def download_from_url(): with open(tarpath, "wb") as f: f.write(response.raw.read()) -shutil.unpack_archive(tarpath, base_dir) # $result=BAD \ No newline at end of file +shutil.unpack_archive(tarpath, base_dir) # $result=BAD + +# the django upload functionality +# see HttpRequest.FILES: https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.HttpRequest.FILES +from django.shortcuts import render +from django.core.files.storage import FileSystemStorage +import shutil + +def simple_upload(request): + + base_dir = "/tmp/baase_dir" + if request.method == 'POST': + # Read uploaded files by chunks of data + # see chunks(): https://docs.djangoproject.com/en/4.1/ref/files/uploads/#django.core.files.uploadedfile.UploadedFile.chunks + savepath = os.path.join(base_dir, "tarball_compressed.tar.gz") + with open(savepath, 'wb+') as wfile: + for chunk in request.FILES["ufile1"].chunks(): + wfile.write(chunk) + shutil.unpack_archive(savepath, base_dir) # $result=BAD + + # Write in binary the uploaded tarball + myfile = request.FILES.get("ufile1") + file_path = os.path.join(base_dir, "tarball.tar") + with file_path.open('wb') as f: + f.write(myfile.read()) + shutil.unpack_archive(file_path, base_dir) # $result=BAD + + # Save uploaded files using FileSystemStorage Django API + # see FileSystemStorage: https://docs.djangoproject.com/en/4.1/ref/files/storage/#django.core.files.storage.FileSystemStorage + for ufile in request.FILES.getlist(): + fs = FileSystemStorage() + filename = fs.save(ufile.name, ufile) + uploaded_file_path = fs.path(filename) + shutil.unpack_archive(uploaded_file_path, base_dir) # $result=BAD + + return render(request, 'simple_upload.html') + + elif request.method == 'GET': + return render(request, 'simple_upload.html') \ No newline at end of file From 1a211485a47c814f461ccfb8488b3a1626e2a845 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 17:07:59 +0100 Subject: [PATCH 27/52] Restrain the source and add two steps. --- .../Security/CWE-022bis/UnsafeUnpackQuery.qll | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll index 29c41b74dab7..7409d30fdfa5 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll @@ -50,17 +50,9 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { ) ) or - // catch the uploaded files as a source - exists(Subscript s, Attribute at | - at = s.getObject() and at.getAttr() = "FILES" and source.asExpr() = s - ) - or - // Retrieve Django uploaded files + // catch the Django uploaded files as a source // see HttpRequest.FILES: https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.HttpRequest.FILES - exists(Node obj, AttrRead ar | - ar.getAMethodCall(["getlist", "get"]).flowsTo(source) and - ar.accesses(obj, "FILES") - ) + source.(AttrRead).getAttributeName() = "FILES" } override predicate isSink(DataFlow::Node sink) { @@ -107,6 +99,12 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeFrom = cn.getArg(0) ) or + // Retrieve Django uploaded files + // see HttpRequest.FILES.getlist(): https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.QueryDict.getlist + exists(MethodCallNode mc | + nodeFrom = mc.getObject() and mc.getMethodName() = ["getlist", "get"] and nodeTo = mc + ) + or // Accessing the name or raw content exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) or From 998f1bf215ea357943baf702bc98d5efce68ddb8 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 26 Jan 2023 18:54:36 +0100 Subject: [PATCH 28/52] Some reformatting --- .../Security/CWE-022bis/UnsafeUnpackQuery.qll | 69 +++++++++---------- .../Security/CWE-022/UnsafeUnpack.py | 19 +++-- 2 files changed, 40 insertions(+), 48 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll index 7409d30fdfa5..4e7e97188c0a 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll @@ -61,23 +61,6 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - // Writing the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc.calls(f, "write") and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) - or - // Copying the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc = API::moduleImport("shutil").getMember("copyfileobj").getACall() and - f = mc.getArg(1) and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) - or // Reading the response exists(MethodCallNode mc | nodeFrom = mc.getObject() and @@ -94,20 +77,37 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { or // Write for access exists(MethodCallNode cn | - nodeTo = cn.getObject() and + nodeFrom = cn.getObject() and cn.getMethodName() = "write" and - nodeFrom = cn.getArg(0) + nodeTo = cn.getArg(0) ) or // Retrieve Django uploaded files // see HttpRequest.FILES.getlist(): https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.QueryDict.getlist exists(MethodCallNode mc | - nodeFrom = mc.getObject() and mc.getMethodName() = ["getlist", "get"] and nodeTo = mc + nodeFrom = mc.getObject() and + mc.getMethodName() = ["getlist", "get"] and + nodeTo = mc ) or // Accessing the name or raw content exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) or + // Considering the use of "fs" + exists(API::CallNode fs, MethodCallNode mcn | + fs = + API::moduleImport("django") + .getMember("core") + .getMember("files") + .getMember("storage") + .getMember("FileSystemStorage") + .getACall() and + fs.flowsTo(mcn.getObject()) and + mcn.getMethodName() = ["save", "path"] and + nodeFrom = mcn.getArg(0) and + nodeTo = mcn + ) + or //Use of join of filename exists(API::CallNode mcn | mcn = API::moduleImport("os").getMember("path").getMember("join").getACall() and @@ -120,26 +120,19 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeFrom = mc.getObject() and mc.getMethodName() = "chunks" and mc.flowsTo(nodeTo) ) or - // Considering the use of closing() - exists(API::CallNode closing | - closing = API::moduleImport("contextlib").getMember("closing").getACall() and - closing.flowsTo(nodeTo) and - nodeFrom = closing.getArg(0) + // Write access + exists(MethodCallNode cn | + nodeTo = cn.getObject() and + cn.getMethodName() = "write" and + nodeFrom = cn.getArg(0) ) or - // Considering the use of "fs" - exists(API::CallNode fs, MethodCallNode mcn | - fs = - API::moduleImport("django") - .getMember("core") - .getMember("files") - .getMember("storage") - .getMember("FileSystemStorage") - .getACall() and - fs.flowsTo(mcn.getObject()) and - mcn.getMethodName() = ["save", "path"] and - nodeFrom = mcn.getArg(0) and - nodeTo = mcn + // Writing the response data to the archive + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc.calls(f, "write") and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) ) } } diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index fe1b6fa6d456..ae9ee0559992 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -8,16 +8,15 @@ # Consider any RemoteFlowSource as a source @app.route("/download_from_url") def download_from_url(): - filename = request.args.get('filename', '') - if not filename: - response = requests.get(filename, stream=True) - - tarpath = "/tmp/tmp456/tarball.tar.gz" - with open(tarpath, "wb") as f: - f.write(response.raw.read()) - - untarredpath = "/tmp/tmp123" - shutil.unpack_archive(tarpath, untarredpath) # $result=BAD + filename = request.args.get('filename', '') + if not filename: + response = requests.get(filename, stream=True) + + tarpath = "/tmp/tmp456/tarball.tar.gz" + with open(tarpath, "wb") as f: + f.write(response.raw.read()) + untarredpath = "/tmp/tmp123" + shutil.unpack_archive(tarpath, untarredpath) # $result=BAD # A source catching an S3 filename download From bca053f85554cd0d21a55219740d275035c70f16 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 13:42:14 +0100 Subject: [PATCH 29/52] Move the config query to the parent directory --- .../experimental/Security/{CWE-022bis => }/UnsafeUnpackQuery.qll | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename python/ql/src/experimental/Security/{CWE-022bis => }/UnsafeUnpackQuery.qll (100%) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll similarity index 100% rename from python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpackQuery.qll rename to python/ql/src/experimental/Security/UnsafeUnpackQuery.qll From 5f0bf1053a3515faf16ec0ef033ab6b2511c97e6 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 13:42:57 +0100 Subject: [PATCH 30/52] Update the dataflow test query and the expected results --- .../Security/CWE-022/DataflowQueryTest.ql | 2 +- .../Security/CWE-022/UnsafeUnpack.expected | 45 +++---------------- 2 files changed, 6 insertions(+), 41 deletions(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql index e3cd5f784ff3..df70ff9fe51d 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql @@ -1,3 +1,3 @@ import python import experimental.dataflow.TestUtil.DataflowQueryTest -import UnsafeUnpackQuery \ No newline at end of file +import experimental.Security.UnsafeUnpackQuery \ No newline at end of file diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected index cda42c13267c..c07d7f1e245e 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -1,40 +1,5 @@ -edges -| UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | UnsafeUnpack.py:11:16:11:22 | ControlFlowNode for request | -| UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | -| UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | -| UnsafeUnpack.py:11:16:11:22 | ControlFlowNode for request | UnsafeUnpack.py:11:16:11:27 | ControlFlowNode for Attribute | -| UnsafeUnpack.py:11:16:11:27 | ControlFlowNode for Attribute | UnsafeUnpack.py:17:23:17:34 | ControlFlowNode for Attribute | -| UnsafeUnpack.py:17:23:17:34 | ControlFlowNode for Attribute | UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | -| UnsafeUnpack.py:34:50:34:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | -| UnsafeUnpack.py:48:20:48:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:52:19:52:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:66:19:66:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:80:16:80:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:86:15:86:26 | ControlFlowNode for Attribute | -| UnsafeUnpack.py:86:15:86:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | -nodes -| UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | semmle.label | ModuleVariableNode for UnsafeUnpack.request | -| UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember | -| UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | semmle.label | GSSA Variable request | -| UnsafeUnpack.py:11:16:11:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | -| UnsafeUnpack.py:11:16:11:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | -| UnsafeUnpack.py:17:23:17:34 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | -| UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | -| UnsafeUnpack.py:34:50:34:65 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | -| UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | -| UnsafeUnpack.py:48:20:48:34 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:52:19:52:36 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:66:19:66:31 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | -| UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | -| UnsafeUnpack.py:80:16:80:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | -| UnsafeUnpack.py:86:15:86:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | -| UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | -subpaths -#select -| UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | UnsafeUnpack.py:20:31:20:37 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:34:50:34:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:35:23:35:38 | ControlFlowNode for local_ziped_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:48:20:48:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:49:23:49:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:52:19:52:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:53:23:53:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:66:19:66:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:67:23:67:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | -| UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | UnsafeUnpack.py:80:16:80:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:88:23:88:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | +ERROR: Could not resolve module DataFlow (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:19,8-16) +ERROR: Could not resolve module DataFlow (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:21,36-44) +ERROR: Could not resolve module DataFlow (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:21,63-71) +ERROR: Could not resolve module UnsafeUnpackQuery (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:18,8-25) +ERROR: Could not resolve type UnsafeUnpackingConfig (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:21,6-27) From e41042418aad3922897caaaac0ecdb58da102e97 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 13:46:57 +0100 Subject: [PATCH 31/52] Update the import relative to the dataflow config --- python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql index 1e17cb728cab..41d46218ec68 100644 --- a/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql +++ b/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql @@ -15,7 +15,7 @@ */ import python -import UnsafeUnpackQuery +import experimental.Security.UnsafeUnpackQuery import DataFlow::PathGraph from UnsafeUnpackingConfig config, DataFlow::PathNode source, DataFlow::PathNode sink From 18d8bbc9a4f09be753ad93c8085f767d6f07755d Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 14:05:25 +0100 Subject: [PATCH 32/52] Updated the expected results accordingly --- .../Security/CWE-022/UnsafeUnpack.expected | 65 +++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected index c07d7f1e245e..eed5e0e45d75 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -1,5 +1,60 @@ -ERROR: Could not resolve module DataFlow (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:19,8-16) -ERROR: Could not resolve module DataFlow (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:21,36-44) -ERROR: Could not resolve module DataFlow (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:21,63-71) -ERROR: Could not resolve module UnsafeUnpackQuery (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:18,8-25) -ERROR: Could not resolve type UnsafeUnpackingConfig (/home/sim4n6/Desktop/Ghsecuritylab/my-codeql-PR-3/python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.ql:21,6-27) +edges +| UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | UnsafeUnpack.py:11:18:11:24 | ControlFlowNode for request | +| UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | +| UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | +| UnsafeUnpack.py:11:18:11:24 | ControlFlowNode for request | UnsafeUnpack.py:11:18:11:29 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:11:18:11:29 | ControlFlowNode for Attribute | UnsafeUnpack.py:17:27:17:38 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:17:27:17:38 | ControlFlowNode for Attribute | UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | +| UnsafeUnpack.py:33:50:33:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:34:23:34:38 | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:47:20:47:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:48:23:48:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:51:19:51:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:52:23:52:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:65:19:65:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:66:23:66:37 | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:79:16:79:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:85:15:85:26 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:85:15:85:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:87:23:87:29 | ControlFlowNode for tarpath | +| UnsafeUnpack.py:103:23:103:27 | SSA variable chunk | UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | +| UnsafeUnpack.py:103:32:103:44 | ControlFlowNode for Attribute | UnsafeUnpack.py:103:32:103:54 | ControlFlowNode for Subscript | +| UnsafeUnpack.py:103:32:103:54 | ControlFlowNode for Subscript | UnsafeUnpack.py:103:23:103:27 | SSA variable chunk | +| UnsafeUnpack.py:108:22:108:34 | ControlFlowNode for Attribute | UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | +| UnsafeUnpack.py:116:17:116:21 | SSA variable ufile | UnsafeUnpack.py:118:38:118:47 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:116:27:116:39 | ControlFlowNode for Attribute | UnsafeUnpack.py:116:17:116:21 | SSA variable ufile | +| UnsafeUnpack.py:118:38:118:47 | ControlFlowNode for Attribute | UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | +nodes +| UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | semmle.label | ModuleVariableNode for UnsafeUnpack.request | +| UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember | +| UnsafeUnpack.py:5:26:5:32 | GSSA Variable request | semmle.label | GSSA Variable request | +| UnsafeUnpack.py:11:18:11:24 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| UnsafeUnpack.py:11:18:11:29 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:17:27:17:38 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | +| UnsafeUnpack.py:33:50:33:65 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:34:23:34:38 | ControlFlowNode for local_ziped_path | semmle.label | ControlFlowNode for local_ziped_path | +| UnsafeUnpack.py:47:20:47:34 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:48:23:48:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:51:19:51:36 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| UnsafeUnpack.py:52:23:52:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:65:19:65:31 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:66:23:66:37 | ControlFlowNode for compressed_file | semmle.label | ControlFlowNode for compressed_file | +| UnsafeUnpack.py:79:16:79:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:85:15:85:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:87:23:87:29 | ControlFlowNode for tarpath | semmle.label | ControlFlowNode for tarpath | +| UnsafeUnpack.py:103:23:103:27 | SSA variable chunk | semmle.label | SSA variable chunk | +| UnsafeUnpack.py:103:32:103:44 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:103:32:103:54 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript | +| UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | semmle.label | ControlFlowNode for savepath | +| UnsafeUnpack.py:108:22:108:34 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | semmle.label | ControlFlowNode for file_path | +| UnsafeUnpack.py:116:17:116:21 | SSA variable ufile | semmle.label | SSA variable ufile | +| UnsafeUnpack.py:116:27:116:39 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:118:38:118:47 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | semmle.label | ControlFlowNode for uploaded_file_path | +subpaths +#select +| UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:34:23:34:38 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:33:50:33:65 | ControlFlowNode for local_ziped_path | UnsafeUnpack.py:34:23:34:38 | ControlFlowNode for local_ziped_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:48:23:48:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:47:20:47:34 | ControlFlowNode for compressed_file | UnsafeUnpack.py:48:23:48:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:52:23:52:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:51:19:51:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:52:23:52:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:66:23:66:37 | ControlFlowNode for compressed_file | UnsafeUnpack.py:65:19:65:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:66:23:66:37 | ControlFlowNode for compressed_file | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:87:23:87:29 | ControlFlowNode for tarpath | UnsafeUnpack.py:79:16:79:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:87:23:87:29 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | UnsafeUnpack.py:103:32:103:44 | ControlFlowNode for Attribute | UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | UnsafeUnpack.py:108:22:108:34 | ControlFlowNode for Attribute | UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | UnsafeUnpack.py:116:27:116:39 | ControlFlowNode for Attribute | UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | From 207ed3da9c3c4aa52b03c34fcfc20eaaeda44f05 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 15:07:20 +0100 Subject: [PATCH 33/52] Constrain the object & the call --- .../Security/UnsafeUnpackQuery.qll | 39 ++++++------------- 1 file changed, 11 insertions(+), 28 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index 4e7e97188c0a..182ef30f84b6 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -61,13 +61,6 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - // Reading the response - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and - mc.getMethodName() = "read" and - nodeTo = mc - ) - or // Open for access exists(MethodCallNode cn | nodeTo = cn.getObject() and @@ -77,21 +70,20 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { or // Write for access exists(MethodCallNode cn | - nodeFrom = cn.getObject() and - cn.getMethodName() = "write" and + cn.calls(nodeFrom, "write") and nodeTo = cn.getArg(0) ) or // Retrieve Django uploaded files - // see HttpRequest.FILES.getlist(): https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.QueryDict.getlist - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and - mc.getMethodName() = ["getlist", "get"] and - nodeTo = mc - ) + // see getlist(): https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.QueryDict.getlist + // see chunks(): https://docs.djangoproject.com/en/4.1/ref/files/uploads/#django.core.files.uploadedfile.UploadedFile.chunks + nodeTo.(MethodCallNode).calls(nodeFrom, ["getlist", "get", "chunks"]) + or + // Reading the response + nodeTo.(MethodCallNode).calls(nodeFrom, "read") or // Accessing the name or raw content - exists(AttrRead ar | ar.accesses(nodeFrom, ["name", "raw"]) and ar.flowsTo(nodeTo)) + nodeTo.(AttrRead).accesses(nodeFrom, ["name", "raw"]) or // Considering the use of "fs" exists(API::CallNode fs, MethodCallNode mcn | @@ -109,21 +101,12 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { ) or //Use of join of filename - exists(API::CallNode mcn | - mcn = API::moduleImport("os").getMember("path").getMember("join").getACall() and - nodeFrom = mcn.getArg(1) and - mcn.flowsTo(nodeTo) - ) - or - // Read by chunks - exists(MethodCallNode mc | - nodeFrom = mc.getObject() and mc.getMethodName() = "chunks" and mc.flowsTo(nodeTo) - ) + nodeTo = API::moduleImport("os").getMember("path").getMember("join").getACall() and + nodeFrom = nodeTo.(API::CallNode).getArg(1) or // Write access exists(MethodCallNode cn | - nodeTo = cn.getObject() and - cn.getMethodName() = "write" and + cn.calls(nodeTo, "write") and nodeFrom = cn.getArg(0) ) or From 8ef2aa00e73814b6c84d40cee53b0a3afdb650c5 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 16:07:39 +0100 Subject: [PATCH 34/52] Update python/ql/src/experimental/Security/UnsafeUnpackQuery.qll Co-authored-by: yoff --- .../src/experimental/Security/UnsafeUnpackQuery.qll | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index 182ef30f84b6..c0a370fa9f45 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -32,12 +32,13 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { or // A source catching an S3 filename download // see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file - exists(MethodCallNode mcn, Node s3, Node bc | - bc = API::moduleImport("boto3").getMember("client").getACall() and - bc = s3.getALocalSource() and - mcn.calls(s3, "download_file") and - source = mcn.getArg(2) - ) + source = + API::moduleImport("boto3") + .getMember("client") + .getReturn() + .getMember("download_file") + .getACall() + .getArg(2) or // A source download a file using wget // see wget: https://pypi.org/project/wget/ From 0b27b1314af813607a9a4327eced1aeb46b78a04 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 16:12:08 +0100 Subject: [PATCH 35/52] Update python/ql/src/experimental/Security/UnsafeUnpackQuery.qll Co-authored-by: yoff --- python/ql/src/experimental/Security/UnsafeUnpackQuery.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index c0a370fa9f45..f660294cb599 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -24,7 +24,6 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { args = API::moduleImport("argparse") .getMember("ArgumentParser") - .getACall() .getReturn() .getMember("parse_args") .getACall() From ee213123ac76101840c0762b74973fe03c063e1a Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 18:16:11 +0100 Subject: [PATCH 36/52] Add builtin open as an additional step --- .../src/experimental/Security/UnsafeUnpackQuery.qll | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index f660294cb599..837f84ee21f3 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -61,18 +61,15 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { - // Open for access + // Open a file for access exists(MethodCallNode cn | nodeTo = cn.getObject() and cn.getMethodName() = "open" and cn.flowsTo(nodeFrom) ) or - // Write for access - exists(MethodCallNode cn | - cn.calls(nodeFrom, "write") and - nodeTo = cn.getArg(0) - ) + // Open a file for access using builtin + nodeFrom = API::builtin("open").getACall() and nodeTo = nodeFrom.(API::CallNode).getArg(0) or // Retrieve Django uploaded files // see getlist(): https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.QueryDict.getlist @@ -100,7 +97,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeTo = mcn ) or - //Use of join of filename + // Join the base_dir to the filename nodeTo = API::moduleImport("os").getMember("path").getMember("join").getACall() and nodeFrom = nodeTo.(API::CallNode).getArg(1) or From 0e2f37825ddd56e1dbdd3b131bc4a6e97821cd9c Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 27 Jan 2023 23:58:03 +0100 Subject: [PATCH 37/52] Organize steps to correspond to the sample code --- .../Security/UnsafeUnpackQuery.qll | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index 837f84ee21f3..bdac41e8812b 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -61,6 +61,9 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { + // Reading the response + nodeTo.(MethodCallNode).calls(nodeFrom, "read") + or // Open a file for access exists(MethodCallNode cn | nodeTo = cn.getObject() and @@ -68,6 +71,12 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { cn.flowsTo(nodeFrom) ) or + // Write access + exists(MethodCallNode cn | + cn.calls(nodeTo, "write") and + nodeFrom = cn.getArg(0) + ) + or // Open a file for access using builtin nodeFrom = API::builtin("open").getACall() and nodeTo = nodeFrom.(API::CallNode).getArg(0) or @@ -76,11 +85,13 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // see chunks(): https://docs.djangoproject.com/en/4.1/ref/files/uploads/#django.core.files.uploadedfile.UploadedFile.chunks nodeTo.(MethodCallNode).calls(nodeFrom, ["getlist", "get", "chunks"]) or - // Reading the response - nodeTo.(MethodCallNode).calls(nodeFrom, "read") - or - // Accessing the name or raw content - nodeTo.(AttrRead).accesses(nodeFrom, ["name", "raw"]) + // Writing the response data to the archive + exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | + is.flowsTo(f) and + mc.calls(f, "write") and + nodeFrom = mc.getArg(0) and + nodeTo = is.(CallCfgNode).getArg(0) + ) or // Considering the use of "fs" exists(API::CallNode fs, MethodCallNode mcn | @@ -97,22 +108,11 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeTo = mcn ) or + // Accessing the name or raw content + nodeTo.(AttrRead).accesses(nodeFrom, ["name", "raw"]) + or // Join the base_dir to the filename nodeTo = API::moduleImport("os").getMember("path").getMember("join").getACall() and nodeFrom = nodeTo.(API::CallNode).getArg(1) - or - // Write access - exists(MethodCallNode cn | - cn.calls(nodeTo, "write") and - nodeFrom = cn.getArg(0) - ) - or - // Writing the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc.calls(f, "write") and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) } } From a4aaf0ec6f9ed100ef1966a37cc65c5fc62553a9 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Sat, 28 Jan 2023 09:53:54 +0100 Subject: [PATCH 38/52] Remove a write step & update the builtin open step --- .../Security/UnsafeUnpackQuery.qll | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index bdac41e8812b..d449f5f54334 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -66,8 +66,14 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { or // Open a file for access exists(MethodCallNode cn | - nodeTo = cn.getObject() and - cn.getMethodName() = "open" and + cn.calls(nodeTo, "open") and + cn.flowsTo(nodeFrom) + ) + or + // Open a file for access using builtin + exists(API::CallNode cn | + cn = API::builtin("open").getACall() and + nodeTo = cn.(API::CallNode).getArg(0) and cn.flowsTo(nodeFrom) ) or @@ -77,22 +83,11 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeFrom = cn.getArg(0) ) or - // Open a file for access using builtin - nodeFrom = API::builtin("open").getACall() and nodeTo = nodeFrom.(API::CallNode).getArg(0) - or // Retrieve Django uploaded files // see getlist(): https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.QueryDict.getlist // see chunks(): https://docs.djangoproject.com/en/4.1/ref/files/uploads/#django.core.files.uploadedfile.UploadedFile.chunks nodeTo.(MethodCallNode).calls(nodeFrom, ["getlist", "get", "chunks"]) or - // Writing the response data to the archive - exists(Stdlib::FileLikeObject::InstanceSource is, Node f, MethodCallNode mc | - is.flowsTo(f) and - mc.calls(f, "write") and - nodeFrom = mc.getArg(0) and - nodeTo = is.(CallCfgNode).getArg(0) - ) - or // Considering the use of "fs" exists(API::CallNode fs, MethodCallNode mcn | fs = From 0707064ab511390a79b359880c97f6034a95a871 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Sat, 28 Jan 2023 10:14:24 +0100 Subject: [PATCH 39/52] Constrain the save/path step --- .../Security/UnsafeUnpackQuery.qll | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index d449f5f54334..99ffb443dca1 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -89,19 +89,17 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeTo.(MethodCallNode).calls(nodeFrom, ["getlist", "get", "chunks"]) or // Considering the use of "fs" - exists(API::CallNode fs, MethodCallNode mcn | - fs = - API::moduleImport("django") - .getMember("core") - .getMember("files") - .getMember("storage") - .getMember("FileSystemStorage") - .getACall() and - fs.flowsTo(mcn.getObject()) and - mcn.getMethodName() = ["save", "path"] and - nodeFrom = mcn.getArg(0) and - nodeTo = mcn - ) + // see fs: https://docs.djangoproject.com/en/4.1/ref/files/storage/#the-filesystemstorage-class + nodeTo = + API::moduleImport("django") + .getMember("core") + .getMember("files") + .getMember("storage") + .getMember("FileSystemStorage") + .getReturn() + .getMember(["save", "path"]) + .getACall() and + nodeFrom = nodeTo.(MethodCallNode).getArg(0) or // Accessing the name or raw content nodeTo.(AttrRead).accesses(nodeFrom, ["name", "raw"]) From 7079def7ce31920affd1e585a33116c1ba5a5036 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Mon, 30 Jan 2023 00:49:23 +0100 Subject: [PATCH 40/52] Add an S3 source with Session or download_fileobj --- .../experimental/Security/UnsafeUnpackQuery.qll | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index 99ffb443dca1..7f305a1d734d 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -29,13 +29,25 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { .getACall() ) or - // A source catching an S3 filename download + // A source catching an S3 file download // see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.download_file source = API::moduleImport("boto3") .getMember("client") .getReturn() - .getMember("download_file") + .getMember(["download_file", "download_fileobj"]) + .getACall() + .getArg(2) + or + // A source catching an S3 file download + // see boto3: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html + source = + API::moduleImport("boto3") + .getMember("Session") + .getReturn() + .getMember("client") + .getReturn() + .getMember(["download_file", "download_fileobj"]) .getACall() .getArg(2) or From 1a8c9abee232997cd7d90cb10d65b1bc7c905a60 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 2 Feb 2023 21:09:40 +0100 Subject: [PATCH 41/52] Incorporate Sink & Source as steps from TarSlipQry --- .../Security/UnsafeUnpackQuery.qll | 86 ++++++++++++++++++- .../Security/CWE-022/UnsafeUnpack.py | 78 ++++++++++++++++- 2 files changed, 161 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index 7f305a1d734d..b43410b14ad2 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -10,6 +10,35 @@ import semmle.python.dataflow.new.TaintTracking import semmle.python.frameworks.Stdlib import semmle.python.dataflow.new.RemoteFlowSources +/** + * Handle those three cases of Tarfile opens: + * - `tarfile.open()` + * - `tarfile.TarFile()` + * - `MKtarfile.Tarfile.open()` + */ +API::Node tarfileOpen() { + result in [ + API::moduleImport("tarfile").getMember(["open", "TarFile"]), + API::moduleImport("tarfile").getMember("TarFile").getASubclass().getMember("open") + ] +} + +/** + * Handle the previous three cases, plus the use of `closing` in the previous cases + */ +class AllTarfileOpens extends API::CallNode { + AllTarfileOpens() { + this = tarfileOpen().getACall() + or + exists(API::Node closing, Node arg | + closing = API::moduleImport("contextlib").getMember("closing") and + this = closing.getACall() and + arg = this.getArg(0) and + arg = tarfileOpen().getACall() + ) + } +} + class UnsafeUnpackingConfig extends TaintTracking::Configuration { UnsafeUnpackingConfig() { this = "UnsafeUnpackingConfig" } @@ -68,8 +97,47 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { } override predicate isSink(DataFlow::Node sink) { - // A sink capturing method calls to `unpack_archive`. - sink = API::moduleImport("shutil").getMember("unpack_archive").getACall().getArg(0) + ( + // A sink capturing method calls to `unpack_archive`. + sink = API::moduleImport("shutil").getMember("unpack_archive").getACall().getArg(0) + or + // A sink capturing method calls to `extractall` without `members` argument. + // For a call to `file.extractall` without `members` argument, `file` is considered a sink. + exists(MethodCallNode call, AllTarfileOpens atfo | + call = atfo.getReturn().getMember("extractall").getACall() and + not exists(Node arg | arg = call.getArgByName("members")) and + sink = call.getObject() + ) + or + // A sink capturing method calls to `extractall` with `members` argument. + // For a call to `file.extractall` with `members` argument, `file` is considered a sink if not + // a the `members` argument contains a NameConstant as None, a List or call to the method `getmembers`. + // Otherwise, the argument of `members` is considered a sink. + exists(MethodCallNode call, Node arg, AllTarfileOpens atfo | + call = atfo.getReturn().getMember("extractall").getACall() and + arg = call.getArgByName("members") and + if + arg.asCfgNode() instanceof NameConstantNode or + arg.asCfgNode() instanceof ListNode + then sink = call.getObject() + else + if arg.(MethodCallNode).getMethodName() = "getmembers" + then sink = arg.(MethodCallNode).getObject() + else sink = call.getArgByName("members") + ) + or + // An argument to `extract` is considered a sink. + exists(AllTarfileOpens atfo | + sink = atfo.getReturn().getMember("extract").getACall().getArg(0) + ) + or + //An argument to `_extract_member` is considered a sink. + exists(MethodCallNode call, AllTarfileOpens atfo | + call = atfo.getReturn().getMember("_extract_member").getACall() and + call.getArg(1).(AttrRead).accesses(sink, "name") + ) + ) and + not sink.getScope().getLocation().getFile().inStdlib() } override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { @@ -119,5 +187,19 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // Join the base_dir to the filename nodeTo = API::moduleImport("os").getMember("path").getMember("join").getACall() and nodeFrom = nodeTo.(API::CallNode).getArg(1) + or + // Go through an Open for a Tarfile + nodeTo = tarfileOpen().getACall() and nodeFrom = nodeTo.(MethodCallNode).getArg(0) + or + // Handle the case where the getmembers is used. + nodeTo.(MethodCallNode).calls(nodeFrom, "getmembers") and + nodeFrom instanceof AllTarfileOpens + or + // To handle the case of `with closing(tarfile.open()) as file:` + // we add a step from the first argument of `closing` to the call to `closing`, + // whenever that first argument is a return of `tarfile.open()`. + nodeTo = API::moduleImport("contextlib").getMember("closing").getACall() and + nodeFrom = nodeTo.(API::CallNode).getArg(0) and + nodeFrom = tarfileOpen().getReturn().getAValueReachableFromSource() } } diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py index ae9ee0559992..50574281cbd4 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py @@ -122,4 +122,80 @@ def simple_upload(request): return render(request, 'simple_upload.html') elif request.method == 'GET': - return render(request, 'simple_upload.html') \ No newline at end of file + return render(request, 'simple_upload.html') + + +import shutil +import os +import tarfile +import tempfile +import argparse + +parser = argparse.ArgumentParser(description='Process some integers.') +parser.add_argument('integers', metavar='N', type=int, nargs='+', + help='an integer for the accumulator') +parser.add_argument('filename', help='filename to be provided') + +args = parser.parse_args() +unsafe_filename_tar = args.filename +with tarfile.TarFile(unsafe_filename_tar, mode="r") as tar: + tar.extractall(path="/tmp/unpack/", members=tar) # $result=BAD +tar = tarfile.open(unsafe_filename_tar) + + +from django.shortcuts import render +from django.core.files.storage import FileSystemStorage +import shutil + +def simple_upload(request): + + base_dir = "/tmp/baase_dir" + if request.method == 'POST': + # Read uploaded files by chunks of data + # see chunks(): https://docs.djangoproject.com/en/4.1/ref/files/uploads/#django.core.files.uploadedfile.UploadedFile.chunks + savepath = os.path.join(base_dir, "tarball_compressed.tar.gz") + with open(savepath, 'wb+') as wfile: + for chunk in request.FILES["ufile1"].chunks(): + wfile.write(chunk) + + tar = tarfile.open(savepath) + result = [] + for member in tar: + if member.issym(): + raise ValueError("But it is a symlink") + result.append(member) + tar.extractall(path=tempfile.mkdtemp(), members=result) # $result=BAD + tar.close() + + +response = requests.get(url_filename, stream=True) +tarpath = "/tmp/tmp456/tarball.tar.gz" +with open(tarpath, "wb") as f: + f.write(response.raw.read()) +target_dir = "/tmp/unpack" +tarfile.TarFile(tarpath, mode="r").extractall(path=target_dir) # $result=BAD + + +from pathlib import Path +import tempfile +import boto3 + +def default_session() -> boto3.Session: + _SESSION = None + if _SESSION is None: + _SESSION = boto3.Session() + return _SESSION + +cache = False +cache_dir = "/tmp/artifacts" +object_path = "/objects/obj1" +s3 = default_session().client("s3") +with tempfile.NamedTemporaryFile(suffix=".tar.gz") as tmp: + s3.download_fileobj(bucket_name, object_path, tmp) + tmp.seek(0) + if cache: + cache_dir.mkdir(exist_ok=True, parents=True) + target = cache_dir + else: + target = Path(tempfile.mkdtemp()) + shutil.unpack_archive(tmp.name, target) # $result=BAD \ No newline at end of file From a0150849cbafae04aaee91d9c6b38c4ef01d4f9d Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 2 Feb 2023 21:42:47 +0100 Subject: [PATCH 42/52] Updated the expected test file --- .../Security/CWE-022/UnsafeUnpack.expected | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected index eed5e0e45d75..94bd7276631f 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected @@ -10,6 +10,7 @@ edges | UnsafeUnpack.py:51:19:51:36 | ControlFlowNode for Attribute() | UnsafeUnpack.py:52:23:52:37 | ControlFlowNode for compressed_file | | UnsafeUnpack.py:65:19:65:31 | ControlFlowNode for Attribute | UnsafeUnpack.py:66:23:66:37 | ControlFlowNode for compressed_file | | UnsafeUnpack.py:79:16:79:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:85:15:85:26 | ControlFlowNode for Attribute | +| UnsafeUnpack.py:79:16:79:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:174:15:174:26 | ControlFlowNode for Attribute | | UnsafeUnpack.py:85:15:85:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:87:23:87:29 | ControlFlowNode for tarpath | | UnsafeUnpack.py:103:23:103:27 | SSA variable chunk | UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | | UnsafeUnpack.py:103:32:103:44 | ControlFlowNode for Attribute | UnsafeUnpack.py:103:32:103:54 | ControlFlowNode for Subscript | @@ -18,6 +19,13 @@ edges | UnsafeUnpack.py:116:17:116:21 | SSA variable ufile | UnsafeUnpack.py:118:38:118:47 | ControlFlowNode for Attribute | | UnsafeUnpack.py:116:27:116:39 | ControlFlowNode for Attribute | UnsafeUnpack.py:116:17:116:21 | SSA variable ufile | | UnsafeUnpack.py:118:38:118:47 | ControlFlowNode for Attribute | UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | +| UnsafeUnpack.py:140:23:140:35 | ControlFlowNode for Attribute | UnsafeUnpack.py:142:49:142:51 | ControlFlowNode for tar | +| UnsafeUnpack.py:158:23:158:27 | SSA variable chunk | UnsafeUnpack.py:163:23:163:28 | SSA variable member | +| UnsafeUnpack.py:158:32:158:44 | ControlFlowNode for Attribute | UnsafeUnpack.py:158:32:158:54 | ControlFlowNode for Subscript | +| UnsafeUnpack.py:158:32:158:54 | ControlFlowNode for Subscript | UnsafeUnpack.py:158:23:158:27 | SSA variable chunk | +| UnsafeUnpack.py:163:23:163:28 | SSA variable member | UnsafeUnpack.py:167:67:167:72 | ControlFlowNode for result | +| UnsafeUnpack.py:174:15:174:26 | ControlFlowNode for Attribute | UnsafeUnpack.py:176:1:176:34 | ControlFlowNode for Attribute() | +| UnsafeUnpack.py:194:53:194:55 | ControlFlowNode for tmp | UnsafeUnpack.py:201:29:201:36 | ControlFlowNode for Attribute | nodes | UnsafeUnpack.py:0:0:0:0 | ModuleVariableNode for UnsafeUnpack.request | semmle.label | ModuleVariableNode for UnsafeUnpack.request | | UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember | @@ -47,6 +55,17 @@ nodes | UnsafeUnpack.py:116:27:116:39 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | | UnsafeUnpack.py:118:38:118:47 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | | UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | semmle.label | ControlFlowNode for uploaded_file_path | +| UnsafeUnpack.py:140:23:140:35 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:142:49:142:51 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | +| UnsafeUnpack.py:158:23:158:27 | SSA variable chunk | semmle.label | SSA variable chunk | +| UnsafeUnpack.py:158:32:158:44 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:158:32:158:54 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript | +| UnsafeUnpack.py:163:23:163:28 | SSA variable member | semmle.label | SSA variable member | +| UnsafeUnpack.py:167:67:167:72 | ControlFlowNode for result | semmle.label | ControlFlowNode for result | +| UnsafeUnpack.py:174:15:174:26 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| UnsafeUnpack.py:176:1:176:34 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| UnsafeUnpack.py:194:53:194:55 | ControlFlowNode for tmp | semmle.label | ControlFlowNode for tmp | +| UnsafeUnpack.py:201:29:201:36 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | subpaths #select | UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | UnsafeUnpack.py:5:26:5:32 | ControlFlowNode for ImportMember | UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | Unsafe extraction from a malicious tarball retrieved from a remote location. | @@ -58,3 +77,7 @@ subpaths | UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | UnsafeUnpack.py:103:32:103:44 | ControlFlowNode for Attribute | UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | Unsafe extraction from a malicious tarball retrieved from a remote location. | | UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | UnsafeUnpack.py:108:22:108:34 | ControlFlowNode for Attribute | UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | | UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | UnsafeUnpack.py:116:27:116:39 | ControlFlowNode for Attribute | UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:142:49:142:51 | ControlFlowNode for tar | UnsafeUnpack.py:140:23:140:35 | ControlFlowNode for Attribute | UnsafeUnpack.py:142:49:142:51 | ControlFlowNode for tar | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:167:67:167:72 | ControlFlowNode for result | UnsafeUnpack.py:158:32:158:44 | ControlFlowNode for Attribute | UnsafeUnpack.py:167:67:167:72 | ControlFlowNode for result | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:176:1:176:34 | ControlFlowNode for Attribute() | UnsafeUnpack.py:79:16:79:28 | ControlFlowNode for Attribute | UnsafeUnpack.py:176:1:176:34 | ControlFlowNode for Attribute() | Unsafe extraction from a malicious tarball retrieved from a remote location. | +| UnsafeUnpack.py:201:29:201:36 | ControlFlowNode for Attribute | UnsafeUnpack.py:194:53:194:55 | ControlFlowNode for tmp | UnsafeUnpack.py:201:29:201:36 | ControlFlowNode for Attribute | Unsafe extraction from a malicious tarball retrieved from a remote location. | From ec82d61991f205bdb43681a82c2fe7dff19d0d37 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Sun, 5 Feb 2023 14:36:17 +0100 Subject: [PATCH 43/52] Add another frequently used step --- .../experimental/Security/UnsafeUnpackQuery.qll | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index b43410b14ad2..bc8682f9e356 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -190,7 +190,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { or // Go through an Open for a Tarfile nodeTo = tarfileOpen().getACall() and nodeFrom = nodeTo.(MethodCallNode).getArg(0) - or + or // Handle the case where the getmembers is used. nodeTo.(MethodCallNode).calls(nodeFrom, "getmembers") and nodeFrom instanceof AllTarfileOpens @@ -201,5 +201,17 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { nodeTo = API::moduleImport("contextlib").getMember("closing").getACall() and nodeFrom = nodeTo.(API::CallNode).getArg(0) and nodeFrom = tarfileOpen().getReturn().getAValueReachableFromSource() + or + // see Path : https://docs.python.org/3/library/pathlib.html#pathlib.Path + nodeTo = API::moduleImport("pathlib").getMember("Path").getACall() and + nodeFrom = nodeTo.(API::CallNode).getArg(0) + or + // Use of absolutepath + // see absolute : https://docs.python.org/3/library/pathlib.html#pathlib.Path.absolute + exists(API::CallNode mcn | + mcn = API::moduleImport("pathlib").getMember("Path").getACall() and + nodeTo = mcn.getAMethodCall("absolute") and + nodeFrom = mcn.getArg(0) + ) } } From 9e285020a10c493052a9c87a031294c71d32378c Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Wed, 8 Feb 2023 21:14:53 +0100 Subject: [PATCH 44/52] Comment modif + remove redundant cast --- python/ql/src/experimental/Security/UnsafeUnpackQuery.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index bc8682f9e356..e2972b446622 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -24,7 +24,7 @@ API::Node tarfileOpen() { } /** - * Handle the previous three cases, plus the use of `closing` in the previous cases + * A class for handling the previous three cases, plus the use of `closing` in with the previous cases */ class AllTarfileOpens extends API::CallNode { AllTarfileOpens() { @@ -153,7 +153,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // Open a file for access using builtin exists(API::CallNode cn | cn = API::builtin("open").getACall() and - nodeTo = cn.(API::CallNode).getArg(0) and + nodeTo = cn.getArg(0) and cn.flowsTo(nodeFrom) ) or From 4196230a8a10d83bdad088de13cfa56538576aac Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Wed, 8 Feb 2023 21:46:50 +0100 Subject: [PATCH 45/52] use if-then-else rather than nested exists --- python/ql/src/experimental/Security/UnsafeUnpackQuery.qll | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index e2972b446622..a1f028134d39 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -84,11 +84,9 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // see wget: https://pypi.org/project/wget/ exists(API::CallNode mcn | mcn = API::moduleImport("wget").getMember("download").getACall() and - ( - source = mcn.getArg(1) - or - source = mcn.getReturn().asSource() and not exists(Node arg | arg = mcn.getArg(1)) - ) + if exists(Node arg | arg = mcn.getArg(1)) + then source = mcn.getArg(1) + else source = mcn.getReturn().asSource() ) or // catch the Django uploaded files as a source From 16ef50401ba251df3898c9acad92916e632bd2e9 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 9 Feb 2023 14:59:28 +0100 Subject: [PATCH 46/52] Update python/ql/src/experimental/Security/UnsafeUnpackQuery.qll Co-authored-by: yoff --- python/ql/src/experimental/Security/UnsafeUnpackQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index a1f028134d39..a92a2cf2f840 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -103,7 +103,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // For a call to `file.extractall` without `members` argument, `file` is considered a sink. exists(MethodCallNode call, AllTarfileOpens atfo | call = atfo.getReturn().getMember("extractall").getACall() and - not exists(Node arg | arg = call.getArgByName("members")) and + not exists(call.getArgByName("members")) and sink = call.getObject() ) or From b04d5684fbabd791ec636f223e03b656279e8aea Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 9 Feb 2023 15:23:58 +0100 Subject: [PATCH 47/52] add a blank line at the end of the file --- .../query-tests/Security/CWE-022/DataflowQueryTest.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql index df70ff9fe51d..f02f8529c96a 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql @@ -1,3 +1,3 @@ import python import experimental.dataflow.TestUtil.DataflowQueryTest -import experimental.Security.UnsafeUnpackQuery \ No newline at end of file +import experimental.Security.UnsafeUnpackQuery From 09df055d86b54b0e1df6facca925dd9f0b8a04be Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Thu, 9 Feb 2023 15:25:54 +0100 Subject: [PATCH 48/52] Fix the exists cast warning --- python/ql/src/experimental/Security/UnsafeUnpackQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index a92a2cf2f840..022a0fb26451 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -84,7 +84,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // see wget: https://pypi.org/project/wget/ exists(API::CallNode mcn | mcn = API::moduleImport("wget").getMember("download").getACall() and - if exists(Node arg | arg = mcn.getArg(1)) + if exists(mcn.getArg(1)) then source = mcn.getArg(1) else source = mcn.getReturn().asSource() ) From eed19a3e15be8e5970f7a62e8f8fc60fca2e2321 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Fri, 10 Feb 2023 21:58:29 +0100 Subject: [PATCH 49/52] Fix autoformatting issues --- python/ql/src/experimental/Security/UnsafeUnpackQuery.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll index 022a0fb26451..ba359ee32f37 100644 --- a/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll +++ b/python/ql/src/experimental/Security/UnsafeUnpackQuery.qll @@ -84,9 +84,7 @@ class UnsafeUnpackingConfig extends TaintTracking::Configuration { // see wget: https://pypi.org/project/wget/ exists(API::CallNode mcn | mcn = API::moduleImport("wget").getMember("download").getACall() and - if exists(mcn.getArg(1)) - then source = mcn.getArg(1) - else source = mcn.getReturn().asSource() + if exists(mcn.getArg(1)) then source = mcn.getArg(1) else source = mcn.getReturn().asSource() ) or // catch the Django uploaded files as a source From 80d4fb5e33c49499bf329e11478e3a338e1ce86b Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Sun, 12 Feb 2023 10:51:53 +0100 Subject: [PATCH 50/52] Organisation TarSlip/UnsafeUnpack into two folders --- .../TarSlip.expected | 0 .../TarSlip.qlref | 0 .../TarSlipImprov.py | 0 .../ZipSlip.expected | 0 .../ZipSlip.qlref | 0 .../zipslip_bad.py | 0 .../zipslip_good.py | 0 .../DataflowQueryTest.expected | 15 +++++++++++ .../DataflowQueryTest.ql | 0 .../UnsafeUnpack.expected | 0 .../UnsafeUnpack.py | 26 +++++++++---------- .../UnsafeUnpack.qlref | 0 .../CWE-022/DataflowQueryTest.expected | 2 -- 13 files changed, 28 insertions(+), 15 deletions(-) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-TarSlip}/TarSlip.expected (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-TarSlip}/TarSlip.qlref (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-TarSlip}/TarSlipImprov.py (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-TarSlip}/ZipSlip.expected (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-TarSlip}/ZipSlip.qlref (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-TarSlip}/zipslip_bad.py (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-TarSlip}/zipslip_good.py (100%) create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-UnsafeUnpacking}/DataflowQueryTest.ql (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-UnsafeUnpacking}/UnsafeUnpack.expected (100%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-UnsafeUnpacking}/UnsafeUnpack.py (89%) rename python/ql/test/experimental/query-tests/Security/{CWE-022 => CWE-022-UnsafeUnpacking}/UnsafeUnpack.qlref (100%) delete mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/TarSlip.expected b/python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/TarSlip.expected similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/TarSlip.expected rename to python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/TarSlip.expected diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/TarSlip.qlref b/python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/TarSlip.qlref similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/TarSlip.qlref rename to python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/TarSlip.qlref diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/TarSlipImprov.py b/python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/TarSlipImprov.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/TarSlipImprov.py rename to python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/TarSlipImprov.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected b/python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/ZipSlip.expected similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected rename to python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/ZipSlip.expected diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref b/python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/ZipSlip.qlref similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref rename to python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/ZipSlip.qlref diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py b/python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/zipslip_bad.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py rename to python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/zipslip_bad.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py b/python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/zipslip_good.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py rename to python/ql/test/experimental/query-tests/Security/CWE-022-TarSlip/zipslip_good.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected new file mode 100644 index 000000000000..5a1382ed87c4 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected @@ -0,0 +1,15 @@ +missingAnnotationOnSink +failures +| UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | Unexpected result: result=BAD | +| UnsafeUnpack.py:34:23:34:38 | ControlFlowNode for local_ziped_path | Unexpected result: result=BAD | +| UnsafeUnpack.py:48:23:48:37 | ControlFlowNode for compressed_file | Unexpected result: result=BAD | +| UnsafeUnpack.py:52:23:52:37 | ControlFlowNode for compressed_file | Unexpected result: result=BAD | +| UnsafeUnpack.py:66:23:66:37 | ControlFlowNode for compressed_file | Unexpected result: result=BAD | +| UnsafeUnpack.py:87:23:87:29 | ControlFlowNode for tarpath | Unexpected result: result=BAD | +| UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | Unexpected result: result=BAD | +| UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | Unexpected result: result=BAD | +| UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | Unexpected result: result=BAD | +| UnsafeUnpack.py:142:49:142:51 | ControlFlowNode for tar | Unexpected result: result=BAD | +| UnsafeUnpack.py:167:67:167:72 | ControlFlowNode for result | Unexpected result: result=BAD | +| UnsafeUnpack.py:176:1:176:34 | ControlFlowNode for Attribute() | Unexpected result: result=BAD | +| UnsafeUnpack.py:201:29:201:36 | ControlFlowNode for Attribute | Unexpected result: result=BAD | diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.ql similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.ql rename to python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.ql diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.expected similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.expected rename to python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.expected diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.py similarity index 89% rename from python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py rename to python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.py index 50574281cbd4..c83714b764c4 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.py @@ -16,7 +16,7 @@ def download_from_url(): with open(tarpath, "wb") as f: f.write(response.raw.read()) untarredpath = "/tmp/tmp123" - shutil.unpack_archive(tarpath, untarredpath) # $result=BAD + shutil.unpack_archive(tarpath, untarredpath) # A source catching an S3 filename download @@ -31,7 +31,7 @@ def download_from_url(): s3 = boto3.client('s3') s3.download_file(bucket_name, remote_ziped_name, local_ziped_path) -shutil.unpack_archive(local_ziped_path, base_dir) # $result=BAD +shutil.unpack_archive(local_ziped_path, base_dir) # wget @@ -45,11 +45,11 @@ def download_from_url(): # download(url, out, bar) contains out parameter wget.download(url, compressed_file) -shutil.unpack_archive(compressed_file, base_dir) # $result=BAD +shutil.unpack_archive(compressed_file, base_dir) # download(url) returns filename compressed_file = wget.download(url) -shutil.unpack_archive(compressed_file, base_dir) # $result=BAD +shutil.unpack_archive(compressed_file, base_dir) # A source coming from a CLI argparse module @@ -63,7 +63,7 @@ def download_from_url(): args = parser.parse_args() compressed_file = args.filename -shutil.unpack_archive(compressed_file, base_dir) # $result=BAD +shutil.unpack_archive(compressed_file, base_dir) # A source coming from a CLI and downloaded @@ -84,7 +84,7 @@ def download_from_url(): with open(tarpath, "wb") as f: f.write(response.raw.read()) -shutil.unpack_archive(tarpath, base_dir) # $result=BAD +shutil.unpack_archive(tarpath, base_dir) # the django upload functionality # see HttpRequest.FILES: https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.HttpRequest.FILES @@ -102,14 +102,14 @@ def simple_upload(request): with open(savepath, 'wb+') as wfile: for chunk in request.FILES["ufile1"].chunks(): wfile.write(chunk) - shutil.unpack_archive(savepath, base_dir) # $result=BAD + shutil.unpack_archive(savepath, base_dir) # Write in binary the uploaded tarball myfile = request.FILES.get("ufile1") file_path = os.path.join(base_dir, "tarball.tar") with file_path.open('wb') as f: f.write(myfile.read()) - shutil.unpack_archive(file_path, base_dir) # $result=BAD + shutil.unpack_archive(file_path, base_dir) # Save uploaded files using FileSystemStorage Django API # see FileSystemStorage: https://docs.djangoproject.com/en/4.1/ref/files/storage/#django.core.files.storage.FileSystemStorage @@ -117,7 +117,7 @@ def simple_upload(request): fs = FileSystemStorage() filename = fs.save(ufile.name, ufile) uploaded_file_path = fs.path(filename) - shutil.unpack_archive(uploaded_file_path, base_dir) # $result=BAD + shutil.unpack_archive(uploaded_file_path, base_dir) return render(request, 'simple_upload.html') @@ -139,7 +139,7 @@ def simple_upload(request): args = parser.parse_args() unsafe_filename_tar = args.filename with tarfile.TarFile(unsafe_filename_tar, mode="r") as tar: - tar.extractall(path="/tmp/unpack/", members=tar) # $result=BAD + tar.extractall(path="/tmp/unpack/", members=tar) tar = tarfile.open(unsafe_filename_tar) @@ -164,7 +164,7 @@ def simple_upload(request): if member.issym(): raise ValueError("But it is a symlink") result.append(member) - tar.extractall(path=tempfile.mkdtemp(), members=result) # $result=BAD + tar.extractall(path=tempfile.mkdtemp(), members=result) tar.close() @@ -173,7 +173,7 @@ def simple_upload(request): with open(tarpath, "wb") as f: f.write(response.raw.read()) target_dir = "/tmp/unpack" -tarfile.TarFile(tarpath, mode="r").extractall(path=target_dir) # $result=BAD +tarfile.TarFile(tarpath, mode="r").extractall(path=target_dir) from pathlib import Path @@ -198,4 +198,4 @@ def default_session() -> boto3.Session: target = cache_dir else: target = Path(tempfile.mkdtemp()) - shutil.unpack_archive(tmp.name, target) # $result=BAD \ No newline at end of file + shutil.unpack_archive(tmp.name, target) \ No newline at end of file diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.qlref b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.qlref similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-022/UnsafeUnpack.qlref rename to python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.qlref diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected deleted file mode 100644 index 3875da4e143c..000000000000 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/DataflowQueryTest.expected +++ /dev/null @@ -1,2 +0,0 @@ -missingAnnotationOnSink -failures From 518684b73685f4529587de9b5af8acb2450f8046 Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Sun, 12 Feb 2023 21:26:12 +0100 Subject: [PATCH 51/52] Put back the annotation result=BAD --- .../CWE-022-UnsafeUnpacking/UnsafeUnpack.py | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.py b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.py index c83714b764c4..6b533462d23c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/UnsafeUnpack.py @@ -16,7 +16,7 @@ def download_from_url(): with open(tarpath, "wb") as f: f.write(response.raw.read()) untarredpath = "/tmp/tmp123" - shutil.unpack_archive(tarpath, untarredpath) + shutil.unpack_archive(tarpath, untarredpath) # $result=BAD # A source catching an S3 filename download @@ -31,7 +31,7 @@ def download_from_url(): s3 = boto3.client('s3') s3.download_file(bucket_name, remote_ziped_name, local_ziped_path) -shutil.unpack_archive(local_ziped_path, base_dir) +shutil.unpack_archive(local_ziped_path, base_dir) # $result=BAD # wget @@ -45,11 +45,11 @@ def download_from_url(): # download(url, out, bar) contains out parameter wget.download(url, compressed_file) -shutil.unpack_archive(compressed_file, base_dir) +shutil.unpack_archive(compressed_file, base_dir) # $result=BAD # download(url) returns filename compressed_file = wget.download(url) -shutil.unpack_archive(compressed_file, base_dir) +shutil.unpack_archive(compressed_file, base_dir) # $result=BAD # A source coming from a CLI argparse module @@ -63,7 +63,7 @@ def download_from_url(): args = parser.parse_args() compressed_file = args.filename -shutil.unpack_archive(compressed_file, base_dir) +shutil.unpack_archive(compressed_file, base_dir) # $result=BAD # A source coming from a CLI and downloaded @@ -84,7 +84,7 @@ def download_from_url(): with open(tarpath, "wb") as f: f.write(response.raw.read()) -shutil.unpack_archive(tarpath, base_dir) +shutil.unpack_archive(tarpath, base_dir) # $result=BAD # the django upload functionality # see HttpRequest.FILES: https://docs.djangoproject.com/en/4.1/ref/request-response/#django.http.HttpRequest.FILES @@ -102,14 +102,14 @@ def simple_upload(request): with open(savepath, 'wb+') as wfile: for chunk in request.FILES["ufile1"].chunks(): wfile.write(chunk) - shutil.unpack_archive(savepath, base_dir) + shutil.unpack_archive(savepath, base_dir) # $result=BAD # Write in binary the uploaded tarball myfile = request.FILES.get("ufile1") file_path = os.path.join(base_dir, "tarball.tar") with file_path.open('wb') as f: f.write(myfile.read()) - shutil.unpack_archive(file_path, base_dir) + shutil.unpack_archive(file_path, base_dir) # $result=BAD # Save uploaded files using FileSystemStorage Django API # see FileSystemStorage: https://docs.djangoproject.com/en/4.1/ref/files/storage/#django.core.files.storage.FileSystemStorage @@ -117,7 +117,7 @@ def simple_upload(request): fs = FileSystemStorage() filename = fs.save(ufile.name, ufile) uploaded_file_path = fs.path(filename) - shutil.unpack_archive(uploaded_file_path, base_dir) + shutil.unpack_archive(uploaded_file_path, base_dir) # $result=BAD return render(request, 'simple_upload.html') @@ -139,7 +139,7 @@ def simple_upload(request): args = parser.parse_args() unsafe_filename_tar = args.filename with tarfile.TarFile(unsafe_filename_tar, mode="r") as tar: - tar.extractall(path="/tmp/unpack/", members=tar) + tar.extractall(path="/tmp/unpack/", members=tar) # $result=BAD tar = tarfile.open(unsafe_filename_tar) @@ -164,7 +164,7 @@ def simple_upload(request): if member.issym(): raise ValueError("But it is a symlink") result.append(member) - tar.extractall(path=tempfile.mkdtemp(), members=result) + tar.extractall(path=tempfile.mkdtemp(), members=result) # $result=BAD tar.close() @@ -173,7 +173,7 @@ def simple_upload(request): with open(tarpath, "wb") as f: f.write(response.raw.read()) target_dir = "/tmp/unpack" -tarfile.TarFile(tarpath, mode="r").extractall(path=target_dir) +tarfile.TarFile(tarpath, mode="r").extractall(path=target_dir) # $result=BAD from pathlib import Path @@ -198,4 +198,4 @@ def default_session() -> boto3.Session: target = cache_dir else: target = Path(tempfile.mkdtemp()) - shutil.unpack_archive(tmp.name, target) \ No newline at end of file + shutil.unpack_archive(tmp.name, target) # $result=BAD \ No newline at end of file From d7af80136e685141b9ec26286f081bd7bab2cded Mon Sep 17 00:00:00 2001 From: Sim4n6 Date: Sun, 12 Feb 2023 21:27:20 +0100 Subject: [PATCH 52/52] Fail tests when missing annotation on sink orfail --- .../DataflowQueryTest.expected | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected index 5a1382ed87c4..3875da4e143c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022-UnsafeUnpacking/DataflowQueryTest.expected @@ -1,15 +1,2 @@ missingAnnotationOnSink failures -| UnsafeUnpack.py:19:35:19:41 | ControlFlowNode for tarpath | Unexpected result: result=BAD | -| UnsafeUnpack.py:34:23:34:38 | ControlFlowNode for local_ziped_path | Unexpected result: result=BAD | -| UnsafeUnpack.py:48:23:48:37 | ControlFlowNode for compressed_file | Unexpected result: result=BAD | -| UnsafeUnpack.py:52:23:52:37 | ControlFlowNode for compressed_file | Unexpected result: result=BAD | -| UnsafeUnpack.py:66:23:66:37 | ControlFlowNode for compressed_file | Unexpected result: result=BAD | -| UnsafeUnpack.py:87:23:87:29 | ControlFlowNode for tarpath | Unexpected result: result=BAD | -| UnsafeUnpack.py:105:35:105:42 | ControlFlowNode for savepath | Unexpected result: result=BAD | -| UnsafeUnpack.py:112:35:112:43 | ControlFlowNode for file_path | Unexpected result: result=BAD | -| UnsafeUnpack.py:120:41:120:58 | ControlFlowNode for uploaded_file_path | Unexpected result: result=BAD | -| UnsafeUnpack.py:142:49:142:51 | ControlFlowNode for tar | Unexpected result: result=BAD | -| UnsafeUnpack.py:167:67:167:72 | ControlFlowNode for result | Unexpected result: result=BAD | -| UnsafeUnpack.py:176:1:176:34 | ControlFlowNode for Attribute() | Unexpected result: result=BAD | -| UnsafeUnpack.py:201:29:201:36 | ControlFlowNode for Attribute | Unexpected result: result=BAD |