Skip to content

SCANPY-181 Only use the the filter parameter of tarfile.extractall if supported#201

Merged
Seppli11 merged 2 commits intomasterfrom
SCANPY-181
May 27, 2025
Merged

SCANPY-181 Only use the the filter parameter of tarfile.extractall if supported#201
Seppli11 merged 2 commits intomasterfrom
SCANPY-181

Conversation

@Seppli11
Copy link
Copy Markdown
Contributor

@Seppli11 Seppli11 commented May 26, 2025

SCANPY-181

So far, pysonar has used the filter parameter of tarfile.extractall(...), which was introduced in Python 3.12. However, the parameter was backported to 3.9.17, 3.10.12, 3.11.4, which all released on 06.06.2023. Because we used up to date Python versions, which had the backport, in our ITs, they didn't fail and we didn't notice.

@sonarqube-next
Copy link
Copy Markdown

sonarqube-next bot commented May 26, 2025

if sys.version_info >= (3, 12):
tar_ref.extractall(target_dir, filter="data")
else:
tar_ref.extractall(target_dir)
Copy link
Copy Markdown
Contributor Author

@Seppli11 Seppli11 May 26, 2025

Choose a reason for hiding this comment

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

There is a security risk of tar bombs here.
I see two possible attacks:

  1. An attacker compromises the SQ server and sends a malicious tar
  2. Since checking the checksum of the tar and actually untarring it are done in separate steps, an attacker could change the content of the tar between the two steps. However, for this, an attacker would have to be able to execute arbitrary commands

Overall, the security risk seems low to me, but it is still a risk.

Link to the sonar security rule: https://next.sonarqube.com/sonarqube/security_hotspots?id=SonarSource_sonar-scanner-python&hotspots=9ec9486e-0271-4c5f-820f-b780ab5863d0&pullRequest=201

Copy link
Copy Markdown
Contributor

@joke1196 joke1196 left a comment

Choose a reason for hiding this comment

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

LGTM! I think as well that the security risk is very low.

@Seppli11 Seppli11 merged commit e8c1010 into master May 27, 2025
19 checks passed
@Seppli11 Seppli11 deleted the SCANPY-181 branch May 27, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants