Python: Unsafe unpacking using shutil.unpack_archive() query and tests

[Sim4n6]

Dear CodeQL team,

Please, find enclosed the CodeQL query and the associated tests for an unsafe unpacking using shutil.unpack_archive() of a malicious tarball retrieved from a remote location.

Regards

[yoff]

It looks like you did a lot of work finding relevant calls to model. It would be nice to document that a bit, both as comments in the code and as tests, so we can see what the code is supposed to model and that it works (and keeps working in the future :-)).

For testing, we now have inline tests for dataflow queries. To use it, you have to put your data flow configuration into a qll file. Then you can import it similarly to this example (just copy that file and change the second import) and you can then annotate your test files like in this example (with # $result=BAD and # $MISSING:result=BAD).
It is not required that you do this, but it might be easier for you and it would be easier for me than it is to read the expected files.

[Sim4n6]

For testing, we now have inline tests for dataflow queries. To use it, you have to put your data flow configuration into a qll file. Then you can import it similarly to this example (just copy that file and change the second import) and you can then annotate your test files like in this example (with # $result=BAD and # $MISSING:result=BAD).

This is what I did for now:

• Add DataflowQueryTest.ql.
• Generate the DataflowQueryTest.expected using the following command codeql test run --learn --format=text --threads=0 .
• Add the tag # $result=BAD at the two expected sinks within UnsafeUnpack.py.
• The commit 10d6ebf would consider the use of inline tests with the tag # $result=BAD set to the two sinks within UnsafeUnpack.py file.

I don't really understand the tag # $MISSING:result=BAD. If you could please point me to the right documentation, please...

[yoff]

I don't really understand the tag # $MISSING:result=BAD. If you could please point me to the right documentation, please...

The documentation is the comment at the top of this file, specifically this part.

It is telling you that from your annotations you expected results on line 12 and line 55 and both of those are missing. In this case, it is because you did not import any data flow configuration into the test. If you put the class UnsafeUnpackingConfig into a file of its own, say UnsafeUnpackQuery.qll, then you can do import UnsafeUnpackQuery both in UnsafeUnpack.ql and in DataflowQueryTest.ql. Then the test will know the configuration and can start finding results.

[yoff]

Cool, I will take a look. We normally use the re-request review button to signal that we are ready for more reviewing, then it pops up in my list of PRs needing review :-)

[yoff]

Thanks for all the changes. I think that between the comments referencing the relevant APIs and the example code in the test cases, it is now clear what flow the various predicates are meant to model.

[github-actions]

QHelp previews:

python/ql/src/experimental/Security/CWE-022bis/UnsafeUnpack.qhelp

Arbitrary file write during a tarball extraction from a user controlled source

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

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

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

Recommendation

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

Consider using a safer module, such as: zipfile

Example

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

import requests
import shutil

url = "https://www.someremote.location/tarball.tar.gz"
response = requests.get(url, stream=True)

tarpath = "/tmp/tmp456/tarball.tar.gz"
with open(tarpath, "wb") as f:
      f.write(response.raw.read())

untarredpath = "/tmp/tmp123"
shutil.unpack_archive(tarpath, untarredpath)

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

import requests
import tarfile

url = "https://www.someremote.location/tarball.tar.gz"
response = requests.get(url, stream=True)

tarpath = "/tmp/tmp456/tarball.tar.gz"
with open(tarpath, "wb") as f:
      f.write(response.raw.read())

untarredpath = "/tmp/tmp123"
with tarfile.open(tarpath) as tar:
	for member in tar.getmembers():
		if member.name.startswith("/") or ".." in member.name:
			raise Exception("Path traversal identified in tarball")

		tar.extract(untarredpath, member)

References

• Shutil official documentation shutil.unpack_archive() warning.
• Common Weakness Enumeration: CWE-22.

[yoff]

It seems the tests do not pass. You can see the failures here and here. This happens because you have shared the test directory with the tar-file query.

The first failure comes from the inline dataflow test picking up TarSlipImprov.py.

For any sink that is not an alert, you have to make one of two annotations:

• MISSING: result=BAD if an alert should actually have been here, but the query does not find it. We usually add a few of these to document which complicated cases the query cannot handle.
• result=OK if there should be no alert here because the sink is not reachable. This can be because there is a sanitiser in place or because it is not connected to dangerous input.

This makes it clear how the test file is intended to be read, i.e. which parts are examples of vulnerable code and which parts are not.

These annotations are not yet present in TarSlipImprov.py because we did not have inline expectations when it was developed.

The second failure comes from the tar slip query now picking up UnsafeUnpack.py and finding some results in there.

The easiest here may be to separate the test into two directories, i.e. to split CWE-022 into CWE-022-TarSlip and CWE-022-UnsafeUnpacking. That should prevent the cross-talk.

[Sim4n6]

Thanks @yoff I removed the annotations since the sinks should be reached and within the same commit I re-organized the test folder 📂 into two seperate for each query :Tarslip and UnsafeUnpack.

[yoff]

Ah, I think you only had to split out the files. For sinks that are reached, we do want the annotation of result=BAD as this marks the vulnerable parts of the test code. The expected file should always be "essentially empty". That is, it should only contain the first two lines of headings

missingAnnotationOnSink
failures

When the tests pass with an empty expected file, then we know that all the annotations are in agreement with what dataflow finds :-)

[yoff]

LGTM