Skip to content

pySCG pillar 664 base 409 #944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

pySCG pillar 664 base 409 #944

wants to merge 5 commits into from

Conversation

myteron
Copy link
Contributor

@myteron myteron commented Jul 15, 2025

Signed-off-by: Helge Wehder <[email protected]>
@s19110
Copy link
Contributor

s19110 commented Jul 17, 2025

reviewing


## Non-Compliant Code Example - No File Validation

The `noncompliant01.py` example simply extracts all the files in the archive without performing any verification. The `extractall()` method will attempt to normalize the path name. Any archive from an untrusted source should be inspected prior to extraction. There is no attempt to control where the files are extracted, which is the script current working directory.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this confluence it is written
"The extractall() method will attempt to normalize the path name but any archive"
the way it is here doesn't read right to me.

Copy link
Contributor

@s19110 s19110 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments and suggestions.

</tr>
<tr>
<td><a href="http://cwe.mitre.org/">MITRE CWE</a></td>
<td>Base: <a href="https://cwe.mitre.org/data/definitions/1335.html">[CWE-1335: Incorrect Bitwise Shift of Integer (4.12)]</a></td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth keeping the info about CWEs being marked as either Base or Class. New contributors may not realize you have to check that on the official website.

Suggested change
<td>Base: <a href="https://cwe.mitre.org/data/definitions/1335.html">[CWE-1335: Incorrect Bitwise Shift of Integer (4.12)]</a></td>
<td>Base or Class (choose which one it is based on the abstraction on the CWE page): <a href="https://cwe.mitre.org/data/definitions/1335.html">[CWE-1335: Incorrect Bitwise Shift of Integer (4.12)]</a></td>

<td>[SEI CERT C 2025]</td>
<td>CERT C Coding Standard [online]. Available from: <a href=https://www.securecoding.cert.org/confluence/display/seccode/CERT+C+Coding+Standard>https://www.securecoding.cert.org/confluence/display/seccode/CERT+C+Coding+Standard</a> [Accessed 6 May 2025]</td>
</tr>
</table>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link to the reference guide is missing.

Suggested change
</table>
</table>
When writing bibliography, follow the [Harvard reference guide](https://dkit.ie.libguides.com/harvard/citing-referencing)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems unrelated to the pull request. I can't find this CWE on the main branch. Was it added by accident or will the readme and the other code examples be added later?


The `noncompliant01.py` code will extract any quantity of payloads. With a unmodified `example01.py` we get only `4 x 150MB` `zipbombfileX.txt`'s that are much bigger than the `0.58MB` `zip_attack_test.zip` archive.

The directory traversal payload will try to extract a `\Temp\zip_slip_windows.txt` for Windows and a `/tmp/zip_slip_posix.txt` for Unix based systems. Depending on the zip library in use the files may either end up in their indented target, under the same directory as the `zipbombfile.txt` files, or not at all.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indented means it has a zigzag pattern, like Python code where we change the indents for different code blocks! I think you meant "intended":

Suggested change
The directory traversal payload will try to extract a `\Temp\zip_slip_windows.txt` for Windows and a `/tmp/zip_slip_posix.txt` for Unix based systems. Depending on the zip library in use the files may either end up in their indented target, under the same directory as the `zipbombfile.txt` files, or not at all.
The directory traversal payload will try to extract a `\Temp\zip_slip_windows.txt` for Windows and a `/tmp/zip_slip_posix.txt` for Unix based systems. Depending on the zip library in use the files may either end up in their intended target, under the same directory as the `zipbombfile.txt` files, or not at all.


Experiment with the code by varying the `MAXSIZE`.

The `noncompliant02.py` code example tries to check the file_size from the `ZipInfo` instances provided by the `infolist()` method from `ZipFile`. This information is read from the `zip` archive metadata, so it is not reliable and can be forged by an attacker. The `extract()` method will attempt to normalize the path name. Again, there is no attempt to control where the files are extracted to in order to prevent traversal attacks. The underlaying zip library may or may not prevent traversal attacks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backticks + spelling fix:

Suggested change
The `noncompliant02.py` code example tries to check the file_size from the `ZipInfo` instances provided by the `infolist()` method from `ZipFile`. This information is read from the `zip` archive metadata, so it is not reliable and can be forged by an attacker. The `extract()` method will attempt to normalize the path name. Again, there is no attempt to control where the files are extracted to in order to prevent traversal attacks. The underlaying zip library may or may not prevent traversal attacks.
The `noncompliant02.py` code example tries to check the `file_size` from the `ZipInfo` instances provided by the `infolist()` method from `ZipFile`. This information is read from the `zip` archive metadata, so it is not reliable and can be forged by an attacker. The `extract()` method will attempt to normalize the path name. Again, there is no attempt to control where the files are extracted to in order to prevent traversal attacks. The underlying zip library may or may not prevent traversal attacks.


```

Depending on the underlaying zip library we should see `noncompliant02.py` prevent a zip bomb but not a traversal attack.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo:

Suggested change
Depending on the underlaying zip library we should see `noncompliant02.py` prevent a zip bomb but not a traversal attack.
Depending on the underlying zip library we should see `noncompliant02.py` prevent a zip bomb but not a traversal attack.

Comment on lines 14 to 15
class ZipExtractException(Exception):
"""Custom Exception"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we use the built-in ValueError instead of creating a custom exception with no distinct characteristics?

ZipExtractException: If there are to big files
ZipExtractException: If a directory traversal is detected
"""
# TODO: avoid exposing sensitive data to a lesser trusted entity via errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could reference CWE-209 to give an example of how to do it once that rule gets merged :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants