fix: zip slip issue #19711
Closed
fix: zip slip issue #19711
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
nitishfy
suggested changes
Aug 28, 2024
McGon-Fid
changed the title
|
Linting errors addressed @nitishfy Thanks |
nitishfy
reviewed
Sep 4, 2024
| // Sanity check to protect against zip-slip | ||
|
|
||
| normalizedHeaderName := filepath.Clean(header.Name) | ||
| if normalizedHeaderName == "." || strings.Contains(normalizedHeaderName, "..") { |
There was a problem hiding this comment.
Suggested change
| if normalizedHeaderName == "." || strings.Contains(normalizedHeaderName, "..") { | |
| if normalizedHeaderName == "." || strings.Contains(normalizedHeaderName, "..") { |
you need to import the strings package in order to use the method.
nitishfy
suggested changes
Sep 4, 2024
There was a problem hiding this comment.
The lint and e2e tests are failing. These are the logs i found from the details
Error: util/io/files/tar.go:87:37: undefined: strings (typecheck)
package files
Error: util/tgzstream/stream.go:13:2: could not import github.com/argoproj/argo-cd/v2/util/io/files (-: # github.com/argoproj/argo-cd/v2/util/io/files
Error: util/io/files/tar.go:87:37: undefined: strings) (typecheck)
"github.com/argoproj/argo-cd/v2/util/io/files"
^
Error: util/cmp/stream.go:20:2: could not import github.com/argoproj/argo-cd/v2/util/io/files (-: # github.com/argoproj/argo-cd/v2/util/io/files
Error: util/io/files/tar.go:87:37: undefined: strings) (typecheck)
"github.com/argoproj/argo-cd/v2/util/io/files"
^
Error: util/manifeststream/stream.go:17:2: could not import github.com/argoproj/argo-cd/v2/util/io/files (-: # github.com/argoproj/argo-cd/v2/util/io/files
Error: util/io/files/tar.go:87:37: undefined: strings) (typecheck)
"github.com/argoproj/argo-cd/v2/util/io/files"
^
Error: test/e2e/fixture/certs/certs.go:21:43: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/certs/certs.go:23:43: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/certs/certs.go:32:43: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/certs/certs.go:49:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/gpgkeys/gpgkeys.go:17:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/gpgkeys/gpgkeys.go:31:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:[34](https://github.com/argoproj/argo-cd/actions/runs/10596799409/job/29365577197?pr=19711#step:4:36):42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:47:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:64:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:79:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:91:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:98:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:116:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:1[35](https://github.com/argoproj/argo-cd/actions/runs/10596799409/job/29365577197?pr=19711#step:4:37):42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:144:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:153:42: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.RunCli(args...))
^
Error: test/e2e/fixture/repos/repos.go:167:79: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.Run("", "helm", "dependency", "build", chartAbsPath))
^
Error: test/e2e/fixture/repos/repos.go:168:94: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
errors.FailOnErr(fixture.Run("", "helm", "package", chartAbsPath, "--destination", tempDest))
^
Error: test/e2e/fixture/repos/repos.go:176:3: not enough arguments in call to errors.FailOnErr
have (unknown type)
want (interface{}, error) (typecheck)
))
^
Error: test/e2e/fixture/app/actions.go:110:52: undefined: Application (typecheck)
func (a *Actions) CreateFromFile(handler func(app *Application), flags ...string) *Actions {
^
Error: test/e2e/fixture/app/actions.go:[39](https://github.com/argoproj/argo-cd/actions/runs/10596799409/job/29365577197?pr=19711#step:4:41)1:39: undefined: RefreshType (typecheck)
func (a *Actions) Refresh(refreshType RefreshType) *Actions {
^
Error: test/e2e/fixture/app/consequences.go:45:44: undefined: Application (typecheck)
func (c *Consequences) And(block func(app *Application)) *Consequences {
Updating PR
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19711 +/- ##
==========================================
+ Coverage 50.69% 55.81% +5.12%
==========================================
Files 315 320 +5
Lines 43381 44284 +903
==========================================
+ Hits 21991 24719 +2728
+ Misses 18882 17003 -1879
- Partials 2508 2562 +54 ☔ View full report in Codecov by Sentry. |
|
Can you create a unit test which proves that this is something which was previously (or currently is) an issue? From my testing it looks like the zip slip exploit is not exploitable in the first place, automated linters not withstanding. e.g t.Run("will protect against zip slip exploit", func(t *testing.T) {
tmpDir := createTmpDir(t)
defer deleteTmpDir(t, tmpDir)
// Create a malicious tar.gz archive in memory
var buf bytes.Buffer
gzw := gzip.NewWriter(&buf)
tw := tar.NewWriter(gzw)
// Add a malicious file entry that tries to escape the target directory
maliciousPath := "../../../etc/passwd"
hdr := &tar.Header{
Name: maliciousPath,
Mode: 0644,
Size: int64(len("malicious content")),
}
if err := tw.WriteHeader(hdr); err != nil {
t.Fatalf("Failed to write tar header: %v", err)
}
if _, err := tw.Write([]byte("malicious content")); err != nil {
t.Fatalf("Failed to write tar content: %v", err)
}
// Close the tar and gzip writers
if err := tw.Close(); err != nil {
t.Fatalf("Failed to close tar writer: %v", err)
}
if err := gzw.Close(); err != nil {
t.Fatalf("Failed to close gzip writer: %v", err)
}
err := files.Untgz(tmpDir, &buf, math.MaxInt64, false)
assert.NoError(t, err)
// Verify that the file was not created outside the temp directory
targetPath := filepath.Join(tmpDir, maliciousPath)
log.Printf(targetPath)
if _, err := os.Stat(targetPath); err == nil {
t.Fatalf("Malicious file was created: %s", targetPath)
}
}) |
nitishfy
suggested changes
Sep 10, 2024
|
I believe this is a false positive (see the related issue). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #19710
a496278_fidelity commented on Jul 23
codeql issue:
rule - go/zipslip
severity - error
level - high
Summary: Arbitrary file access during archive extraction ("Zip Slip")
Unsanitized archive entry, which may contain '..', is used in a file system operation.
Unsanitized archive entry, which may contain '..', is used in a file system operation.
Unsanitized archive entry, which may contain '..', is used in a file system operation.
Fix added to lines 74-94 in util/io/files/tar.go