From 5ed5e0b105b18a75e5069340a7a5f5320346baac Mon Sep 17 00:00:00 2001 From: root Date: Sun, 13 Feb 2022 16:44:27 -0500 Subject: [PATCH 01/64] Add query to detect ZipSlip --- .../Security/CWE-022/ZipSlip.qhelp | 57 +++++++++++++++++++ .../Security/CWE-022/ZipSlipCheck.ql | 49 ++++++++++++++++ .../Security/CWE-022/ZipSlipCheckLib.ql | 35 ++++++++++++ .../Security/CWE-022/zipslip_bad.py | 9 +++ .../Security/CWE-022/zipslip_good.py | 6 ++ 5 files changed, 156 insertions(+) create mode 100644 python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp create mode 100644 python/ql/src/experimental/Security/CWE-022/ZipSlipCheck.ql create mode 100644 python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql create mode 100644 python/ql/src/experimental/Security/CWE-022/zipslip_bad.py create mode 100644 python/ql/src/experimental/Security/CWE-022/zipslip_good.py diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp b/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp new file mode 100644 index 00000000000..dc4b4963a9f --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp @@ -0,0 +1,57 @@ + + + + +

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

+ +

Zip archives 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 Zip archive contains a file entry ..\sneaky-file, and the Zip archive +is extracted to the directory c:\output, then naively combining the paths would result +in an output file path of c:\output\..\sneaky-file, which would cause the file to be +written to c:\sneaky-file.

+ +
+ + +

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

+ +

The recommended way of writing an output file from a Zip archive entry is to use +this function instead of extract() or extractall(). +

+ +
+ + +

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

+ + + +

To fix this vulnerability, we need to this function extractall(). +

+ + + +
+ +
  • +Snyk: +Zip Slip Vulnerability. +
  • + + +
    diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlipCheck.ql b/python/ql/src/experimental/Security/CWE-022/ZipSlipCheck.ql new file mode 100644 index 00000000000..3c3a24fdca8 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlipCheck.ql @@ -0,0 +1,49 @@ +/** + * @name Arbitrary file write during archive extraction ("Zip Slip") + * @description Extracting files from a malicious archive without validating that the + * destination file path is within the destination directory can cause files outside + * the destination directory to be overwritten. + * @kind path-problem + * @id py/zipslip + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @tags security + * external/cwe/cwe-022 + */ + +import python +import semmle.python.dataflow.new.DataFlow +import semmle.python.dataflow.new.TaintTracking +import semmle.python.ApiGraphs +import ZipSlipCheckLib +import DataFlow::PathGraph + +/** + * Taint-tracking configuration tracing flow from opening a zipfile to copy to another place. + */ + +class ZipSlipConfig extends TaintTracking::Configuration { + ZipSlipConfig() { this = "ZipSlipConfig" } + + override predicate isSource(DataFlow::Node source) { + source instanceof OpenZipFile + } + + override predicate isSink(DataFlow::Node sink) { + sink instanceof CopyZipFile + } + + override predicate isSanitizer(DataFlow::Node node) { + exists(Subscript ss | + ss.getObject().(Call).getFunc().(Attribute).getName().matches("%path") and + ss = node.asExpr() + ) + } +} +from ZipSlipConfig config, DataFlow::PathNode source, + DataFlow::PathNode sink +where config.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Extraction of zipfile from $@", source.getNode(), + "a potentially untrusted source" + diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql b/python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql new file mode 100644 index 00000000000..206648b8a0c --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql @@ -0,0 +1,35 @@ +private import python +private import semmle.python.Concepts +private import semmle.python.ApiGraphs +private import semmle.python.dataflow.new.DataFlow +private import semmle.python.dataflow.new.RemoteFlowSources + +abstract class CopyZipFile extends DataFlow::Node { } + +abstract class OpenZipFile extends DataFlow::CallCfgNode { } + +private class CopyZip extends CopyZipFile { + CopyZip() { + exists(DataFlow::CallCfgNode call, DataFlow::Node pred | + call = API::moduleImport("shutil").getMember([ + // these are used to copy files + "copyfile", "copy", "copy2", "copytree", "copyfileobj", + // these are used to move files + "move"]) + .getACall() and + + call.getArg(0) = pred + ) + } + +} +private class OpenZip extends OpenZipFile { + OpenZip() { + exists(DataFlow::CallCfgNode call | + call = API::moduleImport("zipfile").getMember("ZipFile").getMember("open").getACall() + ) + } + +} + + diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py new file mode 100644 index 00000000000..678c8522c95 --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -0,0 +1,9 @@ +import zipfile +import shutil + + +zf = zipfile.ZipFile(filename) +with zf.open() as zipf: + #BAD : This could write any file on the filesystem. + for entry in zipf: + shutil.copyfileobj(entry, "/tmp/unpack/") diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_good.py b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py new file mode 100644 index 00000000000..91627d1fe0e --- /dev/null +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py @@ -0,0 +1,6 @@ +import zipfile + +zf = zipfile.ZipFile(filename) +with zipfile.open() as zipf: + for entry in zipf: + zipf.extract(entry, "/tmp/unpack/") From d0d14be6931f37fa8978c244624d4f072b9e739c Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Sat, 26 Feb 2022 18:25:13 +0100 Subject: [PATCH 02/64] Update ZipSlip.qhelp --- python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp b/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp index dc4b4963a9f..63d43a51541 100644 --- a/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp @@ -28,8 +28,7 @@ written to c:\sneaky-file.

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

    -

    The recommended way of writing an output file from a Zip archive entry is to use -this function instead of extract() or extractall(). +

    The recommended way of writing an output file from a Zip archive entry is to call extract() or extractall().

    @@ -41,7 +40,7 @@ In this example an archive is extracted without validating file paths. -

    To fix this vulnerability, we need to this function extractall(). +

    To fix this vulnerability, we need to call the function extractall().

    From c207294dfc28de8f7d56a438c42ab49936006e63 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Sat, 26 Feb 2022 18:31:22 +0100 Subject: [PATCH 03/64] Update zipslip_good.py --- python/ql/src/experimental/Security/CWE-022/zipslip_good.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_good.py b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py index 91627d1fe0e..0f02abdc7d8 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_good.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py @@ -2,5 +2,5 @@ import zipfile zf = zipfile.ZipFile(filename) with zipfile.open() as zipf: - for entry in zipf: + for entry in zipf: zipf.extract(entry, "/tmp/unpack/") From 21f6ad5190e28d36b39334336b0262e31596f4c0 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:01:06 +0100 Subject: [PATCH 04/64] Update and rename ZipSlipCheck.ql to ZipSlip.ql --- .../CWE-022/{ZipSlipCheck.ql => ZipSlip.ql} | 24 +++---------------- 1 file changed, 3 insertions(+), 21 deletions(-) rename python/ql/src/experimental/Security/CWE-022/{ZipSlipCheck.ql => ZipSlip.ql} (60%) diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlipCheck.ql b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql similarity index 60% rename from python/ql/src/experimental/Security/CWE-022/ZipSlipCheck.ql rename to python/ql/src/experimental/Security/CWE-022/ZipSlip.ql index 3c3a24fdca8..60f52b1d6e3 100644 --- a/python/ql/src/experimental/Security/CWE-022/ZipSlipCheck.ql +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql @@ -19,31 +19,13 @@ import semmle.python.ApiGraphs import ZipSlipCheckLib import DataFlow::PathGraph -/** - * Taint-tracking configuration tracing flow from opening a zipfile to copy to another place. - */ +import python +import experimental.semmle.python.security.ZipSlip +import DataFlow::PathGraph -class ZipSlipConfig extends TaintTracking::Configuration { - ZipSlipConfig() { this = "ZipSlipConfig" } - override predicate isSource(DataFlow::Node source) { - source instanceof OpenZipFile - } - - override predicate isSink(DataFlow::Node sink) { - sink instanceof CopyZipFile - } - - override predicate isSanitizer(DataFlow::Node node) { - exists(Subscript ss | - ss.getObject().(Call).getFunc().(Attribute).getName().matches("%path") and - ss = node.asExpr() - ) - } -} from ZipSlipConfig config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) select sink.getNode(), source, sink, "Extraction of zipfile from $@", source.getNode(), "a potentially untrusted source" - From 3eae13161fd6335d617ebaef85d908bd0c2f2af8 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:01:34 +0100 Subject: [PATCH 05/64] Delete ZipSlipCheckLib.ql --- .../Security/CWE-022/ZipSlipCheckLib.ql | 35 ------------------- 1 file changed, 35 deletions(-) delete mode 100644 python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql b/python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql deleted file mode 100644 index 206648b8a0c..00000000000 --- a/python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql +++ /dev/null @@ -1,35 +0,0 @@ -private import python -private import semmle.python.Concepts -private import semmle.python.ApiGraphs -private import semmle.python.dataflow.new.DataFlow -private import semmle.python.dataflow.new.RemoteFlowSources - -abstract class CopyZipFile extends DataFlow::Node { } - -abstract class OpenZipFile extends DataFlow::CallCfgNode { } - -private class CopyZip extends CopyZipFile { - CopyZip() { - exists(DataFlow::CallCfgNode call, DataFlow::Node pred | - call = API::moduleImport("shutil").getMember([ - // these are used to copy files - "copyfile", "copy", "copy2", "copytree", "copyfileobj", - // these are used to move files - "move"]) - .getACall() and - - call.getArg(0) = pred - ) - } - -} -private class OpenZip extends OpenZipFile { - OpenZip() { - exists(DataFlow::CallCfgNode call | - call = API::moduleImport("zipfile").getMember("ZipFile").getMember("open").getACall() - ) - } - -} - - From abe25da3df1a652397c16b8e6c3d12b62d00139e Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:04:02 +0100 Subject: [PATCH 06/64] Create ZipSlip.qll --- .../experimental/semmle/python/security/ZipSlip.qll | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 python/ql/src/experimental/semmle/python/security/ZipSlip.qll diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll new file mode 100644 index 00000000000..7fafd8e7c7e --- /dev/null +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -0,0 +1,12 @@ +import python +import experimental.semmle.python.Concepts +import semmle.python.dataflow.new.DataFlow +import semmle.python.dataflow.new.TaintTracking + +class ZipSlipConfig extends TaintTracking::Configuration { + ZipSlipConfig() { this = "ZipSlipConfig" } + + override predicate isSource(DataFlow::Node source) { source = any(OpenFile openfile).getAPathArgument() } + + override predicate isSink(DataFlow::Node sink) { sink = any(ZipFile zipfile).getAnInput() } +} From 76bd3317eb34dc35d27c33ed6b0679486da24199 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:05:30 +0100 Subject: [PATCH 07/64] Create Zip.qll --- .../semmle/python/frameworks/Zip.qll | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 python/ql/src/experimental/semmle/python/frameworks/Zip.qll diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll new file mode 100644 index 00000000000..15a751e232d --- /dev/null +++ b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll @@ -0,0 +1,28 @@ +private import python +private import experimental.semmle.python.Concepts +private import semmle.python.dataflow.new.DataFlow +private import semmle.python.ApiGraphs + + +private module Zip { + private API::Node shutil() { result = API::moduleImport("shutil") } + + private class CopyFiles extends DataFlow::CallCfgNode, OpenFile::Range { + CopyFiles() { this = shutil().getMember(["copyfile", "copy", "copy2", "copytree", "move"]).getACall() } + override DataFlow::Node getAPathArgument() { result in [this.getArg(0), this.getArgByName("src"), this.getArg(1), this.getArgByName("dst")] } + } + + private class CopyFileobj extends DataFlow::CallCfgNode, OpenFile::Range { + CopyFileobj() { this = shutil().getMember("copyfileobj").getACall() } + override DataFlow::Node getAPathArgument() { result in [this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst")] } + } + + private class OpenZipFile extends DataFlow::CallCfgNode, ZipFile::Range { + OpenZipFile() { + this = API::moduleImport("zipfile").getMember("ZipFile").getMember("open").getACall() or + this = API::moduleImport("zipfile").getMember("ZipFile").getACall() + } + override DataFlow::Node getAnInput() { result = this.getArg(0) } + } +} + From b29936716d54609f200ca1164649ad7e79f6a3f6 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:06:22 +0100 Subject: [PATCH 08/64] Update Frameworks.qll --- python/ql/src/experimental/semmle/python/Frameworks.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ql/src/experimental/semmle/python/Frameworks.qll b/python/ql/src/experimental/semmle/python/Frameworks.qll index 90fe21cc933..30cf73eb5ef 100644 --- a/python/ql/src/experimental/semmle/python/Frameworks.qll +++ b/python/ql/src/experimental/semmle/python/Frameworks.qll @@ -13,3 +13,4 @@ private import experimental.semmle.python.frameworks.JWT private import experimental.semmle.python.libraries.PyJWT private import experimental.semmle.python.libraries.Authlib private import experimental.semmle.python.libraries.PythonJose +private import experimental.semmle.python.frameworks.Zip From 67d34988917018bdb9d33cd06b00308d6353b9be Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:07:37 +0100 Subject: [PATCH 09/64] Update ZipSlip.ql --- python/ql/src/experimental/Security/CWE-022/ZipSlip.ql | 7 ------- 1 file changed, 7 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql index 60f52b1d6e3..8ec4004e3fb 100644 --- a/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql @@ -12,13 +12,6 @@ * external/cwe/cwe-022 */ -import python -import semmle.python.dataflow.new.DataFlow -import semmle.python.dataflow.new.TaintTracking -import semmle.python.ApiGraphs -import ZipSlipCheckLib -import DataFlow::PathGraph - import python import experimental.semmle.python.security.ZipSlip import DataFlow::PathGraph From c22b032bbea0018bc1b5a02d2109b30bcd808320 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:11:33 +0100 Subject: [PATCH 10/64] Update Zip.qll --- python/ql/src/experimental/semmle/python/frameworks/Zip.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll index 15a751e232d..bb22f03a938 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll @@ -7,12 +7,12 @@ private import semmle.python.ApiGraphs private module Zip { private API::Node shutil() { result = API::moduleImport("shutil") } - private class CopyFiles extends DataFlow::CallCfgNode, OpenFile::Range { + private class CopyFiles extends DataFlow::CallCfgNode, CopyFile::Range { CopyFiles() { this = shutil().getMember(["copyfile", "copy", "copy2", "copytree", "move"]).getACall() } override DataFlow::Node getAPathArgument() { result in [this.getArg(0), this.getArgByName("src"), this.getArg(1), this.getArgByName("dst")] } } - private class CopyFileobj extends DataFlow::CallCfgNode, OpenFile::Range { + private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range { CopyFileobj() { this = shutil().getMember("copyfileobj").getACall() } override DataFlow::Node getAPathArgument() { result in [this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst")] } } From 85bcaa96cebc6e690e6e7ac96cf58579c60530ce Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:23:06 +0100 Subject: [PATCH 11/64] Update Concepts.qll --- .../experimental/semmle/python/Concepts.qll | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index 3b1f6072f0c..4dc5016e72e 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -14,6 +14,68 @@ private import semmle.python.dataflow.new.RemoteFlowSources private import semmle.python.dataflow.new.TaintTracking private import experimental.semmle.python.Frameworks + +/** Provides classes for modeling copying file related APIs. */ +module CopyFile { + /** + * A data flow node for copying file. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `CopyFile` instead. + */ + abstract class Range extends DataFlow::Node { + /** + * Gets the argument containing the path. + */ + abstract DataFlow::Node getAPathArgument(); + } +} + +/** + * A data flow node for copying file. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `CopyFile::Range` instead. + */ +class CopyFile extends DataFlow::Node { + CopyFile::Range range; + + CopyFile() { this = range } + + DataFlow::Node getAPathArgument() { result = range.getAPathArgument() } +} + +/** Provides classes for modeling zip related APIs. */ +module ZipFile { + /** + * A data flow node for zipfile. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `ZipFile` instead. + */ + abstract class Range extends DataFlow::Node { + /** + * Get the parameter value of the zip function. + */ + abstract DataFlow::Node getAnInput(); + + } +} + +/** + * A data flow node for zip. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `ZipFile::Range` instead. + */ +class ZipFile extends DataFlow::Node { + ZipFile::Range range; + + ZipFile() { this = range } + + DataFlow::Node getAnInput() { result = range.getAnInput() } +} + /** Provides classes for modeling log related APIs. */ module LogOutput { /** From 70c0c7e4619d87228022aa893351d01b4ac69490 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 1 Mar 2022 00:24:33 +0100 Subject: [PATCH 12/64] Update zipslip_bad.py --- python/ql/src/experimental/Security/CWE-022/zipslip_bad.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py index 678c8522c95..b08d43c5e05 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -6,4 +6,4 @@ zf = zipfile.ZipFile(filename) with zf.open() as zipf: #BAD : This could write any file on the filesystem. for entry in zipf: - shutil.copyfileobj(entry, "/tmp/unpack/") + shutil.copy(entry, "/tmp/unpack/") From c8f73ec8456e95f7514a8c587348f4014c66e143 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Wed, 2 Mar 2022 18:08:32 +0100 Subject: [PATCH 13/64] Create ZipSlip.qlref --- .../test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref | 1 + 1 file changed, 1 insertion(+) create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref new file mode 100644 index 00000000000..717dc9d0f10 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref @@ -0,0 +1 @@ +experimental/Security/CWE-022/ZipSlip.ql From aef1df122bd52af69cda416c05a3a920f58be46d Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Wed, 2 Mar 2022 18:09:45 +0100 Subject: [PATCH 14/64] Create zipslip_bad.py --- .../query-tests/Security/CWE-022/zipslip_bad.py | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py 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/zipslip_bad.py new file mode 100644 index 00000000000..5a717277948 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -0,0 +1,9 @@ +import zipfile +import shutil + + +zf = zipfile.ZipFile(filename) +with zf.open() as zipf: + #BAD : This could write any file on the filesystem. + for entry in zipf: + shutil.copy(entry, "/tmp/unpack/") From c45b67c3162b04e244ad0864c9c12d2cb71cd1e9 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Wed, 2 Mar 2022 18:10:24 +0100 Subject: [PATCH 15/64] Create zipslip_good.py --- .../query-tests/Security/CWE-022/zipslip_good.py | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.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/zipslip_good.py new file mode 100644 index 00000000000..0f02abdc7d8 --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py @@ -0,0 +1,6 @@ +import zipfile + +zf = zipfile.ZipFile(filename) +with zipfile.open() as zipf: + for entry in zipf: + zipf.extract(entry, "/tmp/unpack/") From 5e14d89714dda4c381f9219e357855387e0e1500 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Thu, 3 Mar 2022 17:12:06 +0100 Subject: [PATCH 16/64] Update ZipSlip.qll --- python/ql/src/experimental/semmle/python/security/ZipSlip.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index 7fafd8e7c7e..307c7ef1df0 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -6,7 +6,7 @@ import semmle.python.dataflow.new.TaintTracking class ZipSlipConfig extends TaintTracking::Configuration { ZipSlipConfig() { this = "ZipSlipConfig" } - override predicate isSource(DataFlow::Node source) { source = any(OpenFile openfile).getAPathArgument() } + override predicate isSource(DataFlow::Node source) { source = any(CopyFile copyfile).getAPathArgument() } override predicate isSink(DataFlow::Node sink) { sink = any(ZipFile zipfile).getAnInput() } } From be7c619ca8c1c4a2aaa58e317714d5e566e739c9 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Fri, 4 Mar 2022 00:48:45 +0100 Subject: [PATCH 17/64] Update zipslip_bad.py --- .../query-tests/Security/CWE-022/zipslip_bad.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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/zipslip_bad.py index 5a717277948..557a098eea1 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -1,9 +1,9 @@ import zipfile import shutil - -zf = zipfile.ZipFile(filename) -with zf.open() as zipf: +def unzip(filename): + zf = zipfile.ZipFile() + with zf.open(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.copy(entry, "/tmp/unpack/") + for entry in zipf: + shutil.copyfileobj(entry, "/tmp/unpack/") From 466f75bad87f395d5302a79e00a0f99b6a5759c3 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Sun, 6 Mar 2022 23:53:00 +0100 Subject: [PATCH 18/64] Update Concepts.qll --- .../experimental/semmle/python/Concepts.qll | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index 4dc5016e72e..20e11ff42aa 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -45,36 +45,6 @@ class CopyFile extends DataFlow::Node { DataFlow::Node getAPathArgument() { result = range.getAPathArgument() } } -/** Provides classes for modeling zip related APIs. */ -module ZipFile { - /** - * A data flow node for zipfile. - * - * Extend this class to model new APIs. If you want to refine existing API models, - * extend `ZipFile` instead. - */ - abstract class Range extends DataFlow::Node { - /** - * Get the parameter value of the zip function. - */ - abstract DataFlow::Node getAnInput(); - - } -} - -/** - * A data flow node for zip. - * - * Extend this class to refine existing API models. If you want to model new APIs, - * extend `ZipFile::Range` instead. - */ -class ZipFile extends DataFlow::Node { - ZipFile::Range range; - - ZipFile() { this = range } - - DataFlow::Node getAnInput() { result = range.getAnInput() } -} /** Provides classes for modeling log related APIs. */ module LogOutput { From 91b5f2ad34c11aaa368222d133e250048920454a Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Sun, 6 Mar 2022 23:54:46 +0100 Subject: [PATCH 19/64] Update Zip.qll --- .../semmle/python/frameworks/Zip.qll | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll index bb22f03a938..3ceffd60900 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll @@ -5,24 +5,24 @@ private import semmle.python.ApiGraphs private module Zip { - private API::Node shutil() { result = API::moduleImport("shutil") } private class CopyFiles extends DataFlow::CallCfgNode, CopyFile::Range { - CopyFiles() { this = shutil().getMember(["copyfile", "copy", "copy2", "copytree", "move"]).getACall() } - override DataFlow::Node getAPathArgument() { result in [this.getArg(0), this.getArgByName("src"), this.getArg(1), this.getArgByName("dst")] } + CopyFiles() { + this = API::moduleImport("shutil").getMember(["copyfile", "copy", "copy2", "copytree", "move"]).getACall() + } + override DataFlow::Node getAPathArgument() { + result in [this.getArg(0), this.getArgByName("src"), this.getArg(1), this.getArgByName("dst")] + } } private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range { - CopyFileobj() { this = shutil().getMember("copyfileobj").getACall() } - override DataFlow::Node getAPathArgument() { result in [this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst")] } + CopyFileobj() { + this = API::moduleImport("shutil").getMember("copyfileobj").getACall() + } + override DataFlow::Node getAPathArgument() { + result in [this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst")] + } } - private class OpenZipFile extends DataFlow::CallCfgNode, ZipFile::Range { - OpenZipFile() { - this = API::moduleImport("zipfile").getMember("ZipFile").getMember("open").getACall() or - this = API::moduleImport("zipfile").getMember("ZipFile").getACall() - } - override DataFlow::Node getAnInput() { result = this.getArg(0) } - } } From 8649375be31b0d6b3fd4e4e5a13efdb83d7e950f Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Sun, 6 Mar 2022 23:56:02 +0100 Subject: [PATCH 20/64] Update ZipSlip.qll --- .../experimental/semmle/python/security/ZipSlip.qll | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index 307c7ef1df0..31efe7fce0b 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -6,7 +6,11 @@ import semmle.python.dataflow.new.TaintTracking class ZipSlipConfig extends TaintTracking::Configuration { ZipSlipConfig() { this = "ZipSlipConfig" } - override predicate isSource(DataFlow::Node source) { source = any(CopyFile copyfile).getAPathArgument() } - - override predicate isSink(DataFlow::Node sink) { sink = any(ZipFile zipfile).getAnInput() } + override predicate isSource(DataFlow::Node source) { + source = API::moduleImport("zipfile").getMember("ZipFile").getACall() + } + + override predicate isSink(DataFlow::Node sink) { + sink = any(CopyFile copyfile).getAPathArgument() + } } From 7f2d242702aa265215522ea08032f2325f807085 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Sun, 6 Mar 2022 23:59:11 +0100 Subject: [PATCH 21/64] Update zipslip_good.py --- .../query-tests/Security/CWE-022/zipslip_good.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) 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/zipslip_good.py index 0f02abdc7d8..b97ec283c8f 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py @@ -1,6 +1,10 @@ import zipfile -zf = zipfile.ZipFile(filename) -with zipfile.open() as zipf: - for entry in zipf: - zipf.extract(entry, "/tmp/unpack/") +def unzip(filename, dir): + zf = zipfile.ZipFile(filename) + zf.extractall(dir) + + +def unzip1(filename, dir): + zf = zipfile.ZipFile(filename) + zf.extract(dir) From 908db6a05fcd354f01ddf011d473b0bf00878e0b Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:01:09 +0100 Subject: [PATCH 22/64] Update zipslip_bad.py --- .../query-tests/Security/CWE-022/zipslip_bad.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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/zipslip_bad.py index 557a098eea1..cb1441fab59 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -2,8 +2,8 @@ import zipfile import shutil def unzip(filename): - zf = zipfile.ZipFile() - with zf.open(filename) as zipf: + + with zipfile.ZipFile(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.copyfileobj(entry, "/tmp/unpack/") + for entry in zipf: + shutil.copy(entry, "/tmp/unpack/") From d7dacfc6bdb80a9ad09bd58c20e97516a62da58f Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:01:55 +0100 Subject: [PATCH 23/64] Update zipslip_good.py --- .../experimental/Security/CWE-022/zipslip_good.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_good.py b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py index 0f02abdc7d8..b97ec283c8f 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_good.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py @@ -1,6 +1,10 @@ import zipfile -zf = zipfile.ZipFile(filename) -with zipfile.open() as zipf: - for entry in zipf: - zipf.extract(entry, "/tmp/unpack/") +def unzip(filename, dir): + zf = zipfile.ZipFile(filename) + zf.extractall(dir) + + +def unzip1(filename, dir): + zf = zipfile.ZipFile(filename) + zf.extract(dir) From b9b52d4c7c0efa976f9327fbf50a96bee6feaf82 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:02:50 +0100 Subject: [PATCH 24/64] Update zipslip_bad.py --- .../ql/src/experimental/Security/CWE-022/zipslip_bad.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py index b08d43c5e05..cb1441fab59 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -1,9 +1,9 @@ import zipfile import shutil +def unzip(filename): -zf = zipfile.ZipFile(filename) -with zf.open() as zipf: + with zipfile.ZipFile(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.copy(entry, "/tmp/unpack/") + for entry in zipf: + shutil.copy(entry, "/tmp/unpack/") From b7d4715c4eb4711ec38a5a744bccaf62ae6d77fa Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:06:24 +0100 Subject: [PATCH 25/64] Create ZipSlip.expected --- .../query-tests/Security/CWE-022/ZipSlip.expected | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected new file mode 100644 index 00000000000..f3b254bf0aa --- /dev/null +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected @@ -0,0 +1,10 @@ +edges +| zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | zipslip_bad.py:8:11:8:15 | SSA variable entry | +| zipslip_bad.py:8:11:8:15 | SSA variable entry | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | +nodes +| zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:8:11:8:15 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +subpaths +#select +| zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | a potentially untrusted source | From e8449d8f403a9b31532ba4c3f3c82a49e8e8f887 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:23:03 +0100 Subject: [PATCH 26/64] Update zipslip_bad.py --- .../query-tests/Security/CWE-022/zipslip_bad.py | 8 ++++++++ 1 file changed, 8 insertions(+) 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/zipslip_bad.py index cb1441fab59..65f74d51f0a 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -7,3 +7,11 @@ def unzip(filename): #BAD : This could write any file on the filesystem. for entry in zipf: shutil.copy(entry, "/tmp/unpack/") + +def unzip1(filename): + + + with zipfile.ZipFile(filename) as zipf: + for entry in zipf: + with open(entry, 'wb') as dstfile: + shutil.copyfileobj(zipf, dstfile) From ce7923c8b3b0bbe0b8829cbcd08c5727422d01b2 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:23:19 +0100 Subject: [PATCH 27/64] Update zipslip_bad.py --- .../ql/src/experimental/Security/CWE-022/zipslip_bad.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py index cb1441fab59..65f74d51f0a 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -7,3 +7,11 @@ def unzip(filename): #BAD : This could write any file on the filesystem. for entry in zipf: shutil.copy(entry, "/tmp/unpack/") + +def unzip1(filename): + + + with zipfile.ZipFile(filename) as zipf: + for entry in zipf: + with open(entry, 'wb') as dstfile: + shutil.copyfileobj(zipf, dstfile) From 6233309028dd0151a9da86013da560e78769ab3d Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:23:48 +0100 Subject: [PATCH 28/64] Update ZipSlip.expected --- .../query-tests/Security/CWE-022/ZipSlip.expected | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected index f3b254bf0aa..0e187255ba5 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected @@ -1,10 +1,14 @@ edges | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | zipslip_bad.py:8:11:8:15 | SSA variable entry | | zipslip_bad.py:8:11:8:15 | SSA variable entry | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | +| zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | nodes | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | zipslip_bad.py:8:11:8:15 | SSA variable entry | semmle.label | SSA variable entry | | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | semmle.label | ControlFlowNode for zipf | subpaths #select | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | Extraction of zipfile from $@ | zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | a potentially untrusted source | From 0d9436892a9eecce7413e27db87a28654f4cabe1 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:24:25 +0100 Subject: [PATCH 29/64] Update zipslip_bad.py --- python/ql/src/experimental/Security/CWE-022/zipslip_bad.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py index 65f74d51f0a..580ff533c75 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -2,15 +2,12 @@ import zipfile import shutil def unzip(filename): - with zipfile.ZipFile(filename) as zipf: #BAD : This could write any file on the filesystem. for entry in zipf: shutil.copy(entry, "/tmp/unpack/") def unzip1(filename): - - with zipfile.ZipFile(filename) as zipf: for entry in zipf: with open(entry, 'wb') as dstfile: From 35a1c80ceb1475df06c13edcb6507ded25e12774 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 00:24:45 +0100 Subject: [PATCH 30/64] Update zipslip_bad.py --- .../experimental/query-tests/Security/CWE-022/zipslip_bad.py | 3 --- 1 file changed, 3 deletions(-) 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/zipslip_bad.py index 65f74d51f0a..580ff533c75 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -2,15 +2,12 @@ import zipfile import shutil def unzip(filename): - with zipfile.ZipFile(filename) as zipf: #BAD : This could write any file on the filesystem. for entry in zipf: shutil.copy(entry, "/tmp/unpack/") def unzip1(filename): - - with zipfile.ZipFile(filename) as zipf: for entry in zipf: with open(entry, 'wb') as dstfile: From 6685c6b4b341b5526bad93f5385f3857e4002473 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 10:09:53 +0100 Subject: [PATCH 31/64] Update ZipSlip.qll --- .../ql/src/experimental/semmle/python/security/ZipSlip.qll | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index 31efe7fce0b..2a3b2b15e03 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -7,7 +7,10 @@ class ZipSlipConfig extends TaintTracking::Configuration { ZipSlipConfig() { this = "ZipSlipConfig" } override predicate isSource(DataFlow::Node source) { - source = API::moduleImport("zipfile").getMember("ZipFile").getACall() + source = API::moduleImport("zipfile").getMember("ZipFile").getACall() or + source = API::moduleImport("tarfile").getMember("open").getACall() or + source = API::moduleImport("gzip").getMember("open").getACall() or + source = API::moduleImport("bz2").getMember("open").getACall() } override predicate isSink(DataFlow::Node sink) { From 8402d661dff72f490074917b3f3aec2a1d7710c0 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 10:11:00 +0100 Subject: [PATCH 32/64] Update zipslip_bad.py --- .../Security/CWE-022/zipslip_bad.py | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) 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/zipslip_bad.py index 580ff533c75..5105c0c445e 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -1,14 +1,38 @@ -import zipfile +import tarfile import shutil +import bz2 +import gzip +import zipfile def unzip(filename): + + with tarfile.open(filename) as zipf: + #BAD : This could write any file on the filesystem. + for entry in zipf: + shutil.move(entry, "/tmp/unpack/") + +def unzip1(filename): + with gzip.open(filename) as zipf: + #BAD : This could write any file on the filesystem. + for entry in zipf: + shutil.copy2(entry, "/tmp/unpack/") + +def unzip2(filename): + with bz2.open(filename) as zipf: + #BAD : This could write any file on the filesystem. + for entry in zipf: + shutil.copyfile(entry, "/tmp/unpack/") + +def unzip3(filename): with zipfile.ZipFile(filename) as zipf: #BAD : This could write any file on the filesystem. for entry in zipf: shutil.copy(entry, "/tmp/unpack/") - -def unzip1(filename): + +def unzip4(filename): with zipfile.ZipFile(filename) as zipf: for entry in zipf: with open(entry, 'wb') as dstfile: shutil.copyfileobj(zipf, dstfile) + + From 3b8c7e89445d10a2752aa06fcab9c6e96c59e738 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Mon, 7 Mar 2022 10:11:34 +0100 Subject: [PATCH 33/64] Update ZipSlip.expected --- .../Security/CWE-022/ZipSlip.expected | 38 ++++++++++++++----- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected index 0e187255ba5..9f5cd92d316 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected @@ -1,14 +1,32 @@ edges -| zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | zipslip_bad.py:8:11:8:15 | SSA variable entry | -| zipslip_bad.py:8:11:8:15 | SSA variable entry | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | -| zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | +| zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | zipslip_bad.py:11:11:11:15 | SSA variable entry | +| zipslip_bad.py:11:11:11:15 | SSA variable entry | zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | +| zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | zipslip_bad.py:17:11:17:15 | SSA variable entry | +| zipslip_bad.py:17:11:17:15 | SSA variable entry | zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | +| zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | zipslip_bad.py:23:11:23:15 | SSA variable entry | +| zipslip_bad.py:23:11:23:15 | SSA variable entry | zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | +| zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | zipslip_bad.py:29:11:29:15 | SSA variable entry | +| zipslip_bad.py:29:11:29:15 | SSA variable entry | zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | +| zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | nodes -| zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:8:11:8:15 | SSA variable entry | semmle.label | SSA variable entry | -| zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | -| zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | semmle.label | ControlFlowNode for zipf | +| zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:11:11:11:15 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:17:11:17:15 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:23:11:23:15 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:29:11:29:15 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | semmle.label | ControlFlowNode for zipf | subpaths #select -| zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | zipslip_bad.py:9:23:9:27 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:6:9:6:33 | ControlFlowNode for Attribute() | a potentially untrusted source | -| zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | zipslip_bad.py:17:32:17:35 | ControlFlowNode for zipf | Extraction of zipfile from $@ | zipslip_bad.py:14:9:14:33 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | Extraction of zipfile from $@ | zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | a potentially untrusted source | From 23bd53a325f568bdbfdedae2bccf3e6811daf7b2 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 8 Mar 2022 23:55:17 +0100 Subject: [PATCH 34/64] Update zipslip_good.py --- .../query-tests/Security/CWE-022/zipslip_good.py | 10 ++++++++++ 1 file changed, 10 insertions(+) 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/zipslip_good.py index b97ec283c8f..f1561a4fd2c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py @@ -1,4 +1,6 @@ import zipfile +import tarfile +import shutil def unzip(filename, dir): zf = zipfile.ZipFile(filename) @@ -8,3 +10,11 @@ def unzip(filename, dir): def unzip1(filename, dir): zf = zipfile.ZipFile(filename) zf.extract(dir) + +def unzip2(filename): + with tarfile.open(filename) as tar: + for entry in tar: + #GOOD: Check that entry is safe + if os.path.isabs(entry.name) or ".." in entry.name: + raise ValueError("Illegal tar archive entry") + shutil.copy(entry, "/tmp/unpack/") From 27b9d6c752a6a4851c7964418568a191deee4cdf Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Tue, 8 Mar 2022 23:59:03 +0100 Subject: [PATCH 35/64] Update ZipSlip.qll --- .../ql/src/experimental/semmle/python/security/ZipSlip.qll | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index 2a3b2b15e03..f380e6481d7 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -9,8 +9,11 @@ class ZipSlipConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source = API::moduleImport("zipfile").getMember("ZipFile").getACall() or source = API::moduleImport("tarfile").getMember("open").getACall() or - source = API::moduleImport("gzip").getMember("open").getACall() or - source = API::moduleImport("bz2").getMember("open").getACall() + source = API::moduleImport("tarfile").getMember("TarFile").getACall() or + source = API::moduleImport("bz2").getMember("open").getACall() or + source = API::moduleImport("bz2").getMember("BZ2File").getACall() or + source = API::moduleImport("gzip").getMember("GzipFile").getACall() or + source = API::moduleImport("gzip").getMember("open").getACall() } override predicate isSink(DataFlow::Node sink) { From 475cca0d7e91f34b86a7f33774efc0262ba2508b Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Wed, 9 Mar 2022 00:00:52 +0100 Subject: [PATCH 36/64] Update ZipSlip.qll --- python/ql/src/experimental/semmle/python/security/ZipSlip.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index f380e6481d7..7517f441bdb 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -13,7 +13,9 @@ class ZipSlipConfig extends TaintTracking::Configuration { source = API::moduleImport("bz2").getMember("open").getACall() or source = API::moduleImport("bz2").getMember("BZ2File").getACall() or source = API::moduleImport("gzip").getMember("GzipFile").getACall() or - source = API::moduleImport("gzip").getMember("open").getACall() + source = API::moduleImport("gzip").getMember("open").getACall() or + source = API::moduleImport("lzma").getMember("open").getACall() or + source = API::moduleImport("lzma").getMember("LZMAFile").getACall() } override predicate isSink(DataFlow::Node sink) { From 0de1cef26e68de8802ed121321fe39b9b6fa5e9a Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Fri, 11 Mar 2022 14:03:17 +0100 Subject: [PATCH 37/64] Update ZipSlip.qll --- python/ql/src/experimental/semmle/python/security/ZipSlip.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index 7517f441bdb..4196b52c760 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -1,6 +1,7 @@ import python import experimental.semmle.python.Concepts import semmle.python.dataflow.new.DataFlow +import semmle.python.ApiGraphs import semmle.python.dataflow.new.TaintTracking class ZipSlipConfig extends TaintTracking::Configuration { From eb71cdf7a2665401afb0eaf5e20be8febaf0dfc4 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Fri, 11 Mar 2022 14:13:28 +0100 Subject: [PATCH 38/64] Update ZipSlip.ql --- python/ql/src/experimental/Security/CWE-022/ZipSlip.ql | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql index 8ec4004e3fb..e979d41aedd 100644 --- a/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql @@ -16,9 +16,8 @@ import python import experimental.semmle.python.security.ZipSlip import DataFlow::PathGraph - -from ZipSlipConfig config, DataFlow::PathNode source, - DataFlow::PathNode sink +from ZipSlipConfig config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) select sink.getNode(), source, sink, "Extraction of zipfile from $@", source.getNode(), "a potentially untrusted source" + From f092cd8d802094116e5a56c6082407a6e184d90d Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed532009@users.noreply.github.com> Date: Fri, 11 Mar 2022 14:15:05 +0100 Subject: [PATCH 39/64] Update Zip.qll --- .../semmle/python/frameworks/Zip.qll | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll index 3ceffd60900..7eec7f71cce 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll @@ -3,26 +3,30 @@ private import experimental.semmle.python.Concepts private import semmle.python.dataflow.new.DataFlow private import semmle.python.ApiGraphs - private module Zip { - private class CopyFiles extends DataFlow::CallCfgNode, CopyFile::Range { - CopyFiles() { - this = API::moduleImport("shutil").getMember(["copyfile", "copy", "copy2", "copytree", "move"]).getACall() - } - override DataFlow::Node getAPathArgument() { - result in [this.getArg(0), this.getArgByName("src"), this.getArg(1), this.getArgByName("dst")] - } - } - - private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range { - CopyFileobj() { - this = API::moduleImport("shutil").getMember("copyfileobj").getACall() - } - override DataFlow::Node getAPathArgument() { - result in [this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst")] - } + CopyFiles() { + this = + API::moduleImport("shutil") + .getMember(["copyfile", "copy", "copy2", "copytree", "move"]) + .getACall() + } + + override DataFlow::Node getAPathArgument() { + result in [this.getArg(0), this.getArgByName("src"), this.getArg(1), this.getArgByName("dst")] + } } + private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range { + CopyFileobj() { this = API::moduleImport("shutil").getMember("copyfileobj").getACall() } + + override DataFlow::Node getAPathArgument() { + result in [ + this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst") + ] + } + } } + + From 3c9de6f4889ff5a48938f0295a15c46f08bd4cad Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Fri, 11 Mar 2022 18:50:37 +0100 Subject: [PATCH 40/64] Update Zip.qll --- python/ql/src/experimental/semmle/python/frameworks/Zip.qll | 3 --- 1 file changed, 3 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll index 7eec7f71cce..b730acd4eeb 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll @@ -27,6 +27,3 @@ private module Zip { } } } - - - From a05318f10cf265cae02fa98ba9b6f53754adea38 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Thu, 24 Mar 2022 00:32:11 +0100 Subject: [PATCH 41/64] Update zipslip_good.py --- .../query-tests/Security/CWE-022/zipslip_good.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) 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/zipslip_good.py index f1561a4fd2c..e1a4b9c5f0c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py @@ -3,18 +3,12 @@ import tarfile import shutil def unzip(filename, dir): - zf = zipfile.ZipFile(filename) - zf.extractall(dir) + zf = zipfile.ZipFile(filename) + zf.extractall(dir) def unzip1(filename, dir): - zf = zipfile.ZipFile(filename) - zf.extract(dir) + zf = zipfile.ZipFile(filename) + zf.extract(dir) -def unzip2(filename): - with tarfile.open(filename) as tar: - for entry in tar: - #GOOD: Check that entry is safe - if os.path.isabs(entry.name) or ".." in entry.name: - raise ValueError("Illegal tar archive entry") - shutil.copy(entry, "/tmp/unpack/") + From b5f1e9de080edca585ef2e8bd90f8988dd9a6d8d Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Thu, 24 Mar 2022 00:33:28 +0100 Subject: [PATCH 42/64] Update zipslip_bad.py --- .../experimental/Security/CWE-022/zipslip_bad.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py index 580ff533c75..8743b2e13e2 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -2,13 +2,13 @@ import zipfile import shutil def unzip(filename): - with zipfile.ZipFile(filename) as zipf: + with zipfile.ZipFile(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.copy(entry, "/tmp/unpack/") + for entry in zipf: + shutil.copy(entry, "/tmp/unpack/") def unzip1(filename): - with zipfile.ZipFile(filename) as zipf: - for entry in zipf: - with open(entry, 'wb') as dstfile: - shutil.copyfileobj(zipf, dstfile) + with zipfile.ZipFile(filename) as zipf: + for entry in zipf: + with open(entry, 'wb') as dstfile: + shutil.copyfileobj(zipf, dstfile) From 8dea7248ea14f6d94471c66766373aaa02b3e612 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Thu, 24 Mar 2022 00:34:52 +0100 Subject: [PATCH 43/64] Update zipslip_bad.py --- .../Security/CWE-022/zipslip_bad.py | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) 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/zipslip_bad.py index 5105c0c445e..aec22e44eb8 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -5,34 +5,33 @@ import gzip import zipfile def unzip(filename): - - with tarfile.open(filename) as zipf: + with tarfile.open(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.move(entry, "/tmp/unpack/") + for entry in zipf: + shutil.move(entry, "/tmp/unpack/") def unzip1(filename): - with gzip.open(filename) as zipf: + with gzip.open(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.copy2(entry, "/tmp/unpack/") + for entry in zipf: + shutil.copy2(entry, "/tmp/unpack/") def unzip2(filename): - with bz2.open(filename) as zipf: + with bz2.open(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.copyfile(entry, "/tmp/unpack/") + for entry in zipf: + shutil.copyfile(entry, "/tmp/unpack/") def unzip3(filename): - with zipfile.ZipFile(filename) as zipf: + with zipfile.ZipFile(filename) as zipf: #BAD : This could write any file on the filesystem. - for entry in zipf: - shutil.copy(entry, "/tmp/unpack/") + for entry in zipf: + shutil.copy(entry, "/tmp/unpack/") def unzip4(filename): - with zipfile.ZipFile(filename) as zipf: - for entry in zipf: - with open(entry, 'wb') as dstfile: - shutil.copyfileobj(zipf, dstfile) + with zipfile.ZipFile(filename) as zipf: + for entry in zipf: + with open(entry, 'wb') as dstfile: + shutil.copyfileobj(zipf, dstfile) From eab6568cdabcc50cdfa326831428967b47e4b91b Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Thu, 24 Mar 2022 00:35:24 +0100 Subject: [PATCH 44/64] Update zipslip_good.py --- .../ql/src/experimental/Security/CWE-022/zipslip_good.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_good.py b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py index b97ec283c8f..b6ae48e1d88 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_good.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_good.py @@ -1,10 +1,10 @@ import zipfile def unzip(filename, dir): - zf = zipfile.ZipFile(filename) - zf.extractall(dir) + zf = zipfile.ZipFile(filename) + zf.extractall(dir) def unzip1(filename, dir): - zf = zipfile.ZipFile(filename) - zf.extract(dir) + zf = zipfile.ZipFile(filename) + zf.extract(dir) From 413f1945ce6e014ea7fa0ffe216046f0f6abff8b Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 00:44:56 +0000 Subject: [PATCH 45/64] Update Zip.qll --- .../experimental/semmle/python/frameworks/Zip.qll | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll index b730acd4eeb..fa78e75b3ad 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll @@ -13,17 +13,20 @@ private module Zip { } override DataFlow::Node getAPathArgument() { - result in [this.getArg(0), this.getArgByName("src"), this.getArg(1), this.getArgByName("dst")] + result in [this.getArg(0), this.getArgByName("src")] } + + override DataFlow::Node getfsrcArgument() { none() } } private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range { CopyFileobj() { this = API::moduleImport("shutil").getMember("copyfileobj").getACall() } - override DataFlow::Node getAPathArgument() { - result in [ - this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst") - ] + override DataFlow::Node getfsrcArgument() { + result in [this.getArg(0), this.getArgByName("fsrc")] } + + override DataFlow::Node getAPathArgument() { none() } } } + From 0fac4f195df0518efbe899a3274d871e3c260596 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 00:47:27 +0000 Subject: [PATCH 46/64] Update Concepts.qll --- python/ql/src/experimental/semmle/python/Concepts.qll | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index 9358176bda6..c7222667d92 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -27,7 +27,11 @@ module CopyFile { /** * Gets the argument containing the path. */ - abstract DataFlow::Node getAPathArgument(); + abstract DataFlow::Node getAPathArgument(); + /** + * Gets fsrc argument. + */ + abstract DataFlow::Node getfsrcArgument(); } } @@ -43,6 +47,8 @@ class CopyFile extends DataFlow::Node { CopyFile() { this = range } DataFlow::Node getAPathArgument() { result = range.getAPathArgument() } + + DataFlow::Node getfsrcArgument() { result = range.getfsrcArgument() } } /** Provides classes for modeling log related APIs. */ From ddba3b7784f67ed2e97c7922042e7f085a76b637 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 00:59:56 +0000 Subject: [PATCH 47/64] Update ZipSlip.qll --- .../ql/src/experimental/semmle/python/security/ZipSlip.qll | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index 4196b52c760..8b131de04a1 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -8,7 +8,8 @@ class ZipSlipConfig extends TaintTracking::Configuration { ZipSlipConfig() { this = "ZipSlipConfig" } override predicate isSource(DataFlow::Node source) { - source = API::moduleImport("zipfile").getMember("ZipFile").getACall() or + source.asCfgNode().(CallNode).getFunction().(AttrNode).getObject("open").pointsTo().getClass() = Module::named("zipfile").attr("ZipFile") or + source.asCfgNode().(CallNode).getFunction().(AttrNode).getObject("namelist").pointsTo().getClass() = Module::named("zipfile").attr("ZipFile") or source = API::moduleImport("tarfile").getMember("open").getACall() or source = API::moduleImport("tarfile").getMember("TarFile").getACall() or source = API::moduleImport("bz2").getMember("open").getACall() or @@ -20,6 +21,7 @@ class ZipSlipConfig extends TaintTracking::Configuration { } override predicate isSink(DataFlow::Node sink) { - sink = any(CopyFile copyfile).getAPathArgument() + sink = any(CopyFile copyfile).getAPathArgument() or + sink = any(CopyFile copyfile).getfsrcArgument() } } From a8c14ed6c357433d674d1d6dfaa211bb1b880d16 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 01:00:38 +0000 Subject: [PATCH 48/64] Update zipslip_bad.py --- .../Security/CWE-022/zipslip_bad.py | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) 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/zipslip_bad.py index aec22e44eb8..40ae577e35b 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -9,29 +9,30 @@ def unzip(filename): #BAD : This could write any file on the filesystem. for entry in zipf: shutil.move(entry, "/tmp/unpack/") - + def unzip1(filename): with gzip.open(filename) as zipf: #BAD : This could write any file on the filesystem. for entry in zipf: shutil.copy2(entry, "/tmp/unpack/") - + def unzip2(filename): with bz2.open(filename) as zipf: #BAD : This could write any file on the filesystem. for entry in zipf: shutil.copyfile(entry, "/tmp/unpack/") - + def unzip3(filename): - with zipfile.ZipFile(filename) as zipf: + zf = zipfile.ZipFile(filename) + filelist = zf.namelist() #BAD : This could write any file on the filesystem. - for entry in zipf: + for filename in filelist: shutil.copy(entry, "/tmp/unpack/") def unzip4(filename): - with zipfile.ZipFile(filename) as zipf: - for entry in zipf: - with open(entry, 'wb') as dstfile: - shutil.copyfileobj(zipf, dstfile) - + zf = zipfile.ZipFile(filename) + filelist = zf.namelist() + for filename in filelist: + with zf.open(filename) as srcf: + shutil.copyfileobj(srcf, dstfile) From f364e41dbe252706ee1a548d2fd07bd74d60408b Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 01:02:38 +0000 Subject: [PATCH 49/64] Update ZipSlip.expected --- .../Security/CWE-022/ZipSlip.expected | 50 ++++++++----------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected index 9f5cd92d316..9e120236f19 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected @@ -1,32 +1,26 @@ edges -| zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | zipslip_bad.py:11:11:11:15 | SSA variable entry | -| zipslip_bad.py:11:11:11:15 | SSA variable entry | zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | -| zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | zipslip_bad.py:17:11:17:15 | SSA variable entry | -| zipslip_bad.py:17:11:17:15 | SSA variable entry | zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | -| zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | zipslip_bad.py:23:11:23:15 | SSA variable entry | -| zipslip_bad.py:23:11:23:15 | SSA variable entry | zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | -| zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | zipslip_bad.py:29:11:29:15 | SSA variable entry | -| zipslip_bad.py:29:11:29:15 | SSA variable entry | zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | -| zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | +| zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | zipslip_bad.py:10:13:10:17 | SSA variable entry | +| zipslip_bad.py:10:13:10:17 | SSA variable entry | zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | +| zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | zipslip_bad.py:16:13:16:17 | SSA variable entry | +| zipslip_bad.py:16:13:16:17 | SSA variable entry | zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | +| zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | zipslip_bad.py:22:13:22:17 | SSA variable entry | +| zipslip_bad.py:22:13:22:17 | SSA variable entry | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | +| zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | nodes -| zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:11:11:11:15 | SSA variable entry | semmle.label | SSA variable entry | -| zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | -| zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:17:11:17:15 | SSA variable entry | semmle.label | SSA variable entry | -| zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | -| zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:23:11:23:15 | SSA variable entry | semmle.label | SSA variable entry | -| zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | -| zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:29:11:29:15 | SSA variable entry | semmle.label | SSA variable entry | -| zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | -| zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | semmle.label | ControlFlowNode for zipf | +| zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:10:13:10:17 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:16:13:16:17 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:22:13:22:17 | SSA variable entry | semmle.label | SSA variable entry | +| zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | semmle.label | ControlFlowNode for srcf | subpaths #select -| zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | zipslip_bad.py:12:23:12:27 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:9:9:9:30 | ControlFlowNode for Attribute() | a potentially untrusted source | -| zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | zipslip_bad.py:18:24:18:28 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:15:9:15:27 | ControlFlowNode for Attribute() | a potentially untrusted source | -| zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | zipslip_bad.py:24:27:24:31 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:21:9:21:26 | ControlFlowNode for Attribute() | a potentially untrusted source | -| zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | zipslip_bad.py:30:23:30:27 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:27:9:27:33 | ControlFlowNode for Attribute() | a potentially untrusted source | -| zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | zipslip_bad.py:36:32:36:35 | ControlFlowNode for zipf | Extraction of zipfile from $@ | zipslip_bad.py:33:9:33:33 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | Extraction of zipfile from $@ | zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | a potentially untrusted source | From cafbd984548cd93b912967e3d781778962adca5a Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 01:08:39 +0000 Subject: [PATCH 50/64] Update zipslip_bad.py --- .../experimental/Security/CWE-022/zipslip_bad.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py index 8743b2e13e2..028d4387243 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -2,13 +2,15 @@ import zipfile import shutil def unzip(filename): - with zipfile.ZipFile(filename) as zipf: + with tarfile.open(filename) as zipf: #BAD : This could write any file on the filesystem. for entry in zipf: - shutil.copy(entry, "/tmp/unpack/") + shutil.copyfile(entry, "/tmp/unpack/") -def unzip1(filename): - with zipfile.ZipFile(filename) as zipf: - for entry in zipf: - with open(entry, 'wb') as dstfile: - shutil.copyfileobj(zipf, dstfile) +def unzip4(filename): + zf = zipfile.ZipFile(filename) + filelist = zf.namelist() + for filename in filelist: + with zf.open(filename) as srcf: + shutil.copyfileobj(srcf, dstfile) + From a50f051cddc47ecf8882c06ce44f94cc8979d410 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 01:38:58 +0000 Subject: [PATCH 51/64] Update zipslip_bad.py --- .../query-tests/Security/CWE-022/zipslip_bad.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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/zipslip_bad.py index 40ae577e35b..551fc733dba 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -24,15 +24,15 @@ def unzip2(filename): def unzip3(filename): zf = zipfile.ZipFile(filename) - filelist = zf.namelist() + with zf.namelist() as filelist: #BAD : This could write any file on the filesystem. - for filename in filelist: - shutil.copy(entry, "/tmp/unpack/") + for x in filelist: + shutil.copy(x, "/tmp/unpack/") def unzip4(filename): zf = zipfile.ZipFile(filename) filelist = zf.namelist() - for filename in filelist: - with zf.open(filename) as srcf: - shutil.copyfileobj(srcf, dstfile) + for x in filelist: + with zf.open(x) as srcf: + shutil.copyfileobj(x, dstfile) From d89ed8b98be25f53c88fafb2d9c4c6f972c3f442 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 01:40:08 +0000 Subject: [PATCH 52/64] Update zipslip_bad.py --- python/ql/src/experimental/Security/CWE-022/zipslip_bad.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py index 028d4387243..24b6ec4d978 100644 --- a/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py +++ b/python/ql/src/experimental/Security/CWE-022/zipslip_bad.py @@ -10,7 +10,7 @@ def unzip(filename): def unzip4(filename): zf = zipfile.ZipFile(filename) filelist = zf.namelist() - for filename in filelist: - with zf.open(filename) as srcf: + for x in filelist: + with zf.open(x) as srcf: shutil.copyfileobj(srcf, dstfile) From 53f756b07808b1967b76eaad00626046f2dd5cad Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Mon, 28 Mar 2022 08:54:44 +0000 Subject: [PATCH 53/64] Update ZipSlip.expected --- .../Security/CWE-022/ZipSlip.expected | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected index 9e120236f19..66594b3374e 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected @@ -5,7 +5,10 @@ edges | zipslip_bad.py:16:13:16:17 | SSA variable entry | zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | zipslip_bad.py:22:13:22:17 | SSA variable entry | | zipslip_bad.py:22:13:22:17 | SSA variable entry | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | -| zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | +| zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | zipslip_bad.py:29:13:29:13 | SSA variable x | +| zipslip_bad.py:29:13:29:13 | SSA variable x | zipslip_bad.py:30:25:30:25 | ControlFlowNode for x | +| zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | zipslip_bad.py:35:9:35:9 | SSA variable x | +| zipslip_bad.py:35:9:35:9 | SSA variable x | zipslip_bad.py:37:32:37:32 | ControlFlowNode for x | nodes | zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | zipslip_bad.py:10:13:10:17 | SSA variable entry | semmle.label | SSA variable entry | @@ -16,11 +19,16 @@ nodes | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | zipslip_bad.py:22:13:22:17 | SSA variable entry | semmle.label | SSA variable entry | | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | -| zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | -| zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | semmle.label | ControlFlowNode for srcf | +| zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:29:13:29:13 | SSA variable x | semmle.label | SSA variable x | +| zipslip_bad.py:30:25:30:25 | ControlFlowNode for x | semmle.label | ControlFlowNode for x | +| zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| zipslip_bad.py:35:9:35:9 | SSA variable x | semmle.label | SSA variable x | +| zipslip_bad.py:37:32:37:32 | ControlFlowNode for x | semmle.label | ControlFlowNode for x | subpaths #select | zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | a potentially untrusted source | | zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | a potentially untrusted source | | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | a potentially untrusted source | -| zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | zipslip_bad.py:37:32:37:35 | ControlFlowNode for srcf | Extraction of zipfile from $@ | zipslip_bad.py:36:14:36:30 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:30:25:30:25 | ControlFlowNode for x | zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | zipslip_bad.py:30:25:30:25 | ControlFlowNode for x | Extraction of zipfile from $@ | zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | a potentially untrusted source | +| zipslip_bad.py:37:32:37:32 | ControlFlowNode for x | zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | zipslip_bad.py:37:32:37:32 | ControlFlowNode for x | Extraction of zipfile from $@ | zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | a potentially untrusted source | From 68bfe38529f8067d2e22dd9495197ded01c4bd2a Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Tue, 5 Apr 2022 12:31:30 +0000 Subject: [PATCH 54/64] Update Zip.qll --- python/ql/src/experimental/semmle/python/frameworks/Zip.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll index fa78e75b3ad..3256a408b2f 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/Zip.qll @@ -3,7 +3,7 @@ private import experimental.semmle.python.Concepts private import semmle.python.dataflow.new.DataFlow private import semmle.python.ApiGraphs -private module Zip { +private module CopyFile { private class CopyFiles extends DataFlow::CallCfgNode, CopyFile::Range { CopyFiles() { this = From 8882bc1533c98332f6b0c879121f967ee906b9b6 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Tue, 5 Apr 2022 12:32:10 +0000 Subject: [PATCH 55/64] Update Frameworks.qll --- python/ql/src/experimental/semmle/python/Frameworks.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/semmle/python/Frameworks.qll b/python/ql/src/experimental/semmle/python/Frameworks.qll index 7263773a88e..45003faada6 100644 --- a/python/ql/src/experimental/semmle/python/Frameworks.qll +++ b/python/ql/src/experimental/semmle/python/Frameworks.qll @@ -14,4 +14,4 @@ private import experimental.semmle.python.libraries.PyJWT private import experimental.semmle.python.libraries.Python_JWT private import experimental.semmle.python.libraries.Authlib private import experimental.semmle.python.libraries.PythonJose -private import experimental.semmle.python.frameworks.Zip +private import experimental.semmle.python.frameworks.CopyFile From 0d6d07886bc91e144c14b544d035da779a1bc347 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Tue, 5 Apr 2022 12:37:14 +0000 Subject: [PATCH 56/64] Rename Zip.qll to CopyFile.qll --- .../semmle/python/frameworks/{Zip.qll => CopyFile.qll} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename python/ql/src/experimental/semmle/python/frameworks/{Zip.qll => CopyFile.qll} (100%) diff --git a/python/ql/src/experimental/semmle/python/frameworks/Zip.qll b/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll similarity index 100% rename from python/ql/src/experimental/semmle/python/frameworks/Zip.qll rename to python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll From dfe7f532ac77dd3dccdab4c8ea63417ca669e6c1 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Tue, 5 Apr 2022 12:42:05 +0000 Subject: [PATCH 57/64] Update CopyFile.qll --- .../semmle/python/frameworks/CopyFile.qll | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll b/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll index 3256a408b2f..b2dab2f40e0 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll @@ -2,8 +2,18 @@ private import python private import experimental.semmle.python.Concepts private import semmle.python.dataflow.new.DataFlow private import semmle.python.ApiGraphs - + private module CopyFile { + + /** + * The `shutil` module provides methods to copy or move files. + * See: + * - https://docs.python.org/3/library/shutil.html#shutil.copyfile + * - https://docs.python.org/3/library/shutil.html#shutil.copy + * - https://docs.python.org/3/library/shutil.html#shutil.copy2 + * - https://docs.python.org/3/library/shutil.html#shutil.copytree + * - https://docs.python.org/3/library/shutil.html#shutil.move + */ private class CopyFiles extends DataFlow::CallCfgNode, CopyFile::Range { CopyFiles() { this = @@ -18,7 +28,10 @@ private module CopyFile { override DataFlow::Node getfsrcArgument() { none() } } - + + // TODO: once we have flow summaries, model `shutil.copyfileobj` which copies the content between its' file-like arguments. + // See https://docs.python.org/3/library/shutil.html#shutil.copyfileobj + private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range { CopyFileobj() { this = API::moduleImport("shutil").getMember("copyfileobj").getACall() } From 29f69bde7531207ab3850600b07aebcbff2e1966 Mon Sep 17 00:00:00 2001 From: Ahmed Farid <53880570+ahmed-farid-dev@users.noreply.github.com> Date: Tue, 5 Apr 2022 12:46:51 +0000 Subject: [PATCH 58/64] Update zipslip_bad.py --- .../experimental/query-tests/Security/CWE-022/zipslip_bad.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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/zipslip_bad.py index 551fc733dba..936dd3f7945 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -34,5 +34,5 @@ def unzip4(filename): filelist = zf.namelist() for x in filelist: with zf.open(x) as srcf: - shutil.copyfileobj(x, dstfile) + shutil.copyfileobj(x, "/tmp/unpack/") From 4b580820c841e373d1ef9be55631cc618f8e1a7e Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 8 Apr 2022 23:12:46 +0200 Subject: [PATCH 59/64] Python: Fix broken QHelp --- python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp b/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp index 63d43a51541..89260db7bd7 100644 --- a/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp @@ -46,7 +46,7 @@ In this example an archive is extracted without validating file paths. - +
  • Snyk: Zip Slip Vulnerability. From 8521f9a008910c37d1d72d1863bd932ece24a014 Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 8 Apr 2022 23:13:38 +0200 Subject: [PATCH 60/64] Python: Autoformat `ZipSlip.ql` --- python/ql/src/experimental/Security/CWE-022/ZipSlip.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql index e979d41aedd..dd89b4d1280 100644 --- a/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql +++ b/python/ql/src/experimental/Security/CWE-022/ZipSlip.ql @@ -20,4 +20,3 @@ from ZipSlipConfig config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) select sink.getNode(), source, sink, "Extraction of zipfile from $@", source.getNode(), "a potentially untrusted source" - From e1371151f931f16098874f21636d71cddc9641bf Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 8 Apr 2022 23:16:41 +0200 Subject: [PATCH 61/64] Python: Autoformat `Concepts.qll` --- python/ql/src/experimental/semmle/python/Concepts.qll | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index c7222667d92..0c6bca7733b 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -14,7 +14,6 @@ private import semmle.python.dataflow.new.RemoteFlowSources private import semmle.python.dataflow.new.TaintTracking private import experimental.semmle.python.Frameworks - /** Provides classes for modeling copying file related APIs. */ module CopyFile { /** @@ -27,7 +26,8 @@ module CopyFile { /** * Gets the argument containing the path. */ - abstract DataFlow::Node getAPathArgument(); + abstract DataFlow::Node getAPathArgument(); + /** * Gets fsrc argument. */ @@ -47,7 +47,7 @@ class CopyFile extends DataFlow::Node { CopyFile() { this = range } DataFlow::Node getAPathArgument() { result = range.getAPathArgument() } - + DataFlow::Node getfsrcArgument() { result = range.getfsrcArgument() } } @@ -79,6 +79,7 @@ class LogOutput extends DataFlow::Node { LogOutput() { this = range } DataFlow::Node getAnInput() { result = range.getAnInput() } +} /** * Since there is both XML module in normal and experimental Concepts, From 57beeaada0bfe08aab42e1878fb1ab5eea98d8df Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 8 Apr 2022 23:18:03 +0200 Subject: [PATCH 62/64] Python: Fix name clash in `CopyFile.qll` --- .../experimental/semmle/python/frameworks/CopyFile.qll | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll b/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll index b2dab2f40e0..ddf5e1f62ec 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll @@ -2,9 +2,8 @@ private import python private import experimental.semmle.python.Concepts private import semmle.python.dataflow.new.DataFlow private import semmle.python.ApiGraphs - -private module CopyFile { - + +private module CopyFileImpl { /** * The `shutil` module provides methods to copy or move files. * See: @@ -28,10 +27,9 @@ private module CopyFile { override DataFlow::Node getfsrcArgument() { none() } } - + // TODO: once we have flow summaries, model `shutil.copyfileobj` which copies the content between its' file-like arguments. // See https://docs.python.org/3/library/shutil.html#shutil.copyfileobj - private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range { CopyFileobj() { this = API::moduleImport("shutil").getMember("copyfileobj").getACall() } @@ -42,4 +40,3 @@ private module CopyFile { override DataFlow::Node getAPathArgument() { none() } } } - From ab81247b7c4c21f3ec0a80ab6fd7c86898cf52c5 Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 8 Apr 2022 23:19:41 +0200 Subject: [PATCH 63/64] Python: Fix modelling in `ZipSlip.qll` - Remove use of points-to. - Exclude sources and sinks in the standard library (to prevent test brittleness). --- .../semmle/python/security/ZipSlip.qll | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll index 8b131de04a1..59f558c67d6 100644 --- a/python/ql/src/experimental/semmle/python/security/ZipSlip.qll +++ b/python/ql/src/experimental/semmle/python/security/ZipSlip.qll @@ -7,21 +7,33 @@ import semmle.python.dataflow.new.TaintTracking class ZipSlipConfig extends TaintTracking::Configuration { ZipSlipConfig() { this = "ZipSlipConfig" } - override predicate isSource(DataFlow::Node source) { - source.asCfgNode().(CallNode).getFunction().(AttrNode).getObject("open").pointsTo().getClass() = Module::named("zipfile").attr("ZipFile") or - source.asCfgNode().(CallNode).getFunction().(AttrNode).getObject("namelist").pointsTo().getClass() = Module::named("zipfile").attr("ZipFile") or - source = API::moduleImport("tarfile").getMember("open").getACall() or - source = API::moduleImport("tarfile").getMember("TarFile").getACall() or - source = API::moduleImport("bz2").getMember("open").getACall() or - source = API::moduleImport("bz2").getMember("BZ2File").getACall() or - source = API::moduleImport("gzip").getMember("GzipFile").getACall() or - source = API::moduleImport("gzip").getMember("open").getACall() or - source = API::moduleImport("lzma").getMember("open").getACall() or - source = API::moduleImport("lzma").getMember("LZMAFile").getACall() + override predicate isSource(DataFlow::Node source) { + ( + source = + API::moduleImport("zipfile").getMember("ZipFile").getReturn().getMember("open").getACall() or + source = + API::moduleImport("zipfile") + .getMember("ZipFile") + .getReturn() + .getMember("namelist") + .getACall() or + source = API::moduleImport("tarfile").getMember("open").getACall() or + source = API::moduleImport("tarfile").getMember("TarFile").getACall() or + source = API::moduleImport("bz2").getMember("open").getACall() or + source = API::moduleImport("bz2").getMember("BZ2File").getACall() or + source = API::moduleImport("gzip").getMember("GzipFile").getACall() or + source = API::moduleImport("gzip").getMember("open").getACall() or + source = API::moduleImport("lzma").getMember("open").getACall() or + source = API::moduleImport("lzma").getMember("LZMAFile").getACall() + ) and + not source.getScope().getLocation().getFile().inStdlib() } - - override predicate isSink(DataFlow::Node sink) { - sink = any(CopyFile copyfile).getAPathArgument() or - sink = any(CopyFile copyfile).getfsrcArgument() + + override predicate isSink(DataFlow::Node sink) { + ( + sink = any(CopyFile copyfile).getAPathArgument() or + sink = any(CopyFile copyfile).getfsrcArgument() + ) and + not sink.getScope().getLocation().getFile().inStdlib() } } From 3d14c5f3c3b77ebe11f3831ce5156de34910b7f7 Mon Sep 17 00:00:00 2001 From: Taus Date: Fri, 8 Apr 2022 23:20:47 +0200 Subject: [PATCH 64/64] Python: Update tests We need to import `tty` in order to be able to detect the standard library correctly. --- .../experimental/query-tests/Security/CWE-022/zipslip_bad.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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/zipslip_bad.py index 936dd3f7945..c622ead874c 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py +++ b/python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py @@ -1,6 +1,6 @@ import tarfile import shutil -import bz2 +import bz2 import gzip import zipfile @@ -23,7 +23,7 @@ def unzip2(filename): shutil.copyfile(entry, "/tmp/unpack/") def unzip3(filename): - zf = zipfile.ZipFile(filename) + zf = zipfile.ZipFile(filename) with zf.namelist() as filelist: #BAD : This could write any file on the filesystem. for x in filelist: @@ -36,3 +36,4 @@ def unzip4(filename): with zf.open(x) as srcf: shutil.copyfileobj(x, "/tmp/unpack/") +import tty # to set the import root so we can identify the standard library