-
Notifications
You must be signed in to change notification settings - Fork 27
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
Vulnerability Fix #197
Vulnerability Fix #197
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #197 +/- ##
=======================================
Coverage 35.58% 35.58%
=======================================
Files 7 7
Lines 1360 1360
=======================================
Hits 484 484
Misses 829 829
Partials 47 47 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this fix, we should try removing the following line to enforce the safety of this with gosec: https://github.com/Jdubrick/registry-support/blob/10b01bc136bd082f59b1ac0c91797f4065792d7b/registry-library/library/util.go#L125
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jdubrick, michael-valdron The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This has been assigned CVE-2024-1485 by Red Hat. |
Please specify the area for this PR
Vulnerability fixes
What does does this PR do / why we need it:
This PR aims to add a fix that ensures no malicious files can be added to an
archive.tar
file that is being read. Currently thefilepath.Join
andfilepath.Clean
functions are not properly accounting for paths added in a relative fashion (ie. "../../filename") and only properly cleans file paths that start with a leading slash. This fix adds that leading slash to all filepaths (relative or absolute) before the cleaning process, this ensures that no filepath can leave the confines of its parent by escaping out. The cleaning process removes any redundant double slashes that may arise because we are prepending the leading slash.Which issue(s) this PR fixes:
No public issue
PR acceptance criteria:
Documentation (WIP)
How to test changes / Special notes to the reviewer: