Add query to detect ZipSlip #8004

ahmed-farid-dev · 2022-02-14T01:29:19Z

Zip Slip is a widespread critical archive extraction vulnerability, allowing attackers to write arbitrary files on the system, typically resulting in remote command execution

ahmed-farid-dev · 2022-02-23T21:40:19Z

Hello, any update on this ?

tausbn

Thank you for your submission!

I have made some comments and suggestions for improving this query and getting it into a state where it can be merged.

In addition to the various things I point out, the main thing missing at this point is a set of tests that demonstrate what the query can (and if relevant, cannot) do in its present state. I would especially like to see a test of the sanitizer.

tausbn · 2022-02-24T12:21:12Z

python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql

+    CopyZip() {
+       exists(DataFlow::CallCfgNode call, DataFlow::Node pred |
+          call = API::moduleImport("shutil").getMember([
+                                                  // these are used to copy files


The formatting here looks really weird. Has this been formatted using the standard autoformatter? If not, it needs to be in order to get merged, since we enforce this on all of our QL code. (To use the autoformatter, you can either invoke it via the VSCode extension, or the CodeQL CLI.)

tausbn · 2022-02-24T12:22:09Z

python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql

+
+abstract class OpenZipFile extends DataFlow::CallCfgNode { }
+
+private class CopyZip extends CopyZipFile {


I don't see anything in this class that makes it specific to zip files, so it's not clear to me why it should contain Zip in its name.

tausbn · 2022-02-24T12:24:14Z

python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp

+to prevent writing files to unexpected locations.</p>
+
+<p>The recommended way of writing an output file from a Zip archive entry is to use
+this function instead of <code>extract()</code> or <code>extractall()</code>.


It is not clear to me what "this function" refers to. Do you in fact mean the opposite of what you've written, that the recommended way is to call extract or extractall?

tausbn · 2022-02-24T12:24:50Z

python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp

+
+<sample src="zipslip_bad.py" />
+
+<p>To fix this vulnerability, we need to this function <code>extractall()</code>.


Suggested change

<p>To fix this vulnerability, we need to this function <code>extractall()</code>.

<p>To fix this vulnerability, we need to call the function <code>extractall()</code>.

tausbn · 2022-02-24T12:31:16Z

python/ql/src/experimental/Security/CWE-022/zipslip_bad.py

+with zf.open() as zipf:
+    #BAD : This could write any file on the filesystem.
+    for entry in zipf:
+    shutil.copyfileobj(entry, "/tmp/unpack/")


Suggested change

shutil.copyfileobj(entry, "/tmp/unpack/")

shutil.copyfileobj(entry, "/tmp/unpack/")

This would be a syntax error otherwise.

tausbn · 2022-02-24T12:54:07Z

python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql

+                                                 "move"])
+                                             .getACall() and
+
+         call.getArg(0) = pred


What purpose does pred serve here? It's only mentioned on this line, as far as I can see. You could equivalently just write exists(call.getArg(0)), or just remove it altogether.

tausbn · 2022-02-24T12:55:43Z

python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql

+abstract class OpenZipFile extends DataFlow::CallCfgNode { }
+
+private class CopyZip extends CopyZipFile {
+    CopyZip() {


This predicate does not mention this, and so it will in fact hold for all dataflow nodes, assuming there is at least one call, somewhere in the source code, to one of the listed methods on shutil.

(This is why it's important to write tests!)

tausbn · 2022-02-24T12:57:02Z

python/ql/src/experimental/Security/CWE-022/ZipSlipCheckLib.ql

+    OpenZip() {
+       exists(DataFlow::CallCfgNode call |
+          call = API::moduleImport("zipfile").getMember("ZipFile").getMember("open").getACall() 
+       )
+    } 


Again, this characteristic predicate does not mention this, and so the set of nodes it encompasses is all the nodes in OpenZipFile. You probably want to write this = ... instead of call = ..., as I imagine you want to capture the return value of the given function.

tausbn · 2022-02-24T12:57:58Z

python/ql/src/experimental/Security/CWE-022/zipslip_good.py

+
+zf = zipfile.ZipFile(filename)
+with zipfile.open() as zipf:
+     for entry in zipf:


Suggested change

for entry in zipf:

for entry in zipf:

To make the indentation consistent.

tausbn · 2022-02-24T13:04:03Z

python/ql/src/experimental/Security/CWE-022/zipslip_bad.py

+with zf.open() as zipf:
+    #BAD : This could write any file on the filesystem.
+    for entry in zipf:
+    shutil.copyfileobj(entry, "/tmp/unpack/")


cf. my earlier comment about copyfileobj, does this code actually do what you intend it to do?

ahmed-farid-dev · 2022-02-28T23:28:14Z

Hi @tausbn,
i get some trouble in running the test, can you help me pls?

ahmed-farid-dev · 2022-03-02T08:33:46Z

Hello @tausbn,
I removed the sanitizer because he will miss some shot

tausbn · 2022-03-02T15:22:47Z

Hi @tausbn, i get some trouble in running the test, can you help me pls?

I'm a bit confused. What tests are you trying to run here? As far as I can tell you have not added any tests to this PR. Tests should live in the python/ql/test/experimental directory.

As for the error you're seeing above, perhaps your branch is out of date compared to the current codeql main? You could try merging in main and see if this fixes the warning about database upgrades.

ahmed-farid-dev · 2022-03-02T17:36:30Z

@tausbn, i try but it still showing up.

tausbn · 2022-03-03T15:22:18Z

@tausbn, i try but it still showing up.

Hmm... It's not really clear to me why that's happening. I tried pulling down your PR branch, and running the tests through VSCode, and that built the database just fine (though the tests fail to compile, see below).

I also tried invoking codeql using the gh command line application, and that had no issue with building the database either:

$ gh codeql test run python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref 
Executing 1 tests in 1 directories.
Extracting test database in /workspaces/codeql/python/ql/test/experimental/query-tests/Security/CWE-022.
Compiling queries in /workspaces/codeql/python/ql/test/experimental/query-tests/Security/CWE-022.
--- expected
+++ actual
@@ -1,1 +1,1 @@
-
+ERROR: Could not resolve type OpenFile (/workspaces/codeql/python/ql/src/experimental/semmle/python/security/ZipSlip.qll:9,69-77)
[1/1 comp 2.4s] FAILED(COMPILATION) /workspaces/codeql/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref
0 tests passed; 1 tests failed:
  FAILED: /workspaces/codeql/python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref

What version of the CLI are you using?

ahmed-farid-dev · 2022-03-31T12:53:28Z

@tausbn i hope this changes will be enough

ahmed-farid-dev · 2022-04-05T12:33:57Z

for the autoformat checks, i use codeql query format

- Remove use of points-to. - Exclude sources and sinks in the standard library (to prevent test brittleness).

We need to import `tty` in order to be able to detect the standard library correctly.

tausbn

I took the liberty of fixing up the issues that were preventing this PR from getting merged.

If you decide to submit any other PRs, please

ensure that all the tests pass and rerun them whenever you make changes, and
ensure that the files are autoformatted at all times.

This will greatly help lessen the amount of friction encountered when reviewing and merging your contributions.

Thank you for the contribution.

ahmed-farid-dev · 2022-04-25T16:26:20Z

Hi @tausbn ,
pls, Can do anything about github/securitylab#538 ?

tausbn · 2022-04-25T21:05:02Z

Hi @tausbn , pls, Can do anything about github/securitylab#538 ?

I'm sorry, but I don't have any further involvement in that issue.

Add query to detect ZipSlip

5ed5e0b

ahmed-farid-dev requested a review from a team as a code owner February 14, 2022 01:29

github-actions bot added documentation Python labels Feb 14, 2022

ahmed-farid-dev mentioned this pull request Feb 14, 2022

[python]: Zip Slip Vulnerability github/securitylab#538

Closed

2 tasks

tausbn self-requested a review February 14, 2022 14:39

RasmusWL assigned tausbn Feb 14, 2022

tausbn requested changes Feb 24, 2022

View reviewed changes

github deleted a comment Feb 25, 2022

ahmed-farid-dev added 11 commits February 26, 2022 18:25

Update ZipSlip.qhelp

d0d14be

Update zipslip_good.py

c207294

Update and rename ZipSlipCheck.ql to ZipSlip.ql

21f6ad5

Delete ZipSlipCheckLib.ql

3eae131

Create ZipSlip.qll

abe25da

Create Zip.qll

76bd331

Update Frameworks.qll

b299367

Update ZipSlip.ql

67d3498

Update Zip.qll

c22b032

Update Concepts.qll

85bcaa9

Update zipslip_bad.py

70c0c7e

ahmed-farid-dev added 3 commits March 2, 2022 18:08

Create ZipSlip.qlref

c8f73ec

Create zipslip_bad.py

aef1df1

Create zipslip_good.py

c45b67c

ahmed-farid-dev added 12 commits March 24, 2022 00:33

Update zipslip_bad.py

b5f1e9d

Update zipslip_bad.py

8dea724

Update zipslip_good.py

eab6568

Update Zip.qll

413f194

Update Concepts.qll

0fac4f1

Update ZipSlip.qll

ddba3b7

Update zipslip_bad.py

a8c14ed

Update ZipSlip.expected

f364e41

Update zipslip_bad.py

cafbd98

Update zipslip_bad.py

a50f051

Update zipslip_bad.py

d89ed8b

Update ZipSlip.expected

53f756b

ahmed-farid-dev added 2 commits April 5, 2022 12:31

Update Zip.qll

68bfe38

Update Frameworks.qll

8882bc1

ahmed-farid-dev and others added 9 commits April 5, 2022 12:37

Rename Zip.qll to CopyFile.qll

0d6d078

Update CopyFile.qll

dfe7f53

Update zipslip_bad.py

29f69bd

Python: Fix broken QHelp

4b58082

Python: Autoformat ZipSlip.ql

8521f9a

Python: Autoformat Concepts.qll

e137115

Python: Fix name clash in CopyFile.qll

57beeaa

Python: Fix modelling in ZipSlip.qll

ab81247

- Remove use of points-to. - Exclude sources and sinks in the standard library (to prevent test brittleness).

Python: Update tests

3d14c5f

We need to import `tty` in order to be able to detect the standard library correctly.

tausbn approved these changes Apr 8, 2022

View reviewed changes

tausbn merged commit 626770a into github:main Apr 8, 2022

ahmed-farid-dev deleted the ZipSlip branch April 20, 2022 12:16

Apr	MAY	Jun
	22
2024	2025	2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add query to detect ZipSlip #8004

Add query to detect ZipSlip #8004

ahmed-farid-dev commented Feb 14, 2022

ahmed-farid-dev commented Feb 23, 2022

tausbn left a comment

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

tausbn Feb 24, 2022

ahmed-farid-dev commented Feb 28, 2022

ahmed-farid-dev commented Mar 2, 2022 •

edited

Loading

tausbn commented Mar 2, 2022

ahmed-farid-dev commented Mar 2, 2022

tausbn commented Mar 3, 2022

ahmed-farid-dev commented Mar 31, 2022

ahmed-farid-dev commented Apr 5, 2022

tausbn left a comment

ahmed-farid-dev commented Apr 25, 2022

tausbn commented Apr 25, 2022


		abstract class OpenZipFile extends DataFlow::CallCfgNode { }

		private class CopyZip extends CopyZipFile {


		<sample src="zipslip_bad.py" />

		<p>To fix this vulnerability, we need to this function <code>extractall()</code>.

	shutil.copyfileobj(entry, "/tmp/unpack/")
	shutil.copyfileobj(entry, "/tmp/unpack/")

Add query to detect ZipSlip #8004

Add query to detect ZipSlip #8004

Conversation

ahmed-farid-dev commented Feb 14, 2022

ahmed-farid-dev commented Feb 23, 2022

tausbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmed-farid-dev commented Feb 28, 2022

ahmed-farid-dev commented Mar 2, 2022 • edited Loading

tausbn commented Mar 2, 2022

ahmed-farid-dev commented Mar 2, 2022

tausbn commented Mar 3, 2022

ahmed-farid-dev commented Mar 31, 2022

ahmed-farid-dev commented Apr 5, 2022

tausbn left a comment

Choose a reason for hiding this comment

ahmed-farid-dev commented Apr 25, 2022

tausbn commented Apr 25, 2022

ahmed-farid-dev commented Mar 2, 2022 •

edited

Loading